public inbox for cygwin-xfree@sourceware.org
help / color / mirror / Atom feed
From: michel hummel <hummel.michel@gmail.com>
To: cygwin-xfree@cygwin.com
Subject: Re: XWin crash after the launch of startkde on a remote Red Hat 5 machine
Date: Sat, 18 Sep 2010 18:44:00 -0000 [thread overview]
Message-ID: <4C950885.5080205@gmail.com> (raw)
In-Reply-To: <4C5EADBB.5000009@dronecode.org.uk>
Hi,
I have checked the code of the clipboard thread and the race condition
you talk about should not be a problem.
Before to start a new clipboard thread, the code waits for the end of
the first thread (pthread_join) then fixe the variable
g_fClipboardLaunched = FALSE;
g_fClipboardStarted = FALSE;
So the new thread will never be launched before the old one has terminated.
I have a proposition of modification to make the clipboard thread more robust.
1/ Adding a cleanup function which will be automaticaly called at the
end of the thread.
I purpose the use of the macro pthread_cleanup_push() pthread_cleanup_pop()
2/ You said that having a long-lived thread will be a good solution to
simplify the code.
I have an other proposition which needs less modification and can
simplify the code :
By using also here the macro pthread_cleanup_push() we can
automaticaly restart the thread on every unexpected exit.
We just have to delete this restart when the exit is expected (ie.
End of the function)
With this solution we don't have to take care about the thread
being killed by anyone, it will be restarted.
3/ An other thing I saw is that when the Xwindow part of the clipboard
is killed by an other application, the thread will still be alive but
the clipboard doesn't work anymore.
A solution can be :
when an winClipboardErrorHandler() is catched, check for
this Xwindow window and whenever this window doesn't exist anymore,
create a new one.
What do you think about my suggestions ?
I tested them and they seem to work as expected.
For example, I skipped the wrapper of XQueryTree (actualy needed for
xdmcp) and the clipboard is now working with xdmcp/gdm, etc.
If you don't like something in my propositions, please tell me what
and I will try to adapt it.
If you are interested in a patch doing this, I will be happy to give
it after a quick review and a clean
Thks
Michel Hummel
Le 08/08/2010 15:14, Jon TURNEY a écrit :
> On 06/08/2010 13:55, Michel Hummel wrote:
> > I didn't saw any response to my patch proposition, did someone tried
> it ?
> > If I didn't posted it in the good place where should I have to
>
> Yes, this is abolutely the right place for your patch.
>
> It looks good and I shall incorporate it in the next X server release.
>
> > I will be happy to explain the problem if my first mail wasn't clear.
>
> Nope, the analysis was very clear, thank you.
>
> I wonder if there also exists a race condition when the clipboard
> thread is stopping? i.e. if we try to start a new one just as the old
> one is stopping, we think it is still running and fail to do so?
>
> In general this code could do with a tidy up: I think it would be much
> more sensible to have a long-lived thread which tries to reconnect to
> the server when ever it gets disconnected, it would avoid all this
> complexity and the complexity of trying to avoid being killed by GDM
> as well.
>
>> 2010/8/3 Michel Hummel:
>>> Hello,
>>> I'm using cygwin on Microsoft Windows XP with XWin version :
>>> sh-3.2# XWin.exe -version
>>> Welcome to the XWin X Server
>>> Vendor: The Cygwin/X Project
>>> Release: 1.8.0.0 (10800000)
>>> Build Date: 2010-04-02
>>> Contact:
>>>
>>> I think I have found a bug in the XWin reset procedure which leads to
>>> the start of two clipboard thread's and sometime to a crash of the X
>>> server.
>>>
>>> The crash can be produced by lauching startkde on a Red Hat 5 remote
>>> machine (The crash should arrive after some minutes of work)
>>> but the bug can simply be produced like this :
>>>
>>> On my installation if I launch Xwin with this options :
>>> XWin :0 -ac&
>>> then I do
>>> xsetroot -solid '#FFFF00'
>>>
>>> After setting the color, Xwin launches its reset procedure because
>>> there is no more client connected ( the server loses the color but it
>>> is the normal comportment isn't it ?
>>> http://sourceware.org/ml/cygwin-xfree/2002-01/msg00106.html)
>>>
>>> The problem comes from the clipboard thread. In this example, when the
>>> server launches its reset procedure, the clipboard is in state
>>> "Launched" but is not in state "Started" (Boolean variable about
>>> clipboard status, see file hw/xwin/winclipboardthread.c).
>>>
>>> This particular status of the ClipboardThread during server reset is
>>> normaly managed by two "if" statements :
>>>
>>> * An "if" in the file hw/xwin/InitOutput.c disables the exit of the
>>> clipboard thread in this state
>>> Extract of the file hw/xwin/InitOutput.c (with line number) :
>>>
>>> 169 winClipboardShutdown (void)
>>> 170 {
>>> 171 /* Close down clipboard resources */
>>> 172 if (g_fClipboard&& g_fClipboardLaunched&&
>>> g_fClipboardStarted)
>>> 173 {
>>> 174 /* Synchronously destroy the clipboard window */
>>> 175 if (g_hwndClipboard != NULL)
>>> 176 {
>>> 177 SendMessage (g_hwndClipboard, WM_DESTROY, 0, 0);
>>> 178 /* NOTE: g_hwndClipboard is set to NULL in
>>> winclipboardthread.c */
>>> 179 }
>>> 180 else
>>> 181 return;
>>> 182
>>> 183 /* Wait for the clipboard thread to exit */
>>> 184 pthread_join (g_ptClipboardProc, NULL);
>>> 185
>>> 186 g_fClipboardLaunched = FALSE;
>>> 187 g_fClipboardStarted = FALSE;
>>> 188
>>> 189 winDebug ("winClipboardShutdown - Clipboard thread has
>>> exited.\n");
>>> 190 }
>>> 191 }
>>>
>>> * An "if" statement in the file hw/xwin/winclipboardwrappers.c
>>> prohibits the launch of clipboard thread if one is already Launched.
>>> Extract of the file hw/xwin/winclipboardwrappers.c (with line number)
>>> 256 /* If the clipboard client has already been started, abort */
>>> 257 if (g_fClipboardLaunched)
>>> 258 {
>>> 259 ErrorF ("winProcEstablishConnection - Clipboard client
>>> already "
>>> 260 "launched, returning.\n");
>>> 261 return iReturn;
>>> 262 }
>>>
>>>
>>> The problem is that the Boolean variables g_fClipboardLaunched and
>>> g_fClipboardStarted are re-initialized by the server reset procedure
>>> (function winInitializeGlobals of the file hw/xwin/winglobals.c)
>>> Extract of the file hw/xwin/winglobals.c (with line number)
>>> 128 /*
>>> 129 * Re-initialize global variables that are invalidated 130 * by a
>>> server reset.
>>> 131 */
>>> 132
>>> 133 void
>>> 134 winInitializeGlobals (void)
>>> 135 {
>>> 136 g_dwCurrentThreadID = GetCurrentThreadId ();
>>> 137 g_hwndKeyboardFocus = NULL;
>>> 138 #ifdef XWIN_CLIPBOARD
>>> 139 g_fClipboardLaunched = FALSE;
>>> 140 g_fClipboardStarted = FALSE;
>>> 141 g_iClipboardWindow = None;
>>> 142 g_pClipboardDisplay = NULL;
>>> 143 g_atomLastOwnedSelection = None;
>>> 144 g_hwndClipboard = NULL;
>>> 145 #endif
>>> 146 }
>>>
>>> The consequence of this Re-initialization in this particular situation
>>> is that the clipboard thread is launched two times and sometimes leads
>>> to a crash of the X server.
>>> You can see the double launch of the clipboard thread at the end of
>>> the attached log file Xwin.0.log ( 2 times the sentence :
>>> winClipboardProc - XOpenDisplay () returned and successfully opened
>>> the display. )
>>>
>>> To fix this bug I purpose to remove the variables g_fClipboardLaunched
>>> and g_fClipboardStarted of the "winInitializeGlobals (void)" function,
>>> as their re-initializations are handled in in the files :
>>> hw/xwin/InitOutput.c.
>>>
>>> That what is doing the patch Attached to this email.
>
>
--
Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple
Problem reports: http://cygwin.com/problems.html
Documentation: http://x.cygwin.com/docs/
FAQ: http://x.cygwin.com/docs/faq/
next prev parent reply other threads:[~2010-09-18 18:44 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <AANLkTikJdnM+_7z5qvrdyannbTUifMGodisQniJd6-GW@mail.gmail.com>
2010-08-03 7:55 ` Michel Hummel
2010-08-06 12:55 ` Michel Hummel
2010-08-08 13:14 ` Jon TURNEY
2010-09-18 18:44 ` michel hummel [this message]
2010-09-20 13:59 ` Jon TURNEY
2010-09-20 14:49 ` Michel Hummel
2010-09-22 9:03 ` Michel Hummel
2010-09-22 14:30 ` Jon TURNEY
2010-09-22 17:11 ` XWin.exe parms wrapped by startwin.exe Fouts, Christopher
2010-09-23 12:39 ` Jon TURNEY
2010-09-24 8:15 ` XWin crash after the launch of startkde on a remote Red Hat 5 machine Michel Hummel
2010-09-24 15:04 ` Michel Hummel
2010-09-28 20:12 ` Jon TURNEY
2010-09-28 21:28 ` Jim Reisert AD1C
2010-09-29 8:59 ` Michel Hummel
2010-09-29 14:05 ` Michel Hummel
2010-10-31 16:59 ` Jon TURNEY
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C950885.5080205@gmail.com \
--to=hummel.michel@gmail.com \
--cc=cygwin-xfree@cygwin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).