Closed Bug 1328829 Opened 4 years ago Closed 4 years ago

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

Categories

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

53 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- disabled
firefox54 --- verified
firefox55 --- verified

People

(Reporter: u459114, Assigned: bobowen)

References

Details

(Keywords: multiprocess, regression, Whiteboard: sbwc2, sblc3, sbmc2,[e10s-multi:M2])

Attachments

(2 files)

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
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.
Flags: needinfo?(bobowencode)
Whiteboard: sbwc2, sblc3, sbmc2,[e10s-multi:M2]
[Tracking Requested - why for this release]: Function failure.
Component: Untriaged → Security: Process Sandboxing
Priority: -- → P2
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
Flags: needinfo?(cku)
(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
Flags: needinfo?(cku)
Never mind, looks like this is nightly only so we don't need to track this for now.
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.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
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?
Flags: needinfo?(bobowencode)
Attachment #8830435 - Flags: review?(gijskruitbosch+bugs)
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 ?
Attachment #8830436 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
Flags: needinfo?(bobowencode) → needinfo?(gijskruitbosch+bugs)
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. :-)
Attachment #8830435 - Flags: review?(mrbkap)
Flags: needinfo?(gijskruitbosch+bugs)
(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 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.
Attachment #8830435 - Flags: review?(mrbkap) → review+
(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?
Flags: needinfo?(mrbkap)
Pushed by bobowencode@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/b814b21c1f54
https://hg.mozilla.org/mozilla-central/rev/64495294736d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(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.
Flags: needinfo?(mrbkap)
Depends on: 1335170
Does this need to be uplifted to Aurora?
Flags: needinfo?(bobowencode)
(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.
Flags: needinfo?(bobowencode)
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.