Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 361118

Summary: Eclipse crashes in ieframe.dll when repeatedly opening the same link in an external browser
Product: [Eclipse Project] Platform Reporter: Helmut J. Haigermoser <helmut.haigermoser>
Component: User AssistanceAssignee: platform-ua-inbox <platform-ua-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: ChrisAustin, grant_gayed, remy.suen
Version: 3.7   
Target Milestone: 3.8 M4   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Attachments:
Description Flags
Oracle crash report file showing the problematic frame as ieframe.dll none

Description Helmut J. Haigermoser CLA 2011-10-17 06:16:02 EDT 
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
Comment 1 Helmut J. Haigermoser CLA 2011-10-17 06:16:33 EDT 
CQ:WIND00310454

*setting Eclipse Version of 3.7*
Comment 2 Helmut J. Haigermoser CLA 2011-10-17 06:17:19 EDT 
Created attachment 205304 [details]
Oracle crash report file showing the problematic frame as ieframe.dll
Comment 3 Grant Gayed CLA 2011-10-21 15:09:00 EDT 
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();
}
Comment 4 Helmut J. Haigermoser CLA 2011-11-14 10:43:09 EST 
(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
Comment 5 Grant Gayed CLA 2011-11-25 15:15:49 EST 
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;
}
Comment 6 Helmut J. Haigermoser CLA 2011-11-28 04:10:44 EST 
(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
Comment 7 Grant Gayed CLA 2011-11-28 11:01:02 EST 
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();
    }
});
Comment 8 Chris Austin CLA 2011-11-29 14:30:30 EST 
Fix looks good.  Committed to master, integration.

http://git.eclipse.org/c/platform/eclipse.platform.ua.git/commit/?id=6e608e3f86e13b043ba22affc3a41b369ffb5cab
Comment 9 Grant Gayed CLA 2011-11-29 14:42:21 EST 
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.
Comment 10 Chris Austin CLA 2011-11-29 15:28:31 EST 
(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();
	    }
	});
}
Comment 11 Grant Gayed CLA 2011-11-29 15:33:10 EST 
> Sorry - forgot about that.  So something like this?

yes
Comment 12 Chris Austin CLA 2011-11-29 15:51:19 EST 
Additional lines committed to master, integration.

http://git.eclipse.org/c/platform/eclipse.platform.ua.git/commit/?id=b39926c0ab2dd7974b66869b9cd310679d4fc926
Comment 13 Helmut J. Haigermoser CLA 2011-11-30 04:49:49 EST 
(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! :)