Fix for #678

Fix for `SIGSEGV` in `Tree.cellDataProc(...)` when calling
`TreeItem.setImage(...)`.

Fixes #678

Reproducing the crash:
- #678 (comment)
- #1611
- #678 (comment)

The cause of the crash is described here:
#678 (comment)
In short, the crash happens due to read accesses to an already disposed
renderer.
The sequence of action leading to the crash was:
- in a `Tree` with `SWT.VIRTUAL` a `TreeItem` is rendered for the first
time or after `clear()`
  - `Tree.cellDataProc()` is executed for the item and one of the
renderers
    - `Tree.checkData(item)` is called for the item
      - `SWT.SetData` event is created and sent for the item
        - `TreeItem.setImage() is executed by the event handler for
`SWT.SetData``
         - `Tree.createRenderers()` executes and disposes the current
renderer
    - further actions in `Tree.cellDataProc()` that access the
already-disposed renderer (they think it's alive)

How it's fixed: in `Tree.cellDataProc()` wrap `Tree.checkData(item)`
into `Display.asyncExec()`.

Why fixed this way:
1. on one hand, `Tree.cellDataProc()` is a [cell data
function](https://docs.gtk.org/gtk3/treeview-tutorial.html#cell-data-functions)
which is not supposed to change tree structure. Violation of this leads
to C memory errors.
2. On the other hand, `SWT.SetData` event handlers are written by swt
users and therefore can contain any code.

Using `Display.asyncExec()` to postpone `SWT.SetData` event handlers
until `Tree.cellDataProc()` is finished seems like the most simple and
bullet-proof solution to #678 and all similar bugs.

Author: zkxvh

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Table.java

@@ -15,6 +15,8 @@
 package org.eclipse.swt.widgets;
 
 
+import java.util.*;
+
 import org.eclipse.swt.*;
 import org.eclipse.swt.events.*;
 import org.eclipse.swt.graphics.*;
@@ -98,6 +100,7 @@ public class Table extends Composite {
 	int headerHeight;
 	boolean boundsChangedSinceLastDraw, headerVisible, wasScrolled;
 	boolean rowActivated;
+	SetDataTask setDataTask = new SetDataTask();
 
 	private long headerCSSProvider;
 
@@ -220,26 +223,27 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
 		}
 	}
 	if (modelIndex == -1) return 0;
-	boolean setData = false;
+	boolean updated = false;
 	if ((style & SWT.VIRTUAL) != 0) {
 		if (!item.cached) {
-			lastIndexOf = index[0];
-			setData = checkData (item);
+			setDataTask.enqueueItem (item);
+		}
+		if (item.updated) {
+			updated = true;
+			item.updated = false;
 		}
 	}
 	long [] ptr = new long [1];
-	if (setData) {
-		ptr [0] = 0;
-		if (isPixbuf) {
-			GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
-			OS.g_object_set (cell, OS.gicon, ptr [0], 0);
-			if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
-		} else {
-			GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
-			if (ptr [0] != 0) {
-				OS.g_object_set (cell, OS.text, ptr [0], 0);
-				OS.g_free (ptr [0]);
-			}
+	ptr [0] = 0;
+	if (isPixbuf) {
+		GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
+		OS.g_object_set (cell, OS.gicon, ptr [0], 0);
+		if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
+	} else {
+		GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
+		if (ptr [0] != 0) {
+			OS.g_object_set (cell, OS.text, ptr [0], 0);
+			OS.g_free (ptr [0]);
 		}
 	}
 	if (customDraw) {
@@ -266,7 +270,7 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
 			}
 		}
 	}
-	if (setData) {
+	if (updated) {
 		ignoreCell = cell;
 		setScrollWidth (tree_column, item);
 		ignoreCell = 0;
@@ -4249,4 +4253,48 @@ public void dispose() {
 		headerCSSProvider = 0;
 	}
 }
+
+class SetDataTask implements Runnable {
+	boolean scheduled;
+	LinkedHashSet<TableItem> itemsQueue = new LinkedHashSet<> ();
+
+	void enqueueItem (TableItem item) {
+		itemsQueue.add (item);
+		ensureExecutionScheduled ();
+	}
+
+	void ensureExecutionScheduled () {
+		if (!scheduled && !isDisposed ()) {
+			display.asyncExec (this);
+			scheduled = true;
+		}
+	}
+
+	@Override
+	public void run () {
+		scheduled = false;
+		if (itemsQueue.isEmpty () || isDisposed ()) {
+			return;
+		}
+		LinkedHashSet<TableItem> items = itemsQueue;
+		itemsQueue = new LinkedHashSet<> ();
+		try {
+			for (Iterator<TableItem> it = items.iterator (); it.hasNext ();) {
+				TableItem item = it.next ();
+				it.remove ();
+				if (!item.cached && !item.isDisposed ()) {
+					if (checkData (item)) {
+						item.updated = true;
+					}
+				}
+			}
+		} catch (Throwable t) {
+			if (!items.isEmpty ()) {
+				itemsQueue.addAll (items);
+				ensureExecutionScheduled ();
+			}
+			throw t;
+		}
+	}
+}
 }

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/TableItem.java

@@ -44,7 +44,7 @@ public class TableItem extends Item {
 	Font font;
 	Font[] cellFont;
 	String [] strings;
-	boolean cached, grayed, settingData;
+	boolean cached, grayed, updated, settingData;
 
 /**
  * Constructs a new instance of this class given its parent

bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java

@@ -110,6 +110,7 @@ public class Tree extends Composite {
 	Color headerBackground, headerForeground;
 	boolean boundsChangedSinceLastDraw, wasScrolled;
 	boolean rowActivated;
+	SetDataTask setDataTask = new SetDataTask();
 
 	private long headerCSSProvider;
 
@@ -296,32 +297,28 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
 		}
 	}
 	if (modelIndex == -1) return 0;
-	boolean setData = false;
 	boolean updated = false;
 	if ((style & SWT.VIRTUAL) != 0) {
 		if (!item.cached) {
-			//lastIndexOf = index [0];
-			setData = checkData (item);
+			setDataTask.enqueueItem (item);
 		}
 		if (item.updated) {
 			updated = true;
 			item.updated = false;
 		}
 	}
 	long [] ptr = new long [1];
-	if (setData) {
-		if (isPixbuf) {
-			ptr [0] = 0;
-			GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
-			OS.g_object_set (cell, OS.gicon, ptr [0], 0);
-			if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
-		} else {
-			ptr [0] = 0;
-			GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
-			if (ptr [0] != 0) {
-				OS.g_object_set (cell, OS.text, ptr[0], 0);
-				OS.g_free (ptr[0]);
-			}
+	if (isPixbuf) {
+		ptr [0] = 0;
+		GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
+		OS.g_object_set (cell, OS.gicon, ptr [0], 0);
+		if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
+	} else {
+		ptr [0] = 0;
+		GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
+		if (ptr [0] != 0) {
+			OS.g_object_set (cell, OS.text, ptr[0], 0);
+			OS.g_free (ptr[0]);
 		}
 	}
 	if (customDraw) {
@@ -348,7 +345,7 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
 			}
 		}
 	}
-	if (setData || updated) {
+	if (updated) {
 		ignoreCell = cell;
 		setScrollWidth (tree_column, item);
 		ignoreCell = 0;
@@ -4333,4 +4330,48 @@ public void dispose() {
 		headerCSSProvider = 0;
 	}
 }
+
+class SetDataTask implements Runnable {
+	boolean scheduled;
+	LinkedHashSet<TreeItem> itemsQueue = new LinkedHashSet<> ();
+
+	void enqueueItem (TreeItem item) {
+		itemsQueue.add (item);
+		ensureExecutionScheduled ();
+	}
+
+	void ensureExecutionScheduled () {
+		if (!scheduled && !isDisposed ()) {
+			display.asyncExec (this);
+			scheduled = true;
+		}
+	}
+
+	@Override
+	public void run () {
+		scheduled = false;
+		if (itemsQueue.isEmpty () || isDisposed ()) {
+			return;
+		}
+		LinkedHashSet<TreeItem> items = itemsQueue;
+		itemsQueue = new LinkedHashSet<> ();
+		try {
+			for (Iterator<TreeItem> it = items.iterator (); it.hasNext ();) {
+				TreeItem item = it.next ();
+				it.remove ();
+				if (!item.cached && !item.isDisposed ()) {
+					if (checkData (item)) {
+						item.updated = true;
+					}
+				}
+			}
+		} catch (Throwable t) {
+			if (!items.isEmpty ()) {
+				itemsQueue.addAll (items);
+				ensureExecutionScheduled ();
+			}
+			throw t;
+		}
+	}
+}
 }

tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Issue678_JvmCrashOnTreeItemSetImage.java

@@ -0,0 +1,89 @@
+package org.eclipse.swt.tests.gtk.snippets;
+
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.graphics.Image;
+import org.eclipse.swt.layout.FillLayout;
+import org.eclipse.swt.widgets.Display;
+import org.eclipse.swt.widgets.Shell;
+import org.eclipse.swt.widgets.Tree;
+import org.eclipse.swt.widgets.TreeItem;
+
+/**
+ * Description: when {@link TreeItem#setImage(Image)} is called within an
+ * {@link SWT#SetData} event handler for a {@link SWT#VIRTUAL} tree, then a JVM
+ * crash can happen because of use-after-free gtk3 renderer in
+ * {@code Tree.cellDataProc()}.
+ * <p>
+ *
+ * <pre>
+ * Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
+ * C  [libgobject-2.0.so.0+0x3b251]  g_type_check_instance_is_fundamentally_a+0x11
+ * C  [libswt-pi3-gtk-4958r2.so+0x4b609]  Java_org_eclipse_swt_internal_gtk_OS_g_1object_1set__J_3BJJ+0x4a
+ *
+ * Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
+ * J 11988  org.eclipse.swt.internal.gtk.OS.g_object_set(J[BJJ)V (0 bytes)
+ * J 10921 c1 org.eclipse.swt.widgets.Tree.cellDataProc(JJJJJ)J (486 bytes)
+ * J 10920 c1 org.eclipse.swt.widgets.Display.cellDataProc(JJJJJ)J (29 bytes)
+ * v  ~StubRoutines::call_stub
+ * J 11619  org.eclipse.swt.internal.gtk3.GTK3.gtk_main_iteration_do(Z)Z (0 bytes)
+ * J 11623 c1 org.eclipse.swt.widgets.Display.readAndDispatch()Z (88 bytes)
+ * </pre>
+ *
+ * Tested on GTK 3.24.37 (Fedora 38)
+ */
+public class Issue678_JvmCrashOnTreeItemSetImage {
+
+	private static final int NUM_ITERATIONS = 100;
+
+	public static void main (String[] args) {
+		Display display = new Display ();
+		Shell shell = new Shell (display);
+		shell.setSize (400, 300);
+		shell.setLayout (new FillLayout ());
+		shell.open ();
+		Image image = new Image (display, 20, 20);
+
+		for (int i = 0; i < NUM_ITERATIONS; i++) {
+			Tree tree = new Tree (shell, SWT.VIRTUAL);
+			tree.addListener (SWT.SetData, e -> {
+				TreeItem item = (TreeItem) e.item;
+				item.setText (0, "A");
+
+				// for some reason sleeping increases probability of crash
+				try {
+					Thread.sleep (50);
+				} catch (InterruptedException ex) {
+					throw new RuntimeException (ex);
+				}
+
+				item.setImage (image); // <-- this is the critical line!
+			});
+			tree.setItemCount (1);
+			shell.layout ();
+
+			waitUntilIdle ();
+
+			tree.dispose ();
+		}
+
+		display.dispose ();
+	}
+
+	private static void waitUntilIdle () {
+		long lastActive = System.currentTimeMillis ();
+		while (true) {
+			if (Thread.interrupted ()) {
+				throw new AssertionError ();
+			}
+			if (Display.getCurrent ().readAndDispatch ()) {
+				lastActive = System.currentTimeMillis ();
+			} else {
+				if (lastActive + 10 < System.currentTimeMillis ()) {
+					return;
+				}
+				Thread.yield ();
+			}
+		}
+	}
+
+}