SIGSEGV in Tree.cellDataProc when calling TreeItem.setImage #678 #1662
Conversation
…latform#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.
|
There were 3 failed tests in the build above. |
|
If you use UPDATE: |
| @@ -4319,6 +4319,80 @@ void checkSetDataInProcessBeforeRemoval() { | |||
| } | |||
| } | |||
|
|
|||
| boolean initPixbufSize(Image image) { | |||
There was a problem hiding this comment.
[minor] SWT has a ridiculous code style, where parenthesis , braces and brackets are space-separated from associated identifier. Consider following the style of surrounding code.
| @@ -4319,6 +4319,80 @@ void checkSetDataInProcessBeforeRemoval() { | |||
| } | |||
| } | |||
|
|
|||
| boolean initPixbufSize(Image image) { | |||
| if (pixbufSizeSet || image == null) { | |||
There was a problem hiding this comment.
[minor] Now that resource management is fixed, are these lines still needed? Can't we monotonically increase the size for every new image?
There was a problem hiding this comment.
As I understand from the swt code, for any Tree:
- all images in the tree must have the same size
- that fixed size is the size of the 1st image set for the tree
In the code this is implemented in TreeItem.setImage(int index, Image image) here:
if (image != null) {
ImageList imageList = parent.imageList;
if (imageList == null) imageList = parent.imageList = new ImageList();
int imageIndex = imageList.indexOf(image);
// When we create a blank image surface gets created with dimensions 0, 0.
// This call recreates the surface with correct dimensions
long tempSurface = ImageList.convertSurface(image);
Cairo.cairo_surface_destroy(tempSurface);
if (imageIndex == -1) {
imageIndex = imageList.add(image);
}
surface = imageList.getSurface(imageIndex);
pixbuf = ImageList.createPixbuf(surface);
}Basically, ImageList has width and height which it initializes from the 1st added image, and then all subsequent added images are scaled to that size.
Here is a snippet that demonstrates how the images in a tree are scaled to the same size:
import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;
public class TryColumnsAndImageSizes {
private Display display;
private Shell shell;
private Composite buttonBox;
private Tree tree;
private Image[] images = new Image[3];
public static void main (String[] args) {
new TryColumnsAndImageSizes ().run ();
}
void run () {
display = new Display ();
shell = new Shell (display);
shell.setLayout (new GridLayout ());
buttonBox = new Composite (shell, 0);
buttonBox.setLayout (new FillLayout ());
addSetImageButton ("r50", 0, 20, display.getSystemColor (SWT.COLOR_RED));
addSetImageButton ("g70", 1, 70, display.getSystemColor (SWT.COLOR_GREEN));
addSetImageButton ("b90", 2, 120, display.getSystemColor (SWT.COLOR_BLUE));
tree = new Tree (shell, SWT.VIRTUAL);
tree.setLayoutData (new GridData (GridData.FILL_BOTH));
for (int i = 1; i <= 3; i++) {
var column = new TreeColumn (tree, SWT.LEFT);
column.setText ("Column " + i);
column.setWidth (200);
}
tree.addListener (SWT.SetData, e -> {
TreeItem item = (TreeItem) e.item;
item.setText (0, "a");
item.setText (1, "b");
item.setText (2, "c");
var idx = tree.indexOf (item);
for (var i = 0; i < images.length; i++) {
if (idx == i + 2) {
item.setImage (i, images[i]);
}
}
});
tree.setItemCount (10);
shell.setSize (400, 300);
shell.open ();
while (!shell.isDisposed ()) {
if (!display.readAndDispatch ()) {
display.sleep ();
}
}
display.dispose ();
}
private void addSetImageButton (String label, int imageIdx, int imageSize, Color imageColor) {
var button = new Button (buttonBox, SWT.PUSH);
button.setText (label);
button.addListener (SWT.Selection, e -> {
images[imageIdx] = createImage (imageSize, imageColor);
tree.clearAll (false);
});
}
private Image createImage (int imageSize, Color color) {
var image = new Image (display, imageSize, imageSize);
var gc = new GC (image);
gc.setForeground (color);
gc.setBackground (display.getSystemColor (SWT.COLOR_WHITE));
gc.setLineWidth (5);
gc.fillRectangle (0, 0, imageSize, imageSize);
gc.drawRectangle (0, 0, imageSize - 1, imageSize - 1);
gc.dispose ();
return image;
}
}There was a problem hiding this comment.
As a result, the answer to this:
Can't we monotonically increase the size for every new image?
is "no, because Tree in swt is designed to have images of the same size".
There was a problem hiding this comment.
Ok, as long as they are not cropped, we can leave this as is.
There was a problem hiding this comment.
is "no, because
Treein swt is designed to have images of the same size".
It does not matter, as on each new image we could resize all previously added ones as necessary.
There was a problem hiding this comment.
OK.
Apparently, this is another issue.
Just be careful, if you are going to increase fixed row size multiple times, then you'll probably have to replace fixed_height_mode + gtk_tree_view_row_changed to gtk_tree_view_set_model(). Here is more details.
| // 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]; |
There was a problem hiding this comment.
[minor] unnecessary reallocation
There was a problem hiding this comment.
Sorry, but I don't understand.
Unnecessary reallocation of what?
As I can see, both iter and value are allocated once.
Could you clarify it, please.
There was a problem hiding this comment.
I had read int[] value = new int[1]; as being a part of loop iteration. I was wrong. Sorry about that.
| 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); |
There was a problem hiding this comment.
[major] Why do we allow to decrease size now?
Would not this corrupt other columns, where large images might be present?
There was a problem hiding this comment.
As I understand, in swt all images in a Tree have the same size. (here is why I think so).
As a result, all GtkCellRendererPixbufs should have the same fixed width and height.
Actually in Tree.resetFixedRowHeight() I added the code that iterates over all GtkCellRendererPixbufs and set them fixed width and height.
So maybe, GTK.gtk_cell_renderer_set_fixed_size() might even be redundant.
I left this code here just in case (e.g. maybe there is code somewhere in swt that adds a new column and doesn't call GTK.gtk_cell_renderer_set_fixed_size() for it)
| } | ||
|
|
||
| private void createTree(boolean withCheckbox, int totalRows, int imgRowIdx, Supplier<Image> getImage) { | ||
| tree = new Tree(shell, SWT.VIRTUAL | SWT.BORDER | (withCheckbox ? SWT.CHECK : 0)); |
There was a problem hiding this comment.
[minor] Now, that a potential regression for setting an image of decreasing sizes in different columns is discovered, we may want some tests for multi-column case.
There was a problem hiding this comment.
It seems to me that Tree doesn't support different images sizes in swt by design.
In this reply I included a snippet that demonstrates what happens when there are multiple columns.
Please, note that images are scaled to fixed size inside ImageList, which is pretty old - that's why I think that Tree expects images of the same size by design.
There was a problem hiding this comment.
BTW, I can add unit test(s) for multiple columns and different images sizes to Test_Gtk_Tree_Virtual_setImage no problem.
I just don't know what additional cases you want to be checked.
The current unit tests in Test_Gtk_Tree_Virtual_setImage are for the bug with row heights, that I noticed (and fixed in this PR) while I've been fixing crash in #678.
There was a problem hiding this comment.
The scenario is:
- for three sequences of images three in length each:
- one monotonically increasing in size
- one monotonically decreasing
- and one non-monotonic
- test that behavior (heights, widths, exceptions, ideally cropping but that's probably too hard) is unchanged when:
- adding images to different columns
- adding to same column
- adding to a tree without columns
There was a problem hiding this comment.
Just to be clear, I'm not a maintainer, so may be wait until one chimes in before investing significant effort. I was going to add such test myself later anyway.
There was a problem hiding this comment.
OK.
As I understand, you are working on some task related to image cropping, multiple image sizes, and multiple columns.
I suppose including those unit tests in the commit for this task I would be more appropriate.
There was a problem hiding this comment.
No, I literally tried to fix the very same problem - crash in VIRTUAL Tree. While I succeeded, your solution might be a bit better.
There was a problem hiding this comment.
No, I fixed the very same problem - crash in VIRTUAL Tree, but your solution might be better.
…latform#678 Added code formatting changed to match the formatting of the rest of the SWT code.
Yes, the method with Also I tried to understand what the old code did (when it did not crash the application).
Note: changing
|
|
Converted to draft because it seems like this bug should be should be fixed in another place in the code. After fixing this bug by changing Stacktrace is this: This is a very similar bug: again we get: The problem again is that a Since callbacks for |
Fix and test for bug #678
Fixes #678
Reproducing the crash:
Tree.cellDataProcwhen callingTreeItem.setImage#678 (comment)Tree.cellDataProcwhen callingTreeItem.setImage#678 (comment)The cause of the crash is described here: #678 (comment)
In short, the problem was executing
Tree.createRenderers()insideTreeItem.setImage().The sequence of action leading to the crash was:
in a
TreewithSWT.VIRTUALaTreeItemis rendered for the first time or afterclear()Tree.cellDataProc()is executed for the item and one of it'srenderers
Tree.checkData(item)is called for the itemSWT.SetDatais invoked and executesTreeItem.setImage()Tree.createRenderers()executes and disposes the current rendererTree.cellDataProc()that access the already-disposed renderer (they think it's alive)How it's fixed:
GTK.gtk_cell_renderer_set_fixed_sizeThis does 2 things:
GtkTreeViewrecompute it's internally stored fixed row height with re-settingfixed_height_modeof the GtkTreeViewgtk_tree_view_row_changed()on each of the rows.(2) and (3) can also be achieved by creating a copy of the current
GtkTreeModeland setting it to theGtkTreeView.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.