bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Christoph Ruegge]

I experience a bug that can be produced as follows:

(1) start emacs -Q --daemon
(2) open an X frame
(3) close the frame
(4) logout.

The result is that the session manager (KDE's, in my case) logout hangs
since Emacs does not respond to the logout signal. The bug does not
occur if I logout before closing the frame, or open other frames and
interact with Emacs in any way that does not lead to no frame being open
at any given time (I can even close the first frame). Moreover, if I
logout before opening any frame, the logout does not hang, but the Emacs
process does not terminate.

I investigated a bit and think the reason has something to do with the
way the SM communication is tied to the first opened terminal. The
connection is opened at the end of x_term_init() when the first terminal
is opened (and is an X terminal). Once the last X frame is closed, the
terminal is closed as well. I don't know precisely why this causes the
bug, since the SM connection is supposed to be shut down in
x_delete_display(), but I am pretty sure that the bug happens if and
only if the first terminal has been closed.

Far as I see, the bug occurs only on non-GTK toolkits, since for GTK the
terminal is kept open to circumvent some GTK some bug, apparently.

If I may add, the current behaviour is rather weird to begin with. The
SM integration's purpose is to cleanly shutdown Emacs on logout, so it
should be tied to the entire process and not to a particular
terminal. There are essentially two scenarios for the Emacs daemon: it
can be run outside a desktop session (in which case there should be no
SM connection at all) or inside (in which case the connection should
last for the lifetime of the process). A cleaner approach would
therefore maybe be to seperate the SM connection from opening and
closing the terminal and have it done at startup if the user wishes,
maybe depending on a command line parameter, the presence of the DISPLAY
variable, or after user init depending on some elisp variable. The
connection itself could maybe be done through a dummy X terminal that is
kept open all the time. I tried simpy adding a call to
`x-open-connection' to startup.el, and this indeed fixes the bug (for
one of the use cases).

I'd be willing to try to write a patch for this, but as I'm not much of
a programmer and do not really understand the way Emacs handles
displays, it would likely not be up to quality standards.

Best regards



In GNU Emacs 24.4.50.16 (x86_64-unknown-linux-gnu)
 of 2014-08-30 on io
Windowing system distributor `The X.Org Foundation', version 11.0.11600000
System Description:	Arch Linux

Configured using:
 `configure --prefix=/home/cs/.local --with-x-toolkit=no --without-gconf
 --without-gsettings

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS NOTIFY ACL GNUTLS
LIBXML2 FREETYPE LIBOTF XFT ZLIB

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Christoph Ruegge]

> I don't know precisely why this causes the
> bug, since the SM connection is supposed to be shut down in
> x_delete_display(), but I am pretty sure that the bug happens if and
> only if the first terminal has been closed.

Adding to this, I think the reason is that the call to x_session_close() in
x_delete_display() deletes the connection's FD, but does not actually tell the
SM to close. Adding a call to SmcCloseConnection() to x_session_close()
solves the hanging behaviour for me, but of course retains the weird behaviour
of Emacs not shutting down if the first terminal was closed.

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Christoph Ruegge]

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

Ok, I wrote a patch that fixes the problem for me and implements the
behaviour I described earlier. Like I said, I don't really know what
I'm doing here, but maybe it can be helpful somehow ;-)

Best regards

[-- Attachment #2: session-manager.patch --]
[-- Type: text/x-patch, Size: 5220 bytes --]

diff --git a/lisp/startup.el b/lisp/startup.el
index bb55080..19bdb84 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -99,6 +99,11 @@ startup message unless he personally acts to inhibit it."
   :type 'boolean
   :group 'initialization)
 
+(defcustom inhibit-x-session-manager nil
+  "Non-nil inhibits connecting to the X session manager on startup."
+  :type 'boolean
+  :group 'initialization)
+
 (defvar command-switch-alist nil
   "Alist of command-line switches.
 Elements look like (SWITCH-STRING . HANDLER-FUNCTION).
@@ -1318,6 +1323,20 @@ Consider using a subdirectory instead, e.g.: %s" dir
   ;; If -batch, terminate after processing the command options.
   (if noninteractive (kill-emacs t))
 
+  (when (and (not inhibit-x-session-manager)
+             (or (get 'x 'window-system-initialized)
+                 (getenv "DISPLAY")))
+    (condition-case err
+        (progn
+          (unless (get 'x 'window-system-initialized)
+            (x-initialize-window-system)
+            (put 'x 'window-system-initialized t))
+          (x-session-initialize x-display-name))
+      (error (display-warning
+	    'initialization
+	    (format "Could not connect to X session manager: %s" err)
+	    :warning))))
+
   ;; In daemon mode, start the server to allow clients to connect.
   ;; This is done after loading the user's init file and after
   ;; processing all command line arguments to allow e.g. `server-name'
diff --git a/src/xsmfns.c b/src/xsmfns.c
index 81b0126..6670604 100644
--- a/src/xsmfns.c
+++ b/src/xsmfns.c
@@ -59,6 +59,10 @@ static int doing_interact;
 
 static SmcConn smc_conn;
 
+/* The X terminal on which the session manager connection is opened. */
+
+static struct terminal *smc_terminal;
+
 /* The client session id for this session.  */
 
 static char *client_id;
@@ -390,8 +394,14 @@ create_client_leader_window (struct x_display_info *dpyinfo, char *client_ID)
 
 /* Try to open a connection to the session manager.  */
 
-void
-x_session_initialize (struct x_display_info *dpyinfo)
+DEFUN ("x-session-initialize", Fx_session_initialize,
+       Sx_session_initialize, 0, 1, 0,
+       doc: /* Initialize the connection to the X session manager.
+This is done automatically on startup if `inhibit-x-session-manager'
+is non-nil.  The optional parameter TERMINAL should be the X terminal
+on which to open the connection.  X should be initialized using
+`x-initialize-window-system' before calling this. */)
+  (Lisp_Object terminal)
 {
 #define SM_ERRORSTRING_LEN 512
   char errorstring[SM_ERRORSTRING_LEN];
@@ -399,6 +409,8 @@ x_session_initialize (struct x_display_info *dpyinfo)
   SmcCallbacks callbacks;
   ptrdiff_t name_len = 0;
 
+  struct x_display_info *dpyinfo = check_x_display_info (terminal);
+
   ice_fd = -1;
   doing_interact = False;
 
@@ -465,15 +477,33 @@ x_session_initialize (struct x_display_info *dpyinfo)
 #else
       create_client_leader_window (dpyinfo, client_id);
 #endif
+
+      smc_terminal = dpyinfo->terminal;
+      smc_terminal->reference_count++;
     }
+
+  return Qnil;
 }
 
 /* Ensure that the session manager is not contacted again. */
 
-void
-x_session_close (void)
+DEFUN ("x-session-close", Fx_session_close,
+       Sx_session_close, 0, 0, 0,
+       doc: /* Close the connection to the X session manager.  */)
+  ()
 {
+  SmcCloseConnection (smc_conn, 0, 0);
   ice_connection_closed ();
+
+  smc_terminal->reference_count--;
+  if (smc_terminal->reference_count == 0)
+    {
+	Lisp_Object tmp;
+	XSETTERMINAL (tmp, smc_terminal);
+        Fdelete_terminal (tmp, Qnil);
+    }
+
+  return Qnil;
 }
 
 
@@ -563,6 +593,8 @@ See also `emacs-save-session-functions', `emacs-session-save' and
   Vx_session_previous_id = Qnil;
 
   defsubr (&Shandle_save_session);
+  defsubr (&Sx_session_initialize);
+  defsubr (&Sx_session_close);
 }
 
 #endif /* HAVE_X_SM */
diff --git a/src/xterm.c b/src/xterm.c
index beb7d78..42ff947 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -11180,14 +11180,6 @@ x_term_init (Lisp_Object display_name, char *xrm_option, char *resource_name)
 #endif
   }
 
-#ifdef HAVE_X_SM
-  /* Only do this for the very first display in the Emacs session.
-     Ignore X session management when Emacs was first started on a
-     tty.  */
-  if (terminal->id == 1)
-    x_session_initialize (dpyinfo);
-#endif
-
   unblock_input ();
 
   return dpyinfo;
@@ -11206,11 +11198,6 @@ x_delete_display (struct x_display_info *dpyinfo)
   for (t = terminal_list; t; t = t->next_terminal)
     if (t->type == output_x_window && t->display_info.x == dpyinfo)
       {
-#ifdef HAVE_X_SM
-        /* Close X session management when we close its display.  */
-        if (t->id == 1 && x_session_have_connection ())
-          x_session_close ();
-#endif
         delete_terminal (t);
         break;
       }
diff --git a/src/xterm.h b/src/xterm.h
index c867312..7f5e087 100644
--- a/src/xterm.h
+++ b/src/xterm.h
@@ -1097,9 +1097,7 @@ extern void initialize_frame_menubar (struct frame *);
 
 /* Defined in xsmfns.c */
 #ifdef HAVE_X_SM
-extern void x_session_initialize (struct x_display_info *dpyinfo);
 extern int x_session_have_connection (void);
-extern void x_session_close (void);
 #endif
 
 /* Defined in xterm.c */

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Stefan Monnier]

> I investigated a bit and think the reason has something to do with the
> way the SM communication is tied to the first opened terminal.

Your analysis makes a lot of sense.

> If I may add, the current behaviour is rather weird to begin with. The
> SM integration's purpose is to cleanly shutdown Emacs on logout, so it
> should be tied to the entire process and not to a particular terminal.

Indeed, we have a problem there.  The `emacs --daemon' is not tied to
a particular display, so it should not shutdown in response to such
SM events.  OTOH, many users use "emacs --daemon" only for their
current session.  So I can't think of any way to resolve this, short of
making it a config var, like in your patch.

Looking at your patch, I like the idea of exposing the session-start/end
to Elisp, but I know too little about this area to really review
your patch.  Could someone else take a look at it?


        Stefan

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Jan D.]

Hi.

2 sep 2014 kl. 20:25 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>> I investigated a bit and think the reason has something to do with the
>> way the SM communication is tied to the first opened terminal.
> 
> Your analysis makes a lot of sense.
> 
>> If I may add, the current behaviour is rather weird to begin with. The
>> SM integration's purpose is to cleanly shutdown Emacs on logout, so it
>> should be tied to the entire process and not to a particular terminal.


No. The reason is to have Emacs restarted when the user logs in again.
Logout is no problem, the X connection will go away.

> Indeed, we have a problem there.  The `emacs --daemon' is not tied to
> a particular display, so it should not shutdown in response to such
> SM events.  OTOH, many users use "emacs --daemon" only for their
> current session.  So I can't think of any way to resolve this, short of
> making it a config var, like in your patch.
> 
> Looking at your patch, I like the idea of exposing the session-start/end
> to Elisp, but I know too little about this area to really review
> your patch.  Could someone else take a look at it?

Why not just skip all session manager interactions if run as a daemon?
You are technically not part of the session if the process can survive the session.

        Jan D. 

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Stefan Monnier]

> Why not just skip all session manager interactions if run as a daemon?

I think that's what his patch does.


        Stefan

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Jan Djärv]

Hello.

3 sep 2014 kl. 02:06 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>> Why not just skip all session manager interactions if run as a daemon?
> 
> I think that's what his patch does.

It does not.  It doesn't do SM if inhibit-x-session-manager is non-nil, which is not the same thing, it requires the user to set it.
It also adds/changes code in several files and exposes things to lisp.

Disabling SM in case of deamon is two additional if-statements in xterm.c.

	Jan D.

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Christoph Ruegge]

>>> If I may add, the current behaviour is rather weird to begin with. The
>>> SM integration's purpose is to cleanly shutdown Emacs on logout, so it
>>> should be tied to the entire process and not to a particular terminal.
>
>
> No. The reason is to have Emacs restarted when the user logs in again.
> Logout is no problem, the X connection will go away.


But that is not necessarily the only function of a session manager
(some SMs aren't even set to restart applications by default). They
also give applications the opportunity to query the user about unsaved
changes and such things. Now Emacs does not do that, but it's fairly
simple to do implement using `emacs-save-session-functions'.
Additionally, the only reason for me to log out is usually to restart
the computer, so Emacs will get killed anyway. Having the opportunity
to query about potential data loss may be useful.

That being said, I realize that my patch is rather intrusive and that
the more minimal solution of the daemon not using the SM at all is
better, especially since the daemon _can_ survive losing the X
connection (maybe in case of an X crash), which would lead to
unexpected behaviour in the next session. So one simply has to rely on
something like auto-save to prevent data loss.

Still, there's a corner case of the same bug occuring in non-daemon
mode, since it's technically possible to close the first terminal
after using e.g. "emacsclient -t", though that is likely a rather rare
situation.

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Jan Djärv]

Hi.

3 sep 2014 kl. 11:53 skrev Christoph Ruegge <chrueg@gmail.com>:

> They
> also give applications the opportunity to query the user about unsaved
> changes and such things. Now Emacs does not do that, but it's fairly
> simple to do implement using `emacs-save-session-functions'.
> Additionally, the only reason for me to log out is usually to restart
> the computer, so Emacs will get killed anyway. Having the opportunity
> to query about potential data loss may be useful.

Save state is part of the session protocol.  But as daemon is supposed to survive the session, there is no saving of state needed when the session dies.

> That being said, I realize that my patch is rather intrusive and that
> the more minimal solution of the daemon not using the SM at all is
> better, especially since the daemon _can_ survive losing the X
> connection (maybe in case of an X crash), which would lead to
> unexpected behaviour in the next session. So one simply has to rely on
> something like auto-save to prevent data loss.
> 
> Still, there's a corner case of the same bug occuring in non-daemon
> mode, since it's technically possible to close the first terminal
> after using e.g. "emacsclient -t", though that is likely a rather rare
> situation.

The tests I done shows that the SM connection is not closed properly, ever.
So it is a bug.  On the other hand, X style session management is kind of deprecated.
You can see this in Fedora 20, where the whole desktop dies when emacsclient -c starts a new frame for a daemon outside the session.  I guess it looses it when a process outside the session attach to the session (or something like that).

I will fix the bug, and disable SM for daemon.

As for exposing SM to lisp, that might not be a bad idea, but it would be better to implement and expose the newer d-bus based SM than the old X based SM.   The latter will probably go away within a couple of years.  Gnome only has a compability layer for it on top of the d-bus SM.

	Jan D.

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Christoph Ruegge]

> The tests I done shows that the SM connection is not closed properly, ever.

Even if you add an SmcCloseConnection() call to x_session_close()?
Seemed to work for me.

> As for exposing SM to lisp, that might not be a bad idea, but it would be better to implement and expose the newer d-bus based SM than the old X based SM. The latter will probably go away within a couple of years. Gnome only has a compability layer for it on top of the d-bus SM.

That could maybe be done in pure Elisp, I suppose. Maybe I'll try that
if I have some spare time.

bug#18375: 24.4.50; Emacs hangs X session manager logout in certain cases

[Jan Djärv]

Bug fixed in trunk.

3 sep 2014 kl. 20:51 skrev Christoph Ruegge <chrueg@gmail.com>:

>> The tests I done shows that the SM connection is not closed properly, ever.
> 
> Even if you add an SmcCloseConnection() call to x_session_close()?
> Seemed to work for me.

Sure, if I change stuff it works, but not as it is.

	Jan D.

> 
>> As for exposing SM to lisp, that might not be a bad idea, but it would be better to implement and expose the newer d-bus based SM than the old X based SM. The latter will probably go away within a couple of years. Gnome only has a compability layer for it on top of the d-bus SM.
> 
> That could maybe be done in pure Elisp, I suppose. Maybe I'll try that
> if I have some spare time.