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

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