Closed Bug 1469072 Opened 6 years ago Closed 6 years ago

Add infrastructure to move Activity Stream into its own content process

Categories

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

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Iteration:
63.1 - July 9
Tracking Flags:
Tracking Status
firefox61 --- unaffected
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: mconley, Assigned: imjching)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1469072 - Add infrastructure to move Activity Stream into its own content process.
59 bytes, text/x-review-board-request
mconley
: review+
kmag
: review+
Details 
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
status-firefox61: --- → unaffected
status-firefox62: --- → wontfix
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.
Blocks: 1416066
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)
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 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+
Status: NEW → ASSIGNED
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 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 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 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 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 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 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]
Attachment #8986527 - Attachment is obsolete: true
Iteration: 62.4 - Jul 2 → 63.1 - July 9
Priority: P1 → P2
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
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
Attachment #8986527 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/22fd5e86fbf6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1482422
Component: Activity Streams: Newtab → New Tab Page
Depends on: 1557133
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: