Add infrastructure to move Activity Stream into its own content process

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
7 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)

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.
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
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

Last year
Blocks: 1416066
Assignee

Comment 3

Last year
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

Last year
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

Last year
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

Last year
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Reporter

Comment 8

Last year
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

Last year
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

Last year
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.
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
Attachment #8986527 - Attachment is obsolete: true
Iteration: 62.4 - Jul 2 → 63.1 - July 9
Priority: P1 → P2
Comment hidden (mozreview-request)
Assignee

Comment 22

Last year
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
Interdiff looks good to me. Try looks solid too. Go ahead and autoland. Thanks!
Summary: Move Activity Stream into its own content process → Add infrastructure to move Activity Stream into its own content process
Comment hidden (mozreview-request)
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)
Attachment #8986527 - Attachment is obsolete: false
Assignee

Updated

Last year
Attachment #8986527 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 27

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/22fd5e86fbf6
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1482422
You need to log in before you can comment on or make changes to this bug.