1343184 - Add pref to allow http content linked from file:// URI to load in file content process

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

Bug 1147911 changed Firefox so that top level loads of file:// URI pages are done in separate file content processes.

If the file:// page frames http pages they are also loaded in the file content process, due to current cross-process referencing limitations.

However if a file:// page causes a top level http load (via link or JS) then that gets loaded in a normal content process.
Again because of cross process referencing limitations, in this case the window.opener is not set and window.open returns null.

There is some concern that this might cause compatibility issues, particularly with corporate users, so we want to add a pref that allows http content linked from file pages to load in the same process even when it is a top level load.

Comment 1

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #0)
> There is some concern that this might cause compatibility issues,
> particularly with corporate users, so we want to add a pref that allows http
> content linked from file pages to load in the same process even when it is a
> top level load.

What does this gain us over the extant pref to just disable the separate file process, and is that really sufficient gain to justify the additional cost + risk in code complexity, testing, etc.?

Comment 2

[Bob Owen (:bobowen)]

(In reply to :Gijs from comment #1)
> (In reply to Bob Owen (:bobowen) from comment #0)
> > There is some concern that this might cause compatibility issues,
> > particularly with corporate users, so we want to add a pref that allows http
> > content linked from file pages to load in the same process even when it is a
> > top level load.
> 
> What does this gain us over the extant pref to just disable the separate
> file process, and is that really sufficient gain to justify the additional
> cost + risk in code complexity, testing, etc.?

If we disable the file content process then we can't remove read access from the normal content process.

The change is very simple, I already have a patch.
I need to write a test, should be ready tomorrow.

Comment 3

[Bob Owen (:bobowen)]

Try push with the fix from bug 1345807 included (I've fixed the lint issues locally):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4c17a736362f90ea8287fb6ed43e4b9e553d0d

Another try push with the new pref flipped on:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3de5ee9d7fe380bc4e28f947049fe3142c65ccd9

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 1: Add pref to allow linked web content to load in file content process

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 2: Check that related web content loads in file content process when pref flipped

Comment 6

[:Gijs (he/him)]

Comment on attachment 8845401 [details] [diff] [review]
Part 2: Check that related web content loads in file content process when pref flipped

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

::: browser/base/content/test/tabs/browser.ini
@@ +8,5 @@
>  skip-if = !e10s # Tab spinner is e10s only.
>  [browser_tabSwitchPrintPreview.js]
>  skip-if = os == 'mac'
>  [browser_navigatePinnedTab.js]
> +[browser_new_web_tab_in_file_process_pref.js]

I suppose this will work in non-e10s, but there's little point running it there, so you could use skip-if = !e10s

::: browser/base/content/test/tabs/browser_new_web_tab_in_file_process_pref.js
@@ +11,5 @@
> +  dir.append(TEST_FILE);
> +  const uriString = Services.io.newFileURI(dir).spec;
> +  let fileTab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, uriString);
> +  registerCleanupFunction(function* () {
> +    yield BrowserTestUtils.removeTab(fileTab);

Here too you could use withNewTab instead for slightly simpler code. :-)

@@ +18,5 @@
> +  // Set pref to allow linked web content in file URI process.
> +  Services.prefs.setBoolPref("browser.tabs.remote.allowLinkedWebInFileUriProcess", true);
> +  registerCleanupFunction(function() {
> +    Services.prefs.clearUserPref("browser.tabs.remote.allowLinkedWebInFileUriProcess");
> +  });

Nit: use:

yield SpecialPowers.pushPrefEnv({set: [["browser.tabs.remote.allowLinkedWebInFileUriProcess", true]]});

I actually expect that we should probably enable the separate file process this way, too, given that that's not guaranteed to be riding the trains yet.

Comment 7

[Bob Owen (:bobowen)]

(In reply to :Gijs from comment #6)
> Comment on attachment 8845401 [details] [diff] [review]
> Part 2: Check that related web content loads in file content process when
> pref flipped

> > +[browser_new_web_tab_in_file_process_pref.js]
> 
> I suppose this will work in non-e10s, but there's little point running it
> there, so you could use skip-if = !e10s

Yes, changed locally, thanks.
 
> Nit: use:
> 
> yield SpecialPowers.pushPrefEnv({set:
> [["browser.tabs.remote.allowLinkedWebInFileUriProcess", true]]});
> 
> I actually expect that we should probably enable the separate file process
> this way, too, given that that's not guaranteed to be riding the trains yet.

Yes again it only really makes sense with that pref set.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ccde656af1c
Part 1: Add pref to allow linked web content to load in file content process. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/0055f45d1315
Part 2: Check that related web content loads in file content process when pref flipped. r=Gijs

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c28111d13613
part 3: Follow-up to fix no-shadow lint issue. r=me

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ccde656af1c
https://hg.mozilla.org/mozilla-central/rev/0055f45d1315
https://hg.mozilla.org/mozilla-central/rev/c28111d13613

Comment 11

[Simona Badau, Desktop QA]

Reproduced the initial issue using Nightly 54.0a1 (Build ID: 20170301030202) on Windows 10 x64.
Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170523030206) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.