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

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

[Michel Hummel]

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

Hello,
I'm using cygwin on Microsoft Windows XP with XWin version :
sh-3.2# XWin.exe -version
Welcome to the XWin X Server
Vendor: The Cygwin/X Project
Release: 1.8.0.0 (10800000)
Build Date: 2010-04-02
Contact: cygwin-xfree@cygwin.com

I think I have found a bug in the XWin reset procedure which leads to
the start of two clipboard thread's and sometime to a crash of the X
server.

The crash can be produced by lauching startkde on a Red Hat 5 remote
machine (The crash should arrive after some minutes of work)
but the bug can simply be produced like this :

On my installation if I launch Xwin with this options :
XWin :0 -ac&
then I do
xsetroot -solid '#FFFF00'

After setting the color, Xwin launches its reset procedure because
there is no more client connected ( the server loses the color but it
is the normal comportment isn't it ?
http://sourceware.org/ml/cygwin-xfree/2002-01/msg00106.html)

The problem comes from the clipboard thread. In this example, when the
server launches its reset procedure, the clipboard is in state
"Launched" but is not in state "Started" (Boolean variable about
clipboard status, see file hw/xwin/winclipboardthread.c).

This particular status of the ClipboardThread during server reset is
normaly managed by two "if" statements :

* An "if" in the file hw/xwin/InitOutput.c disables the exit of the
clipboard thread in this state
Extract of the file hw/xwin/InitOutput.c (with line number) :

 169 winClipboardShutdown (void)
 170 {
 171   /* Close down clipboard resources */
 172   if (g_fClipboard && g_fClipboardLaunched && g_fClipboardStarted)
 173     {
 174       /* Synchronously destroy the clipboard window */
 175       if (g_hwndClipboard != NULL)
 176         {
 177           SendMessage (g_hwndClipboard, WM_DESTROY, 0, 0);
 178           /* NOTE: g_hwndClipboard is set to NULL in
winclipboardthread.c */
 179         }
 180       else
 181         return;
 182
 183       /* Wait for the clipboard thread to exit */
 184       pthread_join (g_ptClipboardProc, NULL);
 185
 186       g_fClipboardLaunched = FALSE;
 187       g_fClipboardStarted = FALSE;
 188
 189       winDebug ("winClipboardShutdown - Clipboard thread has exited.\n");
 190     }
 191 }

* An "if" statement in the file hw/xwin/winclipboardwrappers.c
prohibits the launch of clipboard thread if one is already Launched.
Extract of the file hw/xwin/winclipboardwrappers.c (with line number)
256   /* If the clipboard client has already been started, abort */
257   if (g_fClipboardLaunched)
258     {
259       ErrorF ("winProcEstablishConnection - Clipboard client already "
260               "launched, returning.\n");
261       return iReturn;
262     }


The problem is that the Boolean variables g_fClipboardLaunched and
g_fClipboardStarted are re-initialized by the server reset procedure
(function winInitializeGlobals of the file  hw/xwin/winglobals.c)
Extract of the file hw/xwin/winglobals.c (with line number)
128 /*
129  * Re-initialize global variables that are invalidated 130  * by a
server reset.
131  */
132
133 void
134 winInitializeGlobals (void)
135 {
136   g_dwCurrentThreadID = GetCurrentThreadId ();
137   g_hwndKeyboardFocus = NULL;
138 #ifdef XWIN_CLIPBOARD
139   g_fClipboardLaunched = FALSE;
140   g_fClipboardStarted = FALSE;
141   g_iClipboardWindow = None;
142   g_pClipboardDisplay = NULL;
143   g_atomLastOwnedSelection = None;
144   g_hwndClipboard = NULL;
145 #endif
146 }

The consequence of this Re-initialization in this particular situation
is that the clipboard thread is launched two times and sometimes leads
to a crash of the X server.
You can see the double launch of the clipboard thread at the end of
the attached log file Xwin.0.log ( 2 times the sentence :
winClipboardProc - XOpenDisplay () returned and successfully opened
the display. )

To fix this bug I purpose to remove the variables g_fClipboardLaunched
and g_fClipboardStarted of the "winInitializeGlobals (void)" function,
as their re-initializations are handled in in the files :
hw/xwin/InitOutput.c.

That what is doing the patch Attached to this email.

Regards,
Michel Hummel

[-- Attachment #2: XWin_winglobals_ClipboardThread.patch --]
[-- Type: application/octet-stream, Size: 570 bytes --]

diff -r -u -p /home/hummelm/xserver-cygwin-1.8.0-1//hw/xwin/winglobals.c ./hw/xwin/winglobals.c
--- /home/hummelm/xserver-cygwin-1.8.0-1//hw/xwin/winglobals.c	2010-04-02 11:15:26.000000000 +0200
+++ ./hw/xwin/winglobals.c	2010-06-29 14:12:54.572239300 +0200
@@ -136,8 +136,6 @@ winInitializeGlobals (void)
   g_dwCurrentThreadID = GetCurrentThreadId ();
   g_hwndKeyboardFocus = NULL;
 #ifdef XWIN_CLIPBOARD
-  g_fClipboardLaunched = FALSE;
-  g_fClipboardStarted = FALSE;
   g_iClipboardWindow = None;
   g_pClipboardDisplay = NULL;
   g_atomLastOwnedSelection = None;

[-- Attachment #3: XWin.0.log --]
[-- Type: application/octet-stream, Size: 5054 bytes --]

Welcome to the XWin X Server
Vendor: The Cygwin/X Project
Release: 1.8.0.0 (10800000)
Build Date: 2010-04-02

Contact: cygwin-xfree@cygwin.com
XWin was started with the following command line:

XWin -ac :0 

ddxProcessArgument - Initializing default screens
winInitializeDefaultScreens - primary monitor w 1280 h 1024
winInitializeDefaultScreens - native DPI x 96 y 96
winInitializeDefaultScreens - Returning
[ 16410.531] _XSERVTransSocketOpenCOTSServer: Unable to open socket for inet6
[ 16410.531] _XSERVTransOpen: transport open failed for inet6/PC-2118593:0
[ 16410.531] _XSERVTransMakeAllCOTSServerListeners: failed to open listener for inet6
[ 16410.812] winValidateArgs - g_iNumScreens: 1 iMaxConsecutiveScreen: 1
[ 16410.812] (II) xorg.conf is not supported
[ 16410.812] (II) See http://x.cygwin.com/docs/faq/cygwin-x-faq.html for more information
[ 16410.828] LoadPreferences: /home/hummelm/.XWinrc not found
[ 16410.828] LoadPreferences: Loading /etc/X11/system.XWinrc
[ 16410.828] LoadPreferences: Done parsing the configuration file...
[ 16410.828] winGetDisplay: DISPLAY=:0.0
[ 16410.828] winDetectSupportedEngines - Windows NT/2000/XP
[ 16410.843] winDetectSupportedEngines - DirectDraw installed
[ 16410.843] winDetectSupportedEngines - DirectDraw4 installed
[ 16410.843] winDetectSupportedEngines - Returning, supported engines 00000007
[ 16410.843] winSetEngine - Using Shadow DirectDraw NonLocking
[ 16410.843] winAdjustVideoModeShadowDDNL - Using Windows display depth of 32 bits per pixel
[ 16410.906] winFinishScreenInitFB - Masks: 00ff0000 0000ff00 000000ff
[ 16410.906] Screen 0 added at virtual desktop coordinate (1280,0).
[ 16410.906] MIT-SHM extension disabled due to lack of kernel support
[ 16410.937] XFree86-Bigfont extension local-client optimization disabled due to lack of shared memory support in the kernel
[ 16410.968] (II) AIGLX: Loaded and initialized /usr/lib/dri/swrast_dri.so
[ 16410.968] (II) GLX: Initialized DRISWRAST GL provider for screen 0
[ 16411.796] winPointerWarpCursor - Discarding first warp: 637 481
[ 16411.796] (--) 3 mouse buttons found
[ 16411.796] (--) Setting autorepeat to delay=500, rate=31
[ 16411.796] (--) winConfigKeyboard - Layout: "0000040C" (0000040c) 
[ 16411.796] (--) Using preset keyboard for "French (Standard)" (40c), type "4"
[ 16411.796] Rules = "base" Model = "pc105" Layout = "fr" Variant = "none" Options = "none"
[ 16471.687] winProcEstablishConnection - Hello
[ 16471.687] winInitClipboard ()
[ 16471.687] winProcEstablishConnection - winInitClipboard returned.
[ 16471.687] winClipboardProc - Hello
[ 16471.687] DetectUnicodeSupport - Windows NT/2000/XP
[ 16471.718] winGetDisplay: DISPLAY=:0.0
[ 16471.718] winClipboardProc - DISPLAY=:0.0
[ 16471.750] (II) xorg.conf is not supported
[ 16471.750] (II) See http://x.cygwin.com/docs/faq/cygwin-x-faq.html for more information
[ 16471.750] LoadPreferences: /home/hummelm/.XWinrc not found
[ 16471.750] LoadPreferences: Loading /etc/X11/system.XWinrc
[ 16471.750] LoadPreferences: Done parsing the configuration file...
[ 16471.750] winGetDisplay: DISPLAY=:0.0
[ 16471.750] winDetectSupportedEngines - Windows NT/2000/XP
[ 16471.765] winDetectSupportedEngines - DirectDraw installed
[ 16471.765] winDetectSupportedEngines - DirectDraw4 installed
[ 16471.765] winDetectSupportedEngines - Returning, supported engines 00000007
[ 16471.765] winSetEngine - Using Shadow DirectDraw NonLocking
[ 16471.812] winFinishScreenInitFB - Masks: 00ff0000 0000ff00 000000ff
[ 16471.812] Screen 0 added at virtual desktop coordinate (1280,0).
[ 16471.812] MIT-SHM extension disabled due to lack of kernel support
[ 16471.812] winClipboardProc - Could not open display, try: 1, sleeping: 4
[ 16471.843] XFree86-Bigfont extension local-client optimization disabled due to lack of shared memory support in the kernel
[ 16471.875] (II) AIGLX: Loaded and initialized /usr/lib/dri/swrast_dri.so
[ 16471.875] (II) GLX: Initialized DRISWRAST GL provider for screen 0
[ 16472.640] (--) 3 mouse buttons found
[ 16472.640] (--) Setting autorepeat to delay=500, rate=31
[ 16472.640] (--) winConfigKeyboard - Layout: "0000040C" (0000040c) 
[ 16472.640] (--) Using preset keyboard for "French (Standard)" (40c), type "4"
[ 16472.640] Rules = "base" Model = "pc105" Layout = "fr" Variant = "none" Options = "none"
[ 16473.328] _XSERVTransSocketUNIXAccept: accept() failed
[ 16473.328] winInitClipboard ()
[ 16473.328] winClipboardProc - Hello
[ 16473.328] winProcEstablishConnection - winInitClipboard returned.
[ 16473.328] DetectUnicodeSupport - Windows NT/2000/XP
[ 16473.328] winGetDisplay: DISPLAY=:0.0
[ 16473.328] winClipboardProc - DISPLAY=:0.0
[ 16473.343] winClipboardProc - XOpenDisplay () returned and successfully opened the display.
[ 16473.343] winClipboardProc - XOpenDisplay () returned and successfully opened the display.
[ 16516.375] winClipboardProc - winClipboardFlushWindowsMessageQueue trapped WM_QUIT message, exiting main loop.
[ 16516.375] winClipboardProc - XDestroyWindow succeeded.
[ 16516.390] winClipboardIOErrorHandler!


[-- Attachment #4: 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/

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

[Michel Hummel]

2010/8/3 Michel Hummel <hummel.michel@gmail.com>:
> Hello,
> I'm using cygwin on Microsoft Windows XP with XWin version :
> sh-3.2# XWin.exe -version
> Welcome to the XWin X Server
> Vendor: The Cygwin/X Project
> Release: 1.8.0.0 (10800000)
> Build Date: 2010-04-02
> Contact: cygwin-xfree@cygwin.com
>
> I think I have found a bug in the XWin reset procedure which leads to
> the start of two clipboard thread's and sometime to a crash of the X
> server.
>
> The crash can be produced by lauching startkde on a Red Hat 5 remote
> machine (The crash should arrive after some minutes of work)
> but the bug can simply be produced like this :
>
> On my installation if I launch Xwin with this options :
> XWin :0 -ac&
> then I do
> xsetroot -solid '#FFFF00'
>
> After setting the color, Xwin launches its reset procedure because
> there is no more client connected ( the server loses the color but it
> is the normal comportment isn't it ?
> http://sourceware.org/ml/cygwin-xfree/2002-01/msg00106.html)
>
> The problem comes from the clipboard thread. In this example, when the
> server launches its reset procedure, the clipboard is in state
> "Launched" but is not in state "Started" (Boolean variable about
> clipboard status, see file hw/xwin/winclipboardthread.c).
>
> This particular status of the ClipboardThread during server reset is
> normaly managed by two "if" statements :
>
> * An "if" in the file hw/xwin/InitOutput.c disables the exit of the
> clipboard thread in this state
> Extract of the file hw/xwin/InitOutput.c (with line number) :
>
>  169 winClipboardShutdown (void)
>  170 {
>  171   /* Close down clipboard resources */
>  172   if (g_fClipboard && g_fClipboardLaunched && g_fClipboardStarted)
>  173     {
>  174       /* Synchronously destroy the clipboard window */
>  175       if (g_hwndClipboard != NULL)
>  176         {
>  177           SendMessage (g_hwndClipboard, WM_DESTROY, 0, 0);
>  178           /* NOTE: g_hwndClipboard is set to NULL in
> winclipboardthread.c */
>  179         }
>  180       else
>  181         return;
>  182
>  183       /* Wait for the clipboard thread to exit */
>  184       pthread_join (g_ptClipboardProc, NULL);
>  185
>  186       g_fClipboardLaunched = FALSE;
>  187       g_fClipboardStarted = FALSE;
>  188
>  189       winDebug ("winClipboardShutdown - Clipboard thread has exited.\n");
>  190     }
>  191 }
>
> * An "if" statement in the file hw/xwin/winclipboardwrappers.c
> prohibits the launch of clipboard thread if one is already Launched.
> Extract of the file hw/xwin/winclipboardwrappers.c (with line number)
> 256   /* If the clipboard client has already been started, abort */
> 257   if (g_fClipboardLaunched)
> 258     {
> 259       ErrorF ("winProcEstablishConnection - Clipboard client already "
> 260               "launched, returning.\n");
> 261       return iReturn;
> 262     }
>
>
> The problem is that the Boolean variables g_fClipboardLaunched and
> g_fClipboardStarted are re-initialized by the server reset procedure
> (function winInitializeGlobals of the file  hw/xwin/winglobals.c)
> Extract of the file hw/xwin/winglobals.c (with line number)
> 128 /*
> 129  * Re-initialize global variables that are invalidated 130  * by a
> server reset.
> 131  */
> 132
> 133 void
> 134 winInitializeGlobals (void)
> 135 {
> 136   g_dwCurrentThreadID = GetCurrentThreadId ();
> 137   g_hwndKeyboardFocus = NULL;
> 138 #ifdef XWIN_CLIPBOARD
> 139   g_fClipboardLaunched = FALSE;
> 140   g_fClipboardStarted = FALSE;
> 141   g_iClipboardWindow = None;
> 142   g_pClipboardDisplay = NULL;
> 143   g_atomLastOwnedSelection = None;
> 144   g_hwndClipboard = NULL;
> 145 #endif
> 146 }
>
> The consequence of this Re-initialization in this particular situation
> is that the clipboard thread is launched two times and sometimes leads
> to a crash of the X server.
> You can see the double launch of the clipboard thread at the end of
> the attached log file Xwin.0.log ( 2 times the sentence :
> winClipboardProc - XOpenDisplay () returned and successfully opened
> the display. )
>
> To fix this bug I purpose to remove the variables g_fClipboardLaunched
> and g_fClipboardStarted of the "winInitializeGlobals (void)" function,
> as their re-initializations are handled in in the files :
> hw/xwin/InitOutput.c.
>
> That what is doing the patch Attached to this email.
>
> Regards,
> Michel Hummel
>

Hello,
I didn't saw any response to my patch proposition, did someone tried it ?
If I didn't posted it in the good place where should I have to

I will be happy to explain the problem if my first mail wasn't clear.

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

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

[Jon TURNEY]

On 06/08/2010 13:55, Michel Hummel wrote:
 > I didn't saw any response to my patch proposition, did someone tried it ?
 > If I didn't posted it in the good place where should I have to

Yes, this is abolutely the right place for your patch.

It looks good and I shall incorporate it in the next X server release.

 > I will be happy to explain the problem if my first mail wasn't clear.

Nope, the analysis was very clear, thank you.

I wonder if there also exists a race condition when the clipboard thread is 
stopping? i.e. if we try to start a new one just as the old one is stopping, 
we think it is still running and fail to do so?

In general this code could do with a tidy up: I think it would be much more 
sensible to have a long-lived thread which tries to reconnect to the server 
when ever it gets disconnected, it would avoid all this complexity and the 
complexity of trying to avoid being killed by GDM as well.

> 2010/8/3 Michel Hummel<hummel.michel@gmail.com>:
>> Hello,
>> I'm using cygwin on Microsoft Windows XP with XWin version :
>> sh-3.2# XWin.exe -version
>> Welcome to the XWin X Server
>> Vendor: The Cygwin/X Project
>> Release: 1.8.0.0 (10800000)
>> Build Date: 2010-04-02
>> Contact: cygwin-xfree@cygwin.com
>>
>> I think I have found a bug in the XWin reset procedure which leads to
>> the start of two clipboard thread's and sometime to a crash of the X
>> server.
>>
>> The crash can be produced by lauching startkde on a Red Hat 5 remote
>> machine (The crash should arrive after some minutes of work)
>> but the bug can simply be produced like this :
>>
>> On my installation if I launch Xwin with this options :
>> XWin :0 -ac&
>> then I do
>> xsetroot -solid '#FFFF00'
>>
>> After setting the color, Xwin launches its reset procedure because
>> there is no more client connected ( the server loses the color but it
>> is the normal comportment isn't it ?
>> http://sourceware.org/ml/cygwin-xfree/2002-01/msg00106.html)
>>
>> The problem comes from the clipboard thread. In this example, when the
>> server launches its reset procedure, the clipboard is in state
>> "Launched" but is not in state "Started" (Boolean variable about
>> clipboard status, see file hw/xwin/winclipboardthread.c).
>>
>> This particular status of the ClipboardThread during server reset is
>> normaly managed by two "if" statements :
>>
>> * An "if" in the file hw/xwin/InitOutput.c disables the exit of the
>> clipboard thread in this state
>> Extract of the file hw/xwin/InitOutput.c (with line number) :
>>
>>   169 winClipboardShutdown (void)
>>   170 {
>>   171   /* Close down clipboard resources */
>>   172   if (g_fClipboard&&  g_fClipboardLaunched&&  g_fClipboardStarted)
>>   173     {
>>   174       /* Synchronously destroy the clipboard window */
>>   175       if (g_hwndClipboard != NULL)
>>   176         {
>>   177           SendMessage (g_hwndClipboard, WM_DESTROY, 0, 0);
>>   178           /* NOTE: g_hwndClipboard is set to NULL in
>> winclipboardthread.c */
>>   179         }
>>   180       else
>>   181         return;
>>   182
>>   183       /* Wait for the clipboard thread to exit */
>>   184       pthread_join (g_ptClipboardProc, NULL);
>>   185
>>   186       g_fClipboardLaunched = FALSE;
>>   187       g_fClipboardStarted = FALSE;
>>   188
>>   189       winDebug ("winClipboardShutdown - Clipboard thread has exited.\n");
>>   190     }
>>   191 }
>>
>> * An "if" statement in the file hw/xwin/winclipboardwrappers.c
>> prohibits the launch of clipboard thread if one is already Launched.
>> Extract of the file hw/xwin/winclipboardwrappers.c (with line number)
>> 256   /* If the clipboard client has already been started, abort */
>> 257   if (g_fClipboardLaunched)
>> 258     {
>> 259       ErrorF ("winProcEstablishConnection - Clipboard client already "
>> 260               "launched, returning.\n");
>> 261       return iReturn;
>> 262     }
>>
>>
>> The problem is that the Boolean variables g_fClipboardLaunched and
>> g_fClipboardStarted are re-initialized by the server reset procedure
>> (function winInitializeGlobals of the file  hw/xwin/winglobals.c)
>> Extract of the file hw/xwin/winglobals.c (with line number)
>> 128 /*
>> 129  * Re-initialize global variables that are invalidated 130  * by a
>> server reset.
>> 131  */
>> 132
>> 133 void
>> 134 winInitializeGlobals (void)
>> 135 {
>> 136   g_dwCurrentThreadID = GetCurrentThreadId ();
>> 137   g_hwndKeyboardFocus = NULL;
>> 138 #ifdef XWIN_CLIPBOARD
>> 139   g_fClipboardLaunched = FALSE;
>> 140   g_fClipboardStarted = FALSE;
>> 141   g_iClipboardWindow = None;
>> 142   g_pClipboardDisplay = NULL;
>> 143   g_atomLastOwnedSelection = None;
>> 144   g_hwndClipboard = NULL;
>> 145 #endif
>> 146 }
>>
>> The consequence of this Re-initialization in this particular situation
>> is that the clipboard thread is launched two times and sometimes leads
>> to a crash of the X server.
>> You can see the double launch of the clipboard thread at the end of
>> the attached log file Xwin.0.log ( 2 times the sentence :
>> winClipboardProc - XOpenDisplay () returned and successfully opened
>> the display. )
>>
>> To fix this bug I purpose to remove the variables g_fClipboardLaunched
>> and g_fClipboardStarted of the "winInitializeGlobals (void)" function,
>> as their re-initializations are handled in in the files :
>> hw/xwin/InitOutput.c.
>>
>> That what is doing the patch Attached to this email.


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

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

[michel hummel]

Hi,
I have checked the code of the clipboard thread and the race condition
you talk about should not be a problem.
Before to start a new clipboard thread, the code waits for the end of
the first thread (pthread_join) then fixe the variable
g_fClipboardLaunched = FALSE;
g_fClipboardStarted = FALSE;

So the new thread will never be launched before the old one has terminated.

I have a proposition of modification to make the clipboard thread more robust.

1/ Adding a cleanup function which will be automaticaly called at the
end of  the thread.
   I purpose the use of the macro pthread_cleanup_push() pthread_cleanup_pop()

2/ You said that having a long-lived thread will be a good solution to
simplify the code.
   I have an other proposition which needs less modification and can
simplify the code :
   By using also here the macro pthread_cleanup_push() we can
automaticaly restart the thread on every unexpected exit.
   We just have to delete this restart when the exit is expected (ie.
End of the function)
   With this solution we don't have to take care about the thread
being killed by anyone, it will be restarted.

3/ An other thing I saw is that when the Xwindow part of the clipboard
is killed by an other application, the thread will still be alive but
the clipboard doesn't work anymore.
   A solution can be :
           when an winClipboardErrorHandler() is catched, check for
this Xwindow window and whenever this window doesn't exist anymore,
create a new one.

What do you think about my suggestions ?

I tested them and they seem to work as expected.
For example, I skipped the wrapper of XQueryTree (actualy needed for
xdmcp) and the clipboard is now working with xdmcp/gdm, etc.

If you don't like something in my propositions, please tell me what
and I will try to adapt it.

If you are interested in a patch doing this, I will be happy to give
it after a quick review and a clean

Thks
Michel Hummel


Le 08/08/2010 15:14, Jon TURNEY a écrit :
> On 06/08/2010 13:55, Michel Hummel wrote:
> > I didn't saw any response to my patch proposition, did someone tried 
> it ?
> > If I didn't posted it in the good place where should I have to
>
> Yes, this is abolutely the right place for your patch.
>
> It looks good and I shall incorporate it in the next X server release.
>
> > I will be happy to explain the problem if my first mail wasn't clear.
>
> Nope, the analysis was very clear, thank you.
>
> I wonder if there also exists a race condition when the clipboard 
> thread is stopping? i.e. if we try to start a new one just as the old 
> one is stopping, we think it is still running and fail to do so?
>
> In general this code could do with a tidy up: I think it would be much 
> more sensible to have a long-lived thread which tries to reconnect to 
> the server when ever it gets disconnected, it would avoid all this 
> complexity and the complexity of trying to avoid being killed by GDM 
> as well.
>
>> 2010/8/3 Michel Hummel:
>>> Hello,
>>> I'm using cygwin on Microsoft Windows XP with XWin version :
>>> sh-3.2# XWin.exe -version
>>> Welcome to the XWin X Server
>>> Vendor: The Cygwin/X Project
>>> Release: 1.8.0.0 (10800000)
>>> Build Date: 2010-04-02
>>> Contact:
>>>
>>> I think I have found a bug in the XWin reset procedure which leads to
>>> the start of two clipboard thread's and sometime to a crash of the X
>>> server.
>>>
>>> The crash can be produced by lauching startkde on a Red Hat 5 remote
>>> machine (The crash should arrive after some minutes of work)
>>> but the bug can simply be produced like this :
>>>
>>> On my installation if I launch Xwin with this options :
>>> XWin :0 -ac&
>>> then I do
>>> xsetroot -solid '#FFFF00'
>>>
>>> After setting the color, Xwin launches its reset procedure because
>>> there is no more client connected ( the server loses the color but it
>>> is the normal comportment isn't it ?
>>> http://sourceware.org/ml/cygwin-xfree/2002-01/msg00106.html)
>>>
>>> The problem comes from the clipboard thread. In this example, when the
>>> server launches its reset procedure, the clipboard is in state
>>> "Launched" but is not in state "Started" (Boolean variable about
>>> clipboard status, see file hw/xwin/winclipboardthread.c).
>>>
>>> This particular status of the ClipboardThread during server reset is
>>> normaly managed by two "if" statements :
>>>
>>> * An "if" in the file hw/xwin/InitOutput.c disables the exit of the
>>> clipboard thread in this state
>>> Extract of the file hw/xwin/InitOutput.c (with line number) :
>>>
>>>   169 winClipboardShutdown (void)
>>>   170 {
>>>   171   /* Close down clipboard resources */
>>>   172   if (g_fClipboard&&  g_fClipboardLaunched&&  
>>> g_fClipboardStarted)
>>>   173     {
>>>   174       /* Synchronously destroy the clipboard window */
>>>   175       if (g_hwndClipboard != NULL)
>>>   176         {
>>>   177           SendMessage (g_hwndClipboard, WM_DESTROY, 0, 0);
>>>   178           /* NOTE: g_hwndClipboard is set to NULL in
>>> winclipboardthread.c */
>>>   179         }
>>>   180       else
>>>   181         return;
>>>   182
>>>   183       /* Wait for the clipboard thread to exit */
>>>   184       pthread_join (g_ptClipboardProc, NULL);
>>>   185
>>>   186       g_fClipboardLaunched = FALSE;
>>>   187       g_fClipboardStarted = FALSE;
>>>   188
>>>   189       winDebug ("winClipboardShutdown - Clipboard thread has 
>>> exited.\n");
>>>   190     }
>>>   191 }
>>>
>>> * An "if" statement in the file hw/xwin/winclipboardwrappers.c
>>> prohibits the launch of clipboard thread if one is already Launched.
>>> Extract of the file hw/xwin/winclipboardwrappers.c (with line number)
>>> 256   /* If the clipboard client has already been started, abort */
>>> 257   if (g_fClipboardLaunched)
>>> 258     {
>>> 259       ErrorF ("winProcEstablishConnection - Clipboard client 
>>> already "
>>> 260               "launched, returning.\n");
>>> 261       return iReturn;
>>> 262     }
>>>
>>>
>>> The problem is that the Boolean variables g_fClipboardLaunched and
>>> g_fClipboardStarted are re-initialized by the server reset procedure
>>> (function winInitializeGlobals of the file  hw/xwin/winglobals.c)
>>> Extract of the file hw/xwin/winglobals.c (with line number)
>>> 128 /*
>>> 129  * Re-initialize global variables that are invalidated 130  * by a
>>> server reset.
>>> 131  */
>>> 132
>>> 133 void
>>> 134 winInitializeGlobals (void)
>>> 135 {
>>> 136   g_dwCurrentThreadID = GetCurrentThreadId ();
>>> 137   g_hwndKeyboardFocus = NULL;
>>> 138 #ifdef XWIN_CLIPBOARD
>>> 139   g_fClipboardLaunched = FALSE;
>>> 140   g_fClipboardStarted = FALSE;
>>> 141   g_iClipboardWindow = None;
>>> 142   g_pClipboardDisplay = NULL;
>>> 143   g_atomLastOwnedSelection = None;
>>> 144   g_hwndClipboard = NULL;
>>> 145 #endif
>>> 146 }
>>>
>>> The consequence of this Re-initialization in this particular situation
>>> is that the clipboard thread is launched two times and sometimes leads
>>> to a crash of the X server.
>>> You can see the double launch of the clipboard thread at the end of
>>> the attached log file Xwin.0.log ( 2 times the sentence :
>>> winClipboardProc - XOpenDisplay () returned and successfully opened
>>> the display. )
>>>
>>> To fix this bug I purpose to remove the variables g_fClipboardLaunched
>>> and g_fClipboardStarted of the "winInitializeGlobals (void)" function,
>>> as their re-initializations are handled in in the files :
>>> hw/xwin/InitOutput.c.
>>>
>>> That what is doing the patch Attached to this email.
>
>


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

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

[Jon TURNEY]

On 18/09/2010 19:44, michel hummel wrote:
> I have checked the code of the clipboard thread and the race condition
> you talk about should not be a problem.
> Before to start a new clipboard thread, the code waits for the end of
> the first thread (pthread_join) then fixe the variable
> g_fClipboardLaunched = FALSE;
> g_fClipboardStarted = FALSE;
>
> So the new thread will never be launched before the old one has terminated.
>
> I have a proposition of modification to make the clipboard thread more robust.
>
> 1/ Adding a cleanup function which will be automaticaly called at the
> end of the thread.
> I purpose the use of the macro pthread_cleanup_push() pthread_cleanup_pop()
>
> 2/ You said that having a long-lived thread will be a good solution to
> simplify the code.
> I have an other proposition which needs less modification and can
> simplify the code :
> By using also here the macro pthread_cleanup_push() we can
> automaticaly restart the thread on every unexpected exit.
> We just have to delete this restart when the exit is expected (ie.
> End of the function)
> With this solution we don't have to take care about the thread
> being killed by anyone, it will be restarted.

You will need to have some kind of back-off mechanism (i.e. a delay before 
retrying to connect) so that that this doesn't constantly try to connect if 
the server gets into a state where it can't accept the connection or the 
connection is constantly getting closed.

> 3/ An other thing I saw is that when the Xwindow part of the clipboard
> is killed by an other application, the thread will still be alive but
> the clipboard doesn't work anymore.
> A solution can be :
> when an winClipboardErrorHandler() is catched, check for
> this Xwindow window and whenever this window doesn't exist anymore,
> create a new one.

Would it not be simpler to restart the clipboard thread in this situation as 
well, rather than having code to recrate the window?

> What do you think about my suggestions ?
>
> I tested them and they seem to work as expected.
> For example, I skipped the wrapper of XQueryTree (actualy needed for
> xdmcp) and the clipboard is now working with xdmcp/gdm, etc.
>
> If you don't like something in my propositions, please tell me what
> and I will try to adapt it.
>
> If you are interested in a patch doing this, I will be happy to give
> it after a quick review and a clean

That would be great, thank you.

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

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

[Michel Hummel]

2010/9/20 Jon TURNEY <jon.turney@dronecode.org.uk>:
> On 18/09/2010 19:44, michel hummel wrote:
>>
>> I have checked the code of the clipboard thread and the race condition
>> you talk about should not be a problem.
>> Before to start a new clipboard thread, the code waits for the end of
>> the first thread (pthread_join) then fixe the variable
>> g_fClipboardLaunched = FALSE;
>> g_fClipboardStarted = FALSE;
>>
>> So the new thread will never be launched before the old one has
>> terminated.
>>
>> I have a proposition of modification to make the clipboard thread more
>> robust.
>>
>> 1/ Adding a cleanup function which will be automaticaly called at the
>> end of the thread.
>> I purpose the use of the macro pthread_cleanup_push()
>> pthread_cleanup_pop()
>>
>> 2/ You said that having a long-lived thread will be a good solution to
>> simplify the code.
>> I have an other proposition which needs less modification and can
>> simplify the code :
>> By using also here the macro pthread_cleanup_push() we can
>> automaticaly restart the thread on every unexpected exit.
>> We just have to delete this restart when the exit is expected (ie.
>> End of the function)
>> With this solution we don't have to take care about the thread
>> being killed by anyone, it will be restarted.
>
> You will need to have some kind of back-off mechanism (i.e. a delay before
> retrying to connect) so that that this doesn't constantly try to connect if
> the server gets into a state where it can't accept the connection or the
> connection is constantly getting closed.

Like I’ve implemented it :
- The clipboard thread uses the winProcEstablishConnection wrapper to
restart and so it’s waiting for a new client connection before trying
to reconnect himself.
- An other point, the restart mechanism will only be activated if the
clipboard thread has successfully been connected, otherwise the
clipboard thread will dies and not restarts (this will prevent an
infinite loop on connection error)
Those two mechanisms and the loop on  XOpenDisplay with the sleep
(WIN_CONNECT_DELAY) between each retries should do the job isn't it ?
But if you think a connection delay is also needed in the restart
process can add one without problem

>
>> 3/ An other thing I saw is that when the Xwindow part of the clipboard
>> is killed by an other application, the thread will still be alive but
>> the clipboard doesn't work anymore.
>> A solution can be :
>> when an winClipboardErrorHandler() is catched, check for
>> this Xwindow window and whenever this window doesn't exist anymore,
>> create a new one.
>
> Would it not be simpler to restart the clipboard thread in this situation as
> well, rather than having code to recrate the window?

You are absolutely right, I just have to replace the window creation
with a pthread_exit(NULL);


>
>> What do you think about my suggestions ?
>>
>> I tested them and they seem to work as expected.
>> For example, I skipped the wrapper of XQueryTree (actualy needed for
>> xdmcp) and the clipboard is now working with xdmcp/gdm, etc.
>>
>> If you don't like something in my propositions, please tell me what
>> and I will try to adapt it.
>>
>> If you are interested in a patch doing this, I will be happy to give
>> it after a quick review and a clean
>
> That would be great, thank you.
>
> --
> 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/

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

[Michel Hummel]

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 ?

Michel Hummel

2010/9/20 Michel Hummel <hummel.michel@gmail.com>:
> 2010/9/20 Jon TURNEY <jon.turney@dronecode.org.uk>:
>> On 18/09/2010 19:44, michel hummel wrote:
>>>
>>> I have checked the code of the clipboard thread and the race condition
>>> you talk about should not be a problem.
>>> Before to start a new clipboard thread, the code waits for the end of
>>> the first thread (pthread_join) then fixe the variable
>>> g_fClipboardLaunched = FALSE;
>>> g_fClipboardStarted = FALSE;
>>>
>>> So the new thread will never be launched before the old one has
>>> terminated.
>>>
>>> I have a proposition of modification to make the clipboard thread more
>>> robust.
>>>
>>> 1/ Adding a cleanup function which will be automaticaly called at the
>>> end of the thread.
>>> I purpose the use of the macro pthread_cleanup_push()
>>> pthread_cleanup_pop()
>>>
>>> 2/ You said that having a long-lived thread will be a good solution to
>>> simplify the code.
>>> I have an other proposition which needs less modification and can
>>> simplify the code :
>>> By using also here the macro pthread_cleanup_push() we can
>>> automaticaly restart the thread on every unexpected exit.
>>> We just have to delete this restart when the exit is expected (ie.
>>> End of the function)
>>> With this solution we don't have to take care about the thread
>>> being killed by anyone, it will be restarted.
>>
>> You will need to have some kind of back-off mechanism (i.e. a delay before
>> retrying to connect) so that that this doesn't constantly try to connect if
>> the server gets into a state where it can't accept the connection or the
>> connection is constantly getting closed.
>
> Like I’ve implemented it :
> - The clipboard thread uses the winProcEstablishConnection wrapper to
> restart and so it’s waiting for a new client connection before trying
> to reconnect himself.
> - An other point, the restart mechanism will only be activated if the
> clipboard thread has successfully been connected, otherwise the
> clipboard thread will dies and not restarts (this will prevent an
> infinite loop on connection error)
> Those two mechanisms and the loop on  XOpenDisplay with the sleep
> (WIN_CONNECT_DELAY) between each retries should do the job isn't it ?
> But if you think a connection delay is also needed in the restart
> process can add one without problem
>
>>
>>> 3/ An other thing I saw is that when the Xwindow part of the clipboard
>>> is killed by an other application, the thread will still be alive but
>>> the clipboard doesn't work anymore.
>>> A solution can be :
>>> when an winClipboardErrorHandler() is catched, check for
>>> this Xwindow window and whenever this window doesn't exist anymore,
>>> create a new one.
>>
>> Would it not be simpler to restart the clipboard thread in this situation as
>> well, rather than having code to recrate the window?
>
> You are absolutely right, I just have to replace the window creation
> with a pthread_exit(NULL);
>
>
>>
>>> What do you think about my suggestions ?
>>>
>>> I tested them and they seem to work as expected.
>>> For example, I skipped the wrapper of XQueryTree (actualy needed for
>>> xdmcp) and the clipboard is now working with xdmcp/gdm, etc.
>>>
>>> If you don't like something in my propositions, please tell me what
>>> and I will try to adapt it.
>>>
>>> If you are interested in a patch doing this, I will be happy to give
>>> it after a quick review and a clean
>>
>> That would be great, thank you.
>>
>> --
>> 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/

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

[Jon TURNEY]

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

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

XWin.exe parms wrapped by startwin.exe

[Fouts, Christopher]

I want to run XWin much like startwin does, but with extra parameters
(-logfile). What is the exact command+parms line that startwin executes
to run XWin? So...

Startwin.exe = Xwin.exe parm1 parm2 parm3 etc

What are parm1, parm2, parm3, etc...?

Thanks,
Chris

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

Re: XWin.exe parms wrapped by startwin.exe

[Jon TURNEY]

On 22/09/2010 18:11, Fouts, Christopher wrote:
> I want to run XWin much like startwin does, but with extra parameters
> (-logfile). What is the exact command+parms line that startwin executes
> to run XWin? So...
>
> Startwin.exe = Xwin.exe parm1 parm2 parm3 etc
>
> What are parm1, parm2, parm3, etc...?

Hmm... It's a bit of an omission from the startxwin man page [1], that it 
doesn't actually tell you it's adding '-multiwindow'

However, reading that man page does tell you the syntax for adding extra 
server arguments (e.g. -- -logfile file) to your startxwin invokation.

Note that invoking the server directly will not do the other things that 
startxwin does (i.e. waiting until the server is started and then executing 
~/.startxwinrc)

In future, please start a new email thread, rather than replying to an 
existing one and changing the subject.

[1] http://x.cygwin.com/docs/man1/startxwin.1.html

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

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

[Michel Hummel]

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

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

[Michel Hummel]

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

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

[Jon TURNEY]

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/

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

[Jim Reisert AD1C]

On Tue, Sep 28, 2010 at 2:12 PM, Jon TURNEY wrote:

> You will find attached to this Email a patch (for the git version)
> which implements for the clipboard thread :

Hi Jon,

Can I assume that when you announce a patched version of XWin.exe for
testing, that the test version includes all of the prior patches, up
to and including the one being tested?

Thanks - Jim

-- 
Jim Reisert AD1C, <jjreisert@alum.mit.edu>, http://www.ad1c.us

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

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

[Michel Hummel]

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/

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

[Michel Hummel]

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

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,

Thank you
Michel Hummel

2010/9/29 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
>>
>

[-- Attachment #2: XWin_patch_restart_clipboard1.9.patch --]
[-- Type: application/octet-stream, Size: 9246 bytes --]

--- /home/hummelm/xserver-980c36531228e953058c5b34b6bfaacf8cb1601a-org/hw/xwin/winclipboard.h	2010-09-28 17:22:50.400391900 +0200
+++ hw/xwin//winclipboard.h	2010-09-29 13:43:52.995248000 +0200
@@ -71,6 +71,8 @@
 #define WIN_XEVENTS_SHUTDOWN			1
 #define WIN_XEVENTS_CONVERT			2
 #define WIN_XEVENTS_NOTIFY			3
+#define WIN_CLIPBOARD_RETRIES			40
+#define WIN_CLIPBOARD_DELAY			1
 
 #define WM_WM_REINIT                           (WM_USER + 1)
 
--- /home/hummelm/xserver-980c36531228e953058c5b34b6bfaacf8cb1601a-org/hw/xwin/winclipboardthread.c	2010-09-28 17:22:50.416028100 +0200
+++ hw/xwin//winclipboardthread.c	2010-09-29 15:40:45.632202400 +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;
@@ -60,6 +62,7 @@ extern Window		g_iClipboardWindow;
 static jmp_buf			g_jmpEntry;
 static XIOErrorHandler g_winClipboardOldIOErrorHandler;
 static pthread_t g_winClipboardProcThread;
+static short clipboard_generation = 0;
 
 Bool				g_fUnicodeSupport = FALSE;
 Bool				g_fUseUnicode = FALSE;
@@ -89,9 +92,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;
@@ -103,9 +105,10 @@ winClipboardProc (void *pvNotUsed)
   char			szDisplay[512];
   int			iSelectError;
 
-  pthread_cleanup_push(&winClipboardThreadExit, NULL);
+  pthread_cleanup_push(&winClipboardThreadExit,  (void*) &fdMessageQueue);
 
   ErrorF ("winClipboardProc - Hello\n");
+  ++clipboard_generation;
 
   /* Do we have Unicode support? */
   g_fUnicodeSupport = winClipboardDetectUnicodeSupport ();
@@ -120,7 +123,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 */
@@ -144,7 +147,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)
     {
@@ -194,7 +197,7 @@ winClipboardProc (void *pvNotUsed)
   if (pDisplay == NULL)
     {
       ErrorF ("winClipboardProc - Failed opening the display, giving up\n");
-      pthread_exit (NULL);
+      goto winClipboardProc_Done;
     }
 
   /* Save the display in a global used by the wndproc */
@@ -212,7 +215,7 @@ winClipboardProc (void *pvNotUsed)
   if (fdMessageQueue == -1)
     {
       ErrorF ("winClipboardProc - Failed opening %s\n", WIN_MSG_QUEUE_FNAME);
-      pthread_exit (NULL);
+      goto winClipboardProc_Done;
     }
 
   /* Find max of our file descriptors */
@@ -236,7 +239,7 @@ winClipboardProc (void *pvNotUsed)
   if (iWindow == 0)
     {
       ErrorF ("winClipboardProc - Could not create an X window.\n");
-      pthread_exit (NULL);
+      goto winClipboardProc_Done;
     }
 
   /* Select event types to watch */
@@ -265,7 +268,7 @@ winClipboardProc (void *pvNotUsed)
 	  XGetSelectionOwner (pDisplay, XA_PRIMARY) != iWindow)
 	{
 	  ErrorF ("winClipboardProc - Could not set PRIMARY owner\n");
-	  pthread_exit (NULL);
+          goto winClipboardProc_Done;
 	}
 
       /* CLIPBOARD */
@@ -275,7 +278,7 @@ winClipboardProc (void *pvNotUsed)
 	  XGetSelectionOwner (pDisplay, atomClipboard) != iWindow)
 	{
 	  ErrorF ("winClipboardProc - Could not set CLIPBOARD owner\n");
-	  pthread_exit (NULL);
+          goto winClipboardProc_Done;
 	}
     }
 
@@ -294,7 +297,7 @@ winClipboardProc (void *pvNotUsed)
   if (!winClipboardFlushWindowsMessageQueue (hwnd))
     {
       ErrorF ("winClipboardProc - winClipboardFlushWindowsMessageQueue failed\n");
-      pthread_exit (NULL);
+      goto winClipboardProc_Done;
     }
 
   /* Signal that the clipboard client has started */
@@ -385,50 +388,11 @@ winClipboardProc (void *pvNotUsed)
 	}
     }
 
-winClipboardProc_Done:
-  /* 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
+  /* disable the clipboard means thread will dies */
+  g_fClipboard = FALSE;
 
-#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;
-
-  pthread_cleanup_pop(0);
+winClipboardProc_Done:
+  pthread_cleanup_pop(1);
 
   return NULL;
 }
@@ -453,6 +417,16 @@ winClipboardErrorHandler (Display *pDisp
 	  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;
 }
 
@@ -480,11 +454,104 @@ winClipboardIOErrorHandler (Display *pDi
 
 /*
  * winClipboardThreadExit - Thread exit handler
+ * Cleanup the thread and restart if needed
  */
 
 static void
-winClipboardThreadExit(void *arg)
+winClipboardThreadExit(void *vfdMessageQueue)
 {
-  /* clipboard thread has exited, stop server as well */
-  kill(getpid(), SIGTERM);
+  int iReturn;
+  int fdMessageQueue = (int) vfdMessageQueue;
+
+  winDebug("winClipboardThreadExit - enter \n");
+
+ /* First clean the thread */
+  CloseClipboard();
+  /* Close our Windows window */
+  if (g_hwndClipboard )
+    {
+      /* Destroy the Window window (hwnd) */
+      winDebug("winClipboardThreadExit - 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 ("winClipboardThreadExit - XDestroyWindow returned BadWindow.\n");
+      else
+        winDebug ("winClipboardThreadExit - 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;
+
+  /* checking if we need to restart */
+  if (clipboard_generation >= WIN_CLIPBOARD_RETRIES) 
+    {
+      /* terminates clipboard thread but the main server still lives */
+      ErrorF("winClipboardThreadExit - the clipboard thread seems to be unstable, the clipboard thread will now exit\n");
+      g_fClipboard = FALSE;
+      return;
+    }
+
+  if (g_fClipboard)
+    {
+      sleep(WIN_CLIPBOARD_DELAY);
+      ErrorF("winClipboardThreadExit - trying to restart clipboard thread \n");
+      /* Create the clipboard client thread */
+      if (!winInitClipboard ())
+        {
+          ErrorF ("winClipboardThreadExit - winClipboardInit "
+                  "failed.\n");
+          return;
+        }
+ 
+      winDebug ("winClipboardThreadExit - winInitClipboard returned.\n");
+      /* Flag that clipboard client has been launched */
+      g_fClipboardLaunched = TRUE;
+    }
+  else
+    {
+      ErrorF ("winClipboardThreadExit - Clipboard disabled  - Exit from server \n");
+      /* clipboard thread has exited, stop server as well */
+      kill(getpid(), SIGTERM);
+    }
+  return;  
 }

[-- Attachment #3: XWin_patch_remove_xdmcp_tricks1.9.patch --]
[-- Type: application/octet-stream, Size: 6250 bytes --]

--- /home/hummelm/xserver-980c36531228e953058c5b34b6bfaacf8cb1601a-org/hw/xwin/InitInput.c	2010-09-28 17:22:50.353483300 +0200
+++ hw/xwin//InitInput.c	2010-09-28 18:15:52.327041200 +0200
@@ -60,10 +60,8 @@ DeviceIntPtr g_pwinKeyboard;
 #ifdef HAS_DEVWINDOWS
 extern int			g_fdMessageQueue;
 #endif
-extern Bool			g_fXdmcpEnabled;
 #ifdef XWIN_CLIPBOARD
 extern winDispatchProcPtr	winProcEstablishConnectionOrig;
-extern winDispatchProcPtr	winProcQueryTreeOrig;
 #endif
 
 
@@ -127,12 +125,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-980c36531228e953058c5b34b6bfaacf8cb1601a-org/hw/xwin/winclipboardwrappers.c	2010-09-28 17:22:50.447300500 +0200
+++ hw/xwin//winclipboardwrappers.c	2010-09-29 15:10:50.466764400 +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 */

[-- Attachment #4: 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/

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

[Jon TURNEY]

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/