935799 - Add e10s utilities to mochitest

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

This patch adds a new script, testing/mochitest/mochitest-e10s-utils.js, specifically for helping the test suite run with e10s enabled.

The script is loaded into browser-test-overlay.xul, but is only initialized if --e10s was used to run the tests.  Thus, it should have no impact when the test suite is run normally.

This utility code has 2 main roles:

* Sets a "location stub" object as the browser's contentWindow and contentDocument, which is overwritten as the real CPOW is made available.  This means tests which do, eg, "browser.contentWindow.location = some_url" immediately after creating the browser works as expected - otherwise the tests would fail as browser.contentWindow is null.

This hack could be removed once we update all the tests to not load the browser in this way - but lots of tests do, so this seems a reasonable interim solution.

* Adds new event listeners for events commonly used by the tests.  The content script has the event handlers and send a message back to the parent.  The parent queues these events until the real window/document is available, then re-dispatches them.  This re-dispatching isn't perfect - eg, event.target is different than what it should be, but it is good enough to get most of the test which use these events working.

These events are generally *not* used by the real browser code - only tests rely on them (although the "pageshow" event will need love for the real browser).  For this reason, I'm not proposing to add support for these events to browser-content etc.  I'm not sure what the long term answer should be here - I suspect we will want some "official" way for tests to know that a browser had loaded and should move all affected tests to using that mechanism.

The code uses a new observer notification I'm proposing in bug 935793.  The code could probably do with some cleaning up, but figured I'd get early feedback first.

At this stage I'm requesting feedback from various people:
felipe, billm, dvander: I guess feedback from any one of you would do, but the more the merrier :)
ted: I'd really like a build peer to offer some feedback too - please feel free to pass on to someone else if desired.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 828375 [details] [diff] [review]
Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

Review of attachment 828375 [details] [diff] [review]:
-----------------------------------------------------------------

Very cool! :)

::: testing/mochitest/mochitest-e10s-utils.js
@@ +3,5 @@
> +// There are some tricks/shortcuts that test code takes that we don't see
> +// in the real browser code.  These include setting content.location.href
> +// (which typically doesn't work in test code as the document object is yet to
> +// be created), waiting for certain events the main browser doesn't care about
> +// eg, the "pageshow" or "load" events).

this comment seems contradictory, unless I misunderstood:
"some tricks that test code takes".. vs
"setting content.location.href which typically doesn't work in test code"

@@ +62,5 @@
> +  let browser = subject.ownerElement;
> +  let messageManager = browser.messageManager;
> +  // Listen for an 'oop-browser-crashed' event and log it so people analysing
> +  // test logs have a clue about what is going on.
> +  browser.addEventListener("oop-browser-crashed", () => {

this could be added as an event listener at the window level so you don't need to do it for every particular browser created

@@ +73,5 @@
> +  pendingEvents.set(browser, []);
> +  // A message listener for the events which send the real window - we queue
> +  // up events until this has been called, as the tests are likely to try and
> +  // reference the contentWindow in the event handler (and the pageshow handler
> +  // in browser.js does too)

I'm a bit surprised that the whole pending thing is necessary.. i.e. that the event-forwarding framescript is able to be loaded *and* capture events and forward them, before the first Content:StateChange happens to set-up the CPOW.

@@ +78,5 @@
> +  let stateChangeHandler = function(message) {
> +    // This is dependent on the implementation in RemoteWebProgress.jsm.
> +    // If json.isTopLevel is true, then the real window will become available.
> +    if (message.json.isTopLevel) {
> +      messageManager.removeMessageListener("Content:StateChange", stateChangeHandler)

instead of listening to the message why not use a progress listener at RemoteWebProgress that is already listening to it?

@@ +95,5 @@
> +        }
> +      }, true);
> +    });
> +  }
> +  let dataUrl = "data:application/javascript," + encodeURI(toInject.toSource() + "();");

do you expect that more will need to be injected later? I think that this short function is fine but it's usually easier to find on DXR, understand it etc. if we just use a separate file for it.. and it makes it easier to add more supporting code in the future.

@@ +136,5 @@
> +
> +function e10s_init() {
> +  // We add an observer so we can notice new <browser> elements created and
> +  // if they are remote ones, inject some test-specific code into it.
> +  Services.obs.addObserver(observeNewFrameloader, "oop-frameloader-created", false);

Similar to what I did for oop-browser-crashed, is it possible for this to be an event targeted at the browser instead of a global notification?
/me dislikes observer API

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

(In reply to :Felipe Gomes from comment #1)
> this comment seems contradictory, unless I misunderstood:
> "some tricks that test code takes".. vs
> "setting content.location.href which typically doesn't work in test code"

I clarified this.

> this could be added as an event listener at the window level so you don't
> need to do it for every particular browser created

Yeah, thanks!

> I'm a bit surprised that the whole pending thing is necessary.. i.e. that

I initially started without this pending thing, then diagnosed one test failure as being due to this - but didn't take notes etc.  Now that I remove it, I can't reproduce a problem - so I've nuked this - it makes things much simpler.  If it comes back up, I'll worry about it then.

> do you expect that more will need to be injected later? I think that this
> short function is fine but it's usually easier to find on DXR, understand it
> etc. if we just use a separate file for it.. and it makes it easier to add
> more supporting code in the future.

Yeah - it's a little hard to justify a new file for these 4 lines, but OTOH your points are all valid.  I'll probably create a new file, but that's not included in this update.

> > +function e10s_init() {
> > +  // We add an observer so we can notice new <browser> elements created and
> > +  // if they are remote ones, inject some test-specific code into it.
> > +  Services.obs.addObserver(observeNewFrameloader, "oop-frameloader-created", false);
> 
> Similar to what I did for oop-browser-crashed, is it possible for this to be
> an event targeted at the browser instead of a global notification?
> /me dislikes observer API

I kinda like observers - IIUC, they are very light-weight when no observers exist :)  But as per Smaug's comment in bug 935793, I'll try and do it via a mutation observer.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 828453 [details] [diff] [review]
Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

Review of attachment 828453 [details] [diff] [review]:
-----------------------------------------------------------------

This seems good to me.

One alternate implementation would be to use the globalmessagemanager to load a content script into every <browser> element. That script could check if it's in a child process and send a message in that case. The listeners would also be registered on the globalmessagemanager.

Here's how you get the process type:
       let inParent = Cc["@mozilla.org/xre/app-info;1"]
                        .getService(Ci.nsIXULRuntime)
                        .processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;

Here's an example of using the globalmessagemanager:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#42

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 828453 [details] [diff] [review]
Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

Review of attachment 828453 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/mochitest-e10s-utils.js
@@ +67,5 @@
> +        }
> +      }, true);
> +    });
> +  }
> +  let dataUrl = "data:application/javascript," + encodeURI(toInject.toSource() + "();");

It would be nicer to stick this in a separate script file than using this trick.

@@ +84,5 @@
> +  // if they are remote ones, inject some test-specific code into it.
> +  Services.obs.addObserver(observeNewFrameloader, "oop-frameloader-created", false);
> +  // Listen for an 'oop-browser-crashed' event and log it so people analysing
> +  // test logs have a clue about what is going on.
> +  window.addEventListener("oop-browser-crashed", (event) => {

We probably need to think more about how we want to treat content process crashes. I suspect whatever we do for plugin crashes is probably what we want to emulate.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

This has addressed the previous feedback comments (eg, now a separate content script, using the global message manager, etc) and if now much simpler and easier to understand.

Requesting review from Ted as a build peer and Felipe as someone with good understanding of both e10s and b-c tests.

All dependent bugs are closed, so this is ready to land once r+ is given.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8380397 [details] [diff] [review]
0002-Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

Review of attachment 8380397 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/mochitest-e10s-utils.js
@@ +32,5 @@
> +};
> +
> +// This object is used in place of contentWindow while we wait for it to be
> +// overwritten as the real window becomes available.
> +let windowStub = function(browser) {

Could you name this something like `temporaryWindowStub` to make it easier to figure out that it will be replaced?

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8380397 [details] [diff] [review]
0002-Add-mochitest-e10s-utils.js-to-help-tests-when-run-i.patch

Review of attachment 8380397 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/mochitest-e10s-utils.js
@@ +8,5 @@
> +// or "load" events).
> +// So we make some hacks to pretend these work in the test suite.
> +
> +// Ideally all these hacks could be removed, but this can only happen when
> +// the tests are refactored to not use these tricks.

Do you think this is a feasible goal? If so we should at least get a bug on file with a description of the problem.

@@ +80,5 @@
> +  // test logs have a clue about what is going on.
> +  window.addEventListener("oop-browser-crashed", (event) => {
> +    let uri = event.target.currentURI;
> +    Cu.reportError("remote browser crashed while on " +
> +                   (uri ? uri.spec : "<unknown>") + "\n");

We probably need to do more here, we probably want to quit the entire test run when this happens. See the related discussion (and patch) in bug 976120 re: B2G.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Felipe Gomes from comment #6)
> Could you name this something like `temporaryWindowStub` to make it easier
> to figure out that it will be replaced?

Done - although I named it TemporaryWindowStuff - the object is created via "new ...", so the first letter being caps seems the correct style.


(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)

> > +// Ideally all these hacks could be removed, but this can only happen when
> > +// the tests are refactored to not use these tricks.
> 
> Do you think this is a feasible goal? If so we should at least get a bug on
> file with a description of the problem.

It's feasible, but still unlikely :)  I updated the comment to reflect that.

> @@ +80,5 @@
> > +  // test logs have a clue about what is going on.
> > +  window.addEventListener("oop-browser-crashed", (event) => {
> > +    let uri = event.target.currentURI;
> > +    Cu.reportError("remote browser crashed while on " +
> > +                   (uri ? uri.spec : "<unknown>") + "\n");
> 
> We probably need to do more here, we probably want to quit the entire test
> run when this happens. See the related discussion (and patch) in bug 976120
> re: B2G.

At this stage, such crashes aren't particularly uncommon, and in general the test suite continues happily - so at the moment I think there's more value in letting it try and stumble on.  We can address this later if it becomes a real problem.

Thanks for the reviews!

https://hg.mozilla.org/integration/fx-team/rev/2ed56735cb6a

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2ed56735cb6a

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Mark Hammond [:markh] from comment #8)
> At this stage, such crashes aren't particularly uncommon, and in general the
> test suite continues happily - so at the moment I think there's more value
> in letting it try and stumble on.  We can address this later if it becomes a
> real problem.

Can you file a followup on this at least? We should make sure this does the right thing before we flip this on in automation.