[GTK3] Test Tree crash in SetData event #1611
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results 483 files ±0 483 suites ±0 8m 30s ⏱️ - 4m 4s For more details on these failures, see this check. Results for commit f811bf7. ± Comparison against base commit e6588c2. ♻️ This comment has been updated with latest results. |
basilevs
force-pushed
the
issue_678_renderer_crash
branch
3 times, most recently
from
2dc0189 to
c54d5e7
Compare
The failure only happens in Ubuntu 24 (GTK 3.24.41)
basilevs
force-pushed
the
issue_678_renderer_crash
branch
from
c54d5e7 to
f811bf7
Compare
|
Test failures are caused by #1564 |
zkxvh
added a commit
to zkxvh/eclipse.platform.swt
that referenced
this pull request
Dec 13, 2024
…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.
zkxvh
added a commit
to zkxvh/eclipse.platform.swt
that referenced
this pull request
Dec 13, 2024
…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.
zkxvh
added a commit
to zkxvh/eclipse.platform.swt
that referenced
this pull request
Jan 9, 2025
Fix for `SIGSEGV` in `Tree.cellDataProc(...)` when calling `TreeItem.setImage(...)`. 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 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 eclipse-platform#678 and all similar bugs.
Draft
zkxvh
added a commit
to zkxvh/eclipse.platform.swt
that referenced
this pull request
Jan 10, 2025
Fix for `SIGSEGV` in `Tree.cellDataProc(...)` when calling `TreeItem.setImage(...)`. 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 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 eclipse-platform#678 and all similar bugs.
zkxvh
added a commit
to zkxvh/eclipse.platform.swt
that referenced
this pull request
Jan 10, 2025
Fix for `SIGSEGV` in `Tree.cellDataProc(...)` when calling `TreeItem.setImage(...)`. 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 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 eclipse-platform#678 and all similar bugs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The failure only happens in Ubuntu 24 (GTK 3.24.41)
This is a test for #678
The fix is in #1612.