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: 1877 bytes --]

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_clipboard.path --]
[-- Type: application/octet-stream, Size: 14770 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 08:54:25.162266200 +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 will dies */
+  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,10 @@ winClipboardErrorHandler (Display *pDisp
 	  pErr->serial,
 	  pErr->request_code,
 	  pErr->minor_code);
+
+  // Xerror, restarts the thread
+   ErrorF("winClipboardErrorHandler - 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/