Re: XWin crash after the launch of startkde on a remote Red Hat 5 machine

[Michel Hummel <hummel.michel@gmail.com>]

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/