Skip to content

Commit 72c21ef

Browse files
zkxvhzkxvh
committed
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.
1 parent d7febc4 commit 72c21ef

File tree

3 files changed

+128
-36
lines changed

3 files changed

+128
-36
lines changed

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

Lines changed: 64 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package org.eclipse.swt.widgets;
1616

1717

18+
import java.util.*;
19+
1820
import org.eclipse.swt.*;
1921
import org.eclipse.swt.events.*;
2022
import org.eclipse.swt.graphics.*;
@@ -98,6 +100,7 @@ public class Table extends Composite {
98100
int headerHeight;
99101
boolean boundsChangedSinceLastDraw, headerVisible, wasScrolled;
100102
boolean rowActivated;
103+
SetDataTask setDataTask = new SetDataTask();
101104

102105
private long headerCSSProvider;
103106

@@ -220,26 +223,27 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
220223
}
221224
}
222225
if (modelIndex == -1) return 0;
223-
boolean setData = false;
226+
boolean updated = false;
224227
if ((style & SWT.VIRTUAL) != 0) {
225228
if (!item.cached) {
226-
lastIndexOf = index[0];
227-
setData = checkData (item);
229+
setDataTask.enqueueItem (item);
230+
}
231+
if (item.updated) {
232+
updated = true;
233+
item.updated = false;
228234
}
229235
}
230236
long [] ptr = new long [1];
231-
if (setData) {
232-
ptr [0] = 0;
233-
if (isPixbuf) {
234-
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
235-
OS.g_object_set (cell, OS.gicon, ptr [0], 0);
236-
if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
237-
} else {
238-
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
239-
if (ptr [0] != 0) {
240-
OS.g_object_set (cell, OS.text, ptr [0], 0);
241-
OS.g_free (ptr [0]);
242-
}
237+
ptr [0] = 0;
238+
if (isPixbuf) {
239+
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
240+
OS.g_object_set (cell, OS.gicon, ptr [0], 0);
241+
if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
242+
} else {
243+
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
244+
if (ptr [0] != 0) {
245+
OS.g_object_set (cell, OS.text, ptr [0], 0);
246+
OS.g_free (ptr [0]);
243247
}
244248
}
245249
if (customDraw) {
@@ -266,7 +270,7 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
266270
}
267271
}
268272
}
269-
if (setData) {
273+
if (updated) {
270274
ignoreCell = cell;
271275
setScrollWidth (tree_column, item);
272276
ignoreCell = 0;
@@ -4249,4 +4253,48 @@ public void dispose() {
42494253
headerCSSProvider = 0;
42504254
}
42514255
}
4256+
4257+
class SetDataTask implements Runnable {
4258+
boolean scheduled;
4259+
LinkedHashSet<TableItem> itemsQueue = new LinkedHashSet<> ();
4260+
4261+
void enqueueItem (TableItem item) {
4262+
itemsQueue.add (item);
4263+
ensureExecutionScheduled ();
4264+
}
4265+
4266+
void ensureExecutionScheduled () {
4267+
if (!scheduled && !isDisposed ()) {
4268+
display.asyncExec (this);
4269+
scheduled = true;
4270+
}
4271+
}
4272+
4273+
@Override
4274+
public void run () {
4275+
scheduled = false;
4276+
if (itemsQueue.isEmpty () || isDisposed ()) {
4277+
return;
4278+
}
4279+
LinkedHashSet<TableItem> items = itemsQueue;
4280+
itemsQueue = new LinkedHashSet<> ();
4281+
try {
4282+
for (Iterator<TableItem> it = items.iterator (); it.hasNext ();) {
4283+
TableItem item = it.next ();
4284+
it.remove ();
4285+
if (!item.cached && !item.isDisposed ()) {
4286+
if (checkData (item)) {
4287+
item.updated = true;
4288+
}
4289+
}
4290+
}
4291+
} catch (Throwable t) {
4292+
if (!items.isEmpty ()) {
4293+
itemsQueue.addAll (items);
4294+
ensureExecutionScheduled ();
4295+
}
4296+
throw t;
4297+
}
4298+
}
4299+
}
42524300
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public class TableItem extends Item {
4444
Font font;
4545
Font[] cellFont;
4646
String [] strings;
47-
boolean cached, grayed, settingData;
47+
boolean cached, grayed, updated, settingData;
4848

4949
/**
5050
* Constructs a new instance of this class given its parent

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

Lines changed: 63 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ public class Tree extends Composite {
110110
Color headerBackground, headerForeground;
111111
boolean boundsChangedSinceLastDraw, wasScrolled;
112112
boolean rowActivated;
113+
SetDataTask setDataTask = new SetDataTask();
113114

114115
private long headerCSSProvider;
115116

@@ -296,32 +297,28 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
296297
}
297298
}
298299
if (modelIndex == -1) return 0;
299-
boolean setData = false;
300300
boolean updated = false;
301301
if ((style & SWT.VIRTUAL) != 0) {
302302
if (!item.cached) {
303-
//lastIndexOf = index [0];
304-
setData = checkData (item);
303+
setDataTask.enqueueItem (item);
305304
}
306305
if (item.updated) {
307306
updated = true;
308307
item.updated = false;
309308
}
310309
}
311310
long [] ptr = new long [1];
312-
if (setData) {
313-
if (isPixbuf) {
314-
ptr [0] = 0;
315-
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
316-
OS.g_object_set (cell, OS.gicon, ptr [0], 0);
317-
if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
318-
} else {
319-
ptr [0] = 0;
320-
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
321-
if (ptr [0] != 0) {
322-
OS.g_object_set (cell, OS.text, ptr[0], 0);
323-
OS.g_free (ptr[0]);
324-
}
311+
if (isPixbuf) {
312+
ptr [0] = 0;
313+
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
314+
OS.g_object_set (cell, OS.gicon, ptr [0], 0);
315+
if (ptr [0] != 0) OS.g_object_unref (ptr [0]);
316+
} else {
317+
ptr [0] = 0;
318+
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_TEXT, ptr, -1);
319+
if (ptr [0] != 0) {
320+
OS.g_object_set (cell, OS.text, ptr[0], 0);
321+
OS.g_free (ptr[0]);
325322
}
326323
}
327324
if (customDraw) {
@@ -348,17 +345,20 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
348345
}
349346
}
350347
}
351-
if (setData || updated) {
348+
if (updated) {
352349
ignoreCell = cell;
353350
setScrollWidth (tree_column, item);
354351
ignoreCell = 0;
355352
}
356353
return 0;
357354
}
358355

356+
boolean isDataSetRequired (TreeItem item) {
357+
return !item.cached && ((style & SWT.VIRTUAL) != 0);
358+
}
359+
359360
boolean checkData (TreeItem item) {
360-
if (item.cached) return true;
361-
if ((style & SWT.VIRTUAL) != 0) {
361+
if (isDataSetRequired(item)) {
362362
item.cached = true;
363363
TreeItem parentItem = item.getParentItem ();
364364
Event event = new Event ();
@@ -4333,4 +4333,48 @@ public void dispose() {
43334333
headerCSSProvider = 0;
43344334
}
43354335
}
4336+
4337+
class SetDataTask implements Runnable {
4338+
boolean scheduled;
4339+
LinkedHashSet<TreeItem> itemsQueue = new LinkedHashSet<> ();
4340+
4341+
void enqueueItem (TreeItem item) {
4342+
itemsQueue.add (item);
4343+
ensureExecutionScheduled ();
4344+
}
4345+
4346+
void ensureExecutionScheduled () {
4347+
if (!scheduled && !isDisposed ()) {
4348+
display.asyncExec (this);
4349+
scheduled = true;
4350+
}
4351+
}
4352+
4353+
@Override
4354+
public void run () {
4355+
scheduled = false;
4356+
if (itemsQueue.isEmpty () || isDisposed ()) {
4357+
return;
4358+
}
4359+
LinkedHashSet<TreeItem> items = itemsQueue;
4360+
itemsQueue = new LinkedHashSet<> ();
4361+
try {
4362+
for (Iterator<TreeItem> it = items.iterator (); it.hasNext ();) {
4363+
TreeItem item = it.next ();
4364+
it.remove ();
4365+
if (!item.cached && !item.isDisposed ()) {
4366+
if (checkData (item)) {
4367+
item.updated = true;
4368+
}
4369+
}
4370+
}
4371+
} catch (Throwable t) {
4372+
if (!items.isEmpty ()) {
4373+
itemsQueue.addAll (items);
4374+
ensureExecutionScheduled ();
4375+
}
4376+
throw t;
4377+
}
4378+
}
4379+
}
43364380
}

0 commit comments

Comments
 (0)