forked from eclipse-platform/eclipse.platform.swt
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
- Loading branch information
Showing
4 changed files
with
212 additions
and
34 deletions.
There are no files selected for viewing
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
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
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
89 changes: 89 additions & 0 deletions
89
...k/ManualTests/org/eclipse/swt/tests/gtk/snippets/Issue678_JvmCrashOnTreeItemSetImage.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package org.eclipse.swt.tests.gtk.snippets; | ||
|
|
||
| import org.eclipse.swt.SWT; | ||
| import org.eclipse.swt.graphics.Image; | ||
| import org.eclipse.swt.layout.FillLayout; | ||
| import org.eclipse.swt.widgets.Display; | ||
| import org.eclipse.swt.widgets.Shell; | ||
| import org.eclipse.swt.widgets.Tree; | ||
| import org.eclipse.swt.widgets.TreeItem; | ||
|
|
||
| /** | ||
| * Description: when {@link TreeItem#setImage(Image)} is called within an | ||
| * {@link SWT#SetData} event handler for a {@link SWT#VIRTUAL} tree, then a JVM | ||
| * crash can happen because of use-after-free gtk3 renderer in | ||
| * {@code Tree.cellDataProc()}. | ||
| * <p> | ||
| * | ||
| * <pre> | ||
| * Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) | ||
| * C [libgobject-2.0.so.0+0x3b251] g_type_check_instance_is_fundamentally_a+0x11 | ||
| * C [libswt-pi3-gtk-4958r2.so+0x4b609] Java_org_eclipse_swt_internal_gtk_OS_g_1object_1set__J_3BJJ+0x4a | ||
| * | ||
| * Java frames: (J=compiled Java code, j=interpreted, Vv=VM code) | ||
| * J 11988 org.eclipse.swt.internal.gtk.OS.g_object_set(J[BJJ)V (0 bytes) | ||
| * J 10921 c1 org.eclipse.swt.widgets.Tree.cellDataProc(JJJJJ)J (486 bytes) | ||
| * J 10920 c1 org.eclipse.swt.widgets.Display.cellDataProc(JJJJJ)J (29 bytes) | ||
| * v ~StubRoutines::call_stub | ||
| * J 11619 org.eclipse.swt.internal.gtk3.GTK3.gtk_main_iteration_do(Z)Z (0 bytes) | ||
| * J 11623 c1 org.eclipse.swt.widgets.Display.readAndDispatch()Z (88 bytes) | ||
| * </pre> | ||
| * | ||
| * Tested on GTK 3.24.37 (Fedora 38) | ||
| */ | ||
| public class Issue678_JvmCrashOnTreeItemSetImage { | ||
|
|
||
| private static final int NUM_ITERATIONS = 100; | ||
|
|
||
| public static void main (String[] args) { | ||
| Display display = new Display (); | ||
| Shell shell = new Shell (display); | ||
| shell.setSize (400, 300); | ||
| shell.setLayout (new FillLayout ()); | ||
| shell.open (); | ||
| Image image = new Image (display, 20, 20); | ||
|
|
||
| for (int i = 0; i < NUM_ITERATIONS; i++) { | ||
| Tree tree = new Tree (shell, SWT.VIRTUAL); | ||
| tree.addListener (SWT.SetData, e -> { | ||
| TreeItem item = (TreeItem) e.item; | ||
| item.setText (0, "A"); | ||
|
|
||
| // for some reason sleeping increases probability of crash | ||
| try { | ||
| Thread.sleep (50); | ||
| } catch (InterruptedException ex) { | ||
| throw new RuntimeException (ex); | ||
| } | ||
|
|
||
| item.setImage (image); // <-- this is the critical line! | ||
| }); | ||
| tree.setItemCount (1); | ||
| shell.layout (); | ||
|
|
||
| waitUntilIdle (); | ||
|
|
||
| tree.dispose (); | ||
| } | ||
|
|
||
| display.dispose (); | ||
| } | ||
|
|
||
| private static void waitUntilIdle () { | ||
| long lastActive = System.currentTimeMillis (); | ||
| while (true) { | ||
| if (Thread.interrupted ()) { | ||
| throw new AssertionError (); | ||
| } | ||
| if (Display.getCurrent ().readAndDispatch ()) { | ||
| lastActive = System.currentTimeMillis (); | ||
| } else { | ||
| if (lastActive + 10 < System.currentTimeMillis ()) { | ||
| return; | ||
| } | ||
| Thread.yield (); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } |