XWin crash after the launch of startkde on a remote Red Hat 5 machine
Michel Hummel
hummel.michel@gmail.com
Wed Sep 29 08:59:00 GMT 2010
Thank you for your comments, I really want to help you on this part of
the X server.
I will now do :
1/ Adapt the patch to be cygwin Xorg compatible
1/ Split the patch into two separate patches
2/ Publish as soon as possible a new patches version which will take
into account your comments :
*a) WM_DESTROYCLIPBOARD message (Could you be a little more precise
concerning this, I'm not sure to realy understand the issue
*b) Adding a rate-limit on "clipboard_generation" with a static
variable for example ? and a restart delay (500 Milliseconds should be
enough isn't it ?)
*c) Change the comment about ProcEstablishConnection wrapper and inInitClipboard
*d) Merge the cleanup and the restart function into one
*e) Concerning the use of pthread_cleanup_pop at winClipboardProc end,
do you mean : Replace all pthread_exit() call's by a goto error_exit
label which will call pthread_cleanup_pop(1) (The parameter "1" means
that we have to execute it)
*f) Delete the unnecessary whitespace changes
*g) Concerning the winClipboardErrorHandler tricks, This is for the
situation you said : "Window has been deleted by someone else, but we
are still connected to the server" I will add a comment in the sources
Michel Hummel
2010/9/28 Jon TURNEY <jon.turney@dronecode.org.uk>:
> On 24/09/2010 09:15, Michel Hummel wrote:
>>
>> Hi Jon,
>
> Firstly, thanks very much for looking at this.
>
>> You will find attached to this Email a patch (for the git version)
>> which implements for the clipboard thread :
>> - Auto cleanup function
>> - Auto restart function for unexpected exit of the clipboard or
>> Xerror detection
>> - Deletion of XQuery wrapper (not needed anymore)
>> - Deletion of all the xdmcp related tricks (not needed anymore)
>
> For purposes of review it would be easier to split this patch into two
> separate patches, (i) the clipboard thread changes and (ii) removing the
> unneeded xdmcp tricks.
>
>> One thing, this patch deletes the call to EmptyClipboard () in the
>> winProcSetSelectionOwner function of the winclipboardwrappers.c file
>> when an "owned to not owned transition" has been detected. I don't
>> know why but it was freezing the server for 1 or 2 seconds during
>> clipboard's restart in xdmcp connection.
>
> Hmmm... I wonder if that's something to do with how we process the
> WM_DESTROYCLIPBOARD message? In any case, this should not be happening.
>
>> It would be fine if you could tell me, when you think this patch and
>> the previous one ( which makes the reset of the server working
>> correctly with clipboard) will be included to the official X server?
>
> We don't have a regular release cycle for the cygwin X server, so I can't
> answer that question accurately.
>
> I shall put your previous patch into the next release and assuming it works
> well, be pushing it upstream sometime before xserver 1.10
>
> I think this patch needs a little more work.
>
> I'm still a little concerned that there is no rate-limiting on the clipboard
> restart mechanism, so in a pathological case (e.g. where some unanticipated
> error causes the clipboard to constantly get disconnected), this will spin
> using all available CPU.
>
> Some detailed comments below.
>
>> ---
>> /home/hummelm/xserver-fc091936e2bddbbab9c9a501edc5a5f08388617e-org/hw/xwin/InitInput.c
>> 2010-08-18 22:10:54.000000000 +0200
>> +++ hw/xwin/InitInput.c 2010-09-17 17:03:08.611244200 +0200
>> @@ -127,12 +127,6 @@ InitInput (int argc, char *argv[])
>> winProcEstablishConnectionOrig = InitialVector[2];
>> InitialVector[2] = winProcEstablishConnection;
>> }
>> - if (g_fXdmcpEnabled
>> - && ProcVector[X_QueryTree] != winProcQueryTree)
>> - {
>> - winProcQueryTreeOrig = ProcVector[X_QueryTree];
>> - ProcVector[X_QueryTree] = winProcQueryTree;
>> - }
>> #endif
>>
>> g_pwinPointer = AddInputDevice (serverClient, winMouseProc, TRUE);
>> ---
>> /home/hummelm/xserver-fc091936e2bddbbab9c9a501edc5a5f08388617e-org/hw/xwin//winclipboardthread.c
>> 2010-08-18 22:10:54.000000000 +0200
>> +++ hw/xwin/winclipboardthread.c 2010-09-24 16:48:20.689125100
>> +0200
>> @@ -48,6 +48,8 @@
>> extern Bool g_fUnicodeClipboard;
>> extern unsigned long serverGeneration;
>> extern Bool g_fClipboardStarted;
>> +extern Bool g_fClipboardLaunched;
>> +extern Bool g_fClipboard;
>> extern HWND g_hwndClipboard;
>> extern void *g_pClipboardDisplay;
>> extern Window g_iClipboardWindow;
>> @@ -72,8 +74,113 @@ winClipboardErrorHandler (Display *pDisp
>> static int
>> winClipboardIOErrorHandler (Display *pDisplay);
>>
>> +static void
>> +restartClipboardThread(void *pvNotUsed);
>> +
>> +static void
>> +winClipboardCleanup(void *vfdMessageQueue);
>> +
>>
>> /*
>> + * restartClipboardThread activate the ProcEstablishConnection wrapper
>> which will start
>> + * the thread after next client connection
>> + */
>
> This comment doesn't appear to be correct. winInitClipboard() starts the
> thread itself.
>
> It looks like this could fight with the ProcEstablishConnection wrapper,
> which will call winInitClipboard() once for each server generation.
>
>> +
>> +static void restartClipboardThread(void *pvNotUsed)
>> +{
>> + winDebug("restartClipboardThread - enter \n");
>> + if (g_fClipboard)
>> + {
>> + ErrorF("restartClipboardThread - trying to restart clipboard thread
>> \n");
>> + /* Create the clipboard client thread */
>> + if (!winInitClipboard ())
>> + {
>> + ErrorF ("restartClipboardThread - winClipboardInit "
>> + "failed.\n");
>> + return;
>> + }
>> +
>> + winDebug ("restartClipboardThread - winInitClipboard returned.\n");
>> + /* Flag that clipboard client has been launched */
>> + g_fClipboardLaunched = TRUE;
>> + }
>> + else
>> + {
>> + ErrorF ("restartClipboardThread - Clipboard disabled - Exit from
>> server \n");
>> + return;
>> + }
>> + return;
>> +}
>> +
>> +/*
>> + * winClipboardCleanup clean thread before exit
>> + */
>> +
>> +static void winClipboardCleanup(void *vfdMessageQueue)
>> +{
>> + int iReturn;
>> + int fdMessageQueue = (int) vfdMessageQueue;
>> +
>> + winDebug("winClipboardCleanup - Enter \n");
>> + CloseClipboard();
>> + /* Close our Windows window */
>> + if (g_hwndClipboard )
>> + {
>> + /* Destroy the Window window (hwnd) */
>> + winDebug("winClipboardCleanup - Destroy Windows window\n");
>> + PostMessage (g_hwndClipboard,WM_DESTROY, 0, 0);
>> + winClipboardFlushWindowsMessageQueue(g_hwndClipboard);
>> + }
>> +
>> + /* Close our X window */
>> + if (g_pClipboardDisplay && g_iClipboardWindow)
>> + {
>> + iReturn = XDestroyWindow (g_pClipboardDisplay, g_iClipboardWindow);
>> + if (iReturn == BadWindow)
>> + ErrorF ("winClipboardCleanup - XDestroyWindow returned
>> BadWindow.\n");
>> + else
>> + winDebug ("winClipboardCleanup - winClipboardProc - XDestroyWindow
>> succeeded.\n");
>> + }
>> +
>> +
>> +#ifdef HAS_DEVWINDOWS
>> + /* Close our Win32 message handle */
>> + if (fdMessageQueue)
>> + close (fdMessageQueue);
>> +#endif
>> +
>> +#if 0
>> + /*
>> + * FIXME: XCloseDisplay hangs if we call it, as of 2004/03/26. The
>> + * XSync and XSelectInput calls did not help.
>> + */
>> +
>> + /* Discard any remaining events */
>> + XSync (g_pClipboardDisplay, TRUE);
>> +
>> + /* Select event types to watch */
>> + XSelectInput (g_pClipboardDisplay,
>> + DefaultRootWindow (g_pClipboardDisplay),
>> + None);
>> +
>> + /* Close our X display */
>> + if (g_pClipboardDisplay)
>> + {
>> + XCloseDisplay (g_pClipboardDisplay);
>> + }
>> +#endif
>> +
>> + /* global clipboard variable reset */
>> + g_fClipboardLaunched = FALSE;
>> + g_fClipboardStarted = FALSE;
>> + g_iClipboardWindow = None;
>> + g_hwndClipboard = NULL;
>> + g_fUnicodeClipboard = FALSE;
>> + g_pClipboardDisplay = NULL;
>> + winDebug("winClipboardCleanup - Exit \n");
>> +
>> +}
>
> Rather than 2 separate functions which we pthread_cleanup_push separately,
> but never use independently, can these be combined? Or create a cleanup fn
> which calls both of them, and push that?
>
>> +/*
>> * Main thread function
>> */
>>
>> @@ -84,9 +191,8 @@ winClipboardProc (void *pvNotUsed)
>> int iReturn;
>> HWND hwnd = NULL;
>> int iConnectionNumber = 0;
>> -#ifdef HAS_DEVWINDOWS
>> int fdMessageQueue = 0;
>> -#else
>> +#ifndef HAS_DEVWINDOWS
>> struct timeval tvTimeout;
>> #endif
>> fd_set fdsRead;
>> @@ -122,24 +228,6 @@ winClipboardProc (void *pvNotUsed)
>> ErrorF ("winClipboardProc - Warning: Locale not supported by X.\n");
>> }
>>
>> - /* Set jump point for Error exits */
>> - iReturn = setjmp (g_jmpEntry);
>> -
>> - /* Check if we should continue operations */
>> - if (iReturn != WIN_JMP_ERROR_IO
>> - && iReturn != WIN_JMP_OKAY)
>> - {
>> - /* setjmp returned an unknown value, exit */
>> - ErrorF ("winClipboardProc - setjmp returned: %d exiting\n",
>> - iReturn);
>> - pthread_exit (NULL);
>> - }
>> - else if (iReturn == WIN_JMP_ERROR_IO)
>> - {
>> - /* TODO: Cleanup the Win32 window and free any allocated memory */
>> - ErrorF ("winClipboardProc - setjmp returned for IO Error
>> Handler.\n");
>> - pthread_exit (NULL);
>> - }
>>
>> /* Use our generated cookie for authentication */
>> winSetAuthorization();
>> @@ -216,6 +304,25 @@ winClipboardProc (void *pvNotUsed)
>> iMaxDescriptor = iConnectionNumber + 1;
>> #endif
>>
>> + /* Adding a cleanup process for thread exit
>> + * Warning : A call to pthread_cleanup_push() implies a call to
>> pthread_cleanup_pop() at the same code level (function)
>> + * We also use it to automaticaly restart the thread on unexpected exit
>> (ie. pthread_exit() when g_pClipboard != TRUE )
>> + * this is a LIFO stack ( Last In First Out ) so adding "restart" then
>> "clean"
>> + */
>> + /* Install the restart function to be called */
>> + pthread_cleanup_push(restartClipboardThread, NULL);
>> + /* install the cleanup function to be called at thread exit */
>> + pthread_cleanup_push(winClipboardCleanup, (void*) &fdMessageQueue);
>> + /* The only way to exit from this loop is to :
>> + * 1/ Exit before the installation this cleanup process
>> + * 2/ Doing the "expected" Exit (ie. End of the function) which disable
>> the restart
>> + * by setting g_pClipboard to FALSE;
>> + * before calling the cleanup handler :
>> + * pthread_cleanup_pop(1);
>> + * then the restart handler which will be an Exit handler because
>> g_pClipboard != TRUE :
>> + * pthread_cleanup_pop(1);
>> + */
>> +
>> /* Create atoms */
>> atomClipboard = XInternAtom (pDisplay, "CLIPBOARD", False);
>> atomClipboardManager = XInternAtom (pDisplay, "CLIPBOARD_MANAGER",
>> False);
>> @@ -292,6 +399,26 @@ winClipboardProc (void *pvNotUsed)
>> /* Signal that the clipboard client has started */
>> g_fClipboardStarted = TRUE;
>>
>> +
>> + /* Set jump point for Error exits */
>> + iReturn = setjmp (g_jmpEntry);
>> +
>> + /* Check if we should continue operations */
>> + if (iReturn != WIN_JMP_ERROR_IO
>> + && iReturn != WIN_JMP_OKAY)
>> + {
>> + /* setjmp returned an unknown value, exit */
>> + ErrorF ("winClipboardProc - setjmp returned: %d exiting\n",
>> + iReturn);
>> + pthread_exit (NULL);
>> + }
>> + else if (iReturn == WIN_JMP_ERROR_IO)
>> + {
>> + /* TODO: Cleanup the Win32 window and free any allocated memory */
>> + ErrorF ("winClipboardProc - setjmp returned for IO Error
>> Handler.\n");
>> + pthread_exit (NULL);
>> + }
>> +
>> /* Loop for X events */
>> while (1)
>> {
>> @@ -377,47 +504,10 @@ winClipboardProc (void *pvNotUsed)
>> }
>> }
>>
>> - /* Close our X window */
>> - if (pDisplay && iWindow)
>> - {
>> - iReturn = XDestroyWindow (pDisplay, iWindow);
>> - if (iReturn == BadWindow)
>> - ErrorF ("winClipboardProc - XDestroyWindow returned
>> BadWindow.\n");
>> - else
>> - ErrorF ("winClipboardProc - XDestroyWindow succeeded.\n");
>> - }
>> -
>> -
>> -#ifdef HAS_DEVWINDOWS
>> - /* Close our Win32 message handle */
>> - if (fdMessageQueue)
>> - close (fdMessageQueue);
>> -#endif
>> -
>> -#if 0
>> - /*
>> - * FIXME: XCloseDisplay hangs if we call it, as of 2004/03/26. The
>> - * XSync and XSelectInput calls did not help.
>> - */
>> -
>> - /* Discard any remaining events */
>> - XSync (pDisplay, TRUE);
>> -
>> - /* Select event types to watch */
>> - XSelectInput (pDisplay,
>> - DefaultRootWindow (pDisplay),
>> - None);
>> -
>> - /* Close our X display */
>> - if (pDisplay)
>> - {
>> - XCloseDisplay (pDisplay);
>> - }
>> -#endif
>> -
>> - g_iClipboardWindow = None;
>> - g_pClipboardDisplay = NULL;
>> - g_hwndClipboard = NULL;
>> + /* disable the clipboard means thread restart function will kill the
>> server */
>> + g_fClipboard = FALSE;
>> + pthread_cleanup_pop(1);
>> + pthread_cleanup_pop(1);
>
> The use of pthread_cleanup really obscures what's going on here. It would
> be clearer just to have a label error_exit: here and goto it from the
> various places which pthread_exit...
>
>>
>> return NULL;
>> }
>> @@ -431,7 +521,7 @@ static int
>> winClipboardErrorHandler (Display *pDisplay, XErrorEvent *pErr)
>> {
>> char pszErrorMsg[100];
>> -
>> +
>
> No unnecessary whitespace changes, please.
>
>> XGetErrorText (pDisplay,
>> pErr->error_code,
>> pszErrorMsg,
>> @@ -442,6 +532,13 @@ winClipboardErrorHandler (Display *pDisp
>> pErr->serial,
>> pErr->request_code,
>> pErr->minor_code);
>> +
>> + /* If the Xerror is BadWindow Error, restart the thread */
>> + if ( pErr->request_code == 24 )
>> + {
>> + ErrorF("winClipboardErrorHandler - Badwindow detected, restarting
>> clipboard thread\n");
>> + pthread_exit(NULL);
>> + }
>
> How do we get into this situation? Window has been deleted by someone else,
> but we are still connected to the server?
>
>> return 0;
>> }
>>
>> ---
>> /home/hummelm/xserver-fc091936e2bddbbab9c9a501edc5a5f08388617e-org/hw/xwin/winclipboardwrappers.c
>> 2010-08-18 22:10:54.000000000 +0200
>> +++ hw/xwin/winclipboardwrappers.c 2010-09-22 10:06:00.232451900
>> +0200
>> @@ -53,7 +53,6 @@
>> */
>>
>> DISPATCH_PROC(winProcEstablishConnection);
>> -DISPATCH_PROC(winProcQueryTree);
>> DISPATCH_PROC(winProcSetSelectionOwner);
>>
>>
>> @@ -79,104 +78,6 @@ extern winDispatchProcPtr winProcSetSele
>>
>>
>> /*
>> - * Wrapper for internal QueryTree function.
>> - * Hides the clipboard client when it is the only client remaining.
>> - */
>> -
>> -int
>> -winProcQueryTree (ClientPtr client)
>> -{
>> - int iReturn;
>> -
>> - ErrorF ("winProcQueryTree - Hello\n");
>> -
>> - /*
>> - * This procedure is only used for initialization.
>> - * We can unwrap the original procedure at this point
>> - * so that this function is no longer called until the
>> - * server resets and the function is wrapped again.
>> - */
>> - ProcVector[X_QueryTree] = winProcQueryTreeOrig;
>> -
>> - /*
>> - * Call original function and bail if it fails.
>> - * NOTE: We must do this first, since we need XdmcpOpenDisplay
>> - * to be called before we initialize our clipboard client.
>> - */
>> - iReturn = (*winProcQueryTreeOrig) (client);
>> - if (iReturn != 0)
>> - {
>> - ErrorF ("winProcQueryTree - ProcQueryTree failed, bailing.\n");
>> - return iReturn;
>> - }
>> -
>> - /* Make errors more obvious */
>> - winProcQueryTreeOrig = NULL;
>> -
>> - /* Do nothing if clipboard is not enabled */
>> - if (!g_fClipboard)
>> - {
>> - ErrorF ("winProcQueryTree - Clipboard is not enabled, "
>> - "returning.\n");
>> - return iReturn;
>> - }
>> -
>> - /* If the clipboard client has already been started, abort */
>> - if (g_fClipboardLaunched)
>> - {
>> - ErrorF ("winProcQueryTree - Clipboard client already "
>> - "launched, returning.\n");
>> - return iReturn;
>> - }
>> -
>> - /* Startup the clipboard client if clipboard mode is being used */
>> - if (g_fXdmcpEnabled && g_fClipboard)
>> - {
>> - /*
>> - * NOTE: The clipboard client is started here for a reason:
>> - * 1) Assume you are using XDMCP (e.g. XWin -query %hostname%)
>> - * 2) If the clipboard client attaches during X Server startup,
>> - * then it becomes the "magic client" that causes the X Server
>> - * to reset if it exits.
>> - * 3) XDMCP calls KillAllClients when it starts up.
>> - * 4) The clipboard client is a client, so it is killed.
>> - * 5) The clipboard client is the "magic client", so the X Server
>> - * resets itself.
>> - * 6) This repeats ad infinitum.
>> - * 7) We avoid this by waiting until at least one client (could
>> - * be XDM, could be another client) connects, which makes it
>> - * almost certain that the clipboard client will not connect
>> - * until after XDM when using XDMCP.
>> - * 8) Unfortunately, there is another problem.
>> - * 9) XDM walks the list of windows with XQueryTree,
>> - * killing any client it finds with a window.
>> - * 10)Thus, when using XDMCP we wait until the first call
>> - * to ProcQueryTree before we startup the clipboard client.
>> - * This should prevent XDM from finding the clipboard client,
>> - * since it has not yet created a window.
>> - * 11)Startup when not using XDMCP is handled in
>> - * winProcEstablishConnection.
>> - */
>> -
>> - /* Create the clipboard client thread */
>> - if (!winInitClipboard ())
>> - {
>> - ErrorF ("winProcQueryTree - winClipboardInit "
>> - "failed.\n");
>> - return iReturn;
>> - }
>> -
>> - ErrorF ("winProcQueryTree - winInitClipboard returned.\n");
>> - }
>> -
>> - /* Flag that clipboard client has been launched */
>> - g_fClipboardLaunched = TRUE;
>> -
>> - return iReturn;
>> -}
>> -
>> -
>> -/*
>> * Wrapper for internal EstablishConnection function.
>> * Initializes internal clients that must not be started until
>> * an external client has connected.
>> @@ -217,18 +118,6 @@ winProcEstablishConnection (ClientPtr cl
>> /* Increment call count */
>> ++s_iCallCount;
>>
>> - /* Wait for CLIP_NUM_CALLS when Xdmcp is enabled */
>> - if (g_fXdmcpEnabled
>> - && !g_fClipboardLaunched
>> - && s_iCallCount < CLIP_NUM_CALLS)
>> - {
>> - if (s_iCallCount == 1) ErrorF ("winProcEstablishConnection - Xdmcp,
>> waiting to "
>> - "start clipboard client until %dth call", CLIP_NUM_CALLS);
>> - if (s_iCallCount == CLIP_NUM_CALLS - 1) ErrorF (".\n");
>> - else ErrorF (".");
>> - return (*winProcEstablishConnectionOrig) (client);
>> - }
>> -
>> /*
>> * This procedure is only used for initialization.
>> * We can unwrap the original procedure at this point
>> @@ -279,13 +168,6 @@ winProcEstablishConnection (ClientPtr cl
>> * be XDM, could be another client) connects, which makes it
>> * almost certain that the clipboard client will not connect
>> * until after XDM when using XDMCP.
>> - * 8) Unfortunately, there is another problem.
>> - * 9) XDM walks the list of windows with XQueryTree,
>> - * killing any client it finds with a window.
>> - * 10)Thus, when using XDMCP we wait until CLIP_NUM_CALLS
>> - * to ProcEstablishCeonnection before we startup the clipboard
>> - * client. This should prevent XDM from finding the clipboard
>> - * client, since it has not yet created a window.
>> */
>>
>> /* Create the clipboard client thread */
>> @@ -442,7 +324,8 @@ winProcSetSelectionOwner (ClientPtr clie
>>
>> /* Release ownership of the Windows clipboard */
>> OpenClipboard (NULL);
>> - EmptyClipboard ();
>> + /* on clipboard restart EmptyClipboard (); makes the X server to
>> freeze for 1 or 2 seconds and I don't know why */
>> + /* EmptyClipboard (); */
>> CloseClipboard ();
>>
>> goto winProcSetSelectionOwner_Done;
>
> --
> Jon TURNEY
> Volunteer Cygwin/X X Server maintainer
>
--
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/
More information about the Cygwin-xfree
mailing list