[GTK3] Do not crash in SetData event#1612
[GTK3] Do not crash in SetData event#1612basilevs wants to merge 1 commit intoeclipse-platform:masterfrom
Conversation
basilevs
force-pushed
the
issue_678_setdata_fix
branch
from
e3d540c to
d8ba369
Compare
Test Results 483 files ±0 483 suites ±0 9m 13s ⏱️ +35s For more details on these failures, see this check. Results for commit 09cfb68. ± Comparison against base commit 266cc26. ♻️ This comment has been updated with latest results. |
basilevs
force-pushed
the
issue_678_setdata_fix
branch
from
d8ba369 to
7e7c87c
Compare
|
Test failures are caused by #1564 |
basilevs
force-pushed
the
issue_678_setdata_fix
branch
2 times, most recently
from
7c58564 to
b1bf254
Compare
GTK 3.24.41 (Ubuntu 24) crashes when renderers are replaced during render. If replacement is postponed, the crash no longer happens. Fixes eclipse-platform#678.
basilevs
force-pushed
the
issue_678_setdata_fix
branch
from
b1bf254 to
09cfb68
Compare
|
+1 |
|
Converting to draft until an alternative of renderer reconfiguration instead of recreation is investigated. |
|
The alternative is quite complex in comparison and does not avoid the asyncExec(). |
| * 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; |
There was a problem hiding this comment.
Shouldn't check be computed inside getDisplay ().asyncExec (...) (similarly to modelIndexAsync and columnAsync)?.
There was a problem hiding this comment.
Yes, I've failed to account for column deletion. Thanks.
| if (parent.isDisposed () || Math.max (1, parent.getColumnCount ()) <= index) return; | ||
| // On multiple resize requests, perform only the last one | ||
| if (parent.pixbufHeight != iHeight || parent.pixbufWidth != iWidth) return; | ||
| int modelIndexAsync = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex; |
There was a problem hiding this comment.
Is it possible that another column gets inserted at this index after this async task is scheduled but before it's actually executed?
If that's possible, then createRenderers(...) might not be executed for the original column.
There was a problem hiding this comment.
That's related to the bug with resizing you are trying to fix. parent.pixbufSizeSet is set only once per Tree, so the column index is not important 🤷 AFAIK, the only requirement for the column index - the column should exist.
And column may be deleted, so that's a concern worth investigating. Thanks.
The index can't be trusted here and might need to be clamped.
| } else { | ||
|
|
||
| GTK.gtk_tree_store_set(parent.modelHandle, handle, modelIndex + Tree.CELL_PIXBUF, pixbuf, -1); | ||
| GTK.gtk_tree_store_set(parent.modelHandle, handle, modelIndex + Tree.CELL_SURFACE, surface, -1); |
There was a problem hiding this comment.
This is interesting.
So this line was missing before, which means that TreeItem._getImage(int) always returned null.
And somehow Tree still worked.
There was a problem hiding this comment.
It was not missing. I've moved it from below to manage try-finally scope.
|
Hi @basilevs. |
|
Should this one be looked at or #1743 is considered superior to it? |
@akurtakov this PR contains a bug (throws exceptions when columns are deleted), and is marked Draft until I have the time to fix it. There is no ETA, as I've deleted my Linux environment and would have to redeploy to proceed. Do not review this PR. #1743 is indeed superior, but more invasive too. |
4 participants
The crash happens when renderers are replaced during render. If replacement is postponed, the crash no longer happens. The failure only happens in Ubuntu 24 (GTK 3.24.41).
Fixes #678.
Is tested by #1611.