After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 85933 - smproxy does not properly handle java session
smproxy does not properly handle java session
Status: RESOLVED FIXED
Product: gnome-session
Classification: Core
Component: smproxy
1.5.x
Other All
: High major
: ---
Assigned To: Session Maintainers
Session Maintainers
Depends on:
Blocks:
 
 
Reported: 2002-06-19 13:08 UTC by Brian Cameron
Modified: 2004-12-22 21:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
This is an updated patch (9.40 KB, patch)
2002-06-20 12:51 UTC, Brian Cameron
none Details | Review

Description Brian Cameron 2002-06-19 13:08:12 UTC
smproxy fails to read WM_NAME if it is "COMPOUND_TEXT" instead of
just a "STRING".  This is because it is using the obsolete
XFetchName instead of XGetWMName.

Furthermore there is a problem with Java in that it sets its session
by calling:

   com.sun.dt.Xsession.WMcommand(theAudioControl, cmd_str);

This call must be done after the window is made visible.  Therefore
there is a race condition.  Even if the call to WMcommand is done
immediately after the call to "setVisible", there seems to be about
a 50% chance that the program will have its WM_COMMAND set by the
time smproxy wants to detect it.  In other wrods, the WM_COMMAND is
being set initially after the WM_STATE property change is detected.

The following patch fixes this problem.


 Index: gnome-session/smproxy/smproxy.c
===================================================================
RCS file: /cvs/GNOME-RE/gnome-session/smproxy/smproxy.c,v
retrieving revision 1.16
diff -u -r1.16 smproxy.c
--- gnome-session/smproxy/smproxy.c	2002/05/28 18:17:00	1.16
+++ gnome-session/smproxy/smproxy.c	2002/06/19 13:01:59
@@ -41,6 +41,7 @@
 Atom wmProtocolsAtom;
 Atom wmSaveYourselfAtom;
 Atom wmStateAtom;
+Atom wmCommandAtom;
 Atom smClientIdAtom;
 Atom wmClientLeaderAtom;
 Atom dtSaveModeAtom;
@@ -528,7 +529,7 @@
     IceSetErrorHandler (NULL);
 
     if (winInfo->smc_conn == NULL)
-	return;
+		return;
 
     ice_conn = SmcGetIceConnection (winInfo->smc_conn);
 
@@ -545,8 +546,8 @@
 
     if (debug)
     {
-	printf ("Connected to SM, window = 0x%x\n", (unsigned int)
winInfo->window);
-	printf ("\n");
+		printf ("Connected to SM, window = 0x%x\n", (unsigned int)
winInfo->window);
+		printf ("\n");
     }
 
     proxy_count++;
@@ -628,12 +629,12 @@
     newptr->window = window;
     newptr->smc_conn = NULL;
     newptr->tested_for_sm_client_id = 0;
+    newptr->missing_command = 0;
     newptr->client_id = NULL;
     newptr->wm_command = NULL;
     newptr->wm_command_count = 0;
     newptr->class.res_name = NULL;
     newptr->class.res_class = NULL;
-    newptr->wm_name = NULL;
     newptr->wm_client_machine.value = NULL;
     newptr->wm_client_machine.nitems = 0;
     newptr->has_save_yourself = 0;
@@ -666,9 +667,6 @@
 	if (ptr->wm_command)
 	    XFreeStringList (ptr->wm_command);
 	
-	if (ptr->wm_name)
-	    XFree (ptr->wm_name);
-	
 	if (ptr->wm_client_machine.value)
 	    XFree (ptr->wm_client_machine.value);
 	
@@ -684,10 +682,18 @@
 
 
 
+
+/*
+ * check_again is True if the WM_COMMAND property has been changed.  We
+ * only check again if the last time we checked this window, the
+ * WM_COMMAND was not set.
+ */
+
 static void
-Got_WM_STATE (winptr)
+Got_WM_STATE (winptr, check_again)
 
 WinInfo *winptr;
+Bool check_again;
 
 {
     WinInfo *leader_winptr;
@@ -697,10 +703,14 @@
      * shouldn't do it again.
      */
 
-    if (winptr->tested_for_sm_client_id)
-    {
-	return;
-    }
+	if (check_again == False && winptr->tested_for_sm_client_id)
+	{
+		if (debug) {
+			printf("Already checked window 0x%x, skipping...\n",
+				leader_winptr->window);
+		}
+		return;
+	}
 
 
     /*
@@ -720,63 +730,82 @@
 
     if (caught_error)
     {
-	caught_error = 0;
-	RemoveWindow (winptr);
-	XSetErrorHandler (NULL);
-	return;
-    }
-
-
-    /*
-     * If we already checked for SM_CLIENT_ID on the client leader
-     * window, don't do it again.
-     */
-
-    if (!leader_winptr || leader_winptr->tested_for_sm_client_id)
-    {
-	caught_error = 0;
-	XSetErrorHandler (NULL);
-	return;
+		caught_error = 0;
+		RemoveWindow (winptr);
+		XSetErrorHandler (NULL);
+		return;
     }
 
-    leader_winptr->tested_for_sm_client_id = 1;
 
-    if (!HasXSMPsupport (leader_winptr->window))
-    {
-	XFetchName (disp, leader_winptr->window, &leader_winptr->wm_name);
-
-	XGetCommand (disp, leader_winptr->window,
-	    &leader_winptr->wm_command,
-	    &leader_winptr->wm_command_count);
-
-	XGetClassHint (disp, leader_winptr->window, &leader_winptr->class);
+	/*
+	 * If we already checked for SM_CLIENT_ID on the client leader
+	 * window, don't do it again.  check_again is set if the WM_COMMAND
+     * property was changed.  Only continue if we tried and failed
+     * before (if missing_command is set)
+	 */
+	if (!leader_winptr ||
+		(check_again == False && leader_winptr->tested_for_sm_client_id) ||
+        (check_again == True  && !leader_winptr->missing_command))
+	{
+		caught_error = 0;
+		XSetErrorHandler (NULL);
+		return;
+	}
 
-	XGetWMClientMachine (disp, leader_winptr->window,
-	    &leader_winptr->wm_client_machine);
+	leader_winptr->tested_for_sm_client_id = 1;
 
-	if (leader_winptr->wm_name != NULL &&
-	    leader_winptr->wm_command != NULL &&
-	    leader_winptr->wm_command_count > 0 &&
-	    leader_winptr->class.res_name != NULL &&
-	    leader_winptr->class.res_class != NULL &&
-	    leader_winptr->wm_client_machine.value != NULL &&
-	    leader_winptr->wm_client_machine.nitems != 0)
+	if (!HasXSMPsupport (leader_winptr->window))
 	{
-	    leader_winptr->has_save_yourself =
-		HasSaveYourself (leader_winptr->window);
-
-	    ConnectClientToSM (leader_winptr);
+		if (XGetWMName (disp, leader_winptr->window, &(leader_winptr->wm_name)))
+		{
+			XClientMessageEvent saveYourselfMessage;
+
+			XGetCommand (disp, leader_winptr->window,
+			    &leader_winptr->wm_command,
+			    &leader_winptr->wm_command_count);
+
+			XGetClassHint (disp, leader_winptr->window, &leader_winptr->class);
+
+			XGetWMClientMachine (disp, leader_winptr->window,
+			    &leader_winptr->wm_client_machine);
+
+            /*
+             * Set missing_command if this window doesn't have a valid
+             * SM_WINDOW_COMMAND value.  Then if this atom changes in the
+             * future, we will pick up the proper value.
+             */
+			if (leader_winptr->wm_command == NULL ||
+			    leader_winptr->wm_command_count <= 0)
+			{
+				leader_winptr->missing_command = 1;
+            }
+			else
+			{
+				leader_winptr->missing_command = 0;
+            }
+
+			if (leader_winptr->wm_command != NULL &&
+			    leader_winptr->wm_command_count > 0 &&
+			    leader_winptr->class.res_name != NULL &&
+			    leader_winptr->class.res_class != NULL &&
+			    leader_winptr->wm_client_machine.value != NULL &&
+			    leader_winptr->wm_client_machine.nitems != 0)
+			{
+			    leader_winptr->has_save_yourself =
+				HasSaveYourself (leader_winptr->window);
+			    ConnectClientToSM (leader_winptr);
+			}
+	    }
 	}
-    }
 
     XSync (disp, 0);
     XSetErrorHandler (NULL);
 
-    if (caught_error)
-    {
-	caught_error = 0;
-	RemoveWindow (leader_winptr);
-    }
+	if (caught_error)
+	{
+		caught_error = 0;
+		RemoveWindow (leader_winptr);
+	}
 }
 
 
@@ -800,15 +829,15 @@
      */
 
     if (shutting_down)
-	return;
+		return;
 
 
     /*
      * Add the new window
      */
 
-    if ((winptr = AddNewWindow (event->window)) == NULL)
-	return;
+    if ((winptr = AddNewWindow (event->window)) == NULL) 
+		return;
 
 
     /*
@@ -855,12 +884,12 @@
 
     if (caught_error)
     {
-	caught_error = 0;
-	RemoveWindow (winptr);
+		caught_error = 0;
+		RemoveWindow (winptr);
     }
     else if (got_wm_state)
     {
-	Got_WM_STATE (winptr);
+		Got_WM_STATE (winptr, False);
     }
 }
 
@@ -905,37 +934,41 @@
     WinInfo *winptr;
 
     if (!LookupWindow (window, &winptr, NULL))
-	return;
+		return;
 
     if (event->atom == wmStateAtom)
     {
-	Got_WM_STATE (winptr);
+		Got_WM_STATE (winptr, False);
     }
+    else if (event->atom == wmCommandAtom)
+    {
+		Got_WM_STATE (winptr, True);
+    }
     else if (event->atom == XA_WM_COMMAND && winptr->waiting_for_update)
     {
-	/* Finish off the Save Yourself */
+		/* Finish off the Save Yourself */
 
         char ** argv;
         int argc;
-
-      	/* Some particularly horrid clients (mozilla!) sometimes 
-	   delete the value of the WM_COMMAND property rather 
-	   than changing it. The only feasible response that
-           I can see to this behavior is too ignore that the
-	   WM_COMMAND has been changed at all... */
 
-	if (XGetCommand (disp, window, &argv, &argc) &&
-	    argv && argc > 0)
-	{
-	    if (winptr->wm_command)
-	      XFreeStringList (winptr->wm_command);
-
-	    winptr->wm_command = argv;
-	    winptr->wm_command_count = argc;
-	}
+		/* Some particularly horrid clients (mozilla!) sometimes 
+		delete the value of the WM_COMMAND property rather 
+		than changing it. The only feasible response that
+		I can see to this behavior is too ignore that the
+		WM_COMMAND has been changed at all... */
+
+		if (XGetCommand (disp, window, &argv, &argc) &&
+		    argv && argc > 0)
+		{
+		    if (winptr->wm_command)
+		      XFreeStringList (winptr->wm_command);
+
+		    winptr->wm_command = argv;
+		    winptr->wm_command_count = argc;
+		}
 
-	winptr->waiting_for_update = 0;
-	FinishSaveYourself (winptr, True);
+		winptr->waiting_for_update = 0;
+		FinishSaveYourself (winptr, True);
     }
 }
 
@@ -1427,6 +1460,7 @@
     wmProtocolsAtom = XInternAtom (disp, "WM_PROTOCOLS", False);
     wmSaveYourselfAtom = XInternAtom (disp, "WM_SAVE_YOURSELF", False);
     wmStateAtom = XInternAtom (disp, "WM_STATE", False);
+    wmCommandAtom = XInternAtom (disp, "WM_COMMAND", False);
     smClientIdAtom = XInternAtom (disp, "SM_CLIENT_ID", False);
     wmClientLeaderAtom = XInternAtom (disp, "WM_CLIENT_LEADER", False);
     dtSaveModeAtom = XInternAtom (disp, "_DT_SAVE_MODE", False);
Comment 1 Brian Cameron 2002-06-20 12:51:10 UTC
Created attachment 9345 [details] [review]
This is an updated patch
Comment 2 Brian Cameron 2002-06-20 12:55:16 UTC
Doh!

I just found a nasty bug in the patch listed in the above comments.
The broken patch fixed programs that set SM_COMMAND after SM_STATE
(like Java), but broke all programs that normally set SM_COMMAND
before SM_STATE.

I have fixed this problem.  The fix works for all programs whether
they set SM_COMMAND before or after SM_STATE.

I attached the fixed patch rather than including it in the comments.
Sorry I didn't realize that you could add attachments when I 
submitted the initial bug.

I also am now setting _DT_RESTORE_MODE.  Setting _DT_SAVE_MODE
makes CDE programs work properly when WM_SAVE_YOURSELF is received,
but _DT_RESTORE_MODE is needed when restoring the programs.
This is a more complete fix for bug #81343.  Refer to:

   http://bugzilla.gnome.org/show_bug.cgi?id=81343

I'll also update that bug to point to the patch in this bug report.
Comment 3 Luis Villa 2002-06-26 01:19:15 UTC
Can you comment, Mark?
Comment 4 Mark McLoughlin 2002-07-02 05:46:01 UTC
Brian, my instinct tells me that this is a Java bug, plain and simple
and given the complexity of the workaround it shouldn't go in.

However, you've made a lot of changes in the patch that aren't really
to do with the workaround. If you could make the patch so it only
contains the neccessary changes it'll make it a lot easier to figure
out what you're doing.
Comment 5 Mark McLoughlin 2002-07-02 05:50:21 UTC
Oh, I did take your advice and used XGetWMName instead of XFetchName
Comment 6 Brian Cameron 2002-07-02 09:01:22 UTC
I am not sure that this is just a Java bug, though that could
be simply that I am not that familiar with the ICCCM standards.
The problem seems to be that WM_COMMAND must be set before
gnome_smproxy will send the SAVE_YOURSELF.  I say this because
it seems that ConnectClientToSM() seems to be the function which
sends the WM_SAVE_YOURSELF, but this function won't be called
from Got_WM_STATE() unless the command is already set.

Java doesn't set the WM_COMMAND initially, but only sets it
after receiving a SAVE_YOURSELF.  I looked at the ICCCM 
standard here: http://tronche.com/gui/x/icccm/ and it doesn't
seem to indicate that it is a requirement to set WM_COMMAND
initially, but only set it when a SAVE_YOURSELF is received.
Perhaps this manual isn't complete on this point.

A simpler hack would just be to allow ConnectClientToSM()
to be called in Got_WM_STATE() when the command isn't set.
The complexity of my current hack is that it is keeping a
flag to indicate that it aborted in Got_WM_STATE() since
WM_COMMAND was not there.  Then when it receives notification
that the WM_COMMAND property is set later on, then it
continues.  Perhaps this description makes the complexity
of my current hack more understandable.

If it is necessary for programs to have WM_COMMAND set even
before WM_SAVE_YOURSELF is sent, then this is a Java bug
and we should fix it there.  However, they probably won't
fix it in Java 1.2 since it is EOL.  Therefore we might
want a sun patch on this to make Java 1.2 work on Sun.

Thoughts?


Comment 7 Luis Villa 2002-07-02 15:13:32 UTC
[Search for 'luis spamming' to catch every instance of this email.]
In order to better track Sun's bugs for Sun and Ximian's internal use, I've
added a temporary keyword to some bugs. I apologize for the spam, and for the
use of an additional keyword, but this is the best way for Sun to track 'it's'
bugs without interfering with the community's own triage and bug behavior. If
you have any questions or objections, please drop me a note at louie@ximian.com
or email bugmaster@gnome.org for more open discussion.
Comment 8 Mark McLoughlin 2002-07-09 23:58:32 UTC
Okay, this is the way I'm reading the ICCCM

+ If a client wants to participate in the session - i.e. be restarted
when the saved session is started - it sets the WM_COMMAND property to
the commandline that should be used to start it.

   This is how the client indicates that it is participating in the
session and is completely seperate from the WM_SAVE_YOURSELF protocol.
I admit that it isn't worded very well in the specification, but I
think that is the intention here.

+ *Additionally*, if a client needs to save extra data (either in its
command line or in a session data file) in order to be restarted in a
state consistent with when the session was saved, it needs to
participate in the WM_SAVE_YOURSELF protocol, updating WM_COMMAND on
receipt of the WM_SAVE_YOURSELF ClientMessage.




Note, that it is pretty well explicitly stated what Java should do in
the text :

"
 .... the property should:

....

    * Be initialised from argv
"

I am going to close this out as NOTGNOME, but I will add a workaround
patch.

I don't want this going in to gnome-smproxy, because I only want to
accept fixes that would conceivably be accepted back in the canonical
smproxy.
Comment 9 Mark McLoughlin 2002-07-10 00:08:53 UTC
Okay, I've changed my mind. The workaround is trivial, its going in.

2002-07-10  Mark McLoughlin  <mark@skynet.ie>

        * smproxy.c: loosen the constraint vaguely
        specified in the ICCCM, that clients wishing to
        participate in the session indicate so by
        initialising the WM_COMMAND property, to also
        include clients that are participating in the
        WM_SAVE_YOURSELF protocol. #85933.

Comment 10 Mark McLoughlin 2002-07-10 00:09:38 UTC
Brian: re-open if this doesn't fix it. I haven't tested at all.