You will find attached to this Email the modifications you asked. The X server doesn't freeze anymore when clipboard restarts on xdmcp, certainly fixed by the cygwin patches.
I added a rate-limit of clipboard_generation which disables restart after XWIN_CLIPBOARD_RETRIES retries (defined to 40 in winclipboard.h) and a sleep(XWIN_CLIPBOARD_DELAY) before retries (defined to 1 in winclipboard.h) I hope you will like this new version, Thank you Michel Hummel 2010/9/29 Michel Hummel <[email protected]>: > 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 <[email protected]>: >> 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 >> >
XWin_patch_restart_clipboard1.9.patch
Description: Binary data
XWin_patch_remove_xdmcp_tricks1.9.patch
Description: Binary data
-- 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/
