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.

Author: zkxvh

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

@@ -4319,6 +4319,80 @@ void checkSetDataInProcessBeforeRemoval() {
 	}
 }
 
+boolean initPixbufSize(Image image) {
+	if (pixbufSizeSet || image == null) {
+		return false;
+	}
+	int iWidth, iHeight;
+	if (DPIUtil.useCairoAutoScale()) {
+		iWidth = image.getBounds().width;
+		iHeight = image.getBounds().height;
+	} else {
+		iWidth = image.getBoundsInPixels().width;
+		iHeight = image.getBoundsInPixels().height;
+	}
+	if (iWidth <= 0 || iHeight <= 0) {
+		return false;
+	}
+	pixbufSizeSet = true;
+	pixbufHeight = iHeight;
+	pixbufWidth = iWidth;
+	/*
+	 * Feature in GTK: a Tree with the style SWT.VIRTUAL has fixed-height-mode
+	 * enabled. This will limit the size of any cells, including renderers. In order
+	 * to prevent images from disappearing/being cropped, we update row heights when
+	 * the first image is set. Fix for bug 480261.
+	 */
+	if ((style & SWT.VIRTUAL) != 0) {
+		resetFixedRowHeight();
+	}
+	return true;
+}
+
+private void resetFixedRowHeight() {
+	long columnList = GTK.gtk_tree_view_get_columns(handle);
+	if (columnList == 0) {
+		return;
+	}
+
+	// set fixed width and height for all GtkCellRendererPixbufs
+	for (var colItem = columnList; colItem != 0; colItem = OS.g_list_next(colItem)) {
+		long column = OS.g_list_data(colItem);
+		var cellList = GTK.gtk_cell_layout_get_cells(column);
+		for (var cellItem = cellList; cellItem != 0; cellItem = OS.g_list_next(cellItem)) {
+			var renderer = OS.g_list_data(cellItem);
+			if (GTK.GTK_IS_CELL_RENDERER_PIXBUF(renderer)) {
+				GTK.gtk_cell_renderer_set_fixed_size(renderer, pixbufWidth, pixbufHeight);
+			}
+		}
+		OS.g_list_free(cellList);
+	}
+	OS.g_list_free(columnList);
+
+	// Executed in asyncExec because when this method is invoked from checkData(),
+	// then the "row_changed" signal is blocked (but we need it unblocked to invalidate
+	// existing rows).
+	display.asyncExec(() -> {
+		if (!isDisposed() && modelHandle != 0) {
+			// reset computed 'fixed_height' to '-1'
+			OS.g_object_set(handle, OS.fixed_height_mode, false, 0);
+			OS.g_object_set(handle, OS.fixed_height_mode, true, 0);
+
+			// to update height of the existing rows we need to invalidate them in gtk
+			// we do that by invoking gtk_tree_view_row_changed on each of them
+			long iter = OS.g_malloc(GTK.GtkTreeIter_sizeof());
+			if (GTK.gtk_tree_model_get_iter_first(modelHandle, iter)) {
+				int[] value = new int[1];
+				do {
+					GTK.gtk_tree_model_get(modelHandle, iter, ID_COLUMN, value, -1);
+					GTK.gtk_tree_store_set(modelHandle, iter, ID_COLUMN, value[0], -1);
+				} while (GTK.gtk_tree_model_iter_next(modelHandle, iter));
+			}
+			OS.g_free(iter);
+		}
+	});
+}
+
 private void throwCannotRemoveItem(int i) {
 	String message = "Cannot remove item with index " + i + ".";
 	throw new SWTException(message);

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

@@ -1519,59 +1519,18 @@ public void setImage(int index, Image image) {
 		pixbuf = ImageList.createPixbuf(surface);
 	}
 
-	int modelIndex = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex;
-	long parentHandle = parent.handle;
-	long column = GTK.gtk_tree_view_get_column (parentHandle, index);
-	long pixbufRenderer = parent.getPixbufRenderer (column);
-	int [] currentWidth = new int [1];
-	int [] currentHeight= new int [1];
-	GTK.gtk_cell_renderer_get_fixed_size (pixbufRenderer, currentWidth, currentHeight);
 	if (!parent.pixbufSizeSet) {
-		if (image != null) {
-			int iWidth, iHeight;
-			if (DPIUtil.useCairoAutoScale()) {
-				iWidth = image.getBounds ().width;
-				iHeight = image.getBounds ().height;
-			} else {
-				iWidth = image.getBoundsInPixels ().width;
-				iHeight = image.getBoundsInPixels ().height;
-			}
-			if (iWidth > currentWidth [0] || iHeight > currentHeight [0]) {
-				GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, iWidth, iHeight);
-				parent.pixbufSizeSet = true;
-				parent.pixbufHeight = iHeight;
-				parent.pixbufWidth = iWidth;
-				/*
-				 * Feature in GTK: a Tree with the style SWT.VIRTUAL has
-				 * fixed-height-mode enabled. This will limit the size of
-				 * any cells, including renderers. In order to prevent
-				 * images from disappearing/being cropped, we re-create
-				 * the renderers when the first image is set. Fix for
-				 * bug 480261.
-				 */
-				if ((parent.style & SWT.VIRTUAL) != 0) {
-					/*
-					 * Only re-create SWT.CHECK renderers if this is the first column.
-					 * Otherwise check-boxes will be rendered in columns they are not
-					 * supposed to be rendered in. See bug 513761.
-					 */
-					boolean check = modelIndex == Tree.FIRST_COLUMN && (parent.style & SWT.CHECK) != 0;
-					parent.createRenderers(column, modelIndex, check, parent.style);
-				}
-			}
-		}
+		parent.initPixbufSize (image);
 	} else {
+		long column = GTK.gtk_tree_view_get_column (parent.handle, index);
+		long pixbufRenderer = parent.getPixbufRenderer (column);
 		/*
-		 * Bug 483112: We check to see if the cached value is greater than the size of the pixbufRenderer.
-		 * If it is, then we change the size of the pixbufRenderer accordingly.
-		 * Bug 489025: There is a corner case where the below is triggered when current(Width|Height) is -1,
-		 * which results in icons being set to 0. Fix is to compare only positive sizes.
+		 * Bug 483112: make sure width and height are set for the pixbufRenderer
 		 */
-		if (parent.pixbufWidth > Math.max(currentWidth [0], 0) || parent.pixbufHeight > Math.max(currentHeight [0], 0)) {
-			GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, parent.pixbufWidth, parent.pixbufHeight);
-		}
+		GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, parent.pixbufWidth, parent.pixbufHeight);
 	}
 
+	int modelIndex = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex;
 	GTK.gtk_tree_store_set(parent.modelHandle, handle, modelIndex + Tree.CELL_PIXBUF, pixbuf, -1);
 	/*
 	 * Bug 573633: gtk_tree_store_set() will reference the handle. So we unref the pixbuf here,

tests/org.eclipse.swt.tests.gtk/JUnit Tests/org/eclipse/swt/tests/gtk/Test_Gtk_Tree_Virtual_setImage.java

@@ -0,0 +1,185 @@
+package org.eclipse.swt.tests.gtk;
+
+import java.util.function.Supplier;
+
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.graphics.GC;
+import org.eclipse.swt.graphics.Image;
+import org.eclipse.swt.layout.GridData;
+import org.eclipse.swt.layout.GridLayout;
+import org.eclipse.swt.widgets.Shell;
+import org.eclipse.swt.widgets.Tree;
+import org.eclipse.swt.widgets.TreeItem;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class Test_Gtk_Tree_Virtual_setImage {
+
+	private static final int WINDOW_WIDTH = 300;
+	private static final int WINDOW_HEIGHT = 100;
+	private static final int IMAGE_SIZE = 70;
+
+	private Shell shell;
+	private Tree tree;
+	private Image image;
+
+	@Before
+	public void setUp() {
+		shell = new Shell();
+		shell.setLayout(new GridLayout());
+	}
+
+	@After
+	public void tearDown() {
+		if (tree != null) {
+			tree.dispose();
+			tree = null;
+		}
+		if (image != null) {
+			image.dispose();
+			image = null;
+		}
+		if (shell != null) {
+			shell.dispose();
+			shell = null;
+		}
+	}
+
+	@Test
+	public void test_initial_img_5_2() {
+		do_test_initial_img(false, 5, 2);
+	}
+
+	@Test
+	public void test_initial_img_5_2_wCheckbox() {
+		do_test_initial_img(true, 5, 2);
+	}
+
+	@Test
+	public void test_initial_img_50_22() {
+		do_test_initial_img(false, 50, 22);
+	}
+
+	@Test
+	public void test_initial_img_50_22_wCheckbox() {
+		do_test_initial_img(true, 50, 22);
+	}
+
+	@Test
+	public void test_set_img_50_22() {
+		do_test_set_img(false, 50, 22);
+	}
+
+	@Test
+	public void test_set_img_50_22_wCheckbox() {
+		do_test_set_img(true, 50, 22);
+	}
+
+	private void do_test_initial_img(boolean withCheckbox, int totalRows, int imgRowIdx) {
+		createImage();
+		createTree(withCheckbox, totalRows, imgRowIdx, () -> image);
+		shell.pack();
+		shell.open();
+
+		executePendingUiTasks();
+
+		tree.showItem(tree.getItem(imgRowIdx));
+
+		executePendingUiTasks();
+
+		assertAllTreeRowsResized();
+	}
+
+	private void do_test_set_img(boolean withCheckbox, int totalRows, int imgRowIdx) {
+		createTree(withCheckbox, totalRows, imgRowIdx, () -> image);
+		shell.pack();
+		shell.open();
+
+		executePendingUiTasks();
+
+		tree.showItem(tree.getItem(imgRowIdx));
+
+		executePendingUiTasks();
+
+		tree.showItem(tree.getItem(totalRows - 1));
+
+		executePendingUiTasks();
+
+		createImage();
+
+		executePendingUiTasks();
+
+		tree.clear(imgRowIdx, false);
+
+		executePendingUiTasks();
+
+		tree.showItem(tree.getItem(imgRowIdx));
+
+		executePendingUiTasks();
+
+		assertAllTreeRowsResized();
+	}
+
+	private void createImage() {
+		this.image = new Image(shell.getDisplay(), IMAGE_SIZE, IMAGE_SIZE);
+		var color = shell.getDisplay().getSystemColor(SWT.COLOR_GREEN);
+		var gc = new GC(image);
+		gc.setForeground(color);
+		gc.setBackground(color);
+		gc.fillRectangle(0, 0, IMAGE_SIZE, IMAGE_SIZE);
+		gc.dispose();
+	}
+
+	private void createTree(boolean withCheckbox, int totalRows, int imgRowIdx, Supplier<Image> getImage) {
+		tree = new Tree(shell, SWT.VIRTUAL | SWT.BORDER | (withCheckbox ? SWT.CHECK : 0));
+		var layoutData = new GridData(GridData.FILL_BOTH);
+		layoutData.widthHint = WINDOW_WIDTH;
+		layoutData.heightHint = WINDOW_HEIGHT;
+		tree.setLayoutData(layoutData);
+		tree.addListener(SWT.SetData, event -> {
+			var item = (TreeItem) event.item;
+			var idx = tree.indexOf(item);
+			item.setText("node " + idx);
+			if (idx == imgRowIdx) {
+				var image = getImage.get();
+				item.setImage(image);
+			}
+			item.setItemCount(0);
+		});
+		tree.setItemCount(totalRows);
+	}
+
+	private void executePendingUiTasks() {
+		while (shell.getDisplay().readAndDispatch()) {
+			// perform next pending task
+		}
+		try {
+			// This sleep is required because
+			// - gtk perform some updates/renderings on its own inner
+			// timers (e.g. a timer for every frame drawing)
+			// - readAndDispatch() doesn't wait for those timers
+			// - we need to wait for this timers to get fully updated/rendered tree
+			Thread.sleep(100);
+		} catch (InterruptedException e) {
+			Thread.currentThread().interrupt();
+			throw new RuntimeException(e);
+		}
+		while (shell.getDisplay().readAndDispatch()) {
+			// perform next pending task
+		}
+	}
+
+	private void assertAllTreeRowsResized() {
+		var minHeight = IMAGE_SIZE;
+		var maxHeight = 2 * IMAGE_SIZE - 1;
+		for (var i = 0; i < tree.getItemCount(); i++) {
+			var item = tree.getItem(i);
+			var height = item.getBounds().height;
+			Assert.assertTrue(
+					String.format("Expected row[%d].height in [%d;%d], but was %d.", i, minHeight, maxHeight, height),
+					minHeight <= height && height <= maxHeight);
+		}
+	}
+}