1328829 - Can not open a local HTML file in a view-source tab

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect, P2)

Description

[u459114]

Repro steps:
Nightly 53.0a1 (2017-01-04)
1. Access a web site(yahoo, google, or whatever)
2. Right click on the page and select "view page source"
3. In that new created tab, press ctrl+o(or command+o on Mac) to open file explorer(or Finder on Mac), open any local html file.

Expect result:
Load a local html file correctly.

Actual result:
No content loaded, the title on the tab keep blinking

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49228a69b071bc200360aa43845b42b996759479&tochange=0637dd270ef14763921d3099b6f6d5780fa702f6

Regressed by: Bug 1147911

Comment 2

[Bob Owen (:bobowen)]

Sounds similar to other issues I've had, thanks.

Dealing with release issues at the moment, I'll add this to our next sandbox milestones, so should get to this fairly soon.

Comment 3

[Astley Chen (inactive)]

[Tracking Requested - why for this release]: Function failure.

Comment 4

[Bob Owen (:bobowen)]

The file content process is only enabled in Nightly at the moment, so I don't think we need to track this.

cjku - can you test this with the following set please:
  browser.tabs.remote.separateFileUriProcess=false

Comment 5

[u459114]

(In reply to Bob Owen (:bobowen) from comment #4)
> The file content process is only enabled in Nightly at the moment, so I
> don't think we need to track this.
> 
> cjku - can you test this with the following set please:
>   browser.tabs.remote.separateFileUriProcess=false

Hmm, I can not see this problem after set browser.tabs.remote.separateFileUriProcess=false or disable e10s

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Never mind, looks like this is nightly only so we don't need to track this for now.

Comment 7

[Bob Owen (:bobowen)]

This is happening because we're still setting the related browser when switching remote types, which means we end up in the same process and the load fails.

Comment 8

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71af65d3112bd57943a6743c14518799a31c2c2e

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 1: Don't copy related browser when switching between different remote types

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 2: Check that related browser is not copied when switching remote type

Comment 11

[:Gijs (he/him)]

Comment on attachment 8830435 [details] [diff] [review]
Part 1: Don't copy related browser when switching between different remote types

Not setting the related browser will break inserting the tab right next to the current selected tab, and also won't use the current userContextId. Those seem like sensible things to keep. Can we instead avoid using the data from a related tab to make decisions for file:/// URIs or unbreak this some other way, while keeping the browsers related?

Comment 12

[:Gijs (he/him)]

Comment on attachment 8830436 [details] [diff] [review]
Part 2: Check that related browser is not copied when switching remote type

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

Can we rename this file to something like browser_allow_process_switches_despite_related_browser.js ?

Comment 13

[Bob Owen (:bobowen)]

(In reply to :Gijs from comment #11)
> Comment on attachment 8830435 [details] [diff] [review]
> Part 1: Don't copy related browser when switching between different remote
> types
> 
> Not setting the related browser will break inserting the tab right next to
> the current selected tab, and also won't use the current userContextId.
> Those seem like sensible things to keep. Can we instead avoid using the data
> from a related tab to make decisions for file:/// URIs or unbreak this some
> other way, while keeping the browsers related?

But that all happens when the browser is first created, here we're switching remote types on an existing browser. So, the tab stays where it is and still has the correct userContextId.

Once, you are switching remote types, the previously related browser (used for view-source and print preview I believe at the moment), probably makes little sense.
Certainly, further down, it trumps everything else in determining which process we attempt the load in, but at that point in the code we don't know that we're switching remote types.

We could possibly put something in ContentParent::CreateBrowser to pull out the remote type earlier and reject the aOpenerContentParent, if the remote type doesn't match.
I'm not sure if that doesn't feel like more of a kludge to me, what do you think?
Maybe we should do both.

(In reply to :Gijs from comment #12)
> Comment on attachment 8830436 [details] [diff] [review]
> Part 2: Check that related browser is not copied when switching remote type
> 
> Review of attachment 8830436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we rename this file to something like
> browser_allow_process_switches_despite_related_browser.js ?

Absolutely, I didn't like my name at all. :-)
Naming is often the hardest part of software.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8830435 [details] [diff] [review]
Part 1: Don't copy related browser when switching between different remote types

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

(In reply to Bob Owen (:bobowen) from comment #13)
> (In reply to :Gijs from comment #11)
> > Comment on attachment 8830435 [details] [diff] [review]
> > Part 1: Don't copy related browser when switching between different remote
> > types
> > 
> > Not setting the related browser will break inserting the tab right next to
> > the current selected tab, and also won't use the current userContextId.
> > Those seem like sensible things to keep. Can we instead avoid using the data
> > from a related tab to make decisions for file:/// URIs or unbreak this some
> > other way, while keeping the browsers related?
> 
> But that all happens when the browser is first created, here we're switching
> remote types on an existing browser. So, the tab stays where it is and still
> has the correct userContextId.
> 
> Once, you are switching remote types, the previously related browser (used
> for view-source and print preview I believe at the moment), probably makes
> little sense.
> Certainly, further down, it trumps everything else in determining which
> process we attempt the load in, but at that point in the code we don't know
> that we're switching remote types.

Oh. Today I learn that... we have something called "related tabs" and "related browsers", and for what I'm sure are completely rational reasons, the two are not linked or in any way the same. Sigh.

It looks like the relatedBrowser stuff was added by :mrbkap in bug 1165309.

> We could possibly put something in ContentParent::CreateBrowser to pull out
> the remote type earlier and reject the aOpenerContentParent, if the remote
> type doesn't match.
> I'm not sure if that doesn't feel like more of a kludge to me, what do you
> think?
> Maybe we should do both.


I don't know, because I'm not familiar with these mechanics. I'm going to redirect the review to :mrbkap. :-)

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gijs from comment #14)
> Oh. Today I learn that... we have something called "related tabs" and
> "related browsers", and for what I'm sure are completely rational reasons,
> the two are not linked or in any way the same. Sigh.

I didn't even think of relatedTab when I coined relatedBrowser :-/.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8830435 [details] [diff] [review]
Part 1: Don't copy related browser when switching between different remote types

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

I prefer keeping this logic here and out of ContentParent but don't feel super strongly about it.

::: browser/base/content/tabbrowser.xml
@@ +1759,5 @@
> +            // turns this normal property into a field. When switching remote
> +            // type copying related browser would stop the switch and the
> +            // previously related browser will no longer be relevant.
> +            if (!aShouldBeRemote || currentRemoteType == aOptions.remoteType) {
> +              aBrowser.relatedBrowser = relatedBrowser;

This looks good. I was thinking of asking if we could maybe clear relatedBrowser on all navigations, but I realized that would probably break some use-cases of e.g. clicking on a link in the view source page and going back. Only clearing it when it's about to change is a reasonable compromise.

Comment 17

[:Gijs (he/him)]

(In reply to Blake Kaplan (:mrbkap) from comment #15)
> (In reply to :Gijs from comment #14)
> > Oh. Today I learn that... we have something called "related tabs" and
> > "related browsers", and for what I'm sure are completely rational reasons,
> > the two are not linked or in any way the same. Sigh.
> 
> I didn't even think of relatedTab when I coined relatedBrowser :-/.

Yeah, naming things is hard. Should we file a follow-up bug to rename one of the two?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b814b21c1f54
Part 1: Don't copy related browser when switching between different remote types. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/64495294736d
Part 2: Check that related browser is not copied when switching remote type. r=gijs

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b814b21c1f54
https://hg.mozilla.org/mozilla-central/rev/64495294736d

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Gijs from comment #17)
> Yeah, naming things is hard. Should we file a follow-up bug to rename one of
> the two?

I filed bug 1335281.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Does this need to be uplifted to Aurora?

Comment 22

[Bob Owen (:bobowen)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
> Does this need to be uplifted to Aurora?

No, we don't need an uplift because the separate file content process is only preffed on in Nightly and comment 5 confirmed that we don't see the issue when preffed off.

Comment 23

[Simona Badau, Desktop QA]

Reproduced the initial issue on Nightly 53.0a1 (Build ID: 20170117030218) on Windows 10 x64.

Verified as fixed using the latest Nightly 55.0a1 - Build ID: 20170323030203 and latest Aurora 54.0a2 - Build ID:  20170323004002 (having the preference browser.tabs.remote.separateFileUriProcess=true) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.12