From 72c21efffdc46ca850c43706459dd0ea896703ce Mon Sep 17 00:00:00 2001 From: zkxvh Date: Thu, 9 Jan 2025 18:15:39 +0500 Subject: [PATCH] Fix for #678 Fix for `SIGSEGV` in `Tree.cellDataProc(...)` when calling `TreeItem.setImage(...)`. Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/678 Reproducing the crash: - https://github.com/eclipse-platform/eclipse.platform.swt/issues/678#issue-1715955828 - https://github.com/eclipse-platform/eclipse.platform.swt/pull/1611/ - https://github.com/eclipse-platform/eclipse.platform.swt/issues/678#issuecomment-2507994360 The cause of the crash is described here: https://github.com/eclipse-platform/eclipse.platform.swt/issues/678#issuecomment-2511711696 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. --- .../gtk/org/eclipse/swt/widgets/Table.java | 80 ++++++++++++++---- .../org/eclipse/swt/widgets/TableItem.java | 2 +- .../gtk/org/eclipse/swt/widgets/Tree.java | 82 ++++++++++++++----- 3 files changed, 128 insertions(+), 36 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 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..da9b576ae9e 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; @@ -356,9 +353,12 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long return 0; } +boolean isDataSetRequired (TreeItem item) { + return !item.cached && ((style & SWT.VIRTUAL) != 0); +} + boolean checkData (TreeItem item) { - if (item.cached) return true; - if ((style & SWT.VIRTUAL) != 0) { + if (isDataSetRequired(item)) { item.cached = true; TreeItem parentItem = item.getParentItem (); Event event = new Event (); @@ -4333,4 +4333,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; + } + } +} }