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

[Jon TURNEY <jon.turney@dronecode.org.uk>]

On 29/09/2010 15:05, Michel Hummel wrote:
> You will find attached to this Email the modifications you asked.
>
> The X server doesn't freeze anymore when clipboard restarts on xdmcp,
> certainly fixed by the cygwin patches.
>
> I added a rate-limit of clipboard_generation which disables restart after
> XWIN_CLIPBOARD_RETRIES retries (defined to 40 in winclipboard.h)
> and a sleep(XWIN_CLIPBOARD_DELAY) before retries (defined to 1 in
> winclipboard.h)
>
> I hope you will like this new version

I really appreciate your work on this set of patches, and I'm sorry it's taken 
me so long to get around to looking at them.

I must confess that the size of each patch didn't help.  You can never make 
patches too small :-).  'git gui' and 'git rebase --interactive' are excellent 
tools for splitting, merging and reordering patches.

I've merged most of it, with some tweaks and rearrangement into my latest 
snapshot (code is at [1]), and it should be included in a 1.9.1-1 release I'll 
be making shortly.

A few detailed comments:

I really want to keep fdMessageQueue under HAS_DEVWINDOWS for clarity and for 
ease of sharing patches with XMing, so I had to rearrange things a bit to 
permit that.

diff --git a/hw/xwin/winclipboardthread.c b/hw/xwin/winclipboardthread.c
index f85d8cf..5c8d0be 100644
--- a/hw/xwin/winclipboardthread.c
+++ b/hw/xwin/winclipboardthread.c
@@ -119,7 +119,7 @@ winClipboardProc (void *pvNotUsed)
    if (XInitThreads () == 0)
      {
        ErrorF ("winClipboardProc - XInitThreads failed.\n");
-      pthread_exit (NULL);
+      goto winClipboardProc_Done;
      }

    /* See if X supports the current locale */
@@ -143,7 +143,7 @@ winClipboardProc (void *pvNotUsed)
        /* setjmp returned an unknown value, exit */
        ErrorF ("winClipboardProc - setjmp returned: %d exiting\n",
               iReturn);
-      pthread_exit (NULL);
+      goto winClipboardProc_Done;
      }
    else if (iReturn == WIN_JMP_ERROR_IO)
      {

These two failure modes are unlikely to get better when we restart the thread, 
so I've tweaked these so they will be handled as a critical problem, causing 
the X server to exit.

+  /* global clipboard variable reset */
+  g_fClipboardLaunched = FALSE;
+  g_fClipboardStarted = FALSE;
+  g_iClipboardWindow = None;
+  g_hwndClipboard = NULL;
+  g_fUnicodeClipboard = FALSE;
+  g_pClipboardDisplay = NULL;

Resetting g_fUnicodeClipboard seems unlikely to be right, as that will always 
turn off unicode clipboard handling for all but the first invocation of the 
thread.

diff --git a/hw/xwin/winclipboardthread.c b/hw/xwin/winclipboardthread.c
index 2d1ecf9..437f0fa 100644
--- a/hw/xwin/winclipboardthread.c
+++ b/hw/xwin/winclipboardthread.c
@@ -503,6 +503,16 @@ winClipboardErrorHandler (Display *pDisplay, XErrorEvent 
*pErr)
           pErr->serial,
           pErr->request_code,
           pErr->minor_code);
+  if (pthread_equal(pthread_self(),g_winClipboardProcThread) && 
(pErr->request_code == 24))
+    {
+      /* We are in the clipboard thread and a bad window is detected.
+       * This means the Xwindow part of the clipboard doesn't exist anymore
+       * The call to "pthread_exit(NULL)" will restart the thread
+       */
+       ErrorF("winClipboardErrorHandler - Badwindow detected, restarting 
clipboard thread\n");
+       pthread_exit(NULL);
+    }
+
    return 0;
  }

I left this bit out for the moment.  This code doesn't seem to do what it says 
it does.  Checking for a BadWindow would be pErr->error_code == BadWindow (3). 
  I think checking pErr->request_code == 24 is checking the operation is 
X_ConvertSelection (see /usr/include/X11/Xproto.h)

I'm not sure currently works correctly at all in multiwindow mode, as 
XSetErrorHandler() also gets called by the internal WM thread, so this error 
handler may not be the one installed.

Is there a reason why we can't work out which Xlib function is actually 
failing BadWindow and handle it there?

Can you explain a bit more about the case which this helps with.   Are you 
xkill-ing the clipboard X window?

On another topic, I am still a bit concerned we have a potential race 
condition with the two uses of winInitClipboard() during a server 
regeneration.  g_fClipboardLaunched is FALSE before it's called, and set 
afterwards, but since these two calls happen in different threads, there's 
nothing I can see preventing the thread getting started twice.

It might be that having the clipboard thread to exit after setting 
g_fClipboardLaunched to FALSE if the server generation has changed creates the 
needed ordering of events (if it doesn't already exist), but it would take 
some staring at the code to be sure...

[1] http://cgit.freedesktop.org/~jturney/xserver/log/?h=snapshot

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