Bug 531634 - [GTK] Linux: Handle logoff / shutdown events from session manager

Hotfix for first version of the patch.

Using System.exit() from Shell's 'SWT.Close' event caused deadlock,
because main thread held 'Platform.lock' in its event loop and waited
for shutdown hook, which waited for the lock - see also Bug 546743.

Since multiple Displays are not possible currently (there's only dead
code pretending to support it), it should be OK to have non-static
object. Having it non-static removes the need for shutdown hook.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

3 files changed, 45 insertions, 53 deletions

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/internal/SessionManagerDBus.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/internal/SessionManagerDBus.java
index 02b67d4fd0..cecfac30ac 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/internal/SessionManagerDBus.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/internal/SessionManagerDBus.java
@@ -25,7 +25,10 @@ import java.util.ArrayList;
  * However, it requires GtkApplication, and SWT doesn't use that.
  *
  * Current session manager clients can be seen in:
- *   dbus-send --print-reply --dest=org.gnome.SessionManager /org/gnome/SessionManager org.gnome.SessionManager.GetClients
+ *   Gnome:
+ *     dbus-send --print-reply --dest=org.gnome.SessionManager /org/gnome/SessionManager org.gnome.SessionManager.GetClients
+ *   XFCE:
+ *     dbus-send --print-reply --dest=org.xfce.SessionManager /org/xfce/SessionManager org.xfce.Session.Manager.ListClients
  *
  * If you know clientObjectPath, you can send Stop signal with:
  *   dbus-send --print-reply --dest=org.gnome.SessionManager /org/gnome/SessionManager/ClientXX org.gnome.SessionManager.Client.Stop
@@ -49,48 +52,8 @@ public class SessionManagerDBus {
 		void stop();
 	}
 
-	private static class ShutdownHook extends Thread {
-		private SessionManagerDBus parent;
-
-		public ShutdownHook(SessionManagerDBus parent) {
-			this.parent = parent;
-		}
-
-		public void run() {
-			parent.stop();
-		}
-
-		public void install() {
-			try {
-				Runtime.getRuntime().addShutdownHook(this);
-			} catch (IllegalArgumentException ex) {
-				// Shouldn't happen
-				ex.printStackTrace();
-			} catch (IllegalStateException ex) {
-				// Shouldn't happen
-				ex.printStackTrace();
-			} catch (SecurityException ex) {
-				// That's pity, but not too much of a problem.
-				// Client will stay registered, contributing to clutter a little bit.
-			}
-		}
-
-		public void remove() {
-			try {
-				Runtime.getRuntime().removeShutdownHook(this);
-			} catch (IllegalStateException ex) {
-				// The virtual machine is already in the process of shutting down.
-				// That's expected.
-			} catch (SecurityException ex) {
-				// Shouldn't happen if 'addShutdownHook' worked.
-				ex.printStackTrace();
-			}
-		}
-	}
-
 	private ArrayList<IListener> listeners = new ArrayList<IListener>();
 	private Callback g_signal_callback;
-	private ShutdownHook shutdownHook = new ShutdownHook(this);
 	private long sessionManagerProxy;
 	private long clientProxy;
 	private String clientObjectPath;
@@ -107,6 +70,10 @@ public class SessionManagerDBus {
 		start();
 	}
 
+	public void dispose() {
+		stop();
+	}
+
 	/**
 	 * Subscribes display for session manager events.
 	 *
@@ -131,16 +98,17 @@ public class SessionManagerDBus {
 			return false;
 		}
 
-		// Both XFCE and Gnome will automatically unregister client on exit.
-		// However, to be on the correct side, we should also do it.
-		// Shutdown hook is used, because there's no other exit callback in SWT.
-		// Display.dispose() isn't good because there could be many displays.
-		// Also, in theory Displays can be created and disposed multiple times.
-		shutdownHook.install();
-
 		return true;
 	}
 
+	/**
+	 * Un-subscribes from session manager events.
+	 *
+	 * NOTE: Both Gnome and XFCE will automatically remove client record
+	 * when client's process ends, so it's not a big deal if this is not
+	 * called at all. See comments for this class to find 'dbus-send'
+	 * commands to verify that.
+	 */
 	private void stop() {
 		if ((sessionManagerProxy != 0) && (clientObjectPath != null)) {
 			long args = OS.g_variant_new(
@@ -180,8 +148,6 @@ public class SessionManagerDBus {
 			g_signal_callback.dispose();
 			g_signal_callback = null;
 		}
-
-		shutdownHook.remove();
 	}
 
 	private void sendEndSessionResponse(boolean is_ok, String reason) {
@@ -356,6 +322,11 @@ public class SessionManagerDBus {
 	 *
 	 * Once used, 'DESKTOP_AUTOSTART_ID' must not leak into child
 	 * processes, or they will fail to 'RegisterClient'.
+	 *
+	 * NOTE: calling this function twice will give empty ID on
+	 * second call. I think this is reasonable. If second object
+	 * is created for whatever reason, it's OK to consider it to
+	 * be a separate client.
 	 */
 	private String claimDesktopAutostartID() {
 		byte[] DESKTOP_AUTOSTART_ID = Converter.javaStringToCString("DESKTOP_AUTOSTART_ID");	//$NON-NLS-1$
diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java
index 5c7fd36499..d7d8a36c3d 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java
@@ -210,7 +210,7 @@ public class Display extends Device {
 		}
 	}
 
-	static SessionManagerDBus sessionManagerDBus = new SessionManagerDBus();
+	SessionManagerDBus sessionManagerDBus;
 	SessionManagerListener sessionManagerListener;
 	Runnable [] disposeList;
 
@@ -3982,10 +3982,20 @@ void initializeWindowManager () {
 }
 
 void initializeSessionManager() {
+	sessionManagerDBus = new SessionManagerDBus();
 	sessionManagerListener = new SessionManagerListener(this);
 	sessionManagerDBus.addListener(sessionManagerListener);
 }
 
+void releaseSessionManager() {
+	if (sessionManagerDBus != null) {
+		sessionManagerDBus.dispose();
+		sessionManagerDBus = null;
+	}
+
+	sessionManagerListener = null;
+}
+
 /**
  * Invokes platform specific functionality to dispose a GC handle.
  * <p>
@@ -4718,7 +4728,7 @@ protected void release () {
 	disposeList = null;
 	synchronizer.releaseSynchronizer ();
 	synchronizer = null;
-	sessionManagerDBus.removeListener(sessionManagerListener);
+	releaseSessionManager ();
 	releaseDisplay ();
 	super.release ();
 }
diff --git a/tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Bug531634_LogoffListener.java b/tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Bug531634_LogoffListener.java
index 8187df05c2..4e75c0a3be 100644
--- a/tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Bug531634_LogoffListener.java
+++ b/tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Bug531634_LogoffListener.java
@@ -19,6 +19,8 @@ import org.eclipse.swt.layout.FillLayout;
 import org.eclipse.swt.widgets.*;

 

 public class Bug531634_LogoffListener {

+	private static boolean isUserClosed = false;

+

 	public static void main (String [] args) {

 		Display display = new Display ();

 

@@ -34,7 +36,7 @@ public class Bug531634_LogoffListener {
 		});

 

 		display.addListener(SWT.Dispose, event -> {

-			if (!shell.isDisposed()) {

+			if (!isUserClosed && !shell.isDisposed()) {

 				MessageBox dialog = new MessageBox(shell, SWT.ICON_INFORMATION);

 				dialog.setMessage("EndSession received.\nI will exit when you close this box. Time limit is 10 sec.");

 				dialog.open();

@@ -44,6 +46,15 @@ public class Bug531634_LogoffListener {
 		final Label label = new Label(shell, SWT.WRAP | SWT.CENTER);

 		label.setText("\n\n\nWhen you logoff/shutdown, I will give you messages on 'QueryEndSession' and 'EndSession'");

 

+		// The first version of the patch had deadlock when System.exit() was called in response to UI action.

+		// This happened because 'SWT.Close' is called from inside 'OS.g_main_context_iteration' and

+		// Platform.lock is held by main thread, locking all OS.xxx calls from other threads.

+		shell.addListener(SWT.Close, event -> {

+			isUserClosed = true;

+			display.dispose();

+			System.exit(0);

+		});

+

 		shell.open ();

 

 		while (!shell.isDisposed()) {