Closed Bug 1472212 Opened 6 years ago Closed 6 years ago

Set browser.tabs.remote.separatePrivilegedContentProcess to true by default in Nightly

Categories

(Firefox :: New Tab Page, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: imjching, Assigned: mconley)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(11 files, 3 obsolete files)

59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
Mardak
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
dao
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
52 bytes, text/x-github-pull-request
Details | Review
Now that we have landed Bug 1469072 that adds the infrastructure to move Activity Stream into its own content process, we should probably turn this feature on by default.

We need to set "browser.tabs.remote.separatePrivilegedContentProcess" to true.
Depends on: 1472221
Are you planning on doing this one too?
Flags: needinfo?(jlim)
(In reply to Kate Hudson :k88hudson from comment #1)
> Are you planning on doing this one too?

Yes. I'll be doing this one.
Flags: needinfo?(jlim)
Assignee: nobody → jlim
Depends on: 1473146
Iteration: --- → 63.2 - July 23
Status: NEW → ASSIGNED
Comment on attachment 8991075 [details]
Bug 1472212 - Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds.

https://reviewboard.mozilla.org/r/256074/#review263018

Thanks, Jay! See my notes below.

Also, are we able to wait until the ScriptPreloader stuff lands before landing this? That way, if there are regressions, we'll have a better shot of narrowing down where the problem is.

::: modules/libpref/init/all.js:3266
(Diff revision 1)
>  // sorts of pages, which we have to do when we run them in the normal web
>  // content process, causes compatibility issues.
>  pref("browser.tabs.remote.allowLinkedWebInFileUriProcess", true);
>  
>  // Pref to control whether we use separate privileged content processes.
> -pref("browser.tabs.remote.separatePrivilegedContentProcess", false);
> +pref("browser.tabs.remote.separatePrivilegedContentProcess", true);

Actually, let's only set this to true for Firefox - so let's patch browser/app/profile/firefox.js to override the false with a true.

Also, for now, let's guard this and ensure that it's only shipping on Nightly until we're confident it can go out.

Here's what I recommend:

```js
#ifdef NIGHTLY_BUILD
pref("browser.tabs.remote.separatePrivilegedContentProcess", true);
#else
pref("browser.tabs.remote.separatePrivilegedContentProcess", false);
#endif
```
Attachment #8991075 - Flags: review?(mconley) → review-
Depends on: 1416066
(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #4)
> Comment on attachment 8991075 [details]
> Bug 1472212 - Set browser.tabs.remote.separatePrivilegedContentProcess to
> true by default.
> 
> https://reviewboard.mozilla.org/r/256074/#review263018
> 
> Thanks, Jay! See my notes below.
> 
> Also, are we able to wait until the ScriptPreloader stuff lands before
> landing this? That way, if there are regressions, we'll have a better shot
> of narrowing down where the problem is.
> 
> ::: modules/libpref/init/all.js:3266
> (Diff revision 1)
> >  // sorts of pages, which we have to do when we run them in the normal web
> >  // content process, causes compatibility issues.
> >  pref("browser.tabs.remote.allowLinkedWebInFileUriProcess", true);
> >  
> >  // Pref to control whether we use separate privileged content processes.
> > -pref("browser.tabs.remote.separatePrivilegedContentProcess", false);
> > +pref("browser.tabs.remote.separatePrivilegedContentProcess", true);
> 
> Actually, let's only set this to true for Firefox - so let's patch
> browser/app/profile/firefox.js to override the false with a true.
> 
> Also, for now, let's guard this and ensure that it's only shipping on
> Nightly until we're confident it can go out.
> 
> Here's what I recommend:
> 
> ```js
> #ifdef NIGHTLY_BUILD
> pref("browser.tabs.remote.separatePrivilegedContentProcess", true);
> #else
> pref("browser.tabs.remote.separatePrivilegedContentProcess", false);
> #endif
> ```

Thank you! That works too. I have updated the patch and pushed to TryServer. The tests are still running now. Will take a look at them again later.
Comment on attachment 8991075 [details]
Bug 1472212 - Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds.

https://reviewboard.mozilla.org/r/256074/#review263456

::: modules/libpref/init/all.js
(Diff revision 2)
> -// Pref to control whether we use separate privileged content processes.
> -pref("browser.tabs.remote.separatePrivilegedContentProcess", false);
> -

I think it still makes sense to have this default false over in all.js, and we can override in firefox.js.

Defaulting in all.js is for other things that use Gecko, like Fennec, GeckoView, TB, etc.
Attachment #8991075 - Flags: review?(mconley) → review-
Comment on attachment 8991643 [details]
Bug 1472212 - Expose the pref variable for browser.tabs.remote.separatePrivilegedContentProcess in AboutNewTabService over XPCOM for tests.

https://reviewboard.mozilla.org/r/256566/#review263458

::: commit-message-f9e95:3
(Diff revision 1)
> +We need to default that pref to false in non-firefox builds because we are only defining
> +the pref in browser/app/profile/firefox.js.

We can still define it to false in all.js - firefox.js will just override it.

Does that change the necessity of this patch?
Comment on attachment 8991643 [details]
Bug 1472212 - Expose the pref variable for browser.tabs.remote.separatePrivilegedContentProcess in AboutNewTabService over XPCOM for tests.

https://reviewboard.mozilla.org/r/256566/#review263432

::: browser/components/newtab/aboutNewTabService.js:115
(Diff revision 1)
>    observe(subject, topic, data) {
>      switch (topic) {
>        case "nsPref:changed":
>          if (data === PREF_SEPARATE_PRIVILEGED_CONTENT_PROCESS) {
> -          this._privilegedContentProcess = Services.prefs.getBoolPref(PREF_SEPARATE_PRIVILEGED_CONTENT_PROCESS);
> +          this._privilegedContentProcess = Services.prefs.getBoolPref(PREF_SEPARATE_PRIVILEGED_CONTENT_PROCESS, false);
> +          this.notifyChange();

This is here so that we can wait until `aboutNewTabService` updates the pref variable locally. We need to wait for this event because we rely on that variable to set `defaultURL`, and the value stored in `defaultURL` is used in tests.
Comment on attachment 8991643 [details]
Bug 1472212 - Expose the pref variable for browser.tabs.remote.separatePrivilegedContentProcess in AboutNewTabService over XPCOM for tests.

https://reviewboard.mozilla.org/r/256566/#review263458

> We can still define it to false in all.js - firefox.js will just override it.
> 
> Does that change the necessity of this patch?

I have updated the patch to expose the pref variable over XPCOM. We need the variable in the tests to determine which version of HTML are we rendering in Activity Stream.
Comment on attachment 8991075 [details]
Bug 1472212 - Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds.

https://reviewboard.mozilla.org/r/256074/#review263464

Looks good, thanks!
Attachment #8991075 - Flags: review?(mconley) → review+
Comment on attachment 8991643 [details]
Bug 1472212 - Expose the pref variable for browser.tabs.remote.separatePrivilegedContentProcess in AboutNewTabService over XPCOM for tests.

https://reviewboard.mozilla.org/r/256566/#review263698

Clearing reviews at imjching's request.
Attachment #8991643 - Flags: review?(mconley)
Comment on attachment 8991644 [details]
Bug 1472212 - Update URLs to include the noscripts version in xpcshell and browser tests for newtab.

https://reviewboard.mozilla.org/r/256568/#review263700

Clearing reviews at imjching's request.
Attachment #8991644 - Flags: review?(mconley)
Priority: -- → P1
Comment on attachment 8994205 [details]
Bug 1472212 - Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type.

https://reviewboard.mozilla.org/r/258802/#review265706


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/general/browser_e10s_about_process.js:86
(Diff revision 1)
>  
>  function test_url(url, chromeResult, contentResult) {
>    is(E10SUtils.canLoadURIInProcess(url, CHROME_PROCESS),
>       chromeResult, "Check URL in chrome process.");
> -  is(E10SUtils.canLoadURIInProcess(url, CONTENT_PROCESS),
> -     contentResult, "Check URL in content process.");
> +  is(E10SUtils.canLoadURIInProcess(url, WEB_CONTENT_PROCESS),
> +     webContentResult, "Check URL in web content process.");

Error: 'webContentResult' is not defined. [eslint: no-undef]

::: browser/base/content/test/general/browser_e10s_about_process.js:91
(Diff revision 1)
> -     contentResult, "Check URL in content process.");
> +     webContentResult, "Check URL in web content process.");
>  
>    is(E10SUtils.canLoadURIInProcess(url + "#foo", CHROME_PROCESS),
>       chromeResult, "Check URL with ref in chrome process.");
> -  is(E10SUtils.canLoadURIInProcess(url + "#foo", CONTENT_PROCESS),
> -     contentResult, "Check URL with ref in content process.");
> +  is(E10SUtils.canLoadURIInProcess(url + "#foo", WEB_CONTENT_PROCESS),
> +     webContentResult, "Check URL with ref in web content process.");

Error: 'webContentResult' is not defined. [eslint: no-undef]

::: browser/base/content/test/general/browser_e10s_about_process.js:96
(Diff revision 1)
> -     contentResult, "Check URL with ref in content process.");
> +     webContentResult, "Check URL with ref in web content process.");
>  
>    is(E10SUtils.canLoadURIInProcess(url + "?foo", CHROME_PROCESS),
>       chromeResult, "Check URL with query in chrome process.");
> -  is(E10SUtils.canLoadURIInProcess(url + "?foo", CONTENT_PROCESS),
> -     contentResult, "Check URL with query in content process.");
> +  is(E10SUtils.canLoadURIInProcess(url + "?foo", WEB_CONTENT_PROCESS),
> +     webContentResult, "Check URL with query in web content process.");

Error: 'webContentResult' is not defined. [eslint: no-undef]

::: browser/base/content/test/general/browser_e10s_about_process.js:101
(Diff revision 1)
> -     contentResult, "Check URL with query in content process.");
> +     webContentResult, "Check URL with query in web content process.");
>  
>    is(E10SUtils.canLoadURIInProcess(url + "?foo#bar", CHROME_PROCESS),
>       chromeResult, "Check URL with query and ref in chrome process.");
> -  is(E10SUtils.canLoadURIInProcess(url + "?foo#bar", CONTENT_PROCESS),
> -     contentResult, "Check URL with query and ref in content process.");
> +  is(E10SUtils.canLoadURIInProcess(url + "?foo#bar", WEB_CONTENT_PROCESS),
> +     webContentResult, "Check URL with query and ref in web content process.");

Error: 'webContentResult' is not defined. [eslint: no-undef]
I have updated the patch to fix all the tests that were failing previously. They were relying on the fact that `about:home` and `about:newtab` runs in the same content process as normal web content.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e83562641a42a68dfbd56166f7a35d91e34dc1.
Attachment #8994207 - Attachment is obsolete: true
Attachment #8994207 - Flags: review?(gijskruitbosch+bugs)
Iteration: 63.2 - July 23 → 63.3 - Aug 6
Comment on attachment 8994204 [details]
Bug 1472212 - Remove unnecessary async/await keywords for browser_new_tab_in_privileged_process_pref.js.

https://reviewboard.mozilla.org/r/258800/#review265844
Attachment #8994204 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8994203 [details]
Bug 1472212 - Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window.

https://reviewboard.mozilla.org/r/258798/#review265852

::: commit-message-7dd8b:5
(Diff revision 1)
> +Bug 1472212 - Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window.
> +
> +Now that we have moved some about: pages to the privileged content process,
> +opening these URLs in a new window will trigger SessionStore to restore the tab state.
> +We will disable the busy attribute for these new tabs and set favicons for them

They're not always new tabs though, right?

::: browser/base/content/tabbrowser.js:11
(Diff revision 1)
>  /* eslint-env mozilla/browser-window */
>  
>  /**
>   * A set of known icons to use for internal pages. These are hardcoded so we can
>   * start loading them faster than ContentLinkHandler would normally find them.
> + * This set of icons is also defined in SessionStore.jsm.

We should figure out a way to share these. Duplicating it feels unfortunate - this in addition to the fact that these are of course already duplicated from the actual pages...

Can we think of a better shared place for this to live? Alternatively, it looks like you're already calling into gBrowser from sessionstore to determine if this is the type of page where we should set an icon. Perhaps we could delegate more of that logic and just add a method that takes a tab (or browser) and URL and sets the icon accordingly iff the URL matches tabbrowser's ideas about a URL for which it knows the icon?

::: browser/components/sessionstore/SessionStore.jsm:2453
(Diff revision 1)
> +    let uri = newTab.linkedBrowser.currentURI;
> +    if (!uri || (uri && !aWindow.gBrowser._isLocalAboutURI(uri))) {

Is duplicating this logic across both duplicateTab and navigateAndRestore necessary? Can't we just do it in `restoreTab` ?

::: browser/components/sessionstore/SessionStore.jsm:2455
(Diff revision 1)
>  
>      // Start the throbber to pretend we're doing something while actually
>      // waiting for data from the frame script.
> +    let uri = newTab.linkedBrowser.currentURI;
> +    if (!uri || (uri && !aWindow.gBrowser._isLocalAboutURI(uri))) {
> -    newTab.setAttribute("busy", "true");
> +      newTab.setAttribute("busy", "true");

The summary of this change says we only prevent this in a new window, but as written this will prevent the spinner even in other cases like duplicating tabs, or restoring a closed or crashed tab. Is that intentional?

::: browser/components/sessionstore/SessionStore.jsm:3048
(Diff revision 1)
> +      uri = Services.uriFixup.createFixupURI(loadArguments.uri,
> +                                             Ci.nsIURIFixup.FIXUP_FLAG_NONE);

I'm confused about this... why not Services.io.newURI(), and/or if we're using fixup, why pass the NONE flags rather than the other ones? Why do we need fixup at all, why can't we compare strings?
Attachment #8994203 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8991643 [details]
Bug 1472212 - Expose the pref variable for browser.tabs.remote.separatePrivilegedContentProcess in AboutNewTabService over XPCOM for tests.

https://reviewboard.mozilla.org/r/256566/#review265860

Sorry for not knowing the history here, but why do we need to expose this via XPCOM if it's just for tests? Couldn't tests check the pref themselves? And if they really need to check the actual service object, could we make the JS constructor set `this.wrappedJSObject = this` and have tests check `service.wrappedJSObject._whatever` property instead, rather than adding to the XPCOM interface solely for the sake of tests?
Attachment #8991643 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8994209 [details]
Bug 1472212 - Load `http://example.org/` instead of `about:home` for some tests.

https://reviewboard.mozilla.org/r/258810/#review265864
Attachment #8994209 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8994208 [details]
Bug 1472212 - Handle navigation away from privileged content process in RDM.

https://reviewboard.mozilla.org/r/258808/#review265866

Thanks, looks reasonable to me! :)
Attachment #8994208 - Flags: review?(jryans) → review+
Comment on attachment 8994205 [details]
Bug 1472212 - Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type.

https://reviewboard.mozilla.org/r/258802/#review265862

r=me with the rename and the long-ish nit for E10SUtils (strict equality + comment + no "aFoo" for local variables).

::: toolkit/modules/E10SUtils.jsm:91
(Diff revision 2)
>    FILE_REMOTE_TYPE,
>    EXTENSION_REMOTE_TYPE,
>    PRIVILEGED_REMOTE_TYPE,
>    LARGE_ALLOCATION_REMOTE_TYPE,
>  
> -  canLoadURIInProcess(aURL, aProcess) {
> +  canLoadURIInProcess(aURL, aRemoteType) {

Can we rename this to `canLoadURIInRemoteType` to avoid confusion?

::: toolkit/modules/E10SUtils.jsm:92
(Diff revision 2)
> -    let remoteType = aProcess == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT
> -                     ? DEFAULT_REMOTE_TYPE : NOT_REMOTE;
> -    return remoteType == this.getRemoteTypeForURI(aURL, true, remoteType);
> +    let aPreferredRemoteType = aRemoteType == NOT_REMOTE
> +      ? NOT_REMOTE
> +      : DEFAULT_REMOTE_TYPE;

This is an insidiously simple-looking, but actually somewhat confusing/dangerous change.

`NOT_REMOTE` is null. So this really says:

```js
let aPreferredRemoteType = aRemoteType == null ? foo : bar;
```

This is one of the rare cases where you should use strict equality to compare the two - with a comment explaining why. Although people are unlikely to call this method without the second parameter, you could easily end up with code where we pass "foo.remoteType" as the second argument, and for some reason "foo.remoteType" is not defined on that object, so it'd be undefined, which means we'd load it in the parent process, unsandboxed!

Really, ideally we shouldn't have used `null` as the value for `NOT_REMOTE` but that's fodder for a separate follow-up bug if we want to change it.

Nit: don't use the 'a' "argument" prefix for local variables. :-)
Attachment #8994205 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review265870

We already have some other helpers in BTU that could be used here that do wait for the remoteness change event where necessary. Can we rely more heavily on those instead of hand-rolling our own here, or perhaps update the helpers in BTU so that we *can* rely on those, and so new tests won't have to find out about this type of thing?

::: browser/base/content/test/about/browser_aboutCertError.js:93
(Diff revision 2)
>        returnButton.click();
> -
> -      await ContentTaskUtils.waitForEvent(this, "pageshow", true);
>      });
>  
> +    await promise;

Can't we just use BTU.waitForLocationChange() passing the browser and the URL ?
Attachment #8994206 - Flags: review?(gijskruitbosch+bugs)
Attachment #8994210 - Flags: review?(gijskruitbosch+bugs) → review?(mdeboer)
Comment on attachment 8994211 [details]
Bug 1472212 - Add e10s tests to ensure that URIs with the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag load in the privileged content process when the pref is turned on.

https://reviewboard.mozilla.org/r/258818/#review265872

::: browser/base/content/test/general/browser_e10s_about_process.js:31
(Diff revision 1)
>  
>  const TEST_MODULES = [
>    CHROME,
>    CANREMOTE,
> -  MUSTREMOTE
> +  MUSTREMOTE,
> +  CANPRIVILEGEDREMOTE

I have to admit I'm confused about this flag being called CAN_LOAD rather than MUST_LOAD. Can we still also load about:home in the non-privileged child when the pref is turned on? If not, why isn't the flag called MUST_USE_PRIVILEGED_CHILD_IF_AVAILABLE or something? Perhaps this is something we can change in a follow-up if you agree...

::: browser/base/content/test/general/browser_e10s_about_process.js:95
(Diff revision 1)
> -function test_url(url, chromeResult, webContentResult) {
> +function test_url(url, chromeResult, webContentResult, privilegedContentResult) {
>    is(E10SUtils.canLoadURIInProcess(url, CHROME_PROCESS),
>       chromeResult, "Check URL in chrome process.");
>    is(E10SUtils.canLoadURIInProcess(url, WEB_CONTENT_PROCESS),
>       webContentResult, "Check URL in web content process.");
> +  console.log("url", url);

Nit: please remove the debug output.

::: browser/base/content/test/general/browser_e10s_about_process.js:140
(Diff revision 1)
> +  // This shouldn't be taken literally. We will always use the privileged
> +  // content type if the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag is enabled and
> +  // the pref is turned on.

... then you won't need these comments! :-)
Attachment #8994211 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1477412
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266082

I'm deferring this to Dão, if you don't mind. It might also be an idea to have mconley look at this, because he did quite a bit of the process switching handling here.
Attachment #8994210 - Flags: review?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #46)
> Comment on attachment 8994210 [details]
> Bug 1472212 - Focus the tab's content area before changing remoteness of tab
> so that we could rely on the TabRemotenessChange event to focus on the
> gURLBar, if necessary.
> 
> https://reviewboard.mozilla.org/r/258812/#review266082
> 
> I'm deferring this to Dão, if you don't mind. It might also be an idea to
> have mconley look at this, because he did quite a bit of the process
> switching handling here.

mconley is on PTO, so it got delegated to me, and because this was sessionstore I delegated to you... :-)

I think it'd be good to have more than one set of eyes on this, so I'll take another look...
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266092

How do we make the determination that we want to focus the URL bar? Can't we just tell sessionstore about that instead? It feels fragile to make focus behavior depend on the ordering of events that ought no to be related to focus in and of themselves...

::: browser/components/sessionstore/SessionStore.jsm:4056
(Diff revision 2)
> +
> +      // Focus the tab's content area when we attempt to restore previous session.
> +      if (aTab.selected) {
> +        browser.focus();
> +      }

This avoids the focusing of the browser if we pass a remoteType to restoreTabContent. That seems different from not wanting to focus it only for the new tab case, or wanting to do it earlier so that we guarantee focus has changed once the event has fired.

::: browser/components/uitour/UITour.jsm:1371
(Diff revision 2)
>        aWindow.BrowserPageActions.panelNode.hidePopup();
>      }
>    },
>  
>    showNewTab(aWindow, aBrowser) {
> +    let win = aBrowser.ownerGlobal;

Isn't this just `aWindow`?
Attachment #8994210 - Flags: review-
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

Cancelling review for now since Gijs already raised some issues / questions.
Attachment #8994210 - Flags: review?(dao+bmo)
Comment on attachment 8994203 [details]
Bug 1472212 - Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window.

https://reviewboard.mozilla.org/r/258798/#review266104

::: commit-message-7dd8b:5
(Diff revision 1)
> +Bug 1472212 - Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window.
> +
> +Now that we have moved some about: pages to the privileged content process,
> +opening these URLs in a new window will trigger SessionStore to restore the tab state.
> +We will disable the busy attribute for these new tabs and set favicons for them

Hm. That's true. It will also be disabled whenever we attempt to restore a session or duplicate a tab. I think what I'm trying to convey here is that whenever a tab creation (e.g. opening a tab in a new window, duplicating tabs, and restoring sessions) triggers a process flip, we will disable the busy attribute if these URIs are local about: URIs (about:blank and about: URIs that resolve to jar:// or file://). Will modify the commit message accordingly.

::: browser/base/content/tabbrowser.js:11
(Diff revision 1)
>  /* eslint-env mozilla/browser-window */
>  
>  /**
>   * A set of known icons to use for internal pages. These are hardcoded so we can
>   * start loading them faster than ContentLinkHandler would normally find them.
> + * This set of icons is also defined in SessionStore.jsm.

That delegation of logic to tabbrowser sounds like a reasonable approach to me.

::: browser/components/sessionstore/SessionStore.jsm:2453
(Diff revision 1)
> +    let uri = newTab.linkedBrowser.currentURI;
> +    if (!uri || (uri && !aWindow.gBrowser._isLocalAboutURI(uri))) {

We cannot do it in `restoreTab` because that happens later. We want to show the busy attribute as soon as possible. Since both `duplicateTab` and `navigateAndRestore` happens before `restoreTab`, doing it in `restoreTab` will result in a flicker, which is detected by the tests in `browser/base/content/test/performance/browser_windowopen.js`.

::: browser/components/sessionstore/SessionStore.jsm:2455
(Diff revision 1)
>  
>      // Start the throbber to pretend we're doing something while actually
>      // waiting for data from the frame script.
> +    let uri = newTab.linkedBrowser.currentURI;
> +    if (!uri || (uri && !aWindow.gBrowser._isLocalAboutURI(uri))) {
> -    newTab.setAttribute("busy", "true");
> +      newTab.setAttribute("busy", "true");

Sorry, that was a mistake. I will modify the summary accordingly. To be precise, this patch *prevents the spinner* in all cases whenever a page with a local about URI (about:blank and about: pages that resolves to jar:// or file://) is loaded from a process that the URI cannot load in (e.g. loading about:newtab in the web content process). This triggers a process flip, which triggers the logic in `SessionStore`.

::: browser/components/sessionstore/SessionStore.jsm:3048
(Diff revision 1)
> +      uri = Services.uriFixup.createFixupURI(loadArguments.uri,
> +                                             Ci.nsIURIFixup.FIXUP_FLAG_NONE);

We need a URI object because the function definition for `_isLocalAboutURI` requires a URI object.

The goal is to create a URI object from `loadArguments.uri`. I did not know about `Services.io.newURI()` earlier. I think that is what we want here.
Comment on attachment 8994205 [details]
Bug 1472212 - Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type.

https://reviewboard.mozilla.org/r/258802/#review266422

::: toolkit/modules/E10SUtils.jsm:92
(Diff revision 2)
> -    let remoteType = aProcess == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT
> -                     ? DEFAULT_REMOTE_TYPE : NOT_REMOTE;
> -    return remoteType == this.getRemoteTypeForURI(aURL, true, remoteType);
> +    let aPreferredRemoteType = aRemoteType == NOT_REMOTE
> +      ? NOT_REMOTE
> +      : DEFAULT_REMOTE_TYPE;

Good catch. I have also defined a default value for `aRemoteType` just in case if the function is called without the second parameter, though it is very unlikely to occur.
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266458

::: browser/components/sessionstore/SessionStore.jsm:4056
(Diff revision 2)
> +
> +      // Focus the tab's content area when we attempt to restore previous session.
> +      if (aTab.selected) {
> +        browser.focus();
> +      }

Right. I was focusing too much on the tests earlier. I have a different approach now, and I think this is much more reasonable. Will push the changes in a bit.
Comment on attachment 8991643 [details]
Bug 1472212 - Expose the pref variable for browser.tabs.remote.separatePrivilegedContentProcess in AboutNewTabService over XPCOM for tests.

https://reviewboard.mozilla.org/r/256566/#review265860

The next commit was relying on the variable from XPCOM earlier, but now that I have removed it, we don't need this commit anymore.
Attachment #8991643 - Attachment is obsolete: true
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
Comment on attachment 8994211 [details]
Bug 1472212 - Add e10s tests to ensure that URIs with the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag load in the privileged content process when the pref is turned on.

https://reviewboard.mozilla.org/r/258818/#review266174

::: browser/base/content/test/general/browser_e10s_about_process.js:31
(Diff revision 1)
>  
>  const TEST_MODULES = [
>    CHROME,
>    CANREMOTE,
> -  MUSTREMOTE
> +  MUSTREMOTE,
> +  CANPRIVILEGEDREMOTE

When the pref is turned on, URIs (such as `about:home`) with the flag *must* load in the privileged process. I agree that the naming is a little bit confusing here, and we should rename the flag in a follow-up.
Blocks: 1478482
(In reply to :Gijs (he/him) from comment #44)
> Comment on attachment 8994206 [details]
> Bug 1472212 - Make tests wait for the `TabRemotenessChange` event before
> navigating to about:home or about:newtab.
> 
> https://reviewboard.mozilla.org/r/258804/#review265870
> 
> We already have some other helpers in BTU that could be used here that do
> wait for the remoteness change event where necessary. Can we rely more
> heavily on those instead of hand-rolling our own here, or perhaps update the
> helpers in BTU so that we *can* rely on those, and so new tests won't have
> to find out about this type of thing?
>

Updated the patches accordingly. What do you think of the new approach?
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review266516


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/about/head.js:145
(Diff revision 4)
> -    let mm = aBrowser.messageManager;
> +      let mm = aBrowser.messageManager;
> -    mm.loadFrameScript("data:,(" + content_script.toString() + ")(" + aStopFromProgressListener + ");", true);
> +      mm.loadFrameScript("data:,(" + content_script.toString() + ")(" + aStopFromProgressListener + ");", true);
> -    mm.addMessageListener("Test:WaitForDocLoadAndStopIt", complete);
> +      mm.addMessageListener("Test:WaitForDocLoadAndStopIt", complete);
> -    info("waitForDocLoadAndStopIt: Waiting for URL: " + aExpectedURL);
> +      info("waitForDocLoadAndStopIt: Waiting for URL: " + aExpectedURL);
> -  });
> +    });
> -}
> +  }

Error: Missing semicolon. [eslint: semi]
(In reply to :Gijs (he/him) from comment #48)
> Comment on attachment 8994210 [details]
> Bug 1472212 - Focus the tab's content area before changing remoteness of tab
> so that we could rely on the TabRemotenessChange event to focus on the
> gURLBar, if necessary.
> 
> https://reviewboard.mozilla.org/r/258812/#review266092
> 
> How do we make the determination that we want to focus the URL bar? Can't we
> just tell sessionstore about that instead? It feels fragile to make focus
> behavior depend on the ordering of events that ought no to be related to
> focus in and of themselves...
> 

I am not exactly sure when should we actually focus the URL bar.

I agree that the current approach is a little fragile, and it seems like race conditions can occur easily. The URL bar focus was added originally in Bug 342432 and Bug 1410591 to patch a small bug/regression.

At the moment, I modified it so that the tests (browser/base/content/test/performance/browser_windowopen.js and browser/components/uitour/test/browser_UITour_showNewTab.js) could pass and to prevent the URL bar from flickering.
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266620

::: browser/components/uitour/UITour.jsm:1384
(Diff revision 5)
> +      }, {once: true});
> +    }
> +
>      aWindow.openLinkIn("about:newtab", "current", {targetBrowser: aBrowser});
> +    if (!willFocus) {
> -    aWindow.gURLBar.focus();
> +      aWindow.gURLBar.focus();

openLinkIn won't focus the content area when loading about:newtab while focus is in the address bar: https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/browser/base/content/utilityOverlay.js#518

So it seems to me that:

- this code should focus the address bar before calling openLinkIn, or maybe openLinkIn should actively focus the address bar when loading about:newtab.

- SessionStore shouldn't unconditionally focus the content area. It should probably do checks similar to these in openLinkIn.
Comment on attachment 8994203 [details]
Bug 1472212 - Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window.

https://reviewboard.mozilla.org/r/258798/#review266640

r=me with the below issues addressed.

::: commit-message-7dd8b:8
(Diff revision 2)
> +This patch also prevents the spinner whenever a page with a local about: URI
> +(about:blank and about: pages that resolve to jar:// or file:// URIs) is
> +loaded from a process that the URI cannot load in (e.g. loading about:newtab
> +in the web content process).

While this says it will only affect cases where we load URIs in the wrong process, the actual change will also affect other cases like duplicating tabs, or restoring sessions that have such URIs in them.

I think that's OK, but the commit message should probably be updated.

::: browser/components/sessionstore/SessionStore.jsm:2442
(Diff revision 2)
>        aWindow.gBrowser.addTab(null, {userContextId});
>  
>      // Start the throbber to pretend we're doing something while actually
> -    // waiting for data from the frame script.
> +    // waiting for data from the frame script. This throbber is disabled
> +    // if the URI is a local about: URI.
> +    let uriObj = newTab.linkedBrowser.currentURI;

We're passing `null` to the `addTab` call. Under what circumstances is `currentURI` here not a local about: URI? (I would expect it to be either about:blank or the new tab URL, so about:newtab - even if we're about to put other content into this tab.)

Did you mean to check the URI of the original tab (that we're duplicating) instead?

::: browser/components/sessionstore/SessionStore.jsm:2443
(Diff revision 2)
>  
>      // Start the throbber to pretend we're doing something while actually
> -    // waiting for data from the frame script.
> +    // waiting for data from the frame script. This throbber is disabled
> +    // if the URI is a local about: URI.
> +    let uriObj = newTab.linkedBrowser.currentURI;
> +    if (!uriObj || (uriObj && !aWindow.gBrowser._isLocalAboutURI(uriObj))) {

It's probably not right to rely on a private (prefixed with `_`) helper here. Can you make it public?

::: browser/components/sessionstore/SessionStore.jsm:2794
(Diff revision 2)
> +      // We know that about:blank is safe to load in any remote type. Since
> +      // SessionStore is triggered with about:blank, there must be a process
> +      // flip. We will ignore the first about:blank load to prevent resetting the
> +      // favicon that we have set earlier to avoid flickering and improve
> +      // perceived performance.
> +      if (!activePageData || (activePageData && activePageData.url != "about:blank")) {

I'll note that I don't think this is actually correct, but I also don't think it matters because sessionstore probably doesn't treat about:blank correctly anyway. That is, if a website opens about:blank we definitely can't load it in just any old process - but I'm pretty sure sessionstore doesn't restore window opener relationships, and so once we load an arbitrary about:blank page as just the about:blank URL (possibly with a correct triggering principal), there's still nobody that can talk to it and there's no content in it, so in practice hopefully it doesn't matter right now.

Fixing this properly would require saving more state for such pages (essentially, a serialized DOM + scripts + styles that were inserted into the about:blank copy).
Attachment #8994203 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266644

I trust that Dão's comments here make sense so I won't look into it more right now...
Attachment #8994210 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review266648

::: browser/base/content/test/about/browser_aboutHome_search_composing.js:73
(Diff revision 5)
>      let loadPromise = waitForDocLoadAndStopIt(expectedURL);
> +
>      await BrowserTestUtils.synthesizeMouseAtCenter("#TEMPID", {
>        button: 0
>      }, browser);
> -    await loadPromise;
>  
>      await ContentTask.spawn(browser, null, async function() {
>        let input = content.document.querySelector(["#searchText", "#newtab-search-text"]);
>        ok(input.value == "x", "Input value did not change");
>  
>        let row = content.document.getElementById("TEMPID");
>        if (row) {
>          row.removeAttribute("id");
>        }
>      });
>  
> +    await loadPromise;

This change doesn't really make sense to me. Why is it needed? It looks like we activate a suggestion, which will try to load a URL, which we stop - so we'll still be on the original page. Then we run some more checks on the suggestion list.

Why does it matter at what point we await the promise? Can we revert this now that you've made the change in head.js ?

::: browser/modules/test/browser/browser_UsageTelemetry_content_aboutHome.js:55
(Diff revision 5)
> -
>    info("Load about:home.");
> -  tab.linkedBrowser.loadURI("about:home");
> +  await BrowserTestUtils.loadURI(tab.linkedBrowser, "about:home");
> +
>    info("Wait for ActivityStream search input.");
> -  await promiseAboutHomeSearchLoaded;
> +  await BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "ContentSearchClient", true, null, true).then(() => false);

The `.then(() => false)` bit is unused, because the test `await`s the promise but doesn't use the resolution value, so you can remove that.
Attachment #8994206 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266842
Attachment #8994210 - Flags: review?(dao+bmo)
Comment on attachment 8991644 [details]
Bug 1472212 - Update URLs to include the noscripts version in xpcshell and browser tests for newtab.

https://reviewboard.mozilla.org/r/256568/#review266898

Both test files seem like they'll want slightly different approaches. Also, bug 1474414 is in the process of landing with files renamed as well as activity-stream-specific linting fixes in https://github.com/mozilla/activity-stream/pull/4265 (where in particular, the helper functions are moved to the top before they're called), so there will likely be some conflicts.

::: browser/components/newtab/tests/browser/browser_packaged_as_locales.js:7
(Diff revision 4)
>  XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
>                                     "@mozilla.org/browser/aboutnewtab-service;1",
>                                     "nsIAboutNewTabService");
>  
>  // Tests are by default run with non-debug en-US configuration
> -const DEFAULT_URL = "resource://activity-stream/prerendered/en-US/activity-stream-prerendered.html";
> +const IS_NIGHTLY_BUILD = AppConstants.NIGHTLY_BUILD;

Why not check the pref browser.tabs.remote.separatePrivilegedContentProcess to determine which url to test against? Otherwise, if the condition for the pref default changes, it'll be easy to overlook updating this one as well.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:109
(Diff revision 4)
>  
>    cleanup();
> -});
> +}
> +
> +add_task(test_override_activity_stream_enabled.bind(this, false));
> +add_task(test_override_activity_stream_enabled.bind(this, true));

Given that you do this somewhat confusing bind false/true in 4 places and that this code should be relatively short lived, we can avoid reaching into each of the test functions by having a wrapper that changes the pref before calling the test. Also, if we make ACTIVITY_STREAM_PRERENDER_URL not a const, we can assign the appropriate expected url from the wrapper and leave all the original test logic unchanged.

E.g.,

function add_test(test) {
  add_task(async() => {
    await setPrivileged(false);
    ACTIVITY_STREAM_PRERENDER_URL = WITHSCRIPTS…;
    await test();
  });
  add_task(async() => {
    await setPrivileged(true);
    …
  }
}

add_test(async function test_override_activity_stream_enabled() {
Attachment #8991644 - Flags: review?(edilee)
No longer blocks: 1470324
Attachment #8991644 - Flags: review?(edilee)
Attachment #8994210 - Flags: review?(dao+bmo)
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review266814

::: browser/components/uitour/UITour.jsm:1384
(Diff revision 5)
> +      }, {once: true});
> +    }
> +
>      aWindow.openLinkIn("about:newtab", "current", {targetBrowser: aBrowser});
> +    if (!willFocus) {
> -    aWindow.gURLBar.focus();
> +      aWindow.gURLBar.focus();

Hm. I removed the focusing of the content area in SessionStore and delegated that work to `updateBrowserRemoteness` in `tabbrowser.js`. I made it so that the tab's content area is focused, when asked, whenever we update the remoteness of a tab.

`SessionStore` will only focus on the content (through updateBrowserRemoteness) if the tab is selected and the URL to load is not a special URL for new tabs (about:blank, about:home, about:welcome, and the new tab page url).

What do you think of this approach?
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review268942

::: browser/base/content/test/about/browser_aboutHome_search_composing.js:73
(Diff revision 5)
>      let loadPromise = waitForDocLoadAndStopIt(expectedURL);
> +
>      await BrowserTestUtils.synthesizeMouseAtCenter("#TEMPID", {
>        button: 0
>      }, browser);
> -    await loadPromise;
>  
>      await ContentTask.spawn(browser, null, async function() {
>        let input = content.document.querySelector(["#searchText", "#newtab-search-text"]);
>        ok(input.value == "x", "Input value did not change");
>  
>        let row = content.document.getElementById("TEMPID");
>        if (row) {
>          row.removeAttribute("id");
>        }
>      });
>  
> +    await loadPromise;

<p>Yes, we can revert this. See bug 1478792. However, we will still need those changes in head.js for <code>browser_aboutHome_search_telemetry.js</code>. (That bug removes the unnecessary checks which were causing tests to fail in <code>browser_aboutHome_search_composing.js</code>.)</p>
Comment on attachment 8991644 [details]
Bug 1472212 - Update URLs to include the noscripts version in xpcshell and browser tests for newtab.

https://reviewboard.mozilla.org/r/256568/#review268938

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:109
(Diff revision 4)
>  
>    cleanup();
> -});
> +}
> +
> +add_task(test_override_activity_stream_enabled.bind(this, false));
> +add_task(test_override_activity_stream_enabled.bind(this, true));

Thank you! That makes sense. Updated the code accordingly. :)
Comment on attachment 8991644 [details]
Bug 1472212 - Update URLs to include the noscripts version in xpcshell and browser tests for newtab.

https://reviewboard.mozilla.org/r/256568/#review269032

::: browser/components/newtab/test/xpcshell/test_AboutNewTabService.js:51
(Diff revision 5)
> +    ACTIVITY_STREAM_DEBUG_URL = "resource://activity-stream/prerendered/static/activity-stream-debug.html";
> +  }
> +}
> +
> +// Default URL values to false.
> +update_urls(false);

nit: this "api" doesn't really make much sense on the surface, "update urls to be false" ? Could just have two helper functions setExpectedUrlsWithScripts setExpectedUrlsWithoutScripts

Also, looks like the naming convention is camelCase for these helper functions.. although I do see add_tests_with_pref is just matching the externally defined add_task.
Attachment #8991644 - Flags: review+
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review269060

::: browser/base/content/tabbrowser.js:1536
(Diff revision 7)
>    updateBrowserRemoteness(aBrowser, aShouldBeRemote, {
>      newFrameloader,
>      opener,
>      remoteType,
>      sameProcessAsFrameLoader,
> +    focusContent,

Please maintain alphabetical order

::: browser/base/content/tabbrowser.js:1567
(Diff revision 7)
>      // Abort if we're not going to change anything
>      let oldRemoteType = aBrowser.getAttribute("remoteType");
>      if (isRemote == aShouldBeRemote && !newFrameloader &&
>          (!isRemote || oldRemoteType == remoteType)) {
> +      if (focusContent) {
> +        aBrowser.focus();

Why do we need to focus anything "if we're not going to change anything"?

::: browser/base/content/tabbrowser.js:1678
(Diff revision 7)
>        // Register the new outerWindowID.
>        this._outerWindowIDBrowserMap.set(aBrowser.outerWindowID, aBrowser);
>      }
>  
> -    if (wasActive)
> +    if (wasActive || focusContent) {
>        aBrowser.focus();

Should this also check that the browser is selected?
Attachment #8994210 - Flags: review?(dao+bmo)
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review269094

::: browser/base/content/tabbrowser.js:1567
(Diff revision 7)
>      // Abort if we're not going to change anything
>      let oldRemoteType = aBrowser.getAttribute("remoteType");
>      if (isRemote == aShouldBeRemote && !newFrameloader &&
>          (!isRemote || oldRemoteType == remoteType)) {
> +      if (focusContent) {
> +        aBrowser.focus();

Previously in `SessionStore.restoreTabContent`, we focus regardless of whether `updateBrowserRemoteness` returns true or false (See https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/browser/components/sessionstore/SessionStore.jsm#4000-4012).

I added the check here so that the existing behavior is maintained.

::: browser/base/content/tabbrowser.js:1678
(Diff revision 7)
>        // Register the new outerWindowID.
>        this._outerWindowIDBrowserMap.set(aBrowser.outerWindowID, aBrowser);
>      }
>  
> -    if (wasActive)
> +    if (wasActive || focusContent) {
>        aBrowser.focus();

Hm. I guess we could, but I am unsure if this will result in any side effects. The original intention was to move the responsibility of focusing the tab's content area from `SessionStore` to `tabbrowser` while maintaining its original behavior. 

I made changes and checked to ensure that the browser is selected before focusing on the tab's content. Just pushed to try to see if there are test failures.
Hey :dao, I pushed to try, and I have updated the patches with some changes to `tabbrowser.js`. 

All of the tests (except TV1) seems to be good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=380da234dfa40307356d8250fea309c251e3099a. TV1 is relevant to changes in the commit: `Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab`.
(In reply to Jay Lim [:imjching] from comment #108)
> ::: browser/base/content/tabbrowser.js:1567
> (Diff revision 7)
> >      // Abort if we're not going to change anything
> >      let oldRemoteType = aBrowser.getAttribute("remoteType");
> >      if (isRemote == aShouldBeRemote && !newFrameloader &&
> >          (!isRemote || oldRemoteType == remoteType)) {
> > +      if (focusContent) {
> > +        aBrowser.focus();
> 
> Previously in `SessionStore.restoreTabContent`, we focus regardless of
> whether `updateBrowserRemoteness` returns true or false (See
> https://searchfox.org/mozilla-central/rev/
> 0f4d50ff5211e8dd85e119ef683d04b63062ed90/browser/components/sessionstore/
> SessionStore.jsm#4000-4012).
> 
> I added the check here so that the existing behavior is maintained.

updateBrowserRemoteness should first and foremost update the browser's remoteness and possibly maintain focus as part of that. It shouldn't change focus while leaving the remoteness alone. Setting it up this way really doesn't make sense.
Attachment #8994210 - Flags: review?(dao+bmo) → review-
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review269248

Thanks, Jay! Looks good - though I think I found two places where we might be racing a bit. Perhaps this explains the TV failures?

::: browser/base/content/test/about/head.js:132
(Diff revision 7)
>          wp.removeProgressListener(progressListener);
>        } catch (e) { /* Will most likely fail. */ }
>      });
>    }
>  
> +  let promise = () => {

Can you add a comment here that we're deferring setting up this promise because of the potential for a process flip?

::: browser/base/content/test/general/browser_bug724239.js:16
(Diff revision 7)
>      // Can't load it directly because that'll use a preloaded tab if present.
>      BrowserTestUtils.loadURI(browser, "about:newtab");
>      await BrowserTestUtils.browserLoaded(browser);

Shouldn't this also wait for a process flip?

::: browser/modules/test/browser/browser_UsageTelemetry_content_aboutHome.js:54
(Diff revision 7)
>    info("Wait for ActivityStream search input.");
> -  await promiseAboutHomeSearchLoaded;
> +  await BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "ContentSearchClient", true, null, true);

I think there's a potential for a race here.

about:home might load and fire its ContentSearchClient event before the waitForContentEvent function is able to attach its event listener.

What I recommend is to use linkedBrowser.loadURI to load about:home, but then wait for the TabRemotenessChange event to fire on the tab, and then use waitForContentEvent with ContentSearchClient.

You can also wait for SSTabRestored on the tab - same difference, essentially.
Attachment #8994206 - Flags: review?(mconley) → review-
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review269250

Clearing review, since you got one from dao.
Attachment #8994210 - Flags: review?(mconley)
Depends on: 1482422
:mconley, the TV1 tests seem to pass now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=296d62f6eda40f718a6838b03d012126f8f0471f. Thank you!

:dao, originally there was a regression because the address bar steals focus (https://bugzilla.mozilla.org/show_bug.cgi?id=1410591). What are your thoughts on skipping this for special URLs for new tabs (about:blank, about:welcome, about:home, and about:newtab/overridden newtab URLs)? This means that the address bar will be focused whenever any of those pages are restored. Doing this will prevent the flickering of the URL bar whenever we open a new tab. (I guess it also helps in user experience? Usually when we are on Activity Stream, we will want to navigate to a different page, which explains the default behavior of focusing the URL bar whenever we open a new tab.)

Also, push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51cd68d1677ac1465aa96361d72df0afa75398a0
Flags: needinfo?(dao+bmo)
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review269358

Looks good, thanks!

::: browser/base/content/test/general/browser_bug724239.js:20
(Diff revision 8)
>                                      async function(browser) {
> +    let tab = gBrowser.getTabForBrowser(browser);
> +
>      // Can't load it directly because that'll use a preloaded tab if present.
>      BrowserTestUtils.loadURI(browser, "about:newtab");
> -    await BrowserTestUtils.browserLoaded(browser);
> +    await BrowserTestUtils.waitForEvent(tab, "TabRemotenessChange");

For consistency, maybe let's have this be the SSTabRestored event too, like on like 28.
Attachment #8994206 - Flags: review?(mconley) → review+
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review269360

Clearing review request until we hear back from dao on comment 120.
Attachment #8994210 - Flags: review?(mconley)
Comment on attachment 8994206 [details]
Bug 1472212 - Make some tests wait longer before navigating to about:home or about:newtab.

https://reviewboard.mozilla.org/r/258804/#review269366

::: browser/base/content/test/general/browser_bug724239.js:20
(Diff revision 8)
>                                      async function(browser) {
> +    let tab = gBrowser.getTabForBrowser(browser);
> +
>      // Can't load it directly because that'll use a preloaded tab if present.
>      BrowserTestUtils.loadURI(browser, "about:newtab");
> -    await BrowserTestUtils.browserLoaded(browser);
> +    await BrowserTestUtils.waitForEvent(tab, "TabRemotenessChange");

The `SSTabRestored` event does not seem to be fired, and hence we are using the `TabRemotenessChange` event. My guess is that the `SSTabRestored` event was fired earlier since we are using a preloaded tab (and we only have 1 privileged content process).
Comment on attachment 8994210 [details]
Bug 1472212 - Prevent focusing on the tab's content area during session restoration for special URLs for new tabs.

https://reviewboard.mozilla.org/r/258812/#review269416
Attachment #8994210 - Flags: review?(dao+bmo) → review+
Flags: needinfo?(dao+bmo)
Pushed to try earlier (before rebasing): https://treeherder.mozilla.org/#/jobs?repo=try&revision=629abeac2783082414771dc0b35d9ff36e8b9237

Just pushed another one (after rebasing): https://treeherder.mozilla.org/#/jobs?repo=try&revision=46ed8cd1d6ce3b690c4edf5c6f24e73678b1a01e

If everything is green, we can merge. Also, we will need to backport the changes in https://reviewboard.mozilla.org/r/256568/diff/6#index_header to Activity Stream's GitHub repo (Something similar to this: https://github.com/mozilla/activity-stream/pull/4335).
Depends on: 1277060
Hey kmag, we talked a little bit about this on IRC. Would like to follow up on that.

It seems like there are still some intermittent test failures which are caused by leaks on about:newtab. After digging into the root cause, disabling the reading of script through ScriptPreloader solves the leak issues (i.e. disabling this part: https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/js/xpconnect/loader/mozJSSubScriptLoader.cpp#715-732).

Whenever we open the first new tab which forces the preloaded new tab to load, mozJSSubScriptLoader will inject scripts into the page and the leaks _might_ occur whenever the test completes. (https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/accessible/tests/browser/general/browser_test_doc_creation.js#33) I suspect that this is caused by the ScriptPreloader when it is used during the first session in which the script is not in the cache yet.

Here's a recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36dec2e0589f861152554f6febd605e802cf9f6d&selectedJob=194719988. See bc7 on OS X 10.10 debug.

Do you have any other insights to why disabling the ScriptPreloader solves the leak issue?
Flags: needinfo?(kmaglione+bmo)
Hey imjching, out of curiosity, does the leak go away if we make that single test wait for a few seconds before closing those tabs?
Flags: needinfo?(jay)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #127)
> Hey imjching, out of curiosity, does the leak go away if we make that single
> test wait for a few seconds before closing those tabs?

Looks like it does. I made a few try pushes yesterday (see bc3 for both). We're looking to see if `browser_test_doc_creation.js` fails:

Without waiting: https://treeherder.mozilla.org/#/jobs?repo=try&author=jay@imjching.com&selectedJob=197545231
With waiting: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3d253ec398420229bc7c436786145416972b87c

After rebasing, it seems like other tests are failing again (https://treeherder.mozilla.org/#/jobs?repo=try&revision=f87f310f8e6d4f7d5d1ccf463dd8f044dfc694ff). Will look into those later today.
Flags: needinfo?(jay)
(In reply to Jay Lim [:imjching] from comment #128)
> Looks like it does.

Well that's interesting - so the ScriptPreloader is probably doing something in the background that hasn't completed by the time the test harness shuts down. I assume this leak is in the content process, right?

Assuming so, what are we waiting for the content processes' ScriptPreloader to do?

For reference, I added some code in bug 1458375 to make the mochitest-browser test harness wait until browser-idle-startup-tasks-finished before running the tests... this is the same notification that tells the ScriptPreloader in the parent to write the cache to disk. I wonder if we need to do something similar for the first content process.
(In reply to Mike Conley (:mconley) (:⚙️) from comment #129)
> Well that's interesting - so the ScriptPreloader is probably doing something
> in the background that hasn't completed by the time the test harness shuts
> down. I assume this leak is in the content process, right?
> 
> Assuming so, what are we waiting for the content processes' ScriptPreloader
> to do?
> 
> For reference, I added some code in bug 1458375 to make the
> mochitest-browser test harness wait until
> browser-idle-startup-tasks-finished before running the tests... this is the
> same notification that tells the ScriptPreloader in the parent to write the
> cache to disk. I wonder if we need to do something similar for the first
> content process.

Yes. It looks like the leak is due to the `ScriptPreloader` not sending script cache data back to the parent process in time for it to be written to the cache. kmag added a timer in Bug 1471089 to force the `ScriptPreloader` to finish the content startup.

I *think* that explains why waiting for a few seconds before finishing the test would prevent the leak. I made some changes to `ScriptCacheActors` so that emits an event (`privileged-sp-finished`) when the `ScriptPreloader` in the privileged content process has finished its startup: https://hg.mozilla.org/try/rev/966a619fed9d4592e481acb0080c08d65e559251. I just pushed my changes to try. If this does solve the problem, we might want to make the mochitest-browser test harness wait until this event is fired if there is a preloaded new tab in the background before finishing the test.

Also, I am in the process of fixing the new tests that are failing/timing out due to the new changes over the past few weeks.
I'm afraid we that try push still has the leak here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=694d5779084fc31edbdc998f8d8f045bbeace67a&selectedJob=197963957

Hey kmag, any pointers, ideas or tips to get Jay unstuck here?
(In reply to Mike Conley (:mconley) (:⚙️) from comment #131)
> I'm afraid we that try push still has the leak here:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=694d5779084fc31edbdc998f8d8f045bbeace67a&selectedJob=1
> 97963957
> 
> Hey kmag, any pointers, ideas or tips to get Jay unstuck here?

It looks like the windows are being held alive by CPOWs in those a11y failures. No idea why. Skipping the subscript loader part might just change the timing enough to prevent that leak most of the time for unrelated reasons.

CPOWs need to die by fire... but I'm not sure how to figure out what's keeping this one alive
Flags: needinfo?(kmaglione+bmo)
Iteration: 63.3 - Aug 6 → ---
Hey mconley, do you think we should merge this with the timing delay in it and somehow mark this as a known issue?
Flags: needinfo?(mconley)
(In reply to Jay Lim [:imjching] from comment #133)
> Hey mconley, do you think we should merge this with the timing delay in it
> and somehow mark this as a known issue?

I'm quite reluctant to do that unless there's really no other choice. kmag has narrowed down the issue to a CPOW that's being used (possibly in an earlier test) that's keeping things alive longer than expected. Your changes probably just reveal a pre-existing issue here, which is unfortunate. :/

I've got a try push cooking right now that should, hopefully, make CPOW usage more obvious, and maybe that'll illuminate the culprit.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c00faf33487bf618588e9bac43dfdf0cd4e14ec
Flags: needinfo?(mconley)
Depends on: 1492482
Depends on: 1496848
I'm taking this over.
Assignee: jay → mconley
Whiteboard: [fxperf:p1]
Depends on: 1497901
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/101903689bc0
Update URLs to include the noscripts version in xpcshell and browser tests for newtab. r=Mardak
https://hg.mozilla.org/integration/mozilla-inbound/rev/cecc2d52e72e
Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1cafd30a69a
Remove unnecessary async/await keywords for browser_new_tab_in_privileged_process_pref.js. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bba29ad2cb
Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c2a8d227dc
Handle navigation away from privileged content process in RDM. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/a28443371b94
Load `http://example.org/` instead of `about:home` for some tests. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c22abaf108
Prevent focusing on the tab's content area during session restoration for special URLs for new tabs. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff5280fc25a
Add e10s tests to ensure that URIs with the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag load in the privileged content process when the pref is turned on. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a9a2adf32a
Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds. r=mconley
Summary: Set browser.tabs.remote.separatePrivilegedContentProcess to true by default → Set browser.tabs.remote.separatePrivilegedContentProcess to true by default in Nightly
Backed out 9 changesets (bug 1472212) for browser chrome failures on browser_report_site_issue and wpt failures. 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed&revision=78a9a2adf32a8798aea8c6c5347d2d4957fc939e&selectedJob=205094589

Failure log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=205094589&repo=mozilla-inbound&lineNumber=3761
https://treeherder.mozilla.org/logviewer.html#?job_id=205107793&repo=mozilla-inbound&lineNumber=7206

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4b8f268757c4ea57a7cc490a1585e7e5816811

[task 2018-10-12T15:59:56.513Z] 15:59:56     INFO - TEST-START | browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js
[task 2018-10-12T16:01:26.523Z] 16:01:26     INFO - TEST-INFO | started process screentopng
[task 2018-10-12T16:01:27.134Z] 16:01:27     INFO - TEST-INFO | screentopng: exit 0
[task 2018-10-12T16:01:27.135Z] 16:01:27     INFO - Buffered messages logged at 15:59:56
[task 2018-10-12T16:01:27.136Z] 16:01:27     INFO - Entering test bound test_opened_page
[task 2018-10-12T16:01:27.137Z] 16:01:27     INFO - Buffered messages logged at 15:59:57
[task 2018-10-12T16:01:27.138Z] 16:01:27     INFO - Waiting for main page action button to have non-0 size
[task 2018-10-12T16:01:27.139Z] 16:01:27     INFO - Waiting for main page action panel to be open
[task 2018-10-12T16:01:27.140Z] 16:01:27     INFO - promisePageActionViewChildrenVisible waiting for a child node to be visible
[task 2018-10-12T16:01:27.142Z] 16:01:27     INFO - Console message: Utils: Failed to deserialize principal_b64 '[xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable)]' [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsISerializationHelper.deserializeObject]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/sessionstore/Utils.jsm :: deserializePrincipal :: line 137"  data: no]
[task 2018-10-12T16:01:27.144Z] 16:01:27     INFO - Buffered messages logged at 16:00:41
[task 2018-10-12T16:01:27.146Z] 16:01:27     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2018-10-12T16:01:27.147Z] 16:01:27     INFO - Buffered messages finished
[task 2018-10-12T16:01:27.148Z] 16:01:27     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js | Test timed out - 
[task 2018-10-12T16:01:27.149Z] 16:01:27     INFO - Console message: [JavaScript Error: "[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/MessageChannel.jsm :: _handleMessage/deferred.promise< :: line 1003"  data: no]"]
[task 2018-10-12T16:01:27.151Z] 16:01:27     INFO - _handleMessage/deferred.promise<@resource://gre/modules/MessageChannel.jsm:1003:9
[task 2018-10-12T16:01:27.152Z] 16:01:27     INFO - promise callback*_handleMessage@resource://gre/modules/MessageChannel.jsm:954:8
[task 2018-10-12T16:01:27.153Z] 16:01:27     INFO - receiveMessage/<@resource://gre/modules/MessageChannel.jsm:218:9
[task 2018-10-12T16:01:27.153Z] 16:01:27     INFO - receiveMessage@resource://gre/modules/MessageChannel.jsm:211:5
[task 2018-10-12T16:01:27.154Z] 16:01:27     INFO - MessageListener.receiveMessage*FilteringMessageManager@resource://gre/modules/MessageChannel.jsm:201:5
[task 2018-10-12T16:01:27.155Z] 16:01:27     INFO - get@resource://gre/modules/MessageChannel.jsm:436:14
[task 2018-10-12T16:01:27.156Z] 16:01:27     INFO - addListener@resource://gre/modules/MessageChannel.jsm:763:7
[task 2018-10-12T16:01:27.156Z] 16:01:27     INFO - ExtensionGlobal@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/extension-process-script.js:58:5
[task 2018-10-12T16:01:27.157Z] 16:01:27     INFO - init/<@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/extension-process-script.js:113:42
[task 2018-10-12T16:01:27.158Z] 16:01:27     INFO - @data:,
[task 2018-10-12T16:01:27.159Z] 16:01:27     INFO -       Components.utils.import("resource://gre/modules/Services.jsm");
[task 2018-10-12T16:01:27.159Z] 16:01:27     INFO - 
[task 2018-10-12T16:01:27.160Z] 16:01:27     INFO -       Services.obs.notifyObservers(this, "tab-content-frameloader-created", "");
[task 2018-10-12T16:01:27.161Z] 16:01:27     INFO -     :1:76
[task 2018-10-12T16:01:27.162Z] 16:01:27     INFO - 
[task 2018-10-12T16:01:27.163Z] 16:01:27     INFO - GECKO(6949) | MEMORY STAT | vsize 757MB | residentFast 274MB | heapAllocated 60MB
[task 2018-10-12T16:01:27.164Z] 16:01:27     INFO - TEST-OK | browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js | took 90139ms
[task 2018-10-12T16:01:27.165Z] 16:01:27     INFO - Not taking screenshot here: see the one that was previously logged

For the wpt failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed&revision=78a9a2adf32a8798aea8c6c5347d2d4957fc939e&searchStr=wpt&selectedJob=205122434
Flags: needinfo?(mconley)
Depends on: 1498687
I'm afraid there are more tests outside of mochitest-browser that are failing because of these patches. I'll start looking into those next, but I think this is going to slip to 65.
Flags: needinfo?(mconley)
Depends on: 1500089
Depends on: 1500792
Depends on: 1501044
No longer depends on: 1500089
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4baa63e5b1b
Update URLs to include the noscripts version in xpcshell and browser tests for newtab. r=Mardak
https://hg.mozilla.org/integration/mozilla-inbound/rev/176f3ee14e67
Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/24c257cd18c3
Remove unnecessary async/await keywords for browser_new_tab_in_privileged_process_pref.js. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6996abc7d90
Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8a06d01437e
Handle navigation away from privileged content process in RDM. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdca743668c
Load `http://example.org/` instead of `about:home` for some tests. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94f9883aef0
Prevent focusing on the tab's content area during session restoration for special URLs for new tabs. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5de66c1f60
Add e10s tests to ensure that URIs with the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag load in the privileged content process when the pref is turned on. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e46df44cd1
Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds. r=mconley
According to #taskcluster, the Linux x64 failures are unrelated - that's bug 1503062.

Do you have a log for the browser_ext_windows_create_tabId.js failure I can look at? Or a link to a failing job?
Flags: needinfo?(mconley) → needinfo?(ncsoregi)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #142)
> According to #taskcluster, the Linux x64 failures are unrelated - that's bug
> 1503062.
> 
> Do you have a log for the browser_ext_windows_create_tabId.js failure I can
> look at? Or a link to a failing job?

https://treeherder.mozilla.org/logviewer.html#?job_id=208512091&repo=mozilla-inbound&lineNumber=2971
Flags: needinfo?(ncsoregi)
(In reply to Natalia Csoregi [:nataliaCs] from comment #143)
> https://treeherder.mozilla.org/logviewer.html#?job_id=208512091&repo=mozilla-
> inbound&lineNumber=2971

Thanks. Out of curiosity, what makes you think this isn't bug 1453808 or bug 1292491? Do my patches make this a perma-failure, or just increase the frequency?
Flags: needinfo?(ncsoregi)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #144)
> (In reply to Natalia Csoregi [:nataliaCs] from comment #143)
> > https://treeherder.mozilla.org/logviewer.html#?job_id=208512091&repo=mozilla-
> > inbound&lineNumber=2971
> 
> Thanks. Out of curiosity, what makes you think this isn't bug 1453808 or bug
> 1292491? Do my patches make this a perma-failure, or just increase the
> frequency?

It increased the frequency after this push. Bug 1453808 is strictly, or should be at least, for test-verify failures, tier2. Initially, it was thought that bug 1292491 might be a good match, but based on the IRC conv, we backed out.
Flags: needinfo?(ncsoregi)
Please just disable that test on ASan.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bug1472212.patch (obsolete) — Splinter Review
Disable browser_ext_windows_create_tabId.js on Linux x64 Asan
Attachment #9020987 - Flags: review?(jmaher)
Comment on attachment 9020987 [details] [diff] [review]
bug1472212.patch

># HG changeset patch
># User Raul Gurzau <rgurzau@mozilla.com>
># Parent  e37bf786b5fcbf048cefbea44a85783a68ce4339
>Bug 1472212 - Disable browser_ext_windows_create_tabId.js on Linux x64 asan. r=jmaher
>
>diff --git a/browser/components/extensions/test/browser/browser-common.ini b/browser/components/extensions/test/browser/browser-common.ini
>--- a/browser/components/extensions/test/browser/browser-common.ini
>+++ b/browser/components/extensions/test/browser/browser-common.ini
>@@ -247,6 +247,7 @@ tags = fullscreen
> [browser_ext_windows_create_cookieStoreId.js]
> [browser_ext_windows_create_params.js]
> [browser_ext_windows_create_tabId.js]
>+skip-if = (os == "linux" && asan && bits == 64) # Bug 1472212
> [browser_ext_windows_create_url.js]
> [browser_ext_windows_events.js]
> [browser_ext_windows_remove.js]
Attachment #9020987 - Flags: review?(jmaher) → review+
Be aware that when this landed in comment 140, for a short while we noticed the AWSY perf results bellow.
The backout that followed canceled all of them.

== Change summary for alert #17209 (as of Mon, 29 Oct 2018 15:26:56 GMT) ==

Regressions:

  7%  Resident Memory windows10-64 pgo stylo       469,533,885.92 -> 504,409,177.10
  7%  Heap Unclassified windows10-64 pgo stylo     41,107,847.51 -> 44,041,966.81
  7%  Explicit Memory windows10-64 pgo stylo       315,364,653.72 -> 337,752,535.12
  7%  JS windows10-64 pgo stylo                    109,268,369.54 -> 116,867,693.90

Improvements:

 15%  Base Content Heap Unclassified linux64-qr opt stylo          5,333,626.67 -> 4,552,772.00
 14%  Base Content Explicit linux64-qr opt stylo                   13,772,117.33 -> 11,795,968.00
 11%  Base Content JS linux64-qr opt stylo                         5,171,264.00 -> 4,587,863.33
 10%  Base Content Resident Unique Memory linux64-qr opt stylo     19,656,362.67 -> 17,713,322.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=17209
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #150)
> Be aware that when this landed in comment 140, for a short while we noticed
> the AWSY perf results bellow.
> The backout that followed canceled all of them.
>

I suspect this is because AWSY measures content memory usage by looking at the process name[1], and what we've effectively done here is moved the memory that was being used in a content process into a process that AWSY lumps into the "non-content" bucket.

Does that sound plausible, erahm?

[1]: https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/testing/awsy/awsy/test_base_memory_usage.py#19
Flags: needinfo?(erahm)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #151)

> I suspect this is because AWSY measures content memory usage by looking at
> the process name[1], and what we've effectively done here is moved the
> memory that was being used in a content process into a process that AWSY
> lumps into the "non-content" bucket.

I think it's just because we have an additional process, we only name filter for the 'base' (ab) test, but this regression is in the main test (sy). For resident we sum RSS parent + USS children (no matter the name).

Here's the diff of the tabs loaded, forced GC measure:

> Privileged Content (pid NNN)
> Explicit Allocations
> 
> 28.82 MB (100.0%) -- explicit
> ├───9.10 MB (31.56%) -- js-non-window
> │   ├──4.00 MB (13.88%) ++ runtime
> │   ├──3.92 MB (13.60%) ++ zones/zone(0xNNN)
> │   ├──1.17 MB (04.07%) ++ gc-heap
> │   └──0.00 MB (00.01%) ── helper-thread/heap-other
> ├───6.77 MB (23.49%) -- heap-overhead
> │   ├──5.85 MB (20.28%) ── bin-unused [+]
> │   ├──0.54 MB (01.86%) ── bookkeeping [+]
> │   └──0.39 MB (01.36%) ── page-cache [+]
> ├───4.70 MB (16.32%) -- window-objects/top(about:newtab, id=NNN)
> │   ├──3.30 MB (11.45%) -- active/window(about:newtab)
> │   │  ├──2.19 MB (07.59%) -- js-realm(about:newtab)
> │   │  │  ├──1.19 MB (04.13%) ++ classes
> │   │  │  ├──0.45 MB (01.55%) ++ type-inference
> │   │  │  ├──0.40 MB (01.37%) ++ scripts
> │   │  │  └──0.15 MB (00.54%) ++ (3 tiny)
> │   │  ├──1.01 MB (03.50%) ++ layout
> │   │  └──0.10 MB (00.36%) ++ (3 tiny)
> │   └──1.40 MB (04.87%) ++ js-zone(0xNNN)
> ├───3.15 MB (10.93%) ── heap-unclassified
> ├───2.47 MB (08.55%) ++ threads
> ├───0.78 MB (02.70%) ++ images
> ├───0.77 MB (02.66%) ++ layout
> ├───0.70 MB (02.43%) ++ (16 tiny)
> └───0.39 MB (01.35%) ++ atoms
> ...
>          88.82 MB ── resident [+]
>         38.32 MB ── resident-unique [+]

So there's a bump of 28MB for explicit and 38MB for resident unique which is in line with the regression. If we look at the other subtests [1] it's clear the activity stream process is sticking around when we close all the tabs which leads to a regression there. 

In the past, for the preallocated process we had a step where we would explicitly terminate it [2] for the 'tabs closed' measures, it might make sense to have an option for that as well.

To sum up, this is mostly overhead from having a new process but at least that's fixed overhead. It would be nice to be able to shutdown the process when we close all that tabs so that we clear that regression from a measurement point of view. I'm okay with a regression in the 'extra processes' measure, and a regression due to an extra process for the tabs open measures is okay as long as we do some work to see if we can reduce the overhead as a follow up.

[1] https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=af5a791e7b7e7fc42646f202db59cfbf4c48cd6e&newProject=mozilla-inbound&newRevision=18e46df44cd16a2bd94e8dfd99abb461ce24fe2f&originalSignature=e20efcd8d368e9a6766c801bb2dbbbe1c19c722d&newSignature=e20efcd8d368e9a6766c801bb2dbbbe1c19c722d&framework=4
[2] https://searchfox.org/mozilla-central/source/testing/awsy/awsy/test_memory_usage.py#87-93,147-149,151-152
Flags: needinfo?(erahm)
Cool, thanks erahm.

Also, it looks like the ASAN bc16 failures were something erahm dealt with in bug 1470280. The solution there was to drop the content processes if MOZ_ASAN. I'll do the same thing for the Activity Stream content process for now, and file a follow-up bug.
Comment on attachment 9020987 [details] [diff] [review]
bug1472212.patch

Thanks for the assist on this, Raul! :) This seems like a game of whack-a-mole though. I'm just going to disable the separate Activity Stream content process for ASAN builds instead.

Try build here: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=208662285&revision=a334dfec53ea659689f5bc10095bb8cdb4cb1552
Attachment #9020987 - Attachment is obsolete: true
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48242d39d532
Update URLs to include the noscripts version in xpcshell and browser tests for newtab. r=Mardak
https://hg.mozilla.org/integration/mozilla-inbound/rev/21a6f1a83c73
Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/1978a7837502
Remove unnecessary async/await keywords for browser_new_tab_in_privileged_process_pref.js. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f824674b76
Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe4ec18f2f3
Handle navigation away from privileged content process in RDM. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7fa6ab2229
Load `http://example.org/` instead of `about:home` for some tests. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/95a7ef6102a6
Prevent focusing on the tab's content area during session restoration for special URLs for new tabs. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf02793f802d
Add e10s tests to ensure that URIs with the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag load in the privileged content process when the pref is turned on. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa35078cabaa
Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds. r=mconley
(In reply to Eric Rahm [:erahm] from comment #152)
> In the past, for the preallocated process we had a step where we would
> explicitly terminate it [2] for the 'tabs closed' measures, it might make
> sense to have an option for that as well.

We usually keep a preallocated new tab page around to speed up opening new tabs, so I don't think we'd gain much from this. It might be nice to restart the process occasionally, but I think Jay ran into problems in tests when that happened too often.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc41b1c310cb
Update URLs to include the noscripts version in xpcshell and browser tests for newtab. r=Mardak
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6c51e7793e
Ensure that tab does not show busy or burst status whenever we navigate to about:home, about:newtab, or about:welcome in a new window. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4334e2c4940a
Remove unnecessary async/await keywords for browser_new_tab_in_privileged_process_pref.js. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/8102b1887aed
Rename E10SUtils.canLoadURIInProcess to E10SUtils.canLoadURIInRemoteType and modify it to accept an E10SUtils process type instead of a nsIXULRuntime process type. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d550da9b2d04
Handle navigation away from privileged content process in RDM. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/173e2056930d
Load `http://example.org/` instead of `about:home` for some tests. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12ebb66df3d
Prevent focusing on the tab's content area during session restoration for special URLs for new tabs. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e7c1d6a26d
Add e10s tests to ensure that URIs with the URI_CAN_LOAD_IN_PRIVILEGED_CHILD flag load in the privileged content process when the pref is turned on. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d835874be99a
Set browser.tabs.remote.separatePrivilegedContentProcess to true by default for Firefox Nightly builds. r=mconley
Flags: needinfo?(mconley)
Depends on: 1503796
ni'ing myself to ensure this gets proper security review.
Flags: needinfo?(mconley)
Blocks: 1504754
I've queued the separate privileged content process for security review.
Flags: needinfo?(mconley)
Depends on: 1505185
Depends on: 1505063
I have reproduced this bug with Nightly 63.0a1 (2018-06-29) on Windows 7, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20181112220107
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0
QA Whiteboard: [bugday-20181107]
Hi! Would this bug have contributed to a sudden and lasting change in the duration of the TABCHILD_PAINT_TIME probe? We're seeing an alert about this, and from the changelog that day this seems like a possible culprit: https://groups.google.com/forum/#!topic/mozilla.dev.telemetry-alerts/Hk4ATv04MkU

Near as I can figure it's a good change, but I don't know how to read it.
Flags: needinfo?(mconley)
Depends on: 1509250
(In reply to Chris H-C :chutten from comment #165)
> Hi! Would this bug have contributed to a sudden and lasting change in the
> duration of the TABCHILD_PAINT_TIME probe? We're seeing an alert about this,
> and from the changelog that day this seems like a possible culprit:
> https://groups.google.com/forum/#!topic/mozilla.dev.telemetry-alerts/
> Hk4ATv04MkU
> 
> Near as I can figure it's a good change, but I don't know how to read it.

Sorry for the super-late response!

It's certainly plausible. Now that about:newtab's are isolated to loading within a single, special content process (separate from normal web page loads), I can see how the lack of mixture there preventing one set of pages (web content pages) slowing down about:newtab loads and vice-versa.

So, uh, maybe? Yes indeed, the change is a positive one. If we don't see it hit Beta (since the separate Activity Stream contentp rocess is only set to true on Nightly), then that helps bolster the case that this is the cause.
Flags: needinfo?(mconley)
Blocks: 1513045
No longer depends on: 1277060
No longer depends on: 1503796
No longer depends on: 1505185
No longer depends on: 1505322
No longer depends on: 1509250
No longer depends on: 1510087
Depends on: 1518322
Component: Activity Streams: Newtab → New Tab Page
Regressions: 1563562
Regressions: 1571479
See Also: → 1652069
You need to log in before you can comment on or make changes to this bug.