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>
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 02b67d4..cecfac3 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 @@
* 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 @@
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 @@
start();
}
+ public void dispose() {
+ stop();
+ }
+
/**
* Subscribes display for session manager events.
*
@@ -131,16 +98,17 @@
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 @@
g_signal_callback.dispose();
g_signal_callback = null;
}
-
- shutdownHook.remove();
}
private void sendEndSessionResponse(boolean is_ok, String reason) {
@@ -356,6 +322,11 @@
*
* 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 5c7fd36..d7d8a36 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 @@
}
}
- static SessionManagerDBus sessionManagerDBus = new SessionManagerDBus();
+ SessionManagerDBus sessionManagerDBus;
SessionManagerListener sessionManagerListener;
Runnable [] disposeList;
@@ -3982,10 +3982,20 @@
}
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 @@
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 8187df0..4e75c0a 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.widgets.*;
public class Bug531634_LogoffListener {
+ private static boolean isUserClosed = false;
+
public static void main (String [] args) {
Display display = new Display ();
@@ -34,7 +36,7 @@
});
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 @@
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()) {
