Bug 319188

Comment 1 Grant Gayed 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 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 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 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 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 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 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 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 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 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 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 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 2011-01-12 11:19:09 EST

released to the 3.7 stream > 20110111