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 > -- 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/
