1147911 - Use a separate content process for file:// URLs

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

We are going to want to lock down file access completely at some point on the normal content processes.

Just brokering file access for arbitrary file:// URLs to the broker process, doesn't give us any real security benefits.
(Unless we vetted what you could or couldn't open, which would change browser functionality and possibly cause a lot of problems for users.)

If we only have file:// URLs processed is a separate content process, then a compromised normal content process would not be able to use them to read files.
The file:// URL content process, would have read only permissions.

Comment 1

[Jim Mathies [:jimm]]

I'm betting most of this work will be in the realm of e10s-multi vs. sandboxing.

Comment 2

[Bob Owen (:bobowen)]

Attachment: "WIP: Use a separate content process for file:// URIs page loads."

There's a fair amount of work still to do here, but most things at least seem to work now.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

The work in bug 1276553 for prerendering seems very similar. It might be good to unify the story for navigations across content processes.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Michael Layzell [:mystor] from comment #3)
> The work in bug 1276553 for prerendering seems very similar. It might be
> good to unify the story for navigations across content processes.

Thanks, I'll look into that.

(In reply to Bob Owen (:bobowen) from comment #2)
> Created attachment 8798935 [details] [diff] [review]
> "WIP: Use a separate content process for file:// URIs page loads."
> 
> There's a fair amount of work still to do here, but most things at least
> seem to work now.

"most things" apart from all these tests that are broken:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1821ad01f2c1be3cd925348d16177dbe6da6c234&selectedJob=28809877

Comment 5

[Bob Owen (:bobowen)]

(In reply to Michael Layzell [:mystor] from comment #3)
> The work in bug 1276553 for prerendering seems very similar. It might be
> good to unify the story for navigations across content processes.

Is this the bug you meant to link to?
I don't see too much overlap with the WIP patch on that bug, but I might be missing something.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bob Owen (:bobowen) from comment #5)
> (In reply to Michael Layzell [:mystor] from comment #3)
> > The work in bug 1276553 for prerendering seems very similar. It might be
> > good to unify the story for navigations across content processes.
> 
> Is this the bug you meant to link to?
> I don't see too much overlap with the WIP patch on that bug, but I might be
> missing something.

I was mostly talking about the tools for process-changing loads. You probably don't need to worry about it yet, but once that patch lands, I think we will probably want to use it for handling the history.

Comment 7

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49abfeaba63ed1a53570fa4c8b8df00f2375d808

Comment 8

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77249d9743b4526eb905fe2b2fb2e4e94d381c2b

Comment 9

[Bob Owen (:bobowen)]

Had to do another rebase since comment 8's try push, but I suspect they'll be more changes / rebases before I can land this, so I'm going to start the review process and push to try again before landing.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 1: Fix call to _openURIInNewTab in browser.js to take a URI referrer not a string

This is only a small fix, but I though it would add extra confusion to part 2.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 2: Add a remote type property and use it to drive the process switching in frontend code

This is the bulk of the frontend changes, adding in ochameau for the devtools bits.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 3: Add remote type parameter to forceInitialBrowserRemote

Also change talos pageloader.js to force type to match test URLs.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 4: Fix test to allow for separate file content process

Comment 14

[Bob Owen (:bobowen)]

Attachment: Part 5: Fix tests to allow for window.open to return null

These tests open a parent browser from the child, which means that the returned window isn't actually the real one anyway. Soon we will return null in this scenario.

Comment 15

[Bob Owen (:bobowen)]

Attachment: Part 6: Send remote type down to child

You might want to quickly scan part 2 first.

Comment 16

[Bob Owen (:bobowen)]

Attachment: Part 7: Create browsing context with no opener if URI needs different process

This also means window.open returns null in the same circumstance.

Comment 17

[Bob Owen (:bobowen)]

Attachment: Part 8: Create separate content process for file:// URIs

Comment 18

[Bob Owen (:bobowen)]

Attachment: Part 9: Ensure file read permissions for file content process on Windows

Comment 19

[:Gijs (he/him)]

Comment on attachment 8808120 [details] [diff] [review]
Part 1: Fix call to _openURIInNewTab in browser.js to take a URI referrer not a string

Review of attachment 8808120 [details] [diff] [review]:
-----------------------------------------------------------------

Welp. Why wasn't this caught by tests? Can you file a followup?

Comment 20

[:Gijs (he/him)]

This will break tabbrowser add-ons that manipulate the _openURIInNewTab method stuff. CC'ing folks and marking addon-compat.

Comment 21

[Bob Owen (:bobowen)]

(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8808120 [details] [diff] [review]
> Part 1: Fix call to _openURIInNewTab in browser.js to take a URI referrer
> not a string
> 
> Review of attachment 8808120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Welp. Why wasn't this caught by tests? Can you file a followup?

Filed bug 1315649.
There's a partial explanation there of why I think this didn't cause big problems.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8808122 [details] [diff] [review]
Part 2: Add a remote type property and use it to drive the process switching in frontend code

Review of attachment 8808122 [details] [diff] [review]:
-----------------------------------------------------------------

What happens for a chrome privileged page that tries to frame file:/// ?


While I think mconley or billm are probably better reviewers than me for some of this, I also think we should do better by our bus factor. So I've plodded through and done the best I could. I'm calling in Mike de Boer for backup, tho. :-)

(NB: explicitly haven't reviewed the devtools bits, leaving that for ochameau).

::: browser/base/content/browser.js
@@ +4937,5 @@
>          // we can hand back the nsIDOMWindow. The XULBrowserWindow.shouldLoadURI
>          // will do the job of shuttling off the newly opened browser to run in
>          // the right process once it starts loading a URI.
> +        // null means non-remote, undefined means we'll get the default.
> +        let preferredRemoteType = aOpener ? null : undefined;

Some code smell here. Please can we not encode magic meaning into the null/undefined distinction? That kind of thing is pretty much guaranteed to trip us up in future when someone touches that code.

Instead, can we pass an explicit value, e.g. "force-non-remote" ? Or add (keep) a separate param that indicates that we don't want remoteness at all?

@@ +4982,5 @@
>                            : Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID
>  
>      let referrer = aParams.referrer ? makeURI(aParams.referrer) : null;
> +    let preferredRemoteType = E10SUtils.getRemoteTypeForURI(aParams.referrer,
> +                                                            gMultiProcessBrowser);

Why are we basing this decision on the referrer instead of the URI we're opening?

::: browser/base/content/tabbrowser.xml
@@ +1688,5 @@
>              }
>  
>              // Abort if we're not going to change anything
> +            if (isRemote == aShouldBeRemote && !aFreshProcess &&
> +                // Don't check for remoteType match when non-remote.

Why not?

Maybe the comment should read "Check we're either not remote or in the correct type of remote process" or something.

But at that stage, not sure the comment adds something. :-)

@@ +1898,5 @@
> +            let remoteType =
> +              E10SUtils.getRemoteTypeForURI(BROWSER_NEW_TAB_URL,
> +                                            gMultiProcessBrowser);
> +            let browser = this._createBrowser({isPreloadBrowser: true,
> +                                               remoteType: remoteType});

Nit: again, don't need key: key, which also means this probably fits on 1 line.

@@ +1945,5 @@
>                b.setAttribute("remote", "true");
> +            } else if (aParams.remote) {
> +              // remote parameter used by some addons.
> +              b.setAttribute("remote", "true");
> +              b.setAttribute("remoteType", E10SUtils.DEFAULT_REMOTE_TYPE);

Slightly less duplication, which also fixes a minor issue with the if() statement change below, is to first do:

if (aParams.remote && !aParams.remoteType) {
  aParams.remoteType = E10SUtils.DEFAULT_REMOTE_TYPE;
}

This modifies the caller's options dict. I think that's OK in this case, but if we think that's iffy you could destructure out the remoteType member in the beginning and only override the local copy:

let {remoteType} = aParams;
if (!remoteType && aParams.remote) {
  remoteType = ...;
}

@@ +1950,4 @@
>              }
>  
>              if (aParams.opener) {
> +              if (aParams.remoteType) {

This won't throw with a descriptive message anymore for the add-on case, but probably should?

@@ +2050,5 @@
>  
>              if (!browser) {
>                // No preloaded browser found, create one.
>                browser = this._createBrowser({permanentKey: aTab.permanentKey,
> +                                             remoteType: remoteType,

Nit: don't need to duplicate key: key in ES6.

::: browser/modules/E10SUtils.jsm
@@ +46,3 @@
>      // loadURI in browser.xml treats null as about:blank
> +    if (!aURL || aURL == "about:blank") {
> +      return aPreferredRemoteType;

This change overrides the values we provide for about:blank in the about: handler in network ( https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutBlank.cpp#40-43 ).

Instead, can we leave this as-is, and check in the about: block lower down.

@@ +49,5 @@
> +    }
> +
> +    // Even though we're going to default to web below, check for the most
> +    // common cases before everything else.
> +    if (aURL.startsWith("http")) {

This method is not perf-critical. It only gets called for toplevel loads.

Also, I'm fairly sure that at this point we have no guarantee that the input is a valid URL. It's just some string. Doing this specialcasing feels wrong because it will also match "http foo" and the like. Can we just omit it?

@@ +76,5 @@
> +
> +      // Load remote if we must or can and parent (null) not preferred.
> +      let flags = module.getURIFlags(url);
> +      if ((flags & Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD) ||
> +          ((flags & Ci.nsIAboutModule.URI_CAN_LOAD_IN_CHILD) && aPreferredRemoteType)) {

So explicitly specialcase about:blank here? Also feels like that could be in a later patch, as this one doesn't seem to add any other remote types...

::: toolkit/content/widgets/browser.xml
@@ +345,5 @@
> +            if (!this.isRemoteBrowser) {
> +              return null;
> +            }
> +
> +            return this.getAttribute('remoteType') || E10SUtils.DEFAULT_REMOTE_TYPE;

Nit: double quotes are preferred. (The single quotes above are because they're in an attribute, but you're using a CDATA section so that concern does not apply.)

Sadly, you cannot assume that E10SUtils is defined wherever this XBL binding is loaded. Do we really need the fallback? If we do, you can use something along these lines:

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#404

This is ugly, but happily the JSM loader is pretty good at caching everything and so the perf impact should be ~0.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8808126 [details] [diff] [review]
Part 3: Add remote type parameter to forceInitialBrowserRemote

Review of attachment 8808126 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/talos/talos/pageloader/chrome/pageloader.js
@@ +233,5 @@
>                       //
>                       // It also probably means that per test (or, in fact, per pageloader browser
>                       // instance which adds the load listener and injects tpRecordTime), all the
>                       // pages should be able to load in the same mode as the initial page - due
> +                     // pages should be able to load in the same type as the initial page - due

?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8808130 [details] [diff] [review]
Part 6: Send remote type down to child

>+ContentChild::RecvRemoteType(const nsString& aRemoteType)
>+{
>+  mRemoteType.Assign(aRemoteType);
Shouldn't you assert there that existing non-empty value isn't replaced?
Actually, could mRemoteType be initialized to NullString(), and then check that only
null string can be replaced with some other value.

>@@ -1071,18 +1070,24 @@ ContentParent::CreateBrowserOrApp(const 
>     if (isInContentProcess) {
>       MOZ_ASSERT(aContext.IsMozBrowserElement());
>       constructorSender = CreateContentBridgeParent(aContext, initialPriority,
>                                                     openerTabId, &tabId);
>     } else {
>       if (aOpenerContentParent) {
>         constructorSender = aOpenerContentParent;
>       } else {
>+        nsAutoString remoteType;
>+        if (!aFrameElement->GetAttr(kNameSpaceID_None, nsGkAtoms::RemoteType,
>+                                    remoteType)) {
>+          remoteType.Assign(DEFAULT_REMOTE_TYPE);
>+        }
Ok, so one needs to set remoteType on xul:browser. How do we propagate that to the tabs opened from the initial
remoteType="foo" tab? Or does something else guarantee that if a child process crashes and we restart it, we get right kind of process?



>   /**
>+   * The type of the caller's browser.
>+   */
>+  readonly attribute DOMString remoteType;
>+
>+  /**
I don't quite understand the comment. type of what? One child process can be after all used by many tabs/xul:browsers


r- because I don't understand how process type attribute gets propagated to new tabs for the case when child process crashes. Perhaps I'm missing something, please explain or fix.

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 8808122 [details] [diff] [review]
Part 2: Add a remote type property and use it to drive the process switching in frontend code

Review of attachment 8808122 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsive.html/browser/swap.js
@@ +139,5 @@
> +      // 5. Force the original browser tab to match contentBrowser as we're
> +      //    about to swap the content into this tab.
> +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser,
> +                                       contentBrowser.isRemoteBrowser,
> +                                       contentBrowser.remoteType);

I'm not sure the tab will ever be non-remote. I would prefer to have :jryans look at that.

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8808128 [details] [diff] [review]
Part 4: Fix test to allow for separate file content process

Review of attachment 8808128 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/webconsole/test/browser_webconsole_bug_595223_file_uri.js
@@ +21,5 @@
>  
>    dir.append(TEST_FILE);
>    let uri = Services.io.newFileURI(dir);
>  
> +  let { browser } = yield loadTab(TEST_URI, "file");

I don't get why you have to modify this test.
Does this test fail if we load the file:// URI in the child process?

Otherwise, this new argument isn't really obvious. "file" value seems to have no real meaning. It looks like it is just important it to be different than undefined.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8808131 [details] [diff] [review]
Part 7: Create browsing context with no opener if URI needs different process

>@@ -686,18 +723,51 @@ ContentChild::ProvideWindowCommon(TabChi
>                                   bool aForceNoOpener,
>                                   bool* aWindowIsNew,
>                                   mozIDOMWindowProxy** aReturn)
> {
>   *aReturn = nullptr;
> 
>   nsAutoPtr<IPCTabContext> ipcContext;
>   TabId openerTabId = TabId(0);
>-
>+  nsAutoCString features(aFeatures);
>+
>+  nsresult rv;
>   if (aTabOpener) {
>+    // Check if the webbrowser chrome wants the load to proceed; this can be
>+    // used to cancel attempts to load URIs in the wrong process.
>+    nsCOMPtr<nsIWebBrowserChrome3> browserChrome3;
>+    rv = aTabOpener->GetWebBrowserChrome(getter_AddRefs(browserChrome3));
>+    if (NS_SUCCEEDED(rv) && browserChrome3) {
>+      bool shouldLoad;
>+      rv = browserChrome3->ShouldLoadURIInThisProcess(aURI, &shouldLoad);
>+      if (NS_SUCCEEDED(rv) && !shouldLoad) {
>+        nsAutoCString baseURIString;
>+        float fullZoom;
>+        DocShellOriginAttributes originAttributes;
>+        rv = GetWindowParamsFromParent(aParent, baseURIString, &fullZoom,
>+                                       originAttributes);
>+        if (NS_WARN_IF(NS_FAILED(rv))) {
>+          return rv;
>+        }
>+
>+        URIParams uriToLoad;
>+        SerializeURI(aURI, uriToLoad);
>+        Unused << SendCreateWindowNoOpener(aTabOpener, aChromeFlags,
>+                                           aCalledFromJS, aPositionSpecified,
>+                                           aSizeSpecified, uriToLoad, features,
>+                                           baseURIString, originAttributes,
>+                                           fullZoom);

This is really confusing. SendCreateWindowNoOpener isn't used even if aForceNoOpener is true.
So SendCreateWindowNoOpener isn't about noopener but about something else. Either some method renaming is needed
or noopener code path should also use this new method

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8808132 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs


>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
hmm, why firefox.js, why not all.js?
The changes you're making are rather Gecko internal, IMO

>+// Override default dom.ipc.processCount for some remote content process types.
>+pref("dom.ipc.processCount.webLA", 2);
webLA ? Certainly we need a better pref name where one could guess what it is about.
webLargeAllocation perhaps?

>+  // on the aRemoteType and aLargeAllocationProcess flag.
>+  nsAutoString contentProcessType(aLargeAllocationProcess
>+                                  ? LARGE_ALLOCATION_REMOTE_TYPE : aRemoteType);
>+  nsTArray<ContentParent*>* contentParents =
>+    sNonAppContentParents.LookupOrAdd(contentProcessType);
>+
>+  int32_t maxContentParents;
>+  nsAutoCString processCountPref("dom.ipc.processCount.");
>+  processCountPref.Append(NS_ConvertUTF16toUTF8(contentProcessType));
>+  if (NS_FAILED(Preferences::GetInt(processCountPref.get(), &maxContentParents))) {
>     maxContentParents = Preferences::GetInt("dom.ipc.processCount", 1);
>   }
This stuff must get a review from gabor or mrbkap

>-    if (sLargeAllocationContentParents) {
>-      sLargeAllocationContentParents->RemoveElement(this);
>-      if (!sLargeAllocationContentParents->Length()) {
>-        delete sLargeAllocationContentParents;
>-        sLargeAllocationContentParents = nullptr;
>+    nsTArray<ContentParent*>* contentParents =
>+      sNonAppContentParents.Get(mRemoteType);
>+    if (contentParents) {
>+      contentParents->RemoveElement(this);
>+      if (contentParents->IsEmpty()) {
>+        sNonAppContentParents.Remove(mRemoteType);
>       }
>     }
>   }
You don't delete sNonAppContentParents ever because it is a static hashtable.
Why doesn't static nsClassHashtable<nsStringHashKey, nsTArray<ContentParent*>> sNonAppContentParents; add new 
static initializer? Ping glandium about this?
>+static NS_NAMED_LITERAL_STRING(LARGE_ALLOCATION_REMOTE_TYPE, "webLA");
So this shouldn't be mysterious webLA

Comment 29

[:Gijs (he/him)]

Random braindump thought: I think we have per-process child process scripts. We should ensure those get inserted into all of these types of content processes (web, webLA, file).

Comment 30

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8808133 [details] [diff] [review]
Part 9: Ensure file read permissions for file content process on Windows

Review of attachment 8808133 [details] [diff] [review]:
-----------------------------------------------------------------

You'll want to test this on Linux — given the conditional at [1] I'd expect it to fail dramatically on FILEREAD, and you'll probably want to just delete SetCurrentProcessPrivileges given that it's a no-op in the post-B2G world.  A Try run ought to catch that (we do have tests that try to load file: URLs, right?).

r=me with that fixed.
 

[1] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/ipc/chromium/src/base/process_util_linux.cc#226

::: ipc/chromium/src/base/child_privileges.h
@@ +11,5 @@
> +
> +enum ChildPrivileges {
> +  PRIVILEGES_DEFAULT,
> +  PRIVILEGES_UNPRIVILEGED,
> +  PRIVILEGES_INHERIT,

I'm pretty sure we need only one of these three now.  They're a remnant of early B2G when it used Android's group-based permissions (the last of those went away in bug 976398, after which I think everything was UNPRIVILEGED — and UNPRIVILEGED as implemented needs the parent process to be running as root, so it was always B2G-only).  And B2G is gone now.  Currently it looks like everything winds up being INHERIT, but INHERIT being called INHERIT is probably because there used to be actual nested child processes, so it's maybe not the best name.

But cleaning all that up could be a followup bug.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8808129 [details] [diff] [review]
Part 5: Fix tests to allow for window.open to return null

Review of attachment 8808129 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/mochitest/test_ext_background_generated_url.html
@@ +46,3 @@
>    yield extension.awaitMessage("script done");
> +  if (win) {
> +    win.close();

If win is null, why bother doing this? Is it for non-e10s? If we don't need to close it for e10s, then let's not close it for non-e10s as well.

::: toolkit/components/extensions/test/mochitest/test_ext_content_security_policy.html
@@ +137,5 @@
>    checkCSP(contentCSP, "content frame");
>  
>  
> +  if (win1) {
> +    win1.close();

Same here.

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8808130 [details] [diff] [review]
Part 6: Send remote type down to child

Review of attachment 8808130 [details] [diff] [review]:
-----------------------------------------------------------------

I would think :Gijs may want to review this patch as well, since it also modifies E10SUtils.jsm.

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8808132 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs

Review of attachment 8808132 [details] [diff] [review]:
-----------------------------------------------------------------

I would think :Gijs may want to review this patch as well, since it also modifies E10SUtils.jsm.

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8808130 [details] [diff] [review]
Part 6: Send remote type down to child

Review of attachment 8808130 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure if it fits in this part or not, but I think nsFrameLoader::SwapWithOtherLoader[1] should be updated to return NS_ERROR_NOT_IMPLEMENTED if the @remoteType doesn't match between the two frames.

[1]: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/base/nsFrameLoader.cpp#1304

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8808122 [details] [diff] [review]
Part 2: Add a remote type property and use it to drive the process switching in frontend code

Review of attachment 8808122 [details] [diff] [review]:
-----------------------------------------------------------------

I only reviewed the DevTools and View Source files, which seem reasonable overall.

::: devtools/client/responsive.html/browser/swap.js
@@ +138,5 @@
>  
> +      // 5. Force the original browser tab to match contentBrowser as we're
> +      //    about to swap the content into this tab.
> +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser,
> +                                       contentBrowser.isRemoteBrowser,

We require the content browser to be remote in this code path, so let's keep the second arg as `true` to reinforce that assumption.

@@ +139,5 @@
> +      // 5. Force the original browser tab to match contentBrowser as we're
> +      //    about to swap the content into this tab.
> +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser,
> +                                       contentBrowser.isRemoteBrowser,
> +                                       contentBrowser.remoteType);

It seems like the `remoteType` will always be `web` here, since the components/browser.js change forces `web` as well...  I suppose we'll want to extend things here to keep them working for file:// URLs, though.

Can you file a follow up bug in Firefox :: DevTools: Responsive Design Mode to make whatever additional changes are needed to support file:// here?

Comment 36

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8808132 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs

Review of attachment 8808132 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with Smaug's comments addressed. And thanks for keeping me in the loop.

Comment 37

[:Gijs (he/him)]

Comment on attachment 8808130 [details] [diff] [review]
Part 6: Send remote type down to child

Review of attachment 8808130 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the E10SUtils change, but see nit below.

::: xpcom/system/nsIXULRuntime.idl
@@ +91,5 @@
>     */
>    readonly attribute uint64_t uniqueProcessID;
>  
>    /**
> +   * The type of the caller's browser.

Nit: "The type of remote process we're running in. null if we're in the parent/chrome process."

This is unrelated to the <browser>, and strictly related to content. The E10SUtils caller would have to be same-process for it to give correct results, nsIXULRuntime doesn't (and shouldn't) know anything about browsers and their respective content process types.

Is it actually legal to return null for an xpidl DOMString attribute? I'm assuming whoever reviews the core stuff here knows.

Comment 38

[:Gijs (he/him)]

Comment on attachment 8808132 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs

Review of attachment 8808132 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/E10SUtils.jsm
@@ +57,5 @@
>  
>      // Even though we're going to default to web below, check for the most
>      // common cases before everything else.
>      if (aURL.startsWith("http")) {
> +      return validatedWebRemoteType(aPreferredRemoteType);

As I commented earlier I think this early check should go. It makes it harder to reason about what this method does, it could produce the wrong thing because there are no guarantees about valid-url-ness of aURL, and the optimization should be unnecessary.

I'm not sure why the validation code was added here, and why it works the way it does. Right now, if you call this method with a data: URL and pass "gobbledygook" as the preferred remote type, it will happily return "gobbledygook" as the target remote type. That doesn't seem right.

It feels like we should instead, at this point in the file:

- check that the preferred type is either null/force-non-remote or "file" AND the URL starts with "file:", and then return based on the pref as in the if block below
- otherwise, verify that the preferred value is one of web, webLA or null/force-non-remote, and set to "web" otherwise. Then continue down the function for the rest of the checks that may or may not return the preferred type.

Does that make sense?

Comment 39

[Mike de Boer [:mikedeboer]]

Comment on attachment 8808122 [details] [diff] [review]
Part 2: Add a remote type property and use it to drive the process switching in frontend code

Review of attachment 8808122 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed on the sessionstore bits.

::: browser/components/sessionstore/SessionStore.jsm
@@ +3097,5 @@
>                            (tabbrowser.tabs[t].getAttribute("usercontextid") == (userContextId || ""));
>        // If the tab is pinned, then we'll be loading it right away, and
>        // there's no need to cause a remoteness flip by loading it initially
> +      // non-remote. null means non-remote, undefined means default.
> +      let preferredRemoteType = winData.tabs[t].pinned ? undefined : null;

I agree with Gijs here; a more descriptive argument value would do a lot of good. The fact that you need to explain what the distinction between `null` and `undefined` means is code smell.

Comment 40

[Jim Mathies [:jimm]]

Comment on attachment 8808133 [details] [diff] [review]
Part 9: Ensure file read permissions for file content process on Windows

Review of attachment 8808133 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/child_privileges.h
@@ +12,5 @@
> +enum ChildPrivileges {
> +  PRIVILEGES_DEFAULT,
> +  PRIVILEGES_UNPRIVILEGED,
> +  PRIVILEGES_INHERIT,
> +  PRIVILEGES_FILEREAD,

nit - comment me

Comment 41

[Bob Owen (:bobowen)]

Attachment: Part 2: Add a remote type property and use it to drive the process switching in frontend code

Thanks for the reviews everyone.

(In reply to :Gijs Kruitbosch from comment #22)

> What happens for a chrome privileged page that tries to frame file:/// ?

It will attempt to load it in that process.
If that's a normal content process then it might fail once we start to remove file read access.
Do you think we do this anywhere?

> ::: browser/base/content/browser.js
> @@ +4937,5 @@
> >          // we can hand back the nsIDOMWindow. The XULBrowserWindow.shouldLoadURI
> >          // will do the job of shuttling off the newly opened browser to run in
> >          // the right process once it starts loading a URI.
> > +        // null means non-remote, undefined means we'll get the default.
> > +        let preferredRemoteType = aOpener ? null : undefined;
> 
> Some code smell here. Please can we not encode magic meaning into the
> null/undefined distinction? That kind of thing is pretty much guaranteed to
> trip us up in future when someone touches that code.

I didn't really like this either, but having to know the correct magic string didn't seem much better.
Anyway, looks like I can go back to just using forceNotRemote, so this has gone.

> @@ +4982,5 @@
> >                            : Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID
> >  
> >      let referrer = aParams.referrer ? makeURI(aParams.referrer) : null;
> > +    let preferredRemoteType = E10SUtils.getRemoteTypeForURI(aParams.referrer,
> > +                                                            gMultiProcessBrowser);
> 
> Why are we basing this decision on the referrer instead of the URI we're
> opening?

At one point I thought I was going to have to make this work for the use of this in the existing RecvCreateWindow case where aURI was null.
I'd later realised that it uses this magical AutoUseNewTab to force it into a pre-created tab, so I think I can drop back to forceNoRemote.
This gets rid of a few changes and hopefully reduces any addon compatibility issues.
 
> ::: browser/base/content/tabbrowser.xml
> @@ +1688,5 @@
> >              }
> >  
> >              // Abort if we're not going to change anything
> > +            if (isRemote == aShouldBeRemote && !aFreshProcess &&
> > +                // Don't check for remoteType match when non-remote.
> 
> Why not?
> 
> Maybe the comment should read "Check we're either not remote or in the
> correct type of remote process" or something.
> 
> But at that stage, not sure the comment adds something. :-)

Removed.
 
> @@ +1898,5 @@
> > +            let remoteType =
> > +              E10SUtils.getRemoteTypeForURI(BROWSER_NEW_TAB_URL,
> > +                                            gMultiProcessBrowser);
> > +            let browser = this._createBrowser({isPreloadBrowser: true,
> > +                                               remoteType: remoteType});
> 
> Nit: again, don't need key: key, which also means this probably fits on 1
> line.

Fixed.
 
> @@ +1945,5 @@
> >                b.setAttribute("remote", "true");
> > +            } else if (aParams.remote) {
> > +              // remote parameter used by some addons.
> > +              b.setAttribute("remote", "true");
> > +              b.setAttribute("remoteType", E10SUtils.DEFAULT_REMOTE_TYPE);
> 
> Slightly less duplication, which also fixes a minor issue with the if()
> statement change below, is to first do:
> 
> if (aParams.remote && !aParams.remoteType) {
>   aParams.remoteType = E10SUtils.DEFAULT_REMOTE_TYPE;
> }

I went with this, thanks.

> @@ +2050,5 @@
> >  
> >              if (!browser) {
> >                // No preloaded browser found, create one.
> >                browser = this._createBrowser({permanentKey: aTab.permanentKey,
> > +                                             remoteType: remoteType,
> 
> Nit: don't need to duplicate key: key in ES6.

Fixed.

> ::: browser/modules/E10SUtils.jsm
> @@ +46,3 @@
> >      // loadURI in browser.xml treats null as about:blank
> > +    if (!aURL || aURL == "about:blank") {
> > +      return aPreferredRemoteType;
> 
> This change overrides the values we provide for about:blank in the about:
> handler in network (
> https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/
> nsAboutBlank.cpp#40-43 ).
> 
> Instead, can we leave this as-is, and check in the about: block lower down.

I've changed it so that about: and chrome: that can load remote can load in any.
So, I don't need to special case about:blank.
 
> @@ +49,5 @@
> > +    }
> > +
> > +    // Even though we're going to default to web below, check for the most
> > +    // common cases before everything else.
> > +    if (aURL.startsWith("http")) {
> 
> This method is not perf-critical. It only gets called for toplevel loads.

I just thought it was a bit odd that the most common case is probably the slowest.

> Also, I'm fairly sure that at this point we have no guarantee that the input
> is a valid URL. It's just some string. Doing this specialcasing feels wrong
> because it will also match "http foo" and the like. Can we just omit it?

True, but it was never validated ... but removed anyway.

> ::: toolkit/content/widgets/browser.xml
> @@ +345,5 @@
> > +            if (!this.isRemoteBrowser) {
> > +              return null;
> > +            }
> > +
> > +            return this.getAttribute('remoteType') || E10SUtils.DEFAULT_REMOTE_TYPE;
> 
> Nit: double quotes are preferred. (The single quotes above are because
> they're in an attribute, but you're using a CDATA section so that concern
> does not apply.)

Fixed.

> Sadly, you cannot assume that E10SUtils is defined wherever this XBL binding
> is loaded. Do we really need the fallback? If we do, you can use something
> along these lines:

Tests and possibly some addons set the "remote" attribute directly, so I've used that trick.
But it will only be run in these cases, so should be fine.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35)

> ::: devtools/client/responsive.html/browser/swap.js
> @@ +138,5 @@
> >  
> > +      // 5. Force the original browser tab to match contentBrowser as we're
> > +      //    about to swap the content into this tab.
> > +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser,
> > +                                       contentBrowser.isRemoteBrowser,
> 
> We require the content browser to be remote in this code path, so let's keep
> the second arg as `true` to reinforce that assumption.

Done.
 
> @@ +139,5 @@
> > +      // 5. Force the original browser tab to match contentBrowser as we're
> > +      //    about to swap the content into this tab.
> > +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser,
> > +                                       contentBrowser.isRemoteBrowser,
> > +                                       contentBrowser.remoteType);
> 
> It seems like the `remoteType` will always be `web` here, since the
> components/browser.js change forces `web` as well...  I suppose we'll want
> to extend things here to keep them working for file:// URLs, though.
> 
> Can you file a follow up bug in Firefox :: DevTools: Responsive Design Mode
> to make whatever additional changes are needed to support file:// here?

This seems to work for file:// anyway, do you still want me to file a follow-up?

Comment 42

[Bob Owen (:bobowen)]

Attachment: Part 3: Add remote type parameter to forceInitialBrowserRemote

Also change talos pageloader.js to force type to match test URLs.

(In reply to :Gijs Kruitbosch from comment #23)
> Comment on attachment 8808126 [details] [diff] [review]
> Part 3: Add remote type parameter to forceInitialBrowserRemote

> >                       // pages should be able to load in the same mode as the initial page - due
> > +                     // pages should be able to load in the same type as the initial page - due
> 
> ?

Ah, the pitfalls of doing rewrites by manually hacking patches ... removed.

Comment 43

[Bob Owen (:bobowen)]

Comment on attachment 8809359 [details] [diff] [review]
Part 3: Add remote type parameter to forceInitialBrowserRemote

Carrying r=gijs from comment 23.

Comment 44

[Bob Owen (:bobowen)]

Attachment: Part 4: Fix test to allow for separate file content process

(In reply to Alexandre Poirot [:ochameau] from comment #26)
> Comment on attachment 8808128 [details] [diff] [review]
> Part 4: Fix test to allow for separate file content process

> ::: devtools/client/webconsole/test/browser_webconsole_bug_595223_file_uri.js
> @@ +21,5 @@
> >  
> >    dir.append(TEST_FILE);
> >    let uri = Services.io.newFileURI(dir);
> >  
> > +  let { browser } = yield loadTab(TEST_URI, "file");
> 
> I don't get why you have to modify this test.
> Does this test fail if we load the file:// URI in the child process?
> 
> Otherwise, this new argument isn't really obvious. "file" value seems to
> have no real meaning. It looks like it is just important it to be different
> than undefined.

We need the browser to be in the file content process, otherwise when we load
the file URI the switch messes up the test. I've added a comment.
Also, just used about:blank for the initial URI as I don't think we need a data: URI.

Sorry, this should have probably gone in with part 8, but I don't really
want to re-number now.

Comment 45

[Bob Owen (:bobowen)]

Attachment: Part 5: Fix tests to allow for window.open to return null

These tests open a parent browser from the child, which means that the returned window isn't actually the real one anyway. Soon we will return null in this scenario.

(In reply to Bill McCloskey (:billm) from comment #31)
> Comment on attachment 8808129 [details] [diff] [review]
> Part 5: Fix tests to allow for window.open to return null
> 
> Review of attachment 8808129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> toolkit/components/extensions/test/mochitest/
> test_ext_background_generated_url.html
> @@ +46,3 @@
> >    yield extension.awaitMessage("script done");
> > +  if (win) {
> > +    win.close();
> 
> If win is null, why bother doing this? Is it for non-e10s? If we don't need
> to close it for e10s, then let's not close it for non-e10s as well.

It doesn't appear to matter for non-e10s, so I've removed it.

Also added a comment to explain why we're using try catch in the one test.

Comment 46

[Bob Owen (:bobowen)]

Comment on attachment 8809362 [details] [diff] [review]
Part 5: Fix tests to allow for window.open to return null

Carrying r=billm from comment 31.

Comment 47

[Bob Owen (:bobowen)]

Attachment: Part 6: Send remote type down to child

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8808130 [details] [diff] [review]
> Part 6: Send remote type down to child
> 
> >+ContentChild::RecvRemoteType(const nsString& aRemoteType)
> >+{
> >+  mRemoteType.Assign(aRemoteType);
> Shouldn't you assert there that existing non-empty value isn't replaced?
> Actually, could mRemoteType be initialized to NullString(), and then check
> that only
> null string can be replaced with some other value.

Yes, this should only be set soon after creation, I've added a default and
assertion. 

> >@@ -1071,18 +1070,24 @@ ContentParent::CreateBrowserOrApp(const 
> >     if (isInContentProcess) {
> >       MOZ_ASSERT(aContext.IsMozBrowserElement());
> >       constructorSender = CreateContentBridgeParent(aContext, initialPriority,
> >                                                     openerTabId, &tabId);
> >     } else {
> >       if (aOpenerContentParent) {
> >         constructorSender = aOpenerContentParent;
> >       } else {
> >+        nsAutoString remoteType;
> >+        if (!aFrameElement->GetAttr(kNameSpaceID_None, nsGkAtoms::RemoteType,
> >+                                    remoteType)) {
> >+          remoteType.Assign(DEFAULT_REMOTE_TYPE);
> >+        }
> Ok, so one needs to set remoteType on xul:browser. How do we propagate that
> to the tabs opened from the initial
> remoteType="foo" tab? Or does something else guarantee that if a child
> process crashes and we restart it, we get right kind of process?

The changes in part 2 should be ensuring we get the correct remote type.
For a crashed tab I think the xul:browser will still have the correct remote type and that will be used when the ContentParent is created/picked.
Even if it doesn't SessionStore.restoreTabContent should do the appropriate switching.

I'm pretty sure there will be some edge cases that will need follow-up fixes.

> >   /**
> >+   * The type of the caller's browser.
> >+   */
> >+  readonly attribute DOMString remoteType;
> >+
> >+  /**
> I don't quite understand the comment. type of what? One child process can be
> after all used by many tabs/xul:browsers

Sorry, hangover from previous incarnation, I've stolen Gijs's suggestion, but
changed it to content process, because it doesn't apply to the other child
process types.



(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #34)
> Comment on attachment 8808130 [details] [diff] [review]
> Part 6: Send remote type down to child
> 
> Review of attachment 8808130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not sure if it fits in this part or not, but I think
> nsFrameLoader::SwapWithOtherLoader[1] should be updated to return
> NS_ERROR_NOT_IMPLEMENTED if the @remoteType doesn't match between the two
> frames.

It actually breaks dragging a tab in and out of the browser if I add this.
That doesn't seem right, but are you OK if I investigate this in a follow-up?

Comment 48

[Bob Owen (:bobowen)]

Attachment: Part 7: Create browsing context with no opener if URI needs different process

This also means window.open returns null in the same circumstance.

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8808131 [details] [diff] [review]
> Part 7: Create browsing context with no opener if URI needs different process
> 
> >@@ -686,18 +723,51 @@ ContentChild::ProvideWindowCommon(TabChi

> >+        Unused << SendCreateWindowNoOpener(aTabOpener, aChromeFlags,
> >+                                           aCalledFromJS, aPositionSpecified,
> >+                                           aSizeSpecified, uriToLoad, features,
> >+                                           baseURIString, originAttributes,
> >+                                           fullZoom);
> 
> This is really confusing. SendCreateWindowNoOpener isn't used even if
> aForceNoOpener is true.
> So SendCreateWindowNoOpener isn't about noopener but about something else.
> Either some method renaming is needed
> or noopener code path should also use this new method

Changed this to CreateWindowInDifferentProcess.
Also improved the comment.

Comment 49

[Bob Owen (:bobowen)]

Attachment: Part 8: Create separate content process for file:// URIs

(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8808132 [details] [diff] [review]
> Part 8: Create separate content process for file:// URIs
> 
> 
> >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
> >--- a/browser/app/profile/firefox.js
> >+++ b/browser/app/profile/firefox.js
> hmm, why firefox.js, why not all.js?
> The changes you're making are rather Gecko internal, IMO

I've moved the large allocation setting to all.js.
Do you think the separate file process one should move as well?

> >+// Override default dom.ipc.processCount for some remote content process types.
> >+pref("dom.ipc.processCount.webLA", 2);
> webLA ? Certainly we need a better pref name where one could guess what it
> is about.
> webLargeAllocation perhaps?

Changed to this. 

> >-    if (sLargeAllocationContentParents) {
> >-      sLargeAllocationContentParents->RemoveElement(this);
> >-      if (!sLargeAllocationContentParents->Length()) {
> >-        delete sLargeAllocationContentParents;
> >-        sLargeAllocationContentParents = nullptr;
> >+    nsTArray<ContentParent*>* contentParents =
> >+      sNonAppContentParents.Get(mRemoteType);
> >+    if (contentParents) {
> >+      contentParents->RemoveElement(this);
> >+      if (contentParents->IsEmpty()) {
> >+        sNonAppContentParents.Remove(mRemoteType);
> >       }
> >     }
> >   }
> You don't delete sNonAppContentParents ever because it is a static hashtable.
> Why doesn't static nsClassHashtable<nsStringHashKey,
> nsTArray<ContentParent*>> sNonAppContentParents; add new 
> static initializer? Ping glandium about this?

glandium - would you mind looking at this, it doesn't seem to create a static
initializer, is it likely to cause problems?


(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8808132 [details] [diff] [review]
> Part 8: Create separate content process for file:// URIs
> 
> Review of attachment 8808132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/E10SUtils.jsm
> @@ +57,5 @@
> >  
> >      // Even though we're going to default to web below, check for the most
> >      // common cases before everything else.
> >      if (aURL.startsWith("http")) {
> > +      return validatedWebRemoteType(aPreferredRemoteType);
> 
> As I commented earlier I think this early check should go. It makes it
> harder to reason about what this method does, it could produce the wrong
> thing because there are no guarantees about valid-url-ness of aURL, and the
> optimization should be unnecessary.
> 
> I'm not sure why the validation code was added here, and why it works the
> way it does. Right now, if you call this method with a data: URL and pass
> "gobbledygook" as the preferred remote type, it will happily return
> "gobbledygook" as the target remote type. That doesn't seem right.
> 
> It feels like we should instead, at this point in the file:
> 
> - check that the preferred type is either null/force-non-remote or "file"
> AND the URL starts with "file:", and then return based on the pref as in the
> if block below
> - otherwise, verify that the preferred value is one of web, webLA or
> null/force-non-remote, and set to "web" otherwise. Then continue down the
> function for the rest of the checks that may or may not return the preferred
> type.
> 
> Does that make sense?

Not quite, because about:blank (at least) needs to be valid for file as well.
I was deliberately not trying to be totally prescriptive about what aPreferredRemoteType can be in the "any remote process should be OK" cases.
I got rid of the data: URI check. I thought I'd need it for a few tests, but it turned out to be one, for which I think I can use "about:blank" instead.

Comment 50

[Bob Owen (:bobowen)]

glandium - just a question over the use of a static nsClassHashtable half way down comment 49.

Comment 51

[Alexandre Poirot [:ochameau]]

Comment on attachment 8809360 [details] [diff] [review]
Part 4: Fix test to allow for separate file content process

Review of attachment 8809360 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, it looks much clearer with the comment!

Comment 52

[:Gijs (he/him)]

Comment on attachment 8809358 [details] [diff] [review]
Part 2: Add a remote type property and use it to drive the process switching in frontend code

Review of attachment 8809358 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bob Owen (:bobowen) from comment #41)
> > Also, I'm fairly sure that at this point we have no guarantee that the input
> > is a valid URL. It's just some string. Doing this specialcasing feels wrong
> > because it will also match "http foo" and the like. Can we just omit it?
> 
> True, but it was never validated ... but removed anyway.

Yes, and this is something we should really fix at some point, and we're slowly getting there - basically, fixing up sloppy user input into real URIs should happen early on in the frontend. Right now it can end up happening in docshell, and everything in the meantime has to deal with the unsanitized input. For now, we just need to grit our teeth and bear it. :-(

::: browser/base/content/tabbrowser.xml
@@ +1682,5 @@
>              // if the browser is _currently_ non-remote, we need the openers to match,
>              // because it is already too late to change it.
>              if (aOpener) {
>                if (aShouldBeRemote) {
> +                throw new Error("Cannot set an opener on a browser which should be remote!");

Out of curiosity, why change this?

Comment 53

[:Gijs (he/him)]

Comment on attachment 8809369 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs

Review of attachment 8809369 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bob Owen (:bobowen) from comment #49)
> Created attachment 8809369 [details] [diff] [review]
> Part 8: Create separate content process for file:// URIs
> 
> (In reply to Olli Pettay [:smaug] from comment #28)
> > Comment on attachment 8808132 [details] [diff] [review]
> > Part 8: Create separate content process for file:// URIs
> > 
> > 
> > >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
> > >--- a/browser/app/profile/firefox.js
> > >+++ b/browser/app/profile/firefox.js
> > hmm, why firefox.js, why not all.js?
> > The changes you're making are rather Gecko internal, IMO
> 
> I've moved the large allocation setting to all.js.
> Do you think the separate file process one should move as well?

Speaking only for myself, yes.

r=me with that and the below addressed one way or another.

::: browser/modules/E10SUtils.jsm
@@ +72,5 @@
>        let module = getAboutModule(url);
>        // If the module doesn't exist then an error page will be loading, that
>        // should be ok to load in any process
>        if (!module) {
>          return aPreferredRemoteType;

So now, if I have a non-about-blank about: URI that can be loaded in a remote process and that we're trying to load in a browser that's currently using the file process, we will use the file process? I think that's only correct for about:blank, and we should probably specialcase it in here.

Comment 54

[Bob Owen (:bobowen)]

(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #30)
> Comment on attachment 8808133 [details] [diff] [review]
> Part 9: Ensure file read permissions for file content process on Windows
> 
> Review of attachment 8808133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You'll want to test this on Linux — given the conditional at [1] I'd expect
> it to fail dramatically on FILEREAD, and you'll probably want to just delete
> SetCurrentProcessPrivileges given that it's a no-op in the post-B2G world. 
> A Try run ought to catch that (we do have tests that try to load file: URLs,
> right?).
> 
> r=me with that fixed.

We do, and they did fail, but I made it so that FILEREAD uses DefaultChildPrivileges, for the moment on Linux/Mac.


(In reply to Jim Mathies [:jimm] from comment #40)
> Comment on attachment 8808133 [details] [diff] [review]
> Part 9: Ensure file read permissions for file content process on Windows
> 
> Review of attachment 8808133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/chromium/src/base/child_privileges.h
> @@ +12,5 @@
> > +enum ChildPrivileges {
> > +  PRIVILEGES_DEFAULT,
> > +  PRIVILEGES_UNPRIVILEGED,
> > +  PRIVILEGES_INHERIT,
> > +  PRIVILEGES_FILEREAD,
> 
> nit - comment me

Comment added locally for FILEREAD, I'll let Jed comment any that are still around after the follow-up bug.

Comment 55

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8809365 [details] [diff] [review]
Part 6: Send remote type down to child

Review of attachment 8809365 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bob Owen (:bobowen) from comment #47)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #34)
> > Comment on attachment 8808130 [details] [diff] [review]
> > Part 6: Send remote type down to child
> > 
> > Review of attachment 8808130 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Not sure if it fits in this part or not, but I think
> > nsFrameLoader::SwapWithOtherLoader[1] should be updated to return
> > NS_ERROR_NOT_IMPLEMENTED if the @remoteType doesn't match between the two
> > frames.
> 
> It actually breaks dragging a tab in and out of the browser if I add this.
> That doesn't seem right, but are you OK if I investigate this in a follow-up?

If it breaks moving tabs, this suggests that the @remoteType isn't being set correctly / early enough for browsers used when moving tabs, so we'll definitely want to fix that.  Since this feature will Nightly only for now, I think it's okay to do this in a follow up, so please file it.

I would anticipate we'll also need a few changes to the same Responsive Design Mode code paths you modified in this bug once this check is added, so we can take care of that in the follow up as well.

Comment 56

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8809358 [details] [diff] [review]
Part 2: Add a remote type property and use it to drive the process switching in frontend code

Review of attachment 8809358 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bob Owen (:bobowen) from comment #41)
> > @@ +139,5 @@
> > > +      // 5. Force the original browser tab to match contentBrowser as we're
> > > +      //    about to swap the content into this tab.
> > > +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser,
> > > +                                       contentBrowser.isRemoteBrowser,
> > > +                                       contentBrowser.remoteType);
> > 
> > It seems like the `remoteType` will always be `web` here, since the
> > components/browser.js change forces `web` as well...  I suppose we'll want
> > to extend things here to keep them working for file:// URLs, though.
> > 
> > Can you file a follow up bug in Firefox :: DevTools: Responsive Design Mode
> > to make whatever additional changes are needed to support file:// here?
> 
> This seems to work for file:// anyway, do you still want me to file a
> follow-up?

Ah, you're right it does seem to work with the current patches.  Okay, I don't think we need an extra bug for this case.  It's possible the situation will change here once you add a check in nsFrameLoader::SwapWithOtherLoader, but we can handle that in the follow up I already requested for that work in comment 55.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8809365 [details] [diff] [review]
Part 6: Send remote type down to child


> bool
>+ContentChild::RecvRemoteType(const nsString& aRemoteType)
>+{
>+  MOZ_ASSERT(DOMStringIsNull(aRemoteType));
>+
>+  mRemoteType.Assign(aRemoteType);
So I was hoping you would assert that mRemoteType is set only once, so you should assert that it is null here.
But I don't understand how you can assert MOZ_ASSERT(DOMStringIsNull(aRemoteType));
Aren't you missing ! there?


r-, because I assume this hasn't been run on debug builds.

Comment 58

[Mike Hommey [:glandium]]

(In reply to Bob Owen (:bobowen) from comment #49)
> > You don't delete sNonAppContentParents ever because it is a static hashtable.
> > Why doesn't static nsClassHashtable<nsStringHashKey,
> > nsTArray<ContentParent*>> sNonAppContentParents; add new 
> > static initializer? Ping glandium about this?
> 
> glandium - would you mind looking at this, it doesn't seem to create a static
> initializer, is it likely to cause problems?

Where do you get that it's not creating a static initializer? I don't see a reason why there wouldn't be one.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8809366 [details] [diff] [review]
Part 7: Create browsing context with no opener if URI needs different process

>>+NS_IMETHODIMP nsContentTreeOwner::ShouldLoadURIInThisProcess(nsIURI *aURI,
>+                                                             bool *_retval)
Be consistent. * goes with the type and all arguments are in form aName

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8809369 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs


>+#if defined(NIGHTLY_BUILD)
>+pref("browser.tabs.remote.separateFile", true);
>+#else
>+pref("browser.tabs.remote.separateFile", false);
>+#endif

tiny bit odd pref name. Couldn't guess from the name what it means.
But don't have good suggestions.

> nsDataHashtable<nsStringHashKey, ContentParent*>* ContentParent::sAppContentParents;
>-nsTArray<ContentParent*>* ContentParent::sNonAppContentParents;
>-nsTArray<ContentParent*>* ContentParent::sLargeAllocationContentParents;
>+nsClassHashtable<nsStringHashKey, nsTArray<ContentParent*>> ContentParent::sNonAppContentParents;
So it is still unclear whether this type is right, or whether nsClassHashtable<nsStringHashKey, nsTArray<ContentParent*>>* should be used

>-    if (sLargeAllocationContentParents) {
>-      sLargeAllocationContentParents->RemoveElement(this);
>-      if (!sLargeAllocationContentParents->Length()) {
>-        delete sLargeAllocationContentParents;
>-        sLargeAllocationContentParents = nullptr;
>+    nsTArray<ContentParent*>* contentParents =
>+      sNonAppContentParents.Get(mRemoteType);
>+    if (contentParents) {
>+      contentParents->RemoveElement(this);
>+      if (contentParents->IsEmpty()) {
>+        sNonAppContentParents.Remove(mRemoteType);
>       }
So if sNonAppContentParents can't be non-pointer static variable, it should be deleted somewhere here


If the hashtable handling needs to be fixed, it could be a separate patch.

Comment 61

[Bob Owen (:bobowen)]

Attachment: Part 6: Send remote type down to child

(In reply to Olli Pettay [:smaug] from comment #57)
> Comment on attachment 8809365 [details] [diff] [review]
> Part 6: Send remote type down to child
> 
> 
> > bool
> >+ContentChild::RecvRemoteType(const nsString& aRemoteType)
> >+{
> >+  MOZ_ASSERT(DOMStringIsNull(aRemoteType));
> >+
> >+  mRemoteType.Assign(aRemoteType);
> So I was hoping you would assert that mRemoteType is set only once, so you
> should assert that it is null here.
> But I don't understand how you can assert
> MOZ_ASSERT(DOMStringIsNull(aRemoteType));
> Aren't you missing ! there?
> 
> 
> r-, because I assume this hasn't been run on debug builds.

Damn, just a typo I meant mRemoteType.

I haven't been running as many debug builds for these changes, because there are a lot of JS changes as JS debugging with a debug build on Windows is really painful.

Seems to work OK now, but I'll be re-pushing to try anyway, as there have been a few changes and I suspect I'll have to rebase again before I can land.

Comment 62

[Bob Owen (:bobowen)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #55)
> Comment on attachment 8809365 [details] [diff] [review]
> Part 6: Send remote type down to child

> > > Not sure if it fits in this part or not, but I think
> > > nsFrameLoader::SwapWithOtherLoader[1] should be updated to return
> > > NS_ERROR_NOT_IMPLEMENTED if the @remoteType doesn't match between the two
> > > frames.
> > 
> > It actually breaks dragging a tab in and out of the browser if I add this.
> > That doesn't seem right, but are you OK if I investigate this in a follow-up?
> 
> If it breaks moving tabs, this suggests that the @remoteType isn't being set
> correctly / early enough for browsers used when moving tabs, so we'll
> definitely want to fix that.  Since this feature will Nightly only for now,
> I think it's okay to do this in a follow up, so please file it.

Bug 1317293 filed.

Comment 63

[Bob Owen (:bobowen)]

(In reply to Mike Hommey [:glandium] from comment #58)
> (In reply to Bob Owen (:bobowen) from comment #49)
> > > You don't delete sNonAppContentParents ever because it is a static hashtable.
> > > Why doesn't static nsClassHashtable<nsStringHashKey,
> > > nsTArray<ContentParent*>> sNonAppContentParents; add new 
> > > static initializer? Ping glandium about this?
> > 
> > glandium - would you mind looking at this, it doesn't seem to create a static
> > initializer, is it likely to cause problems?
> 
> Where do you get that it's not creating a static initializer? I don't see a
> reason why there wouldn't be one.

I was going on the compiler_metrics_num_constructors results for these two try pushes with and without my changes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77249d9743b4526eb905fe2b2fb2e4e94d381c2b&selectedJob=30463506
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ef242daabb80252b171f888473dd0ce8560286e&selectedJob=30468346

Comment 64

[Bob Owen (:bobowen)]

(In reply to Olli Pettay [:smaug] from comment #59)
> Comment on attachment 8809366 [details] [diff] [review]
> Part 7: Create browsing context with no opener if URI needs different process
> 
> >>+NS_IMETHODIMP nsContentTreeOwner::ShouldLoadURIInThisProcess(nsIURI *aURI,
> >+                                                             bool *_retval)
> Be consistent. * goes with the type and all arguments are in form aName

Fixed locally.

Comment 65

[Mike Hommey [:glandium]]

compiler_metrics_num_constructors doesn't report individual static constructors, because there aren't such things. The compiler actually only creates one per source file. So if there's already a static constructor in the same file for something else, the number won't change.

Comment 66

[Bob Owen (:bobowen)]

Attachment: Part 8: Create separate content process for file:// URIs

Thanks again to everyone for the reviews.

Final try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ebc60858b6e1f5ef127cb07bdddcf75032dea24

Filed bug 1317921, to investigate what we need to do for nested URIs and make those changes.

(In reply to Olli Pettay [:smaug] from comment #60)
> Comment on attachment 8809369 [details] [diff] [review]
> Part 8: Create separate content process for file:// URIs

> >+pref("browser.tabs.remote.separateFile", false);
> >+#endif
> 
> tiny bit odd pref name. Couldn't guess from the name what it means.
> But don't have good suggestions.

Changed to the more descriptive separateFileUriProcess and added a comment.

> > nsDataHashtable<nsStringHashKey, ContentParent*>* ContentParent::sAppContentParents;
> >-nsTArray<ContentParent*>* ContentParent::sNonAppContentParents;
> >-nsTArray<ContentParent*>* ContentParent::sLargeAllocationContentParents;
> >+nsClassHashtable<nsStringHashKey, nsTArray<ContentParent*>> ContentParent::sNonAppContentParents;
> So it is still unclear whether this type is right, or whether
> nsClassHashtable<nsStringHashKey, nsTArray<ContentParent*>>* should be used
> 

After glandium's comment and talking on IRC I changed this to a pointer.

> >+    nsTArray<ContentParent*>* contentParents =
> >+      sNonAppContentParents.Get(mRemoteType);
> >+    if (contentParents) {
> >+      contentParents->RemoveElement(this);
> >+      if (contentParents->IsEmpty()) {
> >+        sNonAppContentParents.Remove(mRemoteType);
> >       }
> So if sNonAppContentParents can't be non-pointer static variable, it should
> be deleted somewhere here

sNonAppContentParents now deleted when empty.

(In reply to :Gijs Kruitbosch from comment #53)
> Comment on attachment 8809369 [details] [diff] [review]
> Part 8: Create separate content process for file:// URIs

> > I've moved the large allocation setting to all.js.
> > Do you think the separate file process one should move as well?
> 
> Speaking only for myself, yes.

Done.

> ::: browser/modules/E10SUtils.jsm
> @@ +72,5 @@
> >        let module = getAboutModule(url);
> >        // If the module doesn't exist then an error page will be loading, that
> >        // should be ok to load in any process
> >        if (!module) {
> >          return aPreferredRemoteType;
> 
> So now, if I have a non-about-blank about: URI that can be loaded in a
> remote process and that we're trying to load in a browser that's currently
> using the file process, we will use the file process? I think that's only
> correct for about:blank, and we should probably specialcase it in here.

Just special-cased about:blank now.
Other about: pages and top level chrome:// loads will be in the default remote type (web) if loading remotely.

Comment 67

[Bob Owen (:bobowen)]

Comment on attachment 8811169 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs

Carrying ...
r=gabor from comment 36
r=gijs from comment 53
r=smaug from comment 60

Comment 68

[Bob Owen (:bobowen)]

Attachment: Part 7: Create browsing context with no opener if URI needs different process

I was hoping to land this yesterday, but got wiped by ehsan and then kanru.

Mostly tedious rebasing, but this patch required a bit more, so I'd appreciate it if you would cast your eye over it again.

Comment 69

[Bob Owen (:bobowen)]

Hopefully final try push to check my rebasing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf98e80ac47fe161765aaaf9ebe17f28b602218

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/960112fbae78
Part 1: Fix call to _openURIInNewTab in browser.js to take a URI referrer not a string. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cffb4645bc0
Part 2: Add a remote type property and use it to drive the process switching in frontend code. r=gijs, r=jryans, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/a236e690e2f6
Part 3: Add remote type parameter to forceInitialBrowserRemote. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b1290aa7ca
Part 4: Fix test to allow for separate file content process. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/514e9bc44723
Part 5: Fix tests to allow for window.open to return null. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a72c4919371
Part 6: Send remote type down to child. r=gijs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f768dac3f7a9
Part 7: Create browsing context with no opener if URI needs different process. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c933acd03b
Part 8: Create separate content process for file:// URIs. r=gabor, r=gijs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c68edf3788
Part 9: Ensure file read permissions for file content process on Windows. r=jimm, r=jld

Comment 71

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed this out for T-e10s(o) permafailure:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9358470c0416e01e234ebbf43f39d34904130d4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcdd705d184065be7bc5809d9f606579ec19e83
https://hg.mozilla.org/integration/mozilla-inbound/rev/7642558a7ba1ed52dd59088d5060850bd8907b38
https://hg.mozilla.org/integration/mozilla-inbound/rev/b028d9526e825a2d0f7090cba46de7d60eba8521
https://hg.mozilla.org/integration/mozilla-inbound/rev/68de7c76d5d30b86dff3c408e2876fa8e366b2fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/68de7c76d5d30b86dff3c408e2876fa8e366b2fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ebd5dbf269914c8aefbdc780020f8ee8efd6fb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d62ab46e2854b519ce7815a97f615e9f9e25731
https://hg.mozilla.org/integration/mozilla-inbound/rev/039ddd3834a748a27d9a083fb58fa96669604538
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3dbc507117517a3569a4b2448973aacad0ae40

Push with first failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a5c68edf37887818d25a162d7b8f0bf6d44a73dc

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39383216&repo=mozilla-inbound
09:27:21     INFO -  Running cycle 1/1 for a11yr test...
09:27:21     INFO -  TEST-INFO | started process 3752 (C:\slave\test\build\application\firefox\firefox -profile c:\users\cltbld~1.t-w\appdata\local\temp\tmpqof9ha\profile -tp file:\C:\slave\test\build\tests\talos\talos\tests\a11y\a11y.manifest.develop -tpchrome -tpmozafterpaint -tpnoisy -tpcycles 1 -tppagecycles 25)
09:27:22     INFO -  PROCESS | 3752 | [GFX1-]: Invalid size in UpdateRenderTarget Size(124,0), 0
09:27:33     INFO -  PROCESS | 3752 | RSS: Main: 167276544
09:27:33     INFO -  PROCESS | 3752 |
09:28:23     INFO -  PROCESS | 3752 | 1479403703106	addons.productaddons	ERROR	Request failed certificate checks: [Exception... "SSL is required and URI scheme is not https."  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/CertUtils.jsm :: checkCert :: line 145"  data: no]
10:27:21     INFO -  Terminating psutil.Process(pid=3752, name=u'firefox.exe')
10:27:22     INFO -  TEST-UNEXPECTED-ERROR | a11yr | psutil.NoSuchProcess process no longer exists (pid=3752, name=u'firefox.exe')
10:27:22    ERROR -  Traceback (most recent call last):
10:27:22     INFO -    File "C:\slave\test\build\tests\talos\talos\run_tests.py", line 202, in run_tests
10:27:22     INFO -      talos_results.add(mytest.runTest(browser_config, test))
10:27:22     INFO -    File "C:\slave\test\build\tests\talos\talos\ttest.py", line 70, in runTest
10:27:22     INFO -      return self._runTest(browser_config, test_config, setup)
10:27:22     INFO -    File "C:\slave\test\build\tests\talos\talos\ttest.py", line 174, in _runTest
10:27:22     INFO -      if counter_management else None),
10:27:22     INFO -    File "C:\slave\test\build\tests\talos\talos\talos_process.py", line 132, in run_browser
10:27:22     INFO -      return_code = context.kill_process()
10:27:22     INFO -    File "C:\slave\test\build\tests\talos\talos\talos_process.py", line 37, in kill_process
10:27:22     INFO -      self.process.terminate()
10:27:22     INFO -    File "C:\slave\test\build\venv\lib\site-packages\psutil\__init__.py", line 299, in wrapper
10:27:22     INFO -      raise NoSuchProcess(self.pid, self._name)
10:27:22     INFO -  NoSuchProcess: psutil.NoSuchProcess process no longer exists (pid=3752, name=u'firefox.exe')
10:27:22     INFO -  TEST-INFO took 3611623ms

Comment 72

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a32fd177a4
Backed out changeset 960112fbae78 for T-e10s(o) permafail: Followup to fix syntax error. r=eslint-fix

Comment 73

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This might be the important line from the log linked in comment #71:

09:27:20 ERROR - PROCESS | 724 | JavaScript error: chrome://browser/content/tabbrowser.xml, line 3201: TypeError: this.tabs is undefined

Comment 74

[Bob Owen (:bobowen)]

(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #73)
> This might be the important line from the log linked in comment #71:
> 
> 09:27:20 ERROR - PROCESS | 724 | JavaScript error:
> chrome://browser/content/tabbrowser.xml, line 3201: TypeError: this.tabs is
> undefined

That probably needs looking into, but it was an existing error.

Part of the problem was that I removed the code allowing data: URIs in any remote process, which I'd forgotten we currently need for talos as well as a different test (which I fixed by just using about:blank).

But even if I add that back in, I'm now getting a different crash, which doesn't happen locally:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3fec503c838a33fd0f0df8ee18f9e770376da88

Comment 75

[Bob Owen (:bobowen)]

OK I found that problem, it was because I didn't have the check in ContentParent::NotifyTabDestroyed for processesToKeepAlive > 0.
This meant that it didn't shut done the child properly, but we'd already kicked of the KillHard timer.

Although I think there is something wrong here, because we've already gone through MarkAsDead at that point the count in sBrowserContentParents has already dropped.
I need to look into any other routes into that function, but we need to modify the check somehow or remove it.

... and it's still failing later in the job:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73e01b11bd34c290a465d9b1557111e665772149&selectedJob=31517787

Comment 76

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #75)

> ... and it's still failing later in the job:

... and it looks like that was caused by my debug logging :-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e131229d3f66ed2cdb17588e1ac823b398cd2641

Comment 77

[Bob Owen (:bobowen)]

Attachment: Part 8: Create separate content process for file:// URIs

Gabor - like I said in comment 75 I think that the checks for when we keep alive the process, need to check if we've already been marked as dead.

It looks like we can sometimes end up in NotifyTabDestroyed, without this having happened, so I think we do still need the check there and we can't remove it altogether.
The checks were getting a little confusing, so I also moved them out into their own private function.

Would you mind just checking that part of this patch again.

Comment 78

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=762d7474733a5832a5a18aea4ac10fa33fb57c89

Comment 79

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8812823 [details] [diff] [review]
Part 8: Create separate content process for file:// URIs

Review of attachment 8812823 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +2862,5 @@
>  
>  pref("dom.ipc.processCount", 1);
>  
> +// Override default dom.ipc.processCount for some remote content process types.
> +pref("dom.ipc.processCount.webLargeAllocation", 2);

I'm not sure where did this come from...

Comment 80

[Bob Owen (:bobowen)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #79)
> Comment on attachment 8812823 [details] [diff] [review]
> Part 8: Create separate content process for file:// URIs

> > +// Override default dom.ipc.processCount for some remote content process types.
> > +pref("dom.ipc.processCount.webLargeAllocation", 2);
> 
> I'm not sure where did this come from...

This was because I made the setting of the process count more generic, so I moved the default for the large allocation type into prefs. It was from the following line before:

 maxContentParents = Preferences::GetInt("dom.ipc.dedicatedProcessCount", 2);

Comment 81

[Gabor Krizsanits (INACTIVE)]

(In reply to Bob Owen (:bobowen) from comment #80)
>  maxContentParents = Preferences::GetInt("dom.ipc.dedicatedProcessCount", 2);

Ah thanks, I knew I've seen this somewhere but the old name tricked me and could not find it :)

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e793767cb441
Part 1: Fix call to _openURIInNewTab in browser.js to take a URI referrer not a string. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b26ae9afaea
Part 2: Add a remote type property and use it to drive the process switching in frontend code. r=gijs, r=jryans, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec84ee6acb88
Part 3: Add remote type parameter to forceInitialBrowserRemote. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc106b54960
Part 4: Fix test to allow for separate file content process. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/17ac392560a7
Part 5: Fix tests to allow for window.open to return null. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/360c780c0a95
Part 6: Send remote type down to child. r=gijs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ff074e9145
Part 7: Create browsing context with no opener if URI needs different process. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7569652090
Part 8: Create separate content process for file:// URIs. r=gabor, r=gijs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/11a036eafea2
Part 9: Ensure file read permissions for file content process on Windows. r=jimm, r=jld

Comment 83

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for T-e10s(o) failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d26c678753a4f198649a3a0996cd3a8c9db8ed4
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbaedc341b7c6da5ed780fe657ca80445a814c29
https://hg.mozilla.org/integration/mozilla-inbound/rev/feadf85d42ecbf5ab59d36fc7e86beb7fde54a6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8e1019ebff3a7fccab860f338dc83930f6b6a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2abf973bfdca422d9b38de8005c37446d47c4f35
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c10bb93818ff46a39a6b4515f89a04d94bfb423
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c2a78d4a8c38a86bfff8e58255c7b096a6fdf53
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f172e4d02300d51065d1d8dc985bbd8c4b00d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed16e234d2ebf3372564600ff8cd5f3c7a83f3c

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=11a036eafea224ab9ff1fde88f72ab75768be98c
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39718534&repo=mozilla-inbound

07:36:51     INFO -  TEST-START | a11yr
07:36:51     INFO -  operating with platform_type : w8_
07:36:51     INFO -  Initialising browser for a11yr test...
07:36:51     INFO -  TEST-INFO | started process 372 (C:\slave\test\build\application\firefox\firefox -profile c:\users\cltbld~1.t-w\appdata\local\temp\tmpeybodt\profile http://localhost:49290/getInfo.html)
07:36:59     INFO -  PROCESS | 372 | __metrics	Screen width/height:1600/1200
07:36:59     INFO -  PROCESS | 372 | 	colorDepth:24
07:36:59     INFO -  PROCESS | 372 | 	Browser inner width/height: 1010/674
07:36:59     INFO -  PROCESS | 372 | __metrics
07:37:00     INFO -  PROCESS | 372 | [GPU 432] WARNING: pipe error: 109: file c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346
07:37:00    ERROR -  PROCESS | 372 | JavaScript error: chrome://browser/content/tabbrowser.xml, line 3220: TypeError: this.tabs is undefined
07:37:00     INFO -  PROCESS | 372 | [GPU 432] WARNING: pipe error: 109: file c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346
07:37:00     INFO -  PROCESS | 372 | [Child 1372] WARNING: pipe error: 109: file c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346
07:37:00     INFO -  PROCESS | 372 | [Child 1372] WARNING: pipe error: 109: file c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346
07:37:00     INFO -  PROCESS | 372 | [GPU 432] WARNING: pipe error: 109: file c:/builds/moz2_slave/m-in-w64-000000000000000000000/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346
07:37:01     INFO -  TEST-INFO | 372: exit 0
07:37:01     INFO -  Browser initialized.
07:37:01     INFO -  Running cycle 1/1 for a11yr test...
07:37:01     INFO -  TEST-INFO | started process 2668 (C:\slave\test\build\application\firefox\firefox -profile c:\users\cltbld~1.t-w\appdata\local\temp\tmpeybodt\profile -tp file:\C:\slave\test\build\tests\talos\talos\tests\a11y\a11y.manifest.develop -tpchrome -tpmozafterpaint -tpnoisy -tpcycles 1 -tppagecycles 25)
07:37:02     INFO -  PROCESS | 2668 | [GFX1-]: Invalid size in UpdateRenderTarget Size(124,0), 0
07:37:13     INFO -  PROCESS | 2668 | RSS: Main: 164638720
07:37:13     INFO -  PROCESS | 2668 |
07:38:03     INFO -  PROCESS | 2668 | 1479915483019	addons.productaddons	ERROR	Request failed certificate checks: [Exception... "SSL is required and URI scheme is not https."  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/CertUtils.jsm :: checkCert :: line 145"  data: no]
08:37:02     INFO -  TEST-UNEXPECTED-ERROR | a11yr | timeout
08:37:02    ERROR -  Traceback (most recent call last):
08:37:02     INFO -    File "C:\slave\test\build\tests\talos\talos\run_tests.py", line 202, in run_tests
08:37:02     INFO -      talos_results.add(mytest.runTest(browser_config, test))
08:37:02     INFO -    File "C:\slave\test\build\tests\talos\talos\ttest.py", line 70, in runTest
08:37:02     INFO -      return self._runTest(browser_config, test_config, setup)
08:37:02     INFO -    File "C:\slave\test\build\tests\talos\talos\ttest.py", line 174, in _runTest
08:37:02     INFO -      if counter_management else None),
08:37:02     INFO -    File "C:\slave\test\build\tests\talos\talos\talos_process.py", line 117, in run_browser
08:37:02     INFO -      raise TalosError("timeout")
08:37:02     INFO -  TalosError: timeout

Comment 84

[Gregory Szorc [:gps]]

And this also significantly increased the number of constructors metric: https://treeherder.mozilla.org/perf.html#/alerts?id=4321

Comment 85

[Bob Owen (:bobowen)]

(In reply to Gregory Szorc [:gps] from comment #84)
> And this also significantly increased the number of constructors metric:
> https://treeherder.mozilla.org/perf.html#/alerts?id=4321

Thanks, looks like that was from using static NS_NAMED_LITERAL_STRINGs, I've changed this to use #defined literal strings, which seems to be how this is dealt with elsewhere.

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #83)
> Backed out for T-e10s(o) failures:

RyanVM pointed out that this is almost certainly from the fact that I've changed a talos addon file without re-packaging and signing.
On try we don't check the signatures apparently.

Here's a new try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eee1ff54c21b0f958063a0c276320bd764cf2954

And one that I think does check the talos signatures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b0683fecbe7984d511b4366695e9171be73b1bb

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e3bf665a8e
Part 1: Fix call to _openURIInNewTab in browser.js to take a URI referrer not a string. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/9245c2950d55
Part 2: Add a remote type property and use it to drive the process switching in frontend code. r=gijs, r=jryans, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb78de9e86b
Part 3: Add remote type parameter to forceInitialBrowserRemote. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0845271a62
Part 4: Fix test to allow for separate file content process. r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e9ac4bb5ca
Part 5: Fix tests to allow for window.open to return null. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c21c2ae2c8
Part 6: Send remote type down to child. r=gijs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fee9d778b2ba
Part 7: Create browsing context with no opener if URI needs different process. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6ea50bfb38
Part 8: Create separate content process for file:// URIs. r=gabor, r=gijs, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0637dd270ef1
Part 9: Ensure file read permissions for file content process on Windows. r=jimm, r=jld

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19e3bf665a8e
https://hg.mozilla.org/mozilla-central/rev/9245c2950d55
https://hg.mozilla.org/mozilla-central/rev/abb78de9e86b
https://hg.mozilla.org/mozilla-central/rev/bc0845271a62
https://hg.mozilla.org/mozilla-central/rev/19e9ac4bb5ca
https://hg.mozilla.org/mozilla-central/rev/a5c21c2ae2c8
https://hg.mozilla.org/mozilla-central/rev/fee9d778b2ba
https://hg.mozilla.org/mozilla-central/rev/fe6ea50bfb38
https://hg.mozilla.org/mozilla-central/rev/0637dd270ef1