Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+28 −8
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundles/org.eclipse.ui.navigator.resources/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 19 additions & 4 deletions ...or.resources/src/org/eclipse/ui/internal/navigator/resources/actions/EditActionGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2006, 2015 IBM Corporation and others.
* Copyright (c) 2006, 2025 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand All @@ -18,8 +18,10 @@
import org.eclipse.jface.action.IMenuManager;
import org.eclipse.jface.viewers.IStructuredSelection;
import org.eclipse.swt.SWT;
import org.eclipse.swt.SWTException;
import org.eclipse.swt.dnd.Clipboard;
import org.eclipse.swt.events.KeyEvent;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.ui.IActionBars;
import org.eclipse.ui.ISharedImages;
Expand Down Expand Up @@ -55,9 +57,22 @@ public EditActionGroup(Shell aShell) {

@Override
public void dispose() {
if (clipboard != null) {
clipboard.dispose();
clipboard = null;
Display display = Display.getCurrent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using shell.getDisplay() it seems obvious here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more can you use clipboard itself, it is already is disposed what is the point doing more work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leubi
Sorry, was not able to get your intent. You meant to use Display.getCurrent() or shell.getDisplay()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@laeubi laeubi Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (display != null && !display.isDisposed()) {
if (clipboard != null && !clipboard.isDisposed()) {
clipboard.dispose();
clipboard = null;
}
} else {
// Non-UI thread or display disposed
if (clipboard != null && !clipboard.isDisposed()) {
try {
clipboard.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the clipboard really be disposed outside the UI thread?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

clipboard = null;
} catch (SWTException e) {
// Log a message if required.
}
}
}
super.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call super.dispose() anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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();
    }
}

}
Expand Down
11 changes: 8 additions & 3 deletions ...se.ui.workbench/eclipseui/org/eclipse/ui/internal/e4/compatibility/CompatibilityPart.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2010, 2016 IBM Corporation and others.
* Copyright (c) 2010, 2025 IBM Corporation and others.
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
Expand Down Expand Up @@ -32,6 +32,7 @@
import org.eclipse.e4.ui.di.Focus;
import org.eclipse.e4.ui.di.Persist;
import org.eclipse.e4.ui.di.PersistState;
import org.eclipse.e4.ui.di.UISynchronize;
import org.eclipse.e4.ui.internal.workbench.Activator;
import org.eclipse.e4.ui.internal.workbench.Policy;
import org.eclipse.e4.ui.model.application.ui.basic.MPart;
Expand Down Expand Up @@ -418,9 +419,13 @@ public void create() {
}

@PreDestroy
void destroy() {
void destroy(UISynchronize sync) {
if (!alreadyDisposed) {
invalidate();
sync.syncExec(() -> {
if (!alreadyDisposed) {
invalidate();
}
});
}

eventBroker.unsubscribe(widgetSetHandler);
Expand Down
Loading