-
Notifications
You must be signed in to change notification settings - Fork 227
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.
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
Added guards to safely handle cases when Display is null or disposed. #3241
Conversation
| return null; | ||
| } | ||
|
|
||
| if (display.isDisposed()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
| Shell splashShell = (Shell) display.getData(DATA_SPLASH_SHELL); | ||
| if (splashShell != null) | ||
| if (splashShell != null && !splashShell.isDisposed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the shell is disposed already, should we probably return null here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean this way?
Shell splashShell = (Shell) display.getData(DATA_SPLASH_SHELL);
if (splashShell != null) {
if (!splashShell.isDisposed()) {
return splashShell;
} else {
return null;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but it depends on the error case I think we like to handle. So in what cases the shell is disposed? Is it only during shutdown? Then it should be save to early exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with early exit. So updated as per that.
ac6595f to
3668598
Compare
getActiveWindow()
getSplashShell(Display display)
These changes make both methods more robust during application startup and shutdown, eliminating crashes caused by accessing disposed SWT resources.
Raised this pr as discussed in -> #3232 (comment)