Add infrastructure to move Activity Stream into its own content process

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: mconley, Assigned: imjching)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 unaffected, firefox62 wontfix, firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
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.
(Reporter)

Comment 1

10 months ago
Going to scope this bug down a little to just encompass Activity Stream. We can deal with other about: pages in a follow-up.
Summary: Move Activity Stream (and other about: pages) into their own content process → Move Activity Stream into its own content process
(Reporter)

Updated

10 months ago
Blocks: fission
Severity: normal → enhancement
Iteration: --- → 62.4 - Jul 2
Priority: -- → P1
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.
(Assignee)

Updated

10 months ago
Blocks: 1416066
(Assignee)

Comment 3

10 months ago
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)
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 4

10 months ago
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 hidden (mozreview-request)
(Reporter)

Comment 6

10 months ago
mozreview-review
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.
Attachment #8986872 - Flags: review?(mconley) → review+
(Assignee)

Updated

10 months ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 8

10 months ago
mozreview-review
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

10 months ago
mozreview-review
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.
Attachment #8986872 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

10 months ago
mozreview-review-reply
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

10 months ago
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?
(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
Flags: needinfo?(kmaglione+bmo)

Comment 15

10 months ago
mozreview-review
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.
Attachment #8986872 - Flags: review?(kmaglione+bmo) → review+

Comment 16

10 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 18

10 months ago
mozreview-review-reply
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

10 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8986527 - Attachment is obsolete: true

Updated

10 months ago
Iteration: 62.4 - Jul 2 → 63.1 - July 9
Priority: P1 → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 22

10 months ago
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
(Reporter)

Comment 23

10 months ago
Interdiff looks good to me. Try looks solid too. Go ahead and autoland. Thanks!
(Reporter)

Updated

10 months ago
Summary: Move Activity Stream into its own content process → Add infrastructure to move Activity Stream into its own content process
Comment hidden (mozreview-request)

Comment 25

10 months ago
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)

Updated

10 months ago
Attachment #8986527 - Attachment is obsolete: false
(Assignee)

Updated

10 months ago
Attachment #8986527 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 27

10 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22fd5e86fbf6
Add infrastructure to move Activity Stream into its own content process. r=kmag,mconley

Comment 28

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22fd5e86fbf6
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Blocks: 1460425
You need to log in before you can comment on or make changes to this bug.