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

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

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

I found an error on the previous patch, the clipboard thread was
restarting on every X clipboard error.
I changed this by adding a test on the request_code error to restart
the thread only if the request_code equals to 24 which is the error
code for a BadWindow.
It seems to me that this is better like this.

This new version of the patch is named V2,
Sorry for the trouble.

Regards,
Michel Hummel


2010/9/24 Michel Hummel <hummel.michel@gmail.com>
>
> Hi Jon,
> 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)
>
> 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.
>
> 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?
>
> Regards,
> Michel Hummel
>
>
> 2010/9/22 Jon TURNEY <jon.turney@dronecode.org.uk>
> >
> > On 22/09/2010 10:03, Michel Hummel wrote:
> >>
> >> Hello,
> >> I've modified my patch, to change the restart process.
> >> It does not use anymore the winProcEstablishConnection wrapper to
> >> restart the clipboard but directly the winInitClipboard function.
> >>
> >> This allows to restart the clipboard more quickly and if the clipboard
> >> thread cannot connects to the server (there is a loop on connection
> >> with a delai between retries), it will die.
> >>
> >> One question :
> >> I have written my patch for the git version of the X server and the
> >> patch is not working as it on the 1.8.0-1 version.
> >> Which version would you like,  perhaps the two ?
> >
> > A patch against current git master is fine, although I don't think there have been significant changes in this area recently, so why it doesn't work for 1.8.0-1 as well is mysterious.
> >
> > --
> > Jon TURNEY
> > Volunteer Cygwin/X X Server maintainer

[-- Attachment #2: XWin_patch_restart_clipboardV2.path --]
[-- Type: application/octet-stream, Size: 14903 bytes --]

--- /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
+ */
+ 
+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");
+
+}
+/*
  * 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);
 
   return NULL;
 }
@@ -431,7 +521,7 @@ static int
 winClipboardErrorHandler (Display *pDisplay, XErrorEvent *pErr)
 {
   char pszErrorMsg[100];
-  
+
   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);
+   }
   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;

[-- Attachment #3: Type: text/plain, Size: 223 bytes --]

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