UI component being attempted to be disposed in a non-ui thread.

[deepika-u]

Fixes #2316

As suggested tried fixing in destroy(), but looks like we need extra testing to ensure no shutdown regressions considering the below

• In shutdown, syncExec might block if the display is already disposed or unresponsive.
• EventBroker cleanup still runs in non-UI thread, which is fine. But we have 2 different parts happening - part disposed later, eventbroker unsubscribed immediately.

Note : I have no recreate to test this fix, can this be tested with the existing Unit Test infrastructure in place already?

[github-actions[bot]]

Test Results

 2 904 files  ±0   2 904 suites  ±0   1h 53m 3s ⏱️ - 5m 20s  8 017 tests ±0   7 772 ✅ +1  245 💤 ±0  0 ❌  - 1  23 585 runs  ±0  22 803 ✅ +1  782 💤 ±0  0 ❌  - 1 

Results for commit 8050325a. ± Comparison against base commit 88774cd3.

:recycle: This comment has been updated with latest results.

[eclipse-platform-bot]

This pull request changes some projects for the first time in this development cycle. Therefore the following files need a version increment:

bundles/org.eclipse.ui.navigator.resources/META-INF/MANIFEST.MF

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 patch

From dd1a36b314adcee608aff97fead6bc029ee868bb Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Thu, 4 Sep 2025 12:44:17 +0000
Subject: [PATCH] Version bump(s) for 4.38 stream


diff --git a/bundles/org.eclipse.ui.navigator.resources/META-INF/MANIFEST.MF b/bundles/org.eclipse.ui.navigator.resources/META-INF/MANIFEST.MF
index 30277bf444..484bfaa8e9 100644
--- a/bundles/org.eclipse.ui.navigator.resources/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.ui.navigator.resources/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.ui.navigator.resources; singleton:=true
-Bundle-Version: 3.9.800.qualifier
+Bundle-Version: 3.9.900.qualifier
 Bundle-Activator: org.eclipse.ui.internal.navigator.resources.plugin.WorkbenchNavigatorPlugin
 Bundle-Vendor: %Plugin.providerName
 Bundle-Localization: plugin
-- 
2.51.0

Further information are available in Common Build Issues - Missing version increments.

[deepika-u]

@jukzi : can you take a look at this when you have some time please?

[merks]

Note that he is "missing in action" so don't expect anything from him.

[deepika-u]

@laeubi @jukzi : all the comments are incorporated, can you check now please. Thanks for your suggestions.

[laeubi]

@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?

[deepika-u]

@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() → InjectorImpl.disposed() → EclipseContext.dispose() → OSGI/Equinox bundle stop thread That path is not on the UI thread. It’s on the OSGi framework thread during bundle shutdown. So yes, it can and does happen that destroy() can be called from non-UI thread. That’s how we got the NPE in the first place.

In general

• In normal user-driven part closure, destroy() will indeed be called from the UI thread.
• But during application or bundle shutdown, destroy() can be invoked from OSGi/framework threads (non-UI). That’s exactly how the reported NPE occurred.
• Since IntroPart.dispose() and other cleanup touch SWT/JFace resources are all dealing with UI components, they must be marshaled to the UI thread.

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

• Lifecycle events are still respected.
• invalidate() always runs in UI thread.
• No raw Display usage, just e4-provided UISynchronize.

[laeubi]

But during application or bundle shutdown, destroy() can be invoked from OSGi/framework threads (non-UI). That’s exactly how the reported NPE occurred.

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?

[deepika-u]

@laeubi

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. I dint find anything related to the error in .log file. But could see the same error in console of eclipse. So just copied the content of console to a file and also sharing the .log file for reference. console_output.txt .log

Please have a look when you get some time and let me know your opinion.

[deepika-u]

@laeubi On further anlaysis, i felt EditActionGroup.dispose() also needs to be updated. So adding this change too to make the fix complete. Now with the EditActionGroup.dispose() changes in place along with CompatibilityPart.destroy(), i am not able to get the original reported NPE anymore on running the same CTabItemTest. Attaching the console for the same -> Console_output_4thSept.txt

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" For addressing this problem i have a pr attached which adds safe guards but no functionality being updated via it. Will be adding the pr once raised.

#3241 is created for this.

[laeubi]

In the log I can only see errors about already disposed part / device but not any access from another thread?

[deepika-u]

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.

[laeubi]

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.

[deepika-u]

@laeubi

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 https://github.com/eclipse-platform/eclipse.platform.ui/pull/3232#issuecomment-3248623090

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 https://github.com/eclipse-platform/eclipse.platform.ui/issues/2316#issue-2542988909

Please do let me know if i have to try out anything else or capture any other logs to be specific.

[laeubi]

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

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 EditActionGroup?

[deepika-u]

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 EditActionGroup?

From this comment -> https://github.com/eclipse-platform/eclipse.platform.ui/pull/3232#issuecomment-3248623090 If you go through the full description in this section i have ended up in adding the change in EditActionGroup. Respective log for reference is also attached.

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 https://github.com/eclipse-platform/eclipse.platform.ui/issues/2316

But i see errors related to "org.eclipse.swt.SWTException: Widget is disposed" which i am trying to address with pr https://github.com/eclipse-platform/eclipse.platform.ui/pull/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.

[laeubi]

@deepika-u now we have

• https://github.com/eclipse-platform/eclipse.platform.ui/pull/3241

is this PR actually needed?

[deepika-u]

@laeubi Thanks for merging #3241

is this PR actually needed?

Yes definitely this would be needed. Because both are addressing 2 different problems. Today i have updated my master to latest code. Now when i run CTabItemTest on master, i am still seeing the java.lang.RuntimeException caused by CompatibilityPart and EditActionGroup. Attached the console output here -> console_output_8thSept.txt

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. For reference also attached console output -> console_output_8thSept_onbranch_3232.txt

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.

[laeubi]

Now when i run CTabItemTest on master, i am still seeing the java.lang.RuntimeException caused by CompatibilityPart and EditActionGroup.

What I'm wondering about is the following:

1. Do we see it anywhere when running Eclipse at all?
2. Why does it not happen in the ibuild but only in the IDE?
3. Should maybe the tests be simply marked to be running in the UI thread instead?

[deepika-u]

@laeubi We don’t see this issue in day-to-day Eclipse usage - it only shows up in JUnit test runs because shutdown timing differs between the IDE and the ibuild environment. The change we introduced makes disposal more robust (guards against disposed widgets and device access), so it fixes the test failures without negative side effects. Marking the test as “UI-thread only” would only mask the problem; our fix ensures correctness in both runtime and test scenarios with no behavioral change.

[laeubi]

our fix ensures correctness in both runtime and test scenarios with no behavioral change.

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

Display display = Display.getDefault();
if (display != null && !display.isDisposed()) {

}

if you look at javadoc you will see

Returns the default display. One is created (making the thread that invokes this method its user-interface thread) if it did not already exist.

so your check for null will never succeed. If no display exits one will be created here (and maybe never disposed). So its always hard to guess its really "side-effect-free" to add such code.

[deepika-u]

@laeubi

Display display = Display.getDefault(); if (display != null && !display.isDisposed()) {

}

if you look at javadoc you will see

> Returns the default display. **One is created (making the thread that invokes this method its user-interface thread) if it did not already exist**.

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?

[merks]

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.