Bugzilla – Full Text Bug Listing
| Summary: | AIOOBE when restoring a window with no perspectives | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Brian de Alwis <bsd> |
| Component: | UI | Assignee: | Brian de Alwis <bsd> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bsd, emoffatt, Michael_Rennie, mvi, pwebster, tom.schindl |
| Version: | 4.2 | Flags: | emoffatt:
review+
bsd: review+ |
| Target Milestone: | 4.2 RC1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 377394 | ||
| Attachments: | |||
|
Description
Brian de Alwis
Created attachment 212600 [details] Sample program demonstrating the issue This is rather fascinating to debug. I've attached a little minimalist RCP app that demonstrates the problem. This app doesn't have a default perspective. Run it once, and then quit. Run it again, and notice the NPEs described in comment 0. Basically, a Window advisor whose preWindowOpen() causes the perspective bar to shown causes the status bar MToolControl to be serialized out as toBeRendered=true regardless of the whether it has contents. So this widget is rendered before MWindow has been bound to a WorkbenchWindow. A "normal" E3 compat app has a WorkbenchWindow associated with the E4 MWindow as a side-effect of setting an MPart's context (see attachment ss-ide.txt). Since a no-perspective window has no parts, this side-effect never occurs. Created attachment 212601 [details]
Stacktrace from the IDE before it renders the StatusLine
This is the stack trace from putting a breakpoint on WorkbenchWindow.setup(). This method is the primary place where a IWorkbenchWindow is associated with its corresponding MTrimmedWindow's context. Note that the status line is rendered because an MPart has had its context set.
Created attachment 212602 [details]
Stacktrace from an app with no perspectives
This is the stack trace from putting a breakpoint on StatusTrim#createStatusLine() at the point where it skips creating any controls if there's no corresponding WorkbenchWindow. (I should point out here that there are *no* parts on-screen)
I should add that if you quit the demo app with the Welcome screen open, the subsequent start works with no AIOOBE. My guess is that the part is rendered before the StatusLine, and so the MWindow has been bound to the WorkbenchWindow by the time the StatusLine is rendered. I'm wondering if we shouldn't just associated WorkbenchWindows with the MWindows on startup rather than as a side-effect, perhaps in E4Workbench#createAndRunUI(). I'll try that tomorrow. (In reply to comment #5) > I'm wondering if we shouldn't just associated WorkbenchWindows with the > MWindows on startup rather than as a side-effect, perhaps in > E4Workbench#createAndRunUI(). I'll try that tomorrow. E4Workbench can't see WorkbenchWindow ... possibly in Workbench itself? PW *** Bug 377425 has been marked as a duplicate of this bug. *** this bug is becoming critical for SWT scout applications. is there any plan by when this could be fixed? Matthias, is this actually having a functional impact on Scout? I've only seen cosmetic impacts to date. I actually cooked up a patch last night that solves this bug and bug 366110. There's one small tweak necessary. I'll attach it in a moment; if you could try it, we can get this in for RC1. I saw this AIOOBE occur when starting the IDE last night, which I've never seen previously. Hi Brian, Unfortunately this has functional impact because as soon as this Exception occurs some views of the perspective are not displayed which is of course not the desired behavior :) This would be great! I will test it as soon as possible. Thanks a lot! Created attachment 214925 [details]
Patch to create WorkbenchWindow after MWindow has been realized
This patch causes a WorkbenchWindow to be automatically opened around a new MWindow when the MWindow's widget is set.
WorkbenchWindowConfigurer#setTitle() must update the model's label as the WBWRenderer#createWidget() sets the shell's title to the model's label, which occurs after the set-widget event has been processed.
I tested your patch and with this modifications I did not get any AIOOBE anymore! So for me this is working fine! If this could get in RC1 this would be perfect. Thanks for the very fast solution! Thanks for verifying, Matthias. +1, thanks Brian. When verifying this one please ensure that we aren't making two WBW's anytime... I'm seeing an NPE in WorkbenchPage#setPerspective on a restart with no perspectives open... Caused by: java.lang.NullPointerException at org.eclipse.ui.internal.WorkbenchPage.setPerspective(WorkbenchPage.java:3621) at org.eclipse.ui.internal.WorkbenchWindow.setup(WorkbenchWindow.java:542) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) I'm not sure why we're even calling setPerspective if there isn't any open. Note also that the WorkbenchWindow we create is identifying the 'ResourcePerspective' in its constructor (as is the setPerspective call above). Created attachment 215417 [details] Updated patch (In reply to comment #15) > I'm seeing an NPE in WorkbenchPage#setPerspective on a restart with no > perspectives open... Steps to repeat (from IRC): 1. Start a fresh IDE product 2. Close the Java perspective 3. Quit 4. Restart: should popup an NPE But I see this same NPE without the patch :-) That said, changing the patch from: + createWorkbenchWindow(getDefaultPageInput(), getPerspectiveRegistry() + .findPerspectiveWithId(getDefaultPerspectiveId()), window, false); to + createWorkbenchWindow(getDefaultPageInput(), null, window, false); clears that up. Just to keep track of what's going on the last fix suggested ends up with a fresh workspace ending up with *no* perspective open instead of the Java perspective... We're still trying to figure out how to provide a safe fix for this but anything that regresses 4.2 is out...
Brian, if I add the following code to StandardTrim#createStatusLine I seem to get something that works OK (at least as far as the IDE is concerned (I left in a line above / below my additions for context):
// wbw may be null if workspace is started with no open perspectives.
if (wbw == null) {
// Create one assuming there's no defined perspective
Workbench wb = (Workbench) PlatformUI.getWorkbench();
wb.createWorkbenchWindow(wb.getDefaultPageInput(), null,
modelService.getTopLevelWindowFor(toolControl), false);
wbw = (WorkbenchWindow) context.get(IWorkbenchWindow.class);
}
if (wbw != null) {
The theory here is that if the 'wbw' is null here then we don't have any perspective open (actually it means we've rendered at least one view but hey..;-).
This way this code only fires under exceptional circumstances making it less likely to interfere with the regular (albeit questionable) ordering.
Created attachment 215474 [details]
Alternative patch
Matthias, I've turned Eric's code snippet into the attached patch. Could you please report back on whether it resolves the issues you're seeing with Scout?
Hi Brian, Yes, I can confirm that with this patch I don't get the exception anymore. Thanks matthias Matthias, the real question is whether this fixes the "functional impact" you mentioned in comment #10 ? Brian, could you please +1 this if you're satisfied with the patch ? I'm fine with the patch Yes of course. I am still testing the cases. At the moment I can only say that the exception seems not to be thrown again. As soon as all checks have been completed, I will let you know. matthias Thanks, I'll wait until you get back before committing the fix... Try again to give +1 to the review. Thanks Brian ! commit bf34877735a3f25e9afa1ab0330960641accca1d Implement the code in comment #18 along with replacing the 'assert' in teh ToolControlRenderer with actual code to ensure that the AIOOB won't fire. Matthias, if this does *not* fix the tests feel free to re-open and take another look (may mean that we slip into 4.2.1 though). I have completed the tests and can confirm that our SWT clients now work as expected! So the fix solves our issues. Thanks a lot! Matthias Verified in I20120513-1300. |