[GTK3] Defer disposing Callbacks while GTK still has pointer to them

bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/gtk/org/eclipse/swt/dnd/ClipboardProxy.java

@@ -34,6 +34,9 @@ class ClipboardProxy {
 
 	long clipboardOwner = GTK3.gtk_window_new(GTK.GTK_WINDOW_TOPLEVEL);
 
+	/**
+	 * display == null means that the display has been disposed.
+	 */
 	Display display;
 	Clipboard activeClipboard = null;
 	Clipboard activePrimaryClipboard = null;
@@ -78,7 +81,48 @@ void gtk_gdk_clipboard_clear(long clipboard) {
 	GTK3.gtk_clipboard_clear(clipboard);
 }
 
+/**
+ * The calls to gtk_clipboard_set_with_owner (in
+ * {@link #setData(Clipboard, Object[], Transfer[], int)}) means the
+ * GtkClipboard (in C side) has function pointers saved to the getFunc +
+ * clearFunc callbacks.
+ *
+ * Therefore, when we dispose {@link ClipboardProxy} we cannot dispose the
+ * callbacks until we know that GtkClipboard doesn't have a pointer to these
+ * callbacks. GtkClipboard clears these pointers in clipboard_unset
+ * (https://gitlab.gnome.org/GNOME/gtk/-/blob/716458e86a222f43e64f7a4feda37749f3469ee4/gtk/gtkclipboard.c#L755)
+ * and notifies us that the pointers are no longer stored by calling clearFunc
+ * (https://gitlab.gnome.org/GNOME/gtk/-/blob/716458e86a222f43e64f7a4feda37749f3469ee4/gtk/gtkclipboard.c#L782)
+ *
+ * Therefore, after disposing ClipboardProxy we need to defer disposing the
+ * callbacks until clearFunc has been called.
+ *
+ * We know if we have been called sufficiently (for both clipboards that could
+ * have a handle stored) when both clipboards no longer have data stored.
+ *
+ * If we don't defer the disposal it causes SIGSEGV or other undefined behavior.
+ *
+ * Fix for https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+ */
+private void finishDispose() {
+	if (display == null) {
+		if (clipboardDataTypes == null && primaryClipboardDataTypes == null) {
+			if (getFunc != null ) getFunc.dispose();
+			getFunc = null;
+			if (clearFunc != null) clearFunc.dispose();
+			clearFunc = null;
+			if (clipboardOwner != 0) {
+				GTK3.gtk_widget_destroy(clipboardOwner);
+			}
+			clipboardOwner = 0;
+		}
+	}
+}
+
 long clearFunc(long clipboard,long user_data_or_owner){
+	if (display == null) {
+		System.err.println("***WARNING: Attempt to access SWT clipboard after disposing SWT Display.");
+	}
 	if (clipboard == Clipboard.GTKCLIPBOARD) {
 		activeClipboard = null;
 		clipboardData = null;
@@ -89,6 +133,7 @@ long clearFunc(long clipboard,long user_data_or_owner){
 		primaryClipboardData = null;
 		primaryClipboardDataTypes = null;
 	}
+	finishDispose();
 	return 1;
 }
 
@@ -101,25 +146,18 @@ void dispose () {
 		GTK3.gtk_clipboard_store(Clipboard.GTKPRIMARYCLIPBOARD);
 	}
 	display = null;
-	if (getFunc != null ) getFunc.dispose();
-	getFunc = null;
-	if (clearFunc != null) clearFunc.dispose();
-	clearFunc = null;
-	clipboardData = null;
-	clipboardDataTypes = null;
-	primaryClipboardData = null;
-	primaryClipboardDataTypes = null;
-	if (clipboardOwner != 0) {
-		GTK3.gtk_widget_destroy(clipboardOwner);
-	}
-	clipboardOwner = 0;
+	finishDispose();
 }
 
 /**
  * This function provides the data to the clipboard on request.
  * When this clipboard is disposed, the data will no longer be available.
  */
 long getFunc(long clipboard, long selection_data, long info, long user_data_or_owner){
+	if (display == null) {
+		System.err.println("***WARNING: Attempt to access SWT clipboard after disposing SWT Display.");
+		return 0;
+	}
 	if (selection_data == 0) return 0;
 	long target = GTK3.gtk_selection_data_get_target(selection_data);
 	TransferData tdata = new TransferData();

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/SwtTestUtil.java

@@ -149,6 +149,28 @@ public final static boolean isWayland() {
 		throw new IllegalStateException("unreachable");
 	}
 
+	/**
+	 * Return whether running on GTK3. This is dynamically set at runtime and cannot
+	 * be accessed before the corresponding property is initialized in OS.
+	 *
+	 * <strong>Note:</strong> this method still must be called after the static
+	 * block of OS is run.
+	 */
+	public final static boolean isGTK3() {
+		if (!isGTK) {
+			return false;
+		}
+
+		String version = System.getProperty("org.eclipse.swt.internal.gtk.version", "");
+		if (version.startsWith("3")) {
+			return true;
+		} else if (!version.isBlank()) {
+			return false;
+		}
+		fail("org.eclipse.swt.internal.gtk.version System property is not set yet. Create a new Display (or otherwise access OS) before calling isGTK4");
+		throw new IllegalStateException("unreachable");
+	}
+
 	/**
 	 * Return whether running on GTK4. This is dynamically set at runtime and cannot
 	 * be accessed before the corresponding property is initialized in OS.

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_dnd_Clipboard.java

@@ -19,11 +19,14 @@
 import java.util.Arrays;
 import java.util.concurrent.CompletableFuture;
 
+import org.eclipse.swt.SWT;
 import org.eclipse.swt.dnd.DND;
 import org.eclipse.swt.dnd.RTFTransfer;
 import org.eclipse.swt.dnd.TextTransfer;
 import org.eclipse.swt.dnd.Transfer;
 import org.eclipse.swt.dnd.TransferData;
+import org.eclipse.swt.widgets.Shell;
+import org.eclipse.swt.widgets.Text;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
 import org.junit.jupiter.api.Order;
@@ -243,4 +246,137 @@ public void test_getAvailableTypes(int clipboardId) throws Exception {
 		availableTypes = clipboard.getAvailableTypes(clipboardId);
 		assertTrue(Arrays.stream(availableTypes).anyMatch(rtfTransfer::isSupportedType));
 	}
+
+	/**
+	 * tear down and start again without clearing off the clipboard
+	 */
+	private void tearDownAndStartAgain() {
+		// we can't call tearDown because we don't want to clear the clipboard
+		clipboard.dispose();
+		clipboard = null;
+		shell.dispose();
+		shell = null;
+		SwtTestUtil.processEvents();
+
+		// tearDown doesn't dispose on each test, but the error we are checking
+		// for happens when Display is disposed (causing ClipboardProxy to be disposed)
+		display.dispose();
+		display = null;
+
+		// start back up
+		setUp();
+	}
+
+	private void checkStderrForGTK3Warning(String stderr) {
+		if (SwtTestUtil.isGTK3()) {
+			if ("***WARNING: Attempt to access SWT clipboard after disposing SWT Display.\n".equals(stderr)) {
+				// test passed (VM didn't SIGSEGV) and we exercise the problematic code causing
+				// https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+			} else if (stderr.isEmpty()) {
+				assumeTrue(false, """
+						test passed (VM didn't SIGSEGV) but we didn't exercise the problematic code causing
+						https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+						""");
+			} else {
+				// test passed (VM didn't SIGSEGV) but we don't know why there is other stderr
+				// output, log it to the test results for further investigation.
+				assertEquals("", stderr);
+			}
+		} else {
+			assertEquals("", stderr);
+		}
+	}
+
+	private Text setContentsOnClipboardAndDisposeDisplay() throws InterruptedException {
+		openAndFocusShell(false);
+
+		String helloWorld = getUniqueTestString();
+		clipboard.setContents(new Object[] { helloWorld }, new Transfer[] { textTransfer });
+		assertEquals(helloWorld, clipboard.getContents(textTransfer));
+
+		tearDownAndStartAgain();
+		shell = new Shell(display);
+		Text text = new Text(shell, SWT.SINGLE);
+		String textBoxHello = getUniqueTestString();
+		text.setText(textBoxHello);
+		text.setSelection(0, textBoxHello.length());
+
+		return text;
+	}
+
+	/**
+	 * Regression test for
+	 * https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+	 */
+	@Test
+	public void test_AfterNewDisplay_TextCopy() throws Exception {
+		Text text = setContentsOnClipboardAndDisposeDisplay();
+		SwtTestUtil.openShell(shell);
+
+		/*
+		 * on GTK3 this Text.copy would trigger SIGSEGV on GTK calling disposed callback
+		 * ClipboardProxy.clearFunc
+		 */
+		String stderr = SwtTestUtil.runWithCapturedStderr(() -> text.copy());
+		checkStderrForGTK3Warning(stderr);
+
+		// After the test, clear out the partially disposed clipboard by taking
+		// ownership and then clearing it
+		clipboard.setContents(new Object[] { getUniqueTestString() }, new Transfer[] { textTransfer });
+		clipboard.clearContents();
+	}
+
+	/**
+	 * Regression test for
+	 * https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+	 */
+	@Test
+	public void test_AfterNewDisplay_LocalSetContents() throws Exception {
+		setContentsOnClipboardAndDisposeDisplay();
+		SwtTestUtil.openShell(shell);
+
+		/*
+		 * on GTK3 this setContents would lead to undefined behavior because when the
+		 * ClipboardProxy called bind on the new ClipboardProxy.clearFunc there was a
+		 * pretty good chance it would bind to the same entry in the callback table. But
+		 * depending on exact order of operations that table entry could be empty,
+		 * leading to SIGSEGV.
+		 */
+		String stderr = SwtTestUtil.runWithCapturedStderr(
+				() -> clipboard.setContents(new Object[] { getUniqueTestString() }, new Transfer[] { textTransfer }));
+		checkStderrForGTK3Warning(stderr);
+	}
+
+	/**
+	 * Regression test for
+	 * https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+	 */
+	@Test
+	public void test_AfterNewDisplay_RemoteGetContents() throws Exception {
+		setContentsOnClipboardAndDisposeDisplay();
+		openAndFocusRemote();
+		/*
+		 * on GTK3 this getting from the remote would trigger SIGSEGV on GTK calling
+		 * disposed callback ClipboardProxy.getFunc
+		 * https://github.com/eclipse-platform/eclipse.platform.swt/issues/2675
+		 *
+		 * Caveat: What is available on the clipboard after Display dispose is very
+		 * platform and configuration dependent and no guarantees in the SWT API exist.
+		 * Therefore the return value of remote.getStringContents() is not checked here.
+		 */
+		String stderr = SwtTestUtil
+				.runWithCapturedStderr(() -> SwtTestUtil.runOperationInThread(() -> remote.getStringContents()));
+		checkStderrForGTK3Warning(stderr);
+
+		/*
+		 * On GTK3 At this point the old ClipboardProxy has not been disposed because
+		 * this process still owns the clipboard from the original setContents done at
+		 * the beginning of the test. Therefore set some new contents so that the old
+		 * contents can be cleared.
+		 */
+		SwtTestUtil.openShell(shell);
+		stderr = SwtTestUtil.runWithCapturedStderr(
+				() -> clipboard.setContents(new Object[] { getUniqueTestString() }, new Transfer[] { textTransfer }));
+		checkStderrForGTK3Warning(stderr);
+	}
 }