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 index d2d99021f65..8f967aaa1b0 100644 --- 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 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 items = itemsQueue; + itemsQueue = new LinkedHashSet<> (); + try { + for (Iterator 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 index 92af154acff..b5f4b5e85ae 100644 --- 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 index 808623da98d..8b90c4d9fb7 100644 --- 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,12 +297,10 @@ 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; @@ -309,19 +308,17 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long } } 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 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 items = itemsQueue; + itemsQueue = new LinkedHashSet<> (); + try { + for (Iterator 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; + } + } +} } diff --git a/tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Issue678_JvmCrashOnTreeItemSetImage.java b/tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Issue678_JvmCrashOnTreeItemSetImage.java new file mode 100644 index 00000000000..24e636a4f3f --- /dev/null +++ b/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()}. + *

+ * + * 

+ * 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)
+ *
+ * + * 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 (); + } + } + } + +}