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.
eclipse-platform · Jan 10, 2025 · 04e2d7a · 04e2d7a
1 parent d7febc4
commit 04e2d7a
Show file tree

Hide file tree

Showing 3 changed files with 123 additions and 34 deletions.
diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Table.java b/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;
+		}
+	}
+}
 }
diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/TableItem.java b/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

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java b/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;
+		}
+	}
+}
 }