Skip to content

Added guards to safely handle cases when Display is null or disposed. #3241

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

Merged
merged 1 commit into from
Sep 5, 2025
Merged

Added guards to safely handle cases when Display is null or disposed. #3241

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
14 changes: 12 additions & 2 deletions bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/WorkbenchPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2018 IBM Corporation and others.
* Copyright (c) 2000, 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 @@ -1175,15 +1175,25 @@
*/
public static Shell getSplashShell(Display display)
throws NumberFormatException, IllegalArgumentException {
if (display == null || display.isDisposed()) {
return null;
}
Shell splashShell = (Shell) display.getData(DATA_SPLASH_SHELL);
if (splashShell != null)
if (splashShell != null) {
if (splashShell.isDisposed()) {
return null;
}
return splashShell;
}

String splashHandle = System.getProperty(PROP_SPLASH_HANDLE);
if (splashHandle == null) {
return null;
}

if (display.isDisposed()) {
Copy link
Contributor

@laeubi laeubi Sep 4, 2025

Choose a reason for hiding this comment

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

Should this not already be checked in line 1178?

Copy link
Contributor Author

@deepika-u deepika-u Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since this being a timing issue, have added the check before its being used again.

In shutdown scenarios, it is possible that the Display was disposed in between (e.g., another thread closed the workbench).

Without the second check, Shell.internal_new() would throw an SWTException: Widget is disposed.

So this second check is a race-condition guard. It’s defensive programming against the Display being disposed after the first check.

Copy link
Contributor

@laeubi laeubi Sep 4, 2025

Choose a reason for hiding this comment

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

But who should dispose the display in the meanwhile? This looks a bit fishy here so is this really called outside the event thread?

Copy link
Contributor Author

@deepika-u deepika-u Sep 4, 2025

Choose a reason for hiding this comment

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

You’re right - under normal circumstances, the Display is only disposed on the UI thread at shutdown. If we can guarantee that getSplashShell() is always called from the UI thread, then the second check is redundant.

However, since this is a static utility used during startup/shutdown, I was not 100% sure whether it’s always invoked on the UI thread. If there is no non UI thread caller, I can drop the second check. Otherwise, it serves as a defensive guard against race conditions.

return null;
}

Check warning on line 1196 in bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/WorkbenchPlugin.java

View check run for this annotation

Jenkins - Eclipse Platform / API Tools

ILLEGAL_REFERENCE 

NORMAL:
WorkbenchPlugin illegally references method Shell.internal_new(Display, long)
splashShell = Shell.internal_new(display, Long.parseLong(splashHandle));

display.setData(DATA_SPLASH_SHELL, splashShell);
Expand Down
13 changes: 11 additions & 2 deletions ...ipse.ui.workbench/eclipseui/org/eclipse/ui/internal/services/WorkbenchSourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2009, 2015 IBM Corporation and others.
* Copyright (c) 2009, 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 @@ -403,7 +403,16 @@ private ShowInContext getContext(IWorkbenchPart sourcePart) {
}

private IWorkbenchWindow getActiveWindow() {
final Shell newActiveShell = workbench.getDisplay().getActiveShell();
Display display = workbench.getDisplay();
if (display == null || display.isDisposed()) {
return null;
}

final Shell newActiveShell = display.getActiveShell();
if (newActiveShell == null || newActiveShell.isDisposed()) {
return null;
}

final IContextService contextService = workbench.getService(IContextService.class);
if (contextService != null) {
final int shellType = contextService.getShellType(newActiveShell);
Expand Down
Loading