1012871 - [e10s] browser does not receive load event for new page in multi-process window

Status: RESOLVED WONTFIX

(Core :: DOM: Events, defect)

Description

[Dave Garrett]

Attachment: loadeventtest.xpi

The XUL browser element receives "load" events for newly loaded tabs, however this doesn't work under electrolysis.

This is needed to get form field actions working in Flagfox under e10s. (they're simple form autofill macros) Specifically, I need to load a new page then inject a content script. I'm already using the message manager to do the script and this works fine in current Firefox, however in an e10s window this fails because the browser never receives the "load" event. Attached is a reduced testcase. Below is the JS minus the boilerplate:

function doTest(window)
{
    var browser = window.getBrowser();
    var newTab = browser.addTab("http://example.com/");
    browser.selectedTab = newTab;
    var browserForTab = browser.getBrowserForTab(newTab);

    var testScript = "data:text/javascript,content.window.alert(\'browser received load event and content script was run\')";

    doOnLoad(browserForTab, function() {
        browserForTab.messageManager.loadFrameScript(testScript,true);
    });
}

function doOnLoad(obj,fn)
{
    function onLoad()
    {
        obj.removeEventListener("load",onLoad,true);
        fn();
    }
    obj.addEventListener("load",onLoad,true);
}

Load any new window to test. The above produces the test alert in a normal window but not in an e10s window. Tested under Linux and Windows XP.

Likely not filed in the right place. Please move if need be.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Events don't propagate automatically from child process to parent.

But why you don't add the load event listener in the frame script?
That one does get the load events for content pages.

Comment 2

[Dave Garrett]

(In reply to Olli Pettay [:smaug] from comment #1)
> But why you don't add the load event listener in the frame script?
> That one does get the load events for content pages.

It's not immediately obvious to me that that was possible. The idea of injecting a script into a tab that has yet to load at all and expecting it to work was not something I considered. What do I even attach a listener to, "content.window", or can I listen on "content"? I'll give it a try.

> Events don't propagate automatically from child process to parent.

It'd be a lot more straightforward for addons if this was not something that had to be considered different for e10s. At minimum, it would be good to have the load/unload events.

Comment 3

[Dave Garrett]

(In reply to Dave Garrett from comment #2)
> What do I even attach a listener to,
> "content.window", or can I listen on "content"? I'll give it a try.

Nope. I can't get either to work.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

.content in the frame script is the top level window object.
But you can add listener to the global itself.
So, 

addEventListener("load", function(e) {dump(e.type + "\n");} , true);

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dave Garrett from comment #2)
> > Events don't propagate automatically from child process to parent.
> 
> It'd be a lot more straightforward for addons if this was not something that
> had to be considered different for e10s. At minimum, it would be good to
> have the load/unload events.

It is perhaps something to consider, however any such event forwarding does cost some performance.

Comment 6

[Dave Garrett]

(In reply to Olli Pettay [:smaug] from comment #4)
> .content in the frame script is the top level window object.
> But you can add listener to the global itself.
> So, 
> 
> addEventListener("load", function(e) {dump(e.type + "\n");} , true);

Yay for bad docs. :/

Yeah, that's not documented properly, unfortunately.
https://developer.mozilla.org/en-US/docs/The_message_manager#Content_script_environment

Apparently this gets used in some examples but isn't actually stated to be available in the script environment anywhere I can see on MDN. Having a full page just to describe the script environment would be helpful, though at a minimum that table there should be kept current.

I managed to get my test alert to fire using the global addEventListener() and with a little more work I've now got Flagfox 5.0 form field actions working under e10s. Thanks. :)

By the way, it is a pain in the ass to only be able to run content scripts via URL. I really wish I could just write a function and pass a reference to be run in the contents' context instead. (yes, I know having a separate file might be cleaner, but I generally try to avoid adding little JS files so data: URIs are what I decided to go with for this case)

(In reply to Olli Pettay [:smaug] from comment #5)
> (In reply to Dave Garrett from comment #2)
> > > Events don't propagate automatically from child process to parent.
> > 
> > It'd be a lot more straightforward for addons if this was not something that
> > had to be considered different for e10s. At minimum, it would be good to
> > have the load/unload events.
> 
> It is perhaps something to consider, however any such event forwarding does
> cost some performance.

Whilst my particular problem is now solved, this bug as filed is still valid. I would still like to request that events get propagated for addons to more easily listen for them.

How much of a performance hit would this really be? The event object would have to be sent with a CPOW I would guess, but if that isn't actually used then I would think that the event sending itself could be done asynchronously. Is this within the realm of correctness, or am I wrong completely? (a likely possibility)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Context switch from child process to parent does take some time (especially on slower machines, where
b2g might run), and if the event forwarding wasn't synchronous, catching load in the parent might not
be too useful, since the child side might have loaded something else already.

Comment 8

[Dave Garrett]

Makes sense. In that case, I'm fine with a WONTFIX here if desired, seeing as there is an alternative.

It would be preferable at least if there was a warning or something, but at a minimum I've updated the docs I was using:
https://developer.mozilla.org/en-US/docs/The_message_manager#Content_script_environment

I've just added these global addEventListener/removeEventListener functions to the table.

Thanks for your quick help pointing me in the right direction.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dave Garrett from comment #8)
> I've just added these global addEventListener/removeEventListener functions
> to the table.
Thanks!


I'm wontfixing this, for now at least.