Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 319188 - LocationProvider null when xulrunner is already initialized by another browser
Summary: LocationProvider null when xulrunner is already initialized by another browser
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.5   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.7 M5   Edit
Assignee: Grant Gayed CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-07 16:11 EDT by B. Chen CLA
Modified: 2011-01-12 11:19 EST (History)
6 users (show)

See Also:


Attachments
patch that removes assumption of LocationProvider present (3.26 KB, patch)
2010-07-08 15:29 EDT, Grant Gayed CLA
no flags Details | Diff
patch that adds property to not register factories (6.70 KB, patch)
2010-08-30 14:53 EDT, Grant Gayed CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description B. Chen CLA 2010-07-07 16:11:33 EDT
Build Identifier: 3.5.2

If xulrunner is already initialized by another browser, the following line in Mozilla.java is not executed
		if (!Initialized) {
			LocationProvider = new AppFileLocProvider (mozillaPath);
			LocationProvider.AddRef ();
The LocationProvider is not initialized and caused the exception when executes javascript in public boolean execute (String script)
								String mozillaPath = LocationProvider.mozillaPath + delegate.getJSLibraryName () + '\0';

Reproducible: Always
Comment 1 Grant Gayed CLA 2010-07-08 15:29:26 EDT
Created attachment 173807 [details]
patch that removes assumption of LocationProvider present

Beth can you try this patch?  I believe it will be fine but I don't have a good test case to verify with.
Comment 2 B. Chen CLA 2010-07-08 16:13:54 EDT
Hi Grant,
Thanks for looking into this. I have tested and it works now ! 
However, since we have passed this point, I was able to load our embedded browser first and called java function through BrowserFunction, and then load SWT browser did the same. When I switch back to our embedded Browser. I got this exception. Not sure if this is related to the fix, since without the fix I wouldn't get this far.

Here is the exception stack -

2010/07/08 15:09:15.484 SEVERE CLPDN0031E: Event loop exception ::class.method=com.ibm.rcp.personality.framework.internal.RCPWorkbenchAdvisor.eventLoopException() ::thread=main ::loggername=com.ibm.rcp.personality.framework.internal

	java.lang.ClassCastException: com.ibm.rcp.dombrowser.browser.DOMBrowser incompatible with org.eclipse.swt.browser.Browser
	at org.eclipse.swt.browser.MozillaDelegate.findBrowser(MozillaDelegate.java:31)
	at org.eclipse.swt.browser.Mozilla.findBrowser(Mozilla.java:1871)
	at org.eclipse.swt.browser.Mozilla.findBrowser(Mozilla.java:1916)
	at org.eclipse.swt.browser.PromptService2.getBrowser(PromptService2.java:138)
	at org.eclipse.swt.browser.PromptService2.Alert(PromptService2.java:163)
	at org.eclipse.swt.browser.PromptService2$2.method3(PromptService2.java:56)
	at org.eclipse.swt.internal.mozilla.XPCOMObject.callback3(XPCOMObject.java:266)
	at org.eclipse.swt.internal.win32.OS.DispatchMessageW(Native Method)
	at org.eclipse.swt.internal.win32.OS.DispatchMessage(OS.java:2411)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3501)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2384)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2348)
	at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2200)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:495)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:288)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:490)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at com.ibm.rcp.personality.framework.internal.RCPApplication.run(RCPApplication.java:67)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:48)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:37)
	at java.lang.reflect.Method.invoke(Method.java:600)
	at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:574)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:195)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:386)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:48)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:37)
	at java.lang.reflect.Method.invoke(Method.java:600)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:549)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:504)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1236)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1212)
Comment 3 Grant Gayed CLA 2010-07-08 17:03:39 EDT
This is a different problem, when the swt Browser is created it is overwriting the registered (and unfortunately shared) PromptServiceFactory.  When an alert is to be shown in the DOMBrowser it is coming into swt's handler, which assumes that the parent widget resolves to an swt Browser, not something else.

SWT's Mozilla class could skip registering its factories whenever "Initialized" is true during startup, but this doesn't seem good.  I'll have to think about alternate approaches.

I'll hold off on committing this bug's fix in the meantime.
Comment 4 Jun Yue Liu CLA 2010-08-19 02:12:46 EDT
Hi Grant, any update on this? This is a big impact for us.  In my opinion, it will be good to have a way to tell SWT's Mozilla to skip registering its factories.
Comment 5 Grant Gayed CLA 2010-08-20 16:33:22 EDT
We've been thinking about this.  Not registering our factories when the "Initialized" property is set does not seem good, as the other browser implementation may not provide substitute factories for the full set of factories that the Browser wants to handle.

One possibility would be for you to provide a XULRunnerInitializer (maybe you do this already?) as described in http://www.eclipse.org/swt/faq.php#specifyxulrunner .  This is invoked the first time an swt Browser is created, which is when its factories are registered.  If you provided an implementation of this then you would know when your factories were being overwritten, and could then re-register them.  This would look something like:

public class XULRunnerInitializer {
    static {
        if (!ourFactoriesAreCurrentlyHooked()) return; // nothing to do
        final Display display = Display.getCurrent(); 
        display.asyncExec(new Runnable() {
            public void run() {
                if (display.isDisposed()) return; // run away
                // ... rehook our factories here ...
            }
        });
    }
}

Your thoughts?
Comment 6 pei na CLA 2010-08-24 02:27:35 EDT
To re-register facotries in XULRunnerInitializer may have timing issue. The timing of asynExec is uncertain. If a callback is called before the asynExec being executed, it will still come into the SWT's handler and cause execution failures.

Can we define a new System property to indicate if the factory could be override? It will be like this:
   	
int[] IsCIDRegistered = new int[1];
rc = componentRegistrar.IsCIDRegistered(XPCOM.NS_PROMPTSERVICE_CID,IsCIDRegistered);
	
if("true".equals(System.getProperty(DONT_OVERRIDE_FACTORY)) &&   IsCIDRegistered[0] == 1)
{
    //do nothing if another browser has already registered this factory and set the "don't override" flag to be true.
}
else
{
    rc = componentRegistrar.RegisterFactory (XPCOM.NS_PROMPTSERVICE_CID, aClassName, aContractID, factory.getAddress ());
     if (rc != XPCOM.NS_OK) {
	browser.dispose ();
	error (rc);
      }
}
	
factory.Release ();
	
1.If another browser has already registered the factory and set the DONT_OVERRIDE_FACTORY property. SWT Browser will skip this factory.

2.If another browser set the DONT_OVERRIDE_FACTORY property but doesn't register this factory. SWT Browser will register its own factory.

3.If another browser register the factory but doesn't set the DONT_OVERRIDE_FACTORY property. SWT Browser will still override it with its own factory.
Comment 7 Grant Gayed CLA 2010-08-26 16:28:13 EDT
We can create the property as you suggest, but if it's set then I think the Browser will just skip registering all of its factories.  Checking whether a factory has already been registered for a given service is not too helpful because native mozilla provides default (so non-null) factories in some cases which don't do anything (eg.- for PromptService on Linux).

I can add this property if this seems fine with you.  This would change your example from the last comment to:

if ("true".equals(System.getProperty(DONT_OVERRIDE_FACTORY)) {
    // don't do it
} else {
    // register our factory
}
Comment 8 pei na CLA 2010-08-26 22:31:26 EDT
Thanks, that's exactly what we need.

Can we rename the system property to "DONT_OVERRIDE_XULRUNNER_FACTORY" to make it more specific?

(In reply to comment #7)
> We can create the property as you suggest, but if it's set then I think the
> Browser will just skip registering all of its factories.  Checking whether a
> factory has already been registered for a given service is not too helpful
> because native mozilla provides default (so non-null) factories in some cases
> which don't do anything (eg.- for PromptService on Linux).
> 
> I can add this property if this seems fine with you.  This would change your
> example from the last comment to:
> 
> if ("true".equals(System.getProperty(DONT_OVERRIDE_FACTORY)) {
>     // don't do it
> } else {
>     // register our factory
> }
Comment 9 Grant Gayed CLA 2010-08-30 14:53:38 EDT
Created attachment 177766 [details]
patch that adds property to not register factories

Here's a patch that can be applied to either the 3.6.x stream or 3.7 stream.  If system property "org.eclipse.swt.browser.MozillaFactoriesRegistered" has value "true" then it skips adding all of its factories, as well as its WindowCreator.

The only factory that I was initially not sure about excluding if this property is set was the ExternalFactory that enables JS to call Java.  I concluded that it should be excluded, like all of the other factories, since I believe that your browser implementation offers similar functionality.  If this was the wrong choice then let me know and it can change.

This has not been released yet.  Please give it a try in your environment and follow up here whether any further changes are needed.  The fix that is in this report's initial attachment (the null LocationProvider) is already in the 3.6.x and 3.7 streams.
Comment 10 pei na CLA 2010-08-31 01:49:38 EDT
Yes, the ExternalFactory should be excluded, as well as all of the other factories. I tried this patch and it works fine in our environment. Thank!

This patch is really important for us. It would be appreciated if the patch could be released soon so that it could be included in SWT 3.6.1.
Comment 11 Grant Gayed CLA 2010-08-31 11:51:51 EDT
I don't think this fix is appropriate for 3.6.1, as it does not meet the typical criteria for inclusion in a maintenance stream, and it has not had any real period of testing.  This fix would require PMC approval, and it would be difficult to justify the risk to eclipse given that it's not fixing a crash, etc. in eclipse.

I believe that you build a custom version of swt based on our releases, right?  If so, are you able to apply this patch to your custom swt?
Comment 12 David Klein CLA 2010-12-15 15:13:05 EST
If the fix is not appropriate for a maintenance stream, would ask that it be considered for inclusion in the 3.7 stream which we are targeting for an upcoming release.
Comment 13 Grant Gayed CLA 2011-01-12 11:19:09 EST
released to the 3.7 stream > 20110111