Skip to content

Commit 0e10b93

Browse files
committed
SIGSEGV in Tree.cellDataProc when calling TreeItem.setImage eclipse-platform#678
Fix and test for bug eclipse-platform#678 Fixes eclipse-platform#678 Reproducing the crash: - eclipse-platform#678 (comment) - eclipse-platform#1611 - eclipse-platform#678 (comment) The cause of the crash is described here: eclipse-platform#678 (comment) In short, the problem was executing `Tree.createRenderers()` inside `TreeItem.setImage()`. 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 it's renderers `Tree.checkData(item)` is called for the item `SWT.SetData` is invoked and executes `TreeItem.setImage()` `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: 1. set fixed height+width to pixbuf renderers with `GTK.gtk_cell_renderer_set_fixed_size` This does 2 things: - from now on it makes images rendered by the renderer to be rendered with these height+width - from now on fixed row height calculation will return height of at least fixed height of the pixbuf 2. make `GtkTreeView` recompute it's internally stored fixed row height with re-setting `fixed_height_mode` of the GtkTreeView 3. make all existing tree rows update their height by invoking `gtk_tree_view_row_changed()` on each of the rows. (2) and (3) can also be achieved by creating a copy of the current `GtkTreeModel` and assigning it to the `GtkTreeView`. But this also resets current selected rows, focus and scroll position; that's why I chose re-setting `fixed_height_mode` + `gtk_tree_view_row_changed()` instead.
1 parent 4bf1af8 commit 0e10b93

File tree

3 files changed

+265
-47
lines changed

3 files changed

+265
-47
lines changed

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

+74
Original file line numberDiff line numberDiff line change
@@ -4319,6 +4319,80 @@ void checkSetDataInProcessBeforeRemoval() {
43194319
}
43204320
}
43214321

4322+
boolean initPixbufSize(Image image) {
4323+
if (pixbufSizeSet || image == null) {
4324+
return false;
4325+
}
4326+
int iWidth, iHeight;
4327+
if (DPIUtil.useCairoAutoScale()) {
4328+
iWidth = image.getBounds().width;
4329+
iHeight = image.getBounds().height;
4330+
} else {
4331+
iWidth = image.getBoundsInPixels().width;
4332+
iHeight = image.getBoundsInPixels().height;
4333+
}
4334+
if (iWidth <= 0 || iHeight <= 0) {
4335+
return false;
4336+
}
4337+
pixbufSizeSet = true;
4338+
pixbufHeight = iHeight;
4339+
pixbufWidth = iWidth;
4340+
/*
4341+
* Feature in GTK: a Tree with the style SWT.VIRTUAL has fixed-height-mode
4342+
* enabled. This will limit the size of any cells, including renderers. In order
4343+
* to prevent images from disappearing/being cropped, we update row heights when
4344+
* the first image is set. Fix for bug 480261.
4345+
*/
4346+
if ((style & SWT.VIRTUAL) != 0) {
4347+
resetFixedRowHeight();
4348+
}
4349+
return true;
4350+
}
4351+
4352+
private void resetFixedRowHeight() {
4353+
long columnList = GTK.gtk_tree_view_get_columns(handle);
4354+
if (columnList == 0) {
4355+
return;
4356+
}
4357+
4358+
// set fixed width and height for all GtkCellRendererPixbufs
4359+
for (var colItem = columnList; colItem != 0; colItem = OS.g_list_next(colItem)) {
4360+
long column = OS.g_list_data(colItem);
4361+
var cellList = GTK.gtk_cell_layout_get_cells(column);
4362+
for (var cellItem = cellList; cellItem != 0; cellItem = OS.g_list_next(cellItem)) {
4363+
var renderer = OS.g_list_data(cellItem);
4364+
if (GTK.GTK_IS_CELL_RENDERER_PIXBUF(renderer)) {
4365+
GTK.gtk_cell_renderer_set_fixed_size(renderer, pixbufWidth, pixbufHeight);
4366+
}
4367+
}
4368+
OS.g_list_free(cellList);
4369+
}
4370+
OS.g_list_free(columnList);
4371+
4372+
// Executed in asyncExec because when this method is invoked from checkData(),
4373+
// then the "row_changed" signal is blocked (but we need it unblocked to invalidate
4374+
// existing rows).
4375+
display.asyncExec(() -> {
4376+
if (!isDisposed() && modelHandle != 0) {
4377+
// reset computed 'fixed_height' to '-1'
4378+
OS.g_object_set(handle, OS.fixed_height_mode, false, 0);
4379+
OS.g_object_set(handle, OS.fixed_height_mode, true, 0);
4380+
4381+
// to update height of the existing rows we need to invalidate them in gtk
4382+
// we do that by invoking gtk_tree_view_row_changed on each of them
4383+
long iter = OS.g_malloc(GTK.GtkTreeIter_sizeof());
4384+
if (GTK.gtk_tree_model_get_iter_first(modelHandle, iter)) {
4385+
int[] value = new int[1];
4386+
do {
4387+
GTK.gtk_tree_model_get(modelHandle, iter, ID_COLUMN, value, -1);
4388+
GTK.gtk_tree_store_set(modelHandle, iter, ID_COLUMN, value[0], -1);
4389+
} while (GTK.gtk_tree_model_iter_next(modelHandle, iter));
4390+
}
4391+
OS.g_free(iter);
4392+
}
4393+
});
4394+
}
4395+
43224396
private void throwCannotRemoveItem(int i) {
43234397
String message = "Cannot remove item with index " + i + ".";
43244398
throw new SWTException(message);

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

+6-47
Original file line numberDiff line numberDiff line change
@@ -1519,59 +1519,18 @@ public void setImage(int index, Image image) {
15191519
pixbuf = ImageList.createPixbuf(surface);
15201520
}
15211521

1522-
int modelIndex = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex;
1523-
long parentHandle = parent.handle;
1524-
long column = GTK.gtk_tree_view_get_column (parentHandle, index);
1525-
long pixbufRenderer = parent.getPixbufRenderer (column);
1526-
int [] currentWidth = new int [1];
1527-
int [] currentHeight= new int [1];
1528-
GTK.gtk_cell_renderer_get_fixed_size (pixbufRenderer, currentWidth, currentHeight);
15291522
if (!parent.pixbufSizeSet) {
1530-
if (image != null) {
1531-
int iWidth, iHeight;
1532-
if (DPIUtil.useCairoAutoScale()) {
1533-
iWidth = image.getBounds ().width;
1534-
iHeight = image.getBounds ().height;
1535-
} else {
1536-
iWidth = image.getBoundsInPixels ().width;
1537-
iHeight = image.getBoundsInPixels ().height;
1538-
}
1539-
if (iWidth > currentWidth [0] || iHeight > currentHeight [0]) {
1540-
GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, iWidth, iHeight);
1541-
parent.pixbufSizeSet = true;
1542-
parent.pixbufHeight = iHeight;
1543-
parent.pixbufWidth = iWidth;
1544-
/*
1545-
* Feature in GTK: a Tree with the style SWT.VIRTUAL has
1546-
* fixed-height-mode enabled. This will limit the size of
1547-
* any cells, including renderers. In order to prevent
1548-
* images from disappearing/being cropped, we re-create
1549-
* the renderers when the first image is set. Fix for
1550-
* bug 480261.
1551-
*/
1552-
if ((parent.style & SWT.VIRTUAL) != 0) {
1553-
/*
1554-
* Only re-create SWT.CHECK renderers if this is the first column.
1555-
* Otherwise check-boxes will be rendered in columns they are not
1556-
* supposed to be rendered in. See bug 513761.
1557-
*/
1558-
boolean check = modelIndex == Tree.FIRST_COLUMN && (parent.style & SWT.CHECK) != 0;
1559-
parent.createRenderers(column, modelIndex, check, parent.style);
1560-
}
1561-
}
1562-
}
1523+
parent.initPixbufSize (image);
15631524
} else {
1525+
long column = GTK.gtk_tree_view_get_column (parent.handle, index);
1526+
long pixbufRenderer = parent.getPixbufRenderer (column);
15641527
/*
1565-
* Bug 483112: We check to see if the cached value is greater than the size of the pixbufRenderer.
1566-
* If it is, then we change the size of the pixbufRenderer accordingly.
1567-
* Bug 489025: There is a corner case where the below is triggered when current(Width|Height) is -1,
1568-
* which results in icons being set to 0. Fix is to compare only positive sizes.
1528+
* Bug 483112: make sure width and height are set for the pixbufRenderer
15691529
*/
1570-
if (parent.pixbufWidth > Math.max(currentWidth [0], 0) || parent.pixbufHeight > Math.max(currentHeight [0], 0)) {
1571-
GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, parent.pixbufWidth, parent.pixbufHeight);
1572-
}
1530+
GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, parent.pixbufWidth, parent.pixbufHeight);
15731531
}
15741532

1533+
int modelIndex = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex;
15751534
GTK.gtk_tree_store_set(parent.modelHandle, handle, modelIndex + Tree.CELL_PIXBUF, pixbuf, -1);
15761535
/*
15771536
* Bug 573633: gtk_tree_store_set() will reference the handle. So we unref the pixbuf here,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
package org.eclipse.swt.tests.gtk;
2+
3+
import java.util.function.Supplier;
4+
5+
import org.eclipse.swt.SWT;
6+
import org.eclipse.swt.graphics.GC;
7+
import org.eclipse.swt.graphics.Image;
8+
import org.eclipse.swt.layout.GridData;
9+
import org.eclipse.swt.layout.GridLayout;
10+
import org.eclipse.swt.widgets.Shell;
11+
import org.eclipse.swt.widgets.Tree;
12+
import org.eclipse.swt.widgets.TreeItem;
13+
import org.junit.After;
14+
import org.junit.Assert;
15+
import org.junit.Before;
16+
import org.junit.Test;
17+
18+
public class Test_Gtk_Tree_Virtual_setImage {
19+
20+
private static final int WINDOW_WIDTH = 300;
21+
private static final int WINDOW_HEIGHT = 100;
22+
private static final int IMAGE_SIZE = 70;
23+
24+
private Shell shell;
25+
private Tree tree;
26+
private Image image;
27+
28+
@Before
29+
public void setUp() {
30+
shell = new Shell();
31+
shell.setLayout(new GridLayout());
32+
}
33+
34+
@After
35+
public void tearDown() {
36+
if (tree != null) {
37+
tree.dispose();
38+
tree = null;
39+
}
40+
if (image != null) {
41+
image.dispose();
42+
image = null;
43+
}
44+
if (shell != null) {
45+
shell.dispose();
46+
shell = null;
47+
}
48+
}
49+
50+
@Test
51+
public void test_initial_img_5_2() {
52+
do_test_initial_img(false, 5, 2);
53+
}
54+
55+
@Test
56+
public void test_initial_img_5_2_wCheckbox() {
57+
do_test_initial_img(true, 5, 2);
58+
}
59+
60+
@Test
61+
public void test_initial_img_50_22() {
62+
do_test_initial_img(false, 50, 22);
63+
}
64+
65+
@Test
66+
public void test_initial_img_50_22_wCheckbox() {
67+
do_test_initial_img(true, 50, 22);
68+
}
69+
70+
@Test
71+
public void test_set_img_50_22() {
72+
do_test_set_img(false, 50, 22);
73+
}
74+
75+
@Test
76+
public void test_set_img_50_22_wCheckbox() {
77+
do_test_set_img(true, 50, 22);
78+
}
79+
80+
private void do_test_initial_img(boolean withCheckbox, int totalRows, int imgRowIdx) {
81+
createImage();
82+
createTree(withCheckbox, totalRows, imgRowIdx, () -> image);
83+
shell.pack();
84+
shell.open();
85+
86+
executePendingUiTasks();
87+
88+
tree.showItem(tree.getItem(imgRowIdx));
89+
90+
executePendingUiTasks();
91+
92+
assertAllTreeRowsResized();
93+
}
94+
95+
private void do_test_set_img(boolean withCheckbox, int totalRows, int imgRowIdx) {
96+
createTree(withCheckbox, totalRows, imgRowIdx, () -> image);
97+
shell.pack();
98+
shell.open();
99+
100+
executePendingUiTasks();
101+
102+
tree.showItem(tree.getItem(imgRowIdx));
103+
104+
executePendingUiTasks();
105+
106+
tree.showItem(tree.getItem(totalRows - 1));
107+
108+
executePendingUiTasks();
109+
110+
createImage();
111+
112+
executePendingUiTasks();
113+
114+
tree.clear(imgRowIdx, false);
115+
116+
executePendingUiTasks();
117+
118+
tree.showItem(tree.getItem(imgRowIdx));
119+
120+
executePendingUiTasks();
121+
122+
assertAllTreeRowsResized();
123+
}
124+
125+
private void createImage() {
126+
this.image = new Image(shell.getDisplay(), IMAGE_SIZE, IMAGE_SIZE);
127+
var color = shell.getDisplay().getSystemColor(SWT.COLOR_GREEN);
128+
var gc = new GC(image);
129+
gc.setForeground(color);
130+
gc.setBackground(color);
131+
gc.fillRectangle(0, 0, IMAGE_SIZE, IMAGE_SIZE);
132+
gc.dispose();
133+
}
134+
135+
private void createTree(boolean withCheckbox, int totalRows, int imgRowIdx, Supplier<Image> getImage) {
136+
tree = new Tree(shell, SWT.VIRTUAL | SWT.BORDER | (withCheckbox ? SWT.CHECK : 0));
137+
var layoutData = new GridData(GridData.FILL_BOTH);
138+
layoutData.widthHint = WINDOW_WIDTH;
139+
layoutData.heightHint = WINDOW_HEIGHT;
140+
tree.setLayoutData(layoutData);
141+
tree.addListener(SWT.SetData, event -> {
142+
var item = (TreeItem) event.item;
143+
var idx = tree.indexOf(item);
144+
item.setText("node " + idx);
145+
if (idx == imgRowIdx) {
146+
var image = getImage.get();
147+
item.setImage(image);
148+
}
149+
item.setItemCount(0);
150+
});
151+
tree.setItemCount(totalRows);
152+
}
153+
154+
private void executePendingUiTasks() {
155+
while (shell.getDisplay().readAndDispatch()) {
156+
// perform next pending task
157+
}
158+
try {
159+
// This sleep is required because
160+
// - gtk perform some updates/renderings on its own inner
161+
// timers (e.g. a timer for every frame drawing)
162+
// - readAndDispatch() doesn't wait for those timers
163+
// - we need to wait for this timers to get fully updated/rendered tree
164+
Thread.sleep(100);
165+
} catch (InterruptedException e) {
166+
Thread.currentThread().interrupt();
167+
throw new RuntimeException(e);
168+
}
169+
while (shell.getDisplay().readAndDispatch()) {
170+
// perform next pending task
171+
}
172+
}
173+
174+
private void assertAllTreeRowsResized() {
175+
var minHeight = IMAGE_SIZE;
176+
var maxHeight = 2 * IMAGE_SIZE - 1;
177+
for (var i = 0; i < tree.getItemCount(); i++) {
178+
var item = tree.getItem(i);
179+
var height = item.getBounds().height;
180+
Assert.assertTrue(
181+
String.format("Expected row[%d].height in [%d;%d], but was %d.", i, minHeight, maxHeight, height),
182+
minHeight <= height && height <= maxHeight);
183+
}
184+
}
185+
}

0 commit comments

Comments
 (0)