1469072 - Add infrastructure to move Activity Stream into its own content process

Status: RESOLVED FIXED

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

Description

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

At least about:newtab should be hosted in its own special content process, but this might be extended to other about: pages that we've hosted in content processes up until now.

Comment 1

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

Going to scope this bug down a little to just encompass Activity Stream. We can deal with other about: pages in a follow-up.

Comment 2

[Phabricator Automation]

Attachment: Bug 1469072 - Move Activity Stream into its own content process

This will move Activity Stream (about:newtab, about:home, and about:welcome) into its
own special content process. We can deal with other about:* pages in a follow-up. We
only support a single about process at the moment.

This patch also adds a default 'Uninitialized' ProcessType that is used by the
ScriptPreloader.

Comment 3

[Jay Lim [:imjching]]

In the patch, I added two new process types (Uninitialized and About) to ScriptPreloader.

   enum class ProcessType : uint8_t {
+    Uninitialized,
     Parent,
     Web,
     Extension,
+    About,
   };
  
   static ProcessType CurrentProcessType()
   {
+    MOZ_ASSERT(sProcessType != ProcessType::Uninitialized);
     return sProcessType;
   }

:kmag, does this look good to you? (Ref: https://phabricator.services.mozilla.com/D1731)

Comment 4

[Jay Lim [:imjching]]

Also added a new check in GetChildProcessType. (Sorry, I missed this one earlier)

 ProcessType
 ScriptPreloader::GetChildProcessType(const nsAString& remoteType)
 {
   if (remoteType.EqualsLiteral(EXTENSION_REMOTE_TYPE)) {
     return ProcessType::Extension;
   }
+  if (remoteType.EqualsLiteral(ABOUT_REMOTE_TYPE)) {
+    return ProcessType::About;
+  }
   return ProcessType::Web;
 }

Comment 5

[Jay Lim [:imjching]]

Attachment: Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

Summary:
This will move Activity Stream (about:newtab, about:home, and about:welcome) into its
own special content process. We can deal with other about:* pages in a follow-up. We
only support a single about process at the moment.

This patch also adds a default 'Uninitialized' ProcessType that is used by the
ScriptPreloader.

Reviewers: mconley

Tags: #secure-revision

Bug #: 1469072

Differential Revision: https://phabricator.services.mozilla.com/D1731

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

Comment 6

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

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258610

Carrying over r+ from Phabricator review.

Comment 7

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

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

Comment 8

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

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258620

Thanks for the additional tests! r=me on that stuff.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258628

This looks pretty good. I'm mostly just worried about the naming, at this point.

::: dom/ipc/ContentChild.cpp:2736
(Diff revision 2)
>    if (aRemoteType.EqualsLiteral(FILE_REMOTE_TYPE)) {
>      SetProcessName(NS_LITERAL_STRING("file:// Content"));
>    } else if (aRemoteType.EqualsLiteral(EXTENSION_REMOTE_TYPE)) {
>      SetProcessName(NS_LITERAL_STRING("WebExtensions"));
> +  } else if (aRemoteType.EqualsLiteral(ABOUT_REMOTE_TYPE)) {
> +    SetProcessName(NS_LITERAL_STRING("about:* Content"));

Hm. This is a bit deceptive. I'd rather we call it something like "Privileged Content" or "Browser Content"

::: dom/ipc/ContentParent.h:46
(Diff revision 2)
>  // ContentChild:RecvRemoteType.  Add your value there too or it will be called
>  // "Web Content".
>  #define DEFAULT_REMOTE_TYPE "web"
>  #define FILE_REMOTE_TYPE "file"
>  #define EXTENSION_REMOTE_TYPE "extension"
> +#define ABOUT_REMOTE_TYPE "about"

Same here. There are loads of about: pages that won't run in this process.

::: dom/ipc/ContentParent.cpp:685
(Diff revision 2)
>  {
> -  if (aContentProcessType.EqualsLiteral("web")) {
> +  if (aContentProcessType.EqualsLiteral(DEFAULT_REMOTE_TYPE)) {
>      return GetMaxWebProcessCount();
>    }
> +  if (aContentProcessType.EqualsLiteral(ABOUT_REMOTE_TYPE)) {
> +    return 1;

I'd rather we just set "dom.ipc.processCount.<process-name>" for this, rather than hard coding it.

It might also be a good idea to set "dom.ipc.keepProcessesAlive.<process-name>" to 1 for this process type, too, so we don't wind up thrashing content processes every time we navigate away from about:newtab...

::: js/xpconnect/loader/ScriptPreloader.h:43
(Diff revision 2)
>  namespace loader {
>      class InputBuffer;
>      class ScriptCacheChild;
>  
>      enum class ProcessType : uint8_t {
> +        Uninitialized,

Why is this necessary?

::: toolkit/modules/E10SUtils.jsm:162
(Diff revision 2)
>  
>          let flags = module.getURIFlags(aURI);
>          if (flags & Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD) {
> +          // Load Activity Stream in a separate process.
> +          if (useSeparateAboutUriProcess &&
> +              ["home", "newtab", "welcome"].includes(aURI.filePath)) {

Probably best to define this as a constant at the top of the file. May as well make it a Set(), too.

Comment 10

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

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

Comment 11

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

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

Comment 12

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258628

> Why is this necessary?

It is not necessary in this patch. I will move this to the ScriptPreloader patch (Bug 1416066). I believe it will make more sense in that patch.

Comment 13

[Laurențiu Nicola]

There's been a lot of complaints about the memory overhead of e10s. Will it be possible to avoid this extra content process for those who don't use Activity Stream and have about:blank as their new tab page?

Comment 14

[Kris Maglione [:kmag]]

(In reply to Laurentiu Nicola from comment #13)
> There's been a lot of complaints about the memory overhead of e10s. Will it
> be possible to avoid this extra content process for those who don't use
> Activity Stream and have about:blank as their new tab page?

This won't affect people who don't use Activity Stream

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258910

Looks good. Thanks.

r=me for the preloader and E10s bits. I only skimmed the test, but I'm assuming that mconley gave them a thorough review.

::: toolkit/modules/E10SUtils.jsm:163
(Diff revision 4)
> +          if (useSeparatePrivilegedContentProcess && processCount !== 1) {
> +            console.error("Out-of-process Privileged Content is not supported with multiple child processes.");
> +          }

This shouldn't be necessary. Multiple AS processes should work just fine, but are less than ideal. We have this check for the WebExtensions process because we can't currently deal with more than one.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258964

::: browser/base/content/test/tabs/browser_new_tab_in_privileged_process_pref.js:40
(Diff revision 4)
> +  let remoteType = await ContentTask.spawn(browser, null, () => {
> +    return Services.appinfo.remoteType;
> +  });

Oh, late nit: Please just use `browser.messageManager.remoteType` here. That's guaranteed to match the remote type of the content process. `browser.remoteType` is, of course, just an attribute value.

Comment 17

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

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

Comment 18

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review258910

> This shouldn't be necessary. Multiple AS processes should work just fine, but are less than ideal. We have this check for the WebExtensions process because we can't currently deal with more than one.

Hm. That's right. I will remove this for now. At the moment, multiple AS processes should work fine. However, we will need to decide if we want to limit AS to just one process or multiple processes moving forward. If we know that we can only have 1 AS process, we could potentially implement several optimizations or remove unnecessary code, which may provide us with a performance win.

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

https://reviewboard.mozilla.org/r/252108/#review259224


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


::: toolkit/modules/E10SUtils.jsm:18
(Diff revision 5)
>                                        "browser.tabs.remote.separateFileUriProcess", false);
>  XPCOMUtils.defineLazyPreferenceGetter(this, "allowLinkedWebInFileUriProcess",
>                                        "browser.tabs.remote.allowLinkedWebInFileUriProcess", false);
> +XPCOMUtils.defineLazyPreferenceGetter(this, "useSeparatePrivilegedContentProcess",
> +                                      "browser.tabs.remote.separatePrivilegedContentProcess", false);
> +XPCOMUtils.defineLazyPreferenceGetter(this, "processCount", "dom.ipc.processCount.privileged");

Error: 'processcount' is defined but never used. [eslint: no-unused-vars]

Comment 20

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

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

Comment 21

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252108/diff/6-7/

Comment 22

[Jay Lim [:imjching]]

I made some small changes to `script_cache.py` to include the Privileged process type and called `Services.ppmm.releaseCachedProcesses()` in the tests.

Pushed to TryServer: 

mochitests and e10s tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=350a4d9d44864ccc2733c792ebab7fac2c5f66fc
`browser/base/content/test/tabs/` tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8ab2653b0d1c2d4ab60d1580fae2186fa0eeb4b

Comment 23

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

Interdiff looks good to me. Try looks solid too. Go ahead and autoland. Thanks!

Comment 24

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252108/diff/7-8/

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9154e568ca2822188ef6d0479c69dfd20797021f -d e03976b0784c: rebasing 470069:9154e568ca28 "Bug 1469072 - Add infrastructure to move Activity Stream into its own content process. r=kmag,mconley" (tip)
merging browser/base/content/test/tabs/browser.ini
merging dom/ipc/ContentChild.cpp
merging dom/ipc/ContentParent.cpp
merging js/xpconnect/loader/ScriptPreloader.cpp
merging js/xpconnect/loader/ScriptPreloader.h
merging modules/libpref/init/all.js
warning: conflicts while merging browser/base/content/test/tabs/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Jay Lim [:imjching]]

Comment on attachment 8986872 [details]
Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252108/diff/8-9/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22fd5e86fbf6
Add infrastructure to move Activity Stream into its own content process. r=kmag,mconley

Comment 28

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/22fd5e86fbf6