1345990 - Mechanism to ensure tab is loaded in brand new process for mochitest-browser

Status: RESOLVED FIXED

(Testing :: Mochitest, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

Right now there is no good way to ensure that a new tab opened in a browser test is loaded in a brand new process. Instead, tests like the e10s-multi localStorage tests (http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/dom/tests/browser/browser_localStorage_e10s.js#221-265) have to manually boost the processCount to ensure that each process gets a new process.

It would be nice to have a sanctioned mechanism in BrowserTestUtils or similar like `BrowserTestUtils.withNewTab({ forceNewProcess: true })`. This would make writing tests which want to inspect the state of a fresh process much easier.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6350b739d15806d04551d0d26c9866b28e8103df

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

We have enough arguments now that I think it makes sense to switch to an
options object and away from optional arguments. This is pretty ugly. Is there
a better way to do this? Is it worth it?

Review commit: https://reviewboard.mozilla.org/r/134518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134518/

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1345990 - Allow creation of new tabs in their own processes.

This uses XPCOM to replace the default process selector with one that always
asks for a new process and then put the old one back again. This comes with a
test to prove that it works.

Review commit: https://reviewboard.mozilla.org/r/134520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134520/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8862657 [details]
Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

https://reviewboard.mozilla.org/r/134518/#review138584

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:121
(Diff revision 1)
> +      switch (args.length) {
> +        case 0: args.push("about:blank"); // fall through
> +        case 1: args.push(true); // fall through
> +        case 2: args.push(false); // fall through
> +        case 3: args.push(false); // fall through
> +        default: // do nothing
> +      }

This is pretty clever - I've never seen this sort of technique used before to set default arguments from an Array.

It's a little non-obvious though - it took me a sec to figure out what was going on here.

I think we can actually do this a little more clearly using destructuring. Perhaps:

```JS

let [
  opening = "about:blank",
  waitForLoad = true,
  waitForStateStop = false,
  forceNewProcess = false,
] = args;

options = { opening, waitForLoad, waitForStateStop, forceNewProcess };

```

My testing suggests that this will do the same thing.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:135
(Diff revision 1)
> +      options = tabbrowser;
> +      if (!("opening" in options)) {
> +        options.opening = options.url || "about:blank";
> +      }
> +      if (!("waitForLoad" in options)) {
> +        options.waitForLoad = true;
> +      }
> +      if (!("waitForStateStop" in options)) {
> +        options.waitForStateStop = false;
> +      }
> +
> +      tabbrowser = options.gBrowser;

As above, I think destructuring and default values can help here.

Something lke:

```JS

options = tabbrowser;

let {
  opening = "about:blank",
  waitForLoad = true,
  waitForStateStop = false,
} = options;


```

Check these out:

Default values for arrays: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Default_values

Default values for objects: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Default_values_2

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review138586

This looks gerat! Just a few questions below.

::: dom/base/test/browser_force_process_selector.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

Thankfully, these headers aren't required anymore. :)

::: dom/base/test/browser_force_process_selector.js:16
(Diff revision 1)
> +        yield spawnNewAndTest(browser, recur - 1);
> +      }
> +  });
> +}
> +
> +add_task(async function test() {

Great test! Mind adding a quick doc string at the top to describe what it's testing?

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:57
(Diff revision 1)
> +  }
> +};
> +
> +let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> +let selectorFactory = XPCOMUtils._getFactory(NewProcessSelector);
> +registrar.registerFactory(OUR_PROCESSSELECTOR_CID, "", null, selectorFactory);

Do you mean to register so soon, like, once BTU is loaded? Don't we need to register this thing only if and when openNewForegroundTab is called?

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:194
(Diff revision 1)
>          }
>        })
>      ];
>  
> +    if (options.forceNewProcess) {
> +      registrar.registerFactory(PROCESSELECTOR_CID, "", PROCESSELECTOR_CONTRACTID, null);

Are we supposed to be unregistering here instead?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review138666

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862657 [details]
Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134518/diff/1-2/

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134520/diff/1-2/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hey mrbkap, I see dropped issues in the review request, and a new revision, and I remember you mentioning you were going to respond to my review here... so I'm guessing that you wrote some replies, but perhaps didn't publish them?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review138728

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review138586

> Do you mean to register so soon, like, once BTU is loaded? Don't we need to register this thing only if and when openNewForegroundTab is called?

This is registering the CID (class ID) for our process selector with XPCOM. After this call, the only thing that changes is that `Cc[OUR_PROCESSSELECTOR_CID].getService(Ci.nsIProcessSelector)` will start returning one of our objects. This doesn't have any effect on the contract ID (@mozilla.org/processselector;1). This reduces the amount of work we have to do when this is used.

I was considering making this part lazy, but will leave it as-is if you don't think it's worth it.

> Are we supposed to be unregistering here instead?

The code above here was registering our process selector with the contract ID, which we use over in [1] as we create the new tab. This is now restoring the contract ID to point back to the original CID (which in this case will refer over to `MinTabSelector`) -- note that we're registering `PROCESSSELECTOR_CID` and not `OUR_PROCESSSELECTOR_CID`. We don't need to unregister anything, just restore the right class to the contract ID.

[1] http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/dom/ipc/ContentParent.cpp#823-824

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I kept pushing the wrong button in mozreview :-(

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8862657 [details]
Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

https://reviewboard.mozilla.org/r/134518/#review139280

::: commit-message-a477e:4
(Diff revision 2)
> +options object and away from optional arguments. This is pretty ugly. Is there
> +a better way to do this? Is it worth it?

I think we've cleaned this up enough to remove the "This is pretty ugly ... is it worth it" part here. :)

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:110
(Diff revision 2)
> +   * NB: tabbrowser may be an options object containing the rest of the
> +   *     parameters.

Can you rewrite the docstring here to prioritize / advertise the object argument instead, and then have a comment below about the deprecated arguments?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review139286

::: dom/base/test/browser_force_process_selector.js:1
(Diff revision 2)
> +// Make sure that BTU.withNewTab({ ..., forceNewProcess: true }) loads

Nit: please add "use strict"; at the top.

::: dom/base/test/browser_force_process_selector.js:15
(Diff revision 2)
> +          is(gBrowser.browsers.some((b) => b.frameLoader.tabParent.osPid == lastPid),
> +             true,

Nit: Alternatively (and maybe preferably), we can use ok() here:

```js
ok(gBrowser.browsers.some((b) => b.frameLoader.tabParent.osPid == lastPid),
   "...");
```

::: dom/base/test/browser_force_process_selector.js:17
(Diff revision 2)
> +        yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank" }, function* (browser) {
> +          // This browser should share a PID with one of the existing tabs.
> +          let lastPid = browser.frameLoader.tabParent.osPid;
> +          is(gBrowser.browsers.some((b) => b.frameLoader.tabParent.osPid == lastPid),
> +             true,
> +             "we should be reusing processes if not asked to");

Nit: "asked to force the tab into its own process"

::: dom/base/test/browser_force_process_selector.js:22
(Diff revision 2)
> +        let pids = new Set();
> +        let browsers = gBrowser.browsers;
> +        for (let i = 0; i < browsers.length; i++) {
> +          // All of these browsers should be remote browsers.
> +          let curPid = browsers[i].frameLoader.tabParent.osPid;
> +          ok(!pids.has(curPid), "shouldn't have seen this PID yet");
> +          pids.add(curPid);
> +        }
> +      }

Alternatively (I'm not sure if this is much better, tbh - up to you):

```js

let pids = gBrowser.browsers.map(b => b.frameLoader.tabParent.osPid);
let unique = pids.every((pid, index) => {
  return pids.indexOf(pid) == index;
});

ok(unique, "...");

```

And maybe instead of doing this just at the end, we should put this above line 10 here, so that every time one is added, we scan back to make sure the new tab's osPid is unique?

Again, not sure if that's better.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:36
(Diff revision 2)
>      "chrome://mochikit/content/tests/BrowserTestUtils/content-utils.js", true);
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "E10SUtils",
>    "resource:///modules/E10SUtils.jsm");
>  
> +const PROCESSELECTOR_CONTRACTID = "@mozilla.org/ipc/processselector;1";

Nit: `PROCESSELECTOR_CONTRACTID` -> `PROCESSSELECTOR_CONTRACTID` (missing an S)

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:37
(Diff revision 2)
> +const DEFAULT_PROCESSELECTOR_CID =
> +  Components.ID(Cc[PROCESSELECTOR_CONTRACTID].number);
> +const OUR_PROCESSSELECTOR_CID =

Same as above - missing capital S.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:170
(Diff revision 2)
> +    // If we're asked to force a new process, replace the normal process
> +    // selector with one that always asks for a new process.
> +    if (options.forceNewProcess) {
> +      registrar.registerFactory(OUR_PROCESSSELECTOR_CID, "",
> +                                PROCESSELECTOR_CONTRACTID, null);
> +    }
> +
>      let tab;
>      let promises = [
>        BrowserTestUtils.switchTab(tabbrowser, function () {
>          if (typeof opening == "function") {
>            opening();
>            tab = tabbrowser.selectedTab;
>          }
>          else {
>            tabbrowser.selectedTab = tab = tabbrowser.addTab(opening);
>          }
>        })
>      ];
>  
> +    // Restore the original process selector, if needed.
> +    if (options.forceNewProcess) {
> +      registrar.registerFactory(DEFAULT_PROCESSELECTOR_CID, "",
> +                                PROCESSELECTOR_CONTRACTID, null);
> +    }
> +

I wonder if we should wrap this in a `try {} finally {}` to ensure that exceptions don't cause us to stay with this thing registered.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review139286

> Alternatively (I'm not sure if this is much better, tbh - up to you):
> 
> ```js
> 
> let pids = gBrowser.browsers.map(b => b.frameLoader.tabParent.osPid);
> let unique = pids.every((pid, index) => {
>   return pids.indexOf(pid) == index;
> });
> 
> ok(unique, "...");
> 
> ```
> 
> And maybe instead of doing this just at the end, we should put this above line 10 here, so that every time one is added, we scan back to make sure the new tab's osPid is unique?
> 
> Again, not sure if that's better.

I used a `Set` that's passed down through the recursive calls. However, this exposed a bug in the previous iteration of the test: of course checking that the new browser shared a PID with one of the browsers in `gBrowser.browsers` passed: `browser` was in that list! Unfortunately, fixing that caused the test to fail: we have an extra process lying around from startup (I think it's bug 1352021). We end up choosing that process instead of one of the other ones, so the test fails. I've changed it to a `todo` locally, but I'm a little worried that it'll fail in automation since I think we can randomly *pass* if GC happens to collect enough elements before the test runs. We'll see (and if so, we'll have to disable that part of the test).

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862657 [details]
Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134518/diff/2-3/

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134520/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85bc6a5f7c73
Switch to an options object for BTU.openNewForegroundTab. r=mconley
https://hg.mozilla.org/integration/autoland/rev/34f24ca08fac
Allow creation of new tabs in their own processes. r=mconley

Comment 20

[Iris Hsiao [:ihsiao]]

backed this out for mochitest bc testing uncaught exception at BrowserTestUtils.jsm like https://treeherder.mozilla.org/logviewer.html#?job_id=96760727&repo=autoland&lineNumber=2282

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49138b849ad1
Backed out changeset 34f24ca08fac for mochitest bc testing uncaught exception at BrowserTestUtils.jsm
https://hg.mozilla.org/integration/autoland/rev/ea7b2708a17e
Backed out changeset 85bc6a5f7c73 . a=backout

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862657 [details]
Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134518/diff/3-4/

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134520/diff/3-4/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbd3b49c0c29
Switch to an options object for BTU.openNewForegroundTab. r=mconley
https://hg.mozilla.org/integration/autoland/rev/4cc80402cf1c
Allow creation of new tabs in their own processes. r=mconley

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing M(c2)'s test_modal_windows.html on Android opt:

https://hg.mozilla.org/integration/autoland/rev/e3bceb6eb287275e123cb92c487696c6c7325096
https://hg.mozilla.org/integration/autoland/rev/d5cc139abcfdf2b785fe68ee54afdd9283ccfbc2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4cc80402cf1cd907de1d7d124e620f07171d402f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=97069749&repo=autoland

[task 2017-05-06T03:20:04.494142Z] 03:20:04     INFO -  224 INFO TEST-START | toolkit/components/windowwatcher/test/test_modal_windows.html
[task 2017-05-06T03:20:04.494246Z] 03:20:04     INFO -  225 INFO TEST-UNEXPECTED-FAIL | toolkit/components/windowwatcher/test/test_modal_windows.html | uncaught exception - TypeError: Cc[PROCESSSELECTOR_CONTRACTID] is undefined at @resource://testing-common/BrowserTestUtils.jsm:38:1
[task 2017-05-06T03:20:04.494316Z] 03:20:04     INFO -  @chrome://mochitests/content/chrome/toolkit/components/windowwatcher/test/test_modal_windows.html:19:3
[task 2017-05-06T03:20:04.494382Z] 03:20:04     INFO -  simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1652:11

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862657 [details]
Bug 1345990 - Switch to an options object for BTU.openNewForegroundTab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134518/diff/4-5/

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134520/diff/4-5/

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

I could also add ProcessSelector.{js,manifest} to the Android installer, but that seems like a waste. What do you think, Mike?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8862658 [details]
Bug 1345990 - Allow creation of new tabs in their own processes.

https://reviewboard.mozilla.org/r/134520/#review143196

LGTM! Let's do it! Thanks for this, mrbkap.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/799a1e86a450
Switch to an options object for BTU.openNewForegroundTab. r=mconley
https://hg.mozilla.org/integration/autoland/rev/bc1e581949f7
Allow creation of new tabs in their own processes. r=mconley

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/799a1e86a450
https://hg.mozilla.org/mozilla-central/rev/bc1e581949f7

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

Is there a solution planned for the situation where I want N tabs in N separate processes, but I'm okay if those processes already existed?  Right now I can trivially use forceNewProcess to ensure N processes, but it's not particularly efficient since there were probably already existing processes laying around.

It seems like the default MinTabSelector behavior covers this iff aMaxCount=N+1.  (The +1 is for the about:blank tab that I assume has to exist going into the test because otherwise the browser would close?  That part of the browser chrome tests framework seems under-documented per https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests and I've just sort of left the tab alone out of fear of breaking something.)  Unfortunately, needing to mess with the pref to ensure aMaxCount>=N+1 gets back into the land of hacks.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andrew Sutherland [:asuth] from comment #32)
> Is there a solution planned for the situation where I want N tabs in N
> separate processes, but I'm okay if those processes already existed?  Right
> now I can trivially use forceNewProcess to ensure N processes, but it's not
> particularly efficient since there were probably already existing processes
> laying around.

This should be pretty easy to do following the template I created in BrowserTestUtils.jsm:

class SingleProcessSelector {
  // ...XPCOM cruft...
  constructor() {
    this._i = 0;
  }

  provideProcess(type, opener, processes, count, maxCount) {
    let cur = this._i++;
    if (cur < count) {
      return processes[cur];
    }
    return ...NEW_PROCESS;
  }
}

then, you could have something like:

function openTabsInProcesses(n) {
  // register SingleProcessSelector

  // open tabs

  // restore the original process selector
}

and that should do mostly what you want.