UI component being attempted to be disposed in a non-ui thread. #3232
Conversation
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
from
733cbf5 to
5bf0e5c
Compare
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
from
bc431ae to
76b3d34
Compare
|
@jukzi : can you take a look at this when you have some time please? |
|
Note that he is "missing in action" so don't expect anything from him. |
...lipse.ui.workbench/eclipseui/org/eclipse/ui/internal/e4/compatibility/CompatibilityPart.java
Outdated
Show resolved
Hide resolved
| if (!alreadyDisposed) { | ||
| invalidate(); | ||
| Display display = Display.getDefault(); | ||
| if (display != null && !display.isDisposed() && Display.getCurrent() != display) { |
There was a problem hiding this comment.
typically the isDisposed() check has to be repeated in Display thread, because dispose could happen in the diplay thread meanwhile
There was a problem hiding this comment.
typically the isDisposed() check has to be repeated in Display thread, because dispose could happen in the diplay thread meanwhile
added another check as well.
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
2 times, most recently
from
9128c12 to
467d4f5
Compare
|
@deepika-u I still think it is better fixed at the caller side than in this particular method, so in what cases can it really happen that it is called from non-ui thread? |
CompatibilityPart.destroy() In general
Therefore, the correct place to fix is in CompatibilityPart.destroy, which bridges the generic e4 lifecycle and the UI-thread-sensitive 3.x parts. This way we can ensure
|
I think you should find out where exactly this enters the system and what is triggering that part. There should be one place where we can ensure it is called inside the UI thread. So can you share a stacktrace of the problematic call so we can better decide what would be the best place here? |
I tried myself running CTabItemTest and captured these logs. Please have a look when you get some time and let me know your opinion. |
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
2 times, most recently
from
b4d306b to
cd07743
Compare
|
@laeubi Once you get some time, can you review the changes please. But i see another problem here though not very important but still a problem "org.eclipse.swt.SWTException: Widget is disposed" #3241 is created for this. |
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
2 times, most recently
from
0e23a26 to
671c7ae
Compare
|
In the log I can only see errors about already disposed part / device but not any access from another thread? |
yes exactly now with these both file changes, i dont see NPE anymore. |
The point is that we should not fix side-effects but the callers who call the method outside the Device thread so that callers stacktrace would be required here to decide where to best fix it. Requiring everyone dispose method (and you already discovered now two places) does not really scale well. |
May be i missed your intent and didnt understand you clearly. But what i also tried as updated in #3232 (comment) I have tried running /org.eclipse.e4.ui.tests.css.swt/src/org/eclipse/e4/ui/tests/css/swt/CTabItemTest.java locally to reproduce the NPE. As said by jukzi in #2316 (comment) Please do let me know if i have to try out anything else or capture any other logs to be specific. |
And was this successful? Sorry to ask but its a bit unclear to me if the problem is currently happens, only happens in the test and how it relates to |
From this comment -> #3232 (comment) Now with CompatibilityPart and EditActionGroup changes in place then attempt to run /org.eclipse.e4.ui.tests.css.swt/src/org/eclipse/e4/ui/tests/css/swt/CTabItemTest.java locally i am not seeing the NPE as reported in #2316 But i see errors related to "org.eclipse.swt.SWTException: Widget is disposed" which i am trying to address with pr #3241 Never mind, thanks for asking again. Observation : Now with both the pr changes in place when i run CTabItemTest locally, i dont see both the errors. It is clean. |
|
@deepika-u now we have is this PR actually needed? |
|
@laeubi
Yes definitely this would be needed. Because both are addressing 2 different problems. Now i have updated my branch to be in sync with master and now when i run CTabItemTest, i dont see any exceptions as such. So this confirms that this pr is also needed for successful run of CTabItemTest along with #3241. Hope i have clarified. Thanks for the ask. |
What I'm wondering about is the following:
|
|
@laeubi |
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
from
58f0da9 to
3b19584
Compare
| } | ||
| } | ||
| } | ||
| super.dispose(); |
There was a problem hiding this comment.
Why not call super.dispose() anymore?
There was a problem hiding this comment.
@laeubi
The problem if we still call "super.dispose();" inside dispose(), it would run immediately - before the clipboard is actually disposed, since disposal happens asynchronously later.
This creates a lifecycle ordering ambiguity:
Context cleared too early.
Asynchronous disposal runs after super.dispose()
If that async runnable references anything indirectly tied to the context (logging, resources, actions), you can get into NPEs or disposed-object access.
So this is Why super.dispose() was dropped.
- super.dispose() is trivial (just setContext(null)).
- Avoided calling it before async disposal finishes, to prevent race conditions.
- In practice, leaving context uncleared is not a big resource leak compared to crashing the UI.
So it was sacrificed for safety.
But if we still want to be correct, the better solution is to still call super.dispose() inside the async block. That way we don’t silently skip the context cleanup from ActionGroup.
We can do this way ::
@Override
public void dispose() {
Display display = Display.getDefault();
if (display != null && !display.isDisposed()) {
display.asyncExec(() -> {
if (clipboard != null && !clipboard.isDisposed()) {
clipboard.dispose();
clipboard = null;
}
EditActionGroup.super.dispose();
});
} else {
// Display already gone, dispose synchronously as a fallback
if (clipboard != null && !clipboard.isDisposed()) {
try {
clipboard.dispose();
clipboard = null;
} catch (SWTException e) {
// Log or ignore safely
}
}
super.dispose();
}
}
When disposal happens during call of dispose or async / blocking in UI running alongside could of course seen as behavior change. And if Test + real environments differ there is of course a chance for false positives (or we did not capture problems). Also it might have side-effects and complicates the code. e.g. in your current change I can see if you look at javadoc you will see
so your check for |
| clipboard = null; | ||
| Display display = Display.getDefault(); | ||
| if (display != null && !display.isDisposed()) { | ||
| display.asyncExec(() -> { |
There was a problem hiding this comment.
In the otherplace syncExec is used, why is this different here?
There was a problem hiding this comment.
EditActionGroup.dispose() with asyncExec() because safety during shutdown is more important than ordering.
- Disposing the Clipboard (and similar UI resources) must happen on the UI thread.
- But dispose() might be called from a non-UI thread during shutdown.
- If we used syncExec() here and called it from a non-UI thread while the display is already disposing, it could deadlock (the UI thread might already be blocked, tearing down widgets).
- asyncExec() safely queues the disposal for "later" on the UI thread, avoiding blocking and deadlock risk.
- If the display is already gone, we just try/catch to clean up as best we can.
So in summary asyncExec() => fire and forget cleanup, safe during shutdown.
CompatibilityPart.destroy() with syncExec() because ordering and completion are crucial for consistent teardown.
- destroy() is part of the Eclipse 4 lifecycle (@PreDestroy), which means the framework expects the part to be fully disposed when this method returns.
- If you used asyncExec() here, invalidate() would run later, and there’s a risk that other teardown code(unsubscribes, model cleanup, etc.) would happen before invalidate() ran. That could cause inconsistencies or NPEs.
- By using syncExec(), you ensure that invalidate() actually completes before the method continues and the lifecycle is maintained correctly.
So in summary syncExec() => enforce order, synchronous cleanup before continuing.
deepika-u
force-pushed
the
CompatibilityPart_2316
branch
from
3b19584 to
8050325
Compare
Thanks for the feedback! You're absolutely right about the side effects of Display.getDefault(). I've updated the code to use Display.getCurrent() instead, which avoids creating a new display and ensures we're only disposing UI resources on the UI thread. This should reduce complexity and avoid unintended behavior in both test and runtime environments. When you get some time, could you take a look at it now please? |
| if (clipboard != null) { | ||
| clipboard.dispose(); | ||
| clipboard = null; | ||
| Display display = Display.getCurrent(); |
There was a problem hiding this comment.
why not using shell.getDisplay() it seems obvious here?
There was a problem hiding this comment.
Yeah shell.getDisplay() is also a good choice. But i need to ensure that the Shell reference is not null and not disposed before calling shell.getDisplay(). Can i go ahead with this way?
There was a problem hiding this comment.
Thinking more can you use clipboard itself, it is already is disposed what is the point doing more work here?
There was a problem hiding this comment.
@leubi
Sorry, was not able to get your intent. You meant to use Display.getCurrent() or shell.getDisplay()?
There was a problem hiding this comment.
If we dont have this pr in place, while running CTabItemTest locally we'll again endup into original problem reshaped as( post pr 3241)
this problem shown in the console output below ->
Console_without_pr3232.txt
There was a problem hiding this comment.
@deepika-u where do you get this is it windows?
I think it makes sense to go through each exception step by step, the first I see in your log is:
org.eclipse.swt.SWTException: Device is disposed
at org.eclipse.swt.SWT.error(SWT.java:4946)
at org.eclipse.swt.SWT.error(SWT.java:4861)
at org.eclipse.swt.SWT.error(SWT.java:4832)
at org.eclipse.swt.widgets.Display.error(Display.java:1332)
at org.eclipse.swt.widgets.Display.getThread(Display.java:2696)
at org.eclipse.swt.dnd.Clipboard.dispose(Clipboard.java:219)
at org.eclipse.ui.internal.navigator.resources.actions.EditActionGroup.dispose(EditActionGroup.java:59)
This looks like a bug in SWT (windows), because calling dispose on an already disposed component should be a no-op (and if device is disposed clipboard is gone anyways...).
Looking into other widgets implementations the do
if (display.thread != Thread.currentThread ()) error (SWT.ERROR_THREAD_INVALID_ACCESS);
so calling getThread instead of access the field directly seems the culprit here.
| // Non-UI thread or display disposed | ||
| if (clipboard != null && !clipboard.isDisposed()) { | ||
| try { | ||
| clipboard.dispose(); |
There was a problem hiding this comment.
Can the clipboard really be disposed outside the UI thread?!?
There was a problem hiding this comment.
Ideally not supposed to be which might result into SWTException or resource leaks if disposal fails silently or false positives.
In my opinion - If we are not on the UI thread and can't guarantee access to it, it's safer to skip disposal or log a warning.
Do you mean i can skip this line clipboard.dispose(); at line 70?
|
I hate to be a party pooper here but I think a call to dispose on UI part on a non UI thread is wrong and the caller should not do that. And this doesn't happen in a normal IDE when I exit. But we're changing code here that will affect the normal behavior. Normally this happens before org.eclipse.e4.core.internal.contexts.osgi.EclipseContextOSGi.dispose() is called: 
I really don't think we should add defensive code in the UI to deal with a situation that arises only in a test. I highly recommend finding a way to fix this by modifying the test to dispose the UI (close the workbench) before this destroy handling kicks in. |
5 participants
Fixes #2316
As suggested tried fixing in destroy(), but looks like we need extra testing to ensure no shutdown regressions considering the below
Note : I have no recreate to test this fix, can this be tested with the existing Unit Test infrastructure in place already?