Community
Participate
Working Groups
Build Identifier: I20110613-1736 Eclipse will crash on Windows 7 when opening / closing an external link from the embedded help. I've tried three times, it will always crash, for me it will take 13 times to reproduce, others claim it will take up to 20 times. See attached hs_err report from my Oracle 1.6.0_21 VM for crash information, active thread was "Help Browser UI" Reproducible: Always Steps to Reproduce: 1. Start Eclipse 3.7 on Windows 7: Help -> Help Contents 2. Navigate to Eclipse documentation -> Workbench basics -> Reference -> Team support with CVS -> CVS 3. Click the link to "http://ximbiot.com/cvs/" 4. Close the tab in your browser 5. Repeat steps 3 and 4 for ~20 times: crash
CQ:WIND00310454 *setting Eclipse Version of 3.7*
Created attachment 205304 [details] Oracle crash report file showing the problematic frame as ieframe.dll
snippet: public static void main(String[] args) { final Display display = new Display(); Shell shell = new Shell(display); shell.setLayout(new GridLayout()); final Browser external = new Browser(shell, SWT.NONE); external.setLayoutData(new GridData(0,0)); Browser browser = new Browser(shell, SWT.NONE); browser.setLayoutData(new GridData(200,200)); browser.setText("<html><body><a href=\"http://ximbiot.com/cvs/\" target=\"_blank\">click</a></body></html>"); browser.addOpenWindowListener(new OpenWindowListener() { public void open(WindowEvent event) { event.browser = external; } }); external.addLocationListener(new LocationAdapter() { public void changing(LocationEvent event) { event.doit = false; } }); shell.pack(); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); }
(In reply to comment #3) > snippet: Thanks Grant for providing the snippet! :) All, can we get this one addressed in 3.8, do you need any more input? Thanks in advance, Helmut Haigermoser
The problem is that o.e.help.ui.internal.browser.embedded.EmbeddedBrowser.displayURLExternal() is creating a singleton Browser and reusing it for all OpenWindowListener.open() events. However it is expected that the Browser instance that is returned via this event is unique (new). Native IE depends on this to be the case (see http://msdn.microsoft.com/en-us/library/aa768337%28v=vs.85%29.aspx ), and if it is not then it can cause the native browser's refcount to be decremented by a net amount of 1. This is why the crash happens after ~16 repetitions; it is able to get way with a few premature releases without its refcount ever hitting 0, but eventually it does reach 0 and native resources are released that were still needed. The fix is for the EmbeddedBrowser to not re-use the singleton "externalBrowser". I've confirmed that this change fixes the crash, and th extra Browser creations do not affect the memory footprint, because embedded IE's do not consume any real memory until they are displaying content. The changes to be made appear below in the // commented lines. Moving report to UA. private void displayURLExternal(WindowEvent e) { // if (externalBrowser == null) { final Shell externalShell = new Shell(shell, SWT.NONE); externalBrowser = new Browser(externalShell, SWT.NONE); externalBrowser.addLocationListener(new LocationAdapter() { public void changing(final LocationEvent e) { e.doit = false; try { BaseHelpSystem.getHelpBrowser(true).displayURL(e.location); } catch (Throwable t) { String msg = "Error opening external Web browser"; //$NON-NLS-1$ HelpUIPlugin.logError(msg, t); } } }); // } // also, remove the "EmbeddedBrowser.externalBrowser" field e.browser = externalBrowser; }
(In reply to comment #5) > The problem is that > o.e.help.ui.internal.browser.embedded.EmbeddedBrowser.displayURLExternal() Thanks Grant for tracking this one down! :) The fix looks isolated and save to me, UA, can this be picked up for 3.8? TIA, Ciao, hh
One additional comment on comment 7, the fix is improved by adding the following within the "changing()" block, to dispose the created Shell and Browser once they are no longer needed. externalShell.getDisplay().asyncExec(new Runnable() { public void run() { externalShell.dispose(); } });
Fix looks good. Committed to master, integration. http://git.eclipse.org/c/platform/eclipse.platform.ua.git/commit/?id=6e608e3f86e13b043ba22affc3a41b369ffb5cab
Chris I notice in the patch that the additional lines in comment 7 are not added. These lines should be safe to add, and will clean up the extra Shell and Browser instances that are now being created.
(In reply to comment #9) > Chris I notice in the patch that the additional lines in comment 7 are not > added. These lines should be safe to add, and will clean up the extra Shell > and Browser instances that are now being created. Sorry - forgot about that. So something like this? public void changing(final LocationEvent e) { e.doit = false; try { BaseHelpSystem.getHelpBrowser(true).displayURL(e.location); } catch (Throwable t) { String msg = "Error opening external Web browser"; //$NON-NLS-1$ HelpUIPlugin.logError(msg, t); } externalShell.getDisplay().asyncExec(new Runnable() { public void run() { externalShell.dispose(); } }); }
> Sorry - forgot about that. So something like this? yes
Additional lines committed to master, integration. http://git.eclipse.org/c/platform/eclipse.platform.ua.git/commit/?id=b39926c0ab2dd7974b66869b9cd310679d4fc926
(In reply to comment #12) > Additional lines committed to master, integration. > > http://git.eclipse.org/c/platform/eclipse.platform.ua.git/commit/?id=b39926c0ab2dd7974b66869b9cd310679d4fc926 Thanks Chris, good work! :)
