Fix for #678 #1712
Fix for #678 #1712
Conversation
|
Could you please add the executable snippet that demonstrates the crash to the eclipse.platform.swt/tests/org.eclipse.swt.tests.gtk/ManualTests ? I've tried two snippets on the ticket but none of them crashed on my RHEL 9.2 / gtk3-3.24.31-2.el9.x86_64. |
Test Results 483 files - 3 483 suites - 3 10m 10s ⏱️ +19s For more details on these failures, see this check. Results for commit 72c21ef. ± Comparison against base commit d7febc4. This pull request removes 19 tests. |
I don't know how to reliably reproduce a jvm crash for this bug. The bug itself is a so called use-after-free, i.e. in The crash happens when in between the memory release and the access the freed memory is allocated again and modified in specific way by some other native code. For example, if the value of some pointer accessed inside The problem is that this "some other native code", on which depends if and how the freed memory is modified, is impossible to predict - because it's native code of the JVM and system libraries, both of which are super complicated. Even on my own computer I don't get crash every time I run the snippet. I suppose #1611 offers the highest chance for the app crash. For now IMO the easiest way to make sure that in BTW in C/C++ there are tools to detect errors of this type. |
|
Would be still good to provide best snippet that could produce the crash in same PR. |
zkxvh
force-pushed
the
set-data-asyncexec
branch
from
72c21ef to
04e2d7a
Compare
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
force-pushed
the
set-data-asyncexec
branch
from
04e2d7a to
de2db35
Compare
|
added the snippet as Issue678_JvmCrashOnTreeItemSetImage.java |
Fix for
SIGSEGVinTree.cellDataProc(...)when callingTreeItem.setImage(...).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 crash happens due to read accesses to an already disposed renderer.
The sequence of action leading to the crash was:
TreewithSWT.VIRTUALaTreeItemis rendered for the first time or afterclear()Tree.cellDataProc()is executed for the item and one of the renderersTree.checkData(item)is called for the itemSWT.SetDataevent is created and sent for the itemTreeItem.setImage() is executed by the event handler forSWT.SetData``Tree.createRenderers()executes and disposes the current rendererTree.cellDataProc()that access the already-disposed renderer (they think it's alive)How it's fixed: in
Tree.cellDataProc()wrapTree.checkData(item)intoDisplay.asyncExec().Why fixed this way:
Tree.cellDataProc()is a cell data function which is not supposed to change tree structure. Violation of this leads to C memory errors.SWT.SetDataevent handlers are written by swt users and therefore can contain any code.Using
Display.asyncExec()to postponeSWT.SetDataevent handlers untilTree.cellDataProc()is finished seems like the most simple and bullet-proof solution to #678 and all similar bugs.