1304492 - remote-browser's contentPrincipal initially lies about originAttributes

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[:Gijs (he/him)]

Normally, browser.contentPrincipal gives you, um, the principal that corresponds to the content in the browser.

For remote (e10s) browsers this is a bit tricky because the content is not in the same process. So we cope with it by message-passing the content principal back all the time. The problem is what to do initially - consumer code does not like contentPrincipal to be null. 

We know that for a new browser we basically just load about:blank and then maybe later something else. So we intentionally create a null principal initially:

https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/toolkit/content/widgets/remote-browser.xml#374

Unfortunately, we create this null principal with originAttributes set to {}.

This is wrong, because the browser might be in a private browsing window or a container (have non-0 userContextId), and now I'm going to make bad assumptions about whether the content window is private or in a container. Bad assumptions are bad.

So this is unfortunate. How to fix? Well, remote-browser.xml lives in the parent and can ask the parent if the chrome window is in private browsing and assume the content windows will be consistent. It can also inspect its own userContextId attribute to see if it's supposed to be using a userContextId (the XUL code will not let you change that once set, so that should be OK). So my gut instinct is to just read that stuff and make some assumptions, and not care too much about the other properties for now...

Are there problems with that approach? Should we do something else?

(needinfo-spam to validate the idea before I go and implement something)

Comment 1

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

I agree that the current situation of doing the Wrong Thing for the principal of the about:blank page is not great. Unfortunately, I don't know enough about how the code works to know if what you're planning to do will blow up in your face.

If you do change the way we do it such that we try to guess the right origin attributes, is there any chance you could add some assertions to the code which receives the new principal from content which checks that the important tab-constant attributes (like private browsing and userContextId) match? That way we don't change one code path and forget to change others.

Comment 2

[:Gijs (he/him)]

(In reply to Michael Layzell [:mystor] from comment #1)
> If you do change the way we do it such that we try to guess the right origin
> attributes, is there any chance you could add some assertions to the code
> which receives the new principal from content which checks that the
> important tab-constant attributes (like private browsing and userContextId)
> match? That way we don't change one code path and forget to change others.

It's all JS, so "assertion" in the strict sense, no. I could add telemetry, I guess, but that still doesn't really tell us why or how we missed anything...

Comment 3

[:Gijs (he/him)]

(Clearing assignee because unless we're sure that this approach is sane I don't know that I could write the patch.)

Comment 4

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

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Michael Layzell [:mystor] from comment #1)
> > If you do change the way we do it such that we try to guess the right origin
> > attributes, is there any chance you could add some assertions to the code
> > which receives the new principal from content which checks that the
> > important tab-constant attributes (like private browsing and userContextId)
> > match? That way we don't change one code path and forget to change others.
> 
> It's all JS, so "assertion" in the strict sense, no. I could add telemetry,
> I guess, but that still doesn't really tell us why or how we missed
> anything...

I mostly want to make sure that tests fail if we screw this up, I don't really care how we do it. That's all.

Comment 5

[(no longer active)]

Hmm, sorry if I don't understand the background for this decision, but it seems to me that what we do for contentPrincipal in e10s versus non-e10s is different, and we should do the exact same thing in both cases.

In the non-e10s case, if we end up calling nsDocShell::EnsureContentViewer() (which roughly matches the case in the e10s code path where we haven't loaded the document yet), we try to get the principal using GetInheritedPrincipal().  That however shouldn't let you inherit the parent docshell's principal as that is a chrome docshell.  So |principal| here <http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/docshell/base/nsDocShell.cpp#7963> should be null.  This calls nsContentDLF::CreateBlankDocument() which calls nsDocument::ResetToURI() with a null principal.  That takes us to this code <http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/dom/base/nsDocument.cpp#2137> which creates the principal in the non-e10s case.

Can't we simply call nsIScriptSecurityManager.getLoadContextCodebasePrincipal() in the e10s case, passing "about:blank" and <https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/browser.xml#267>?

Comment 6

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari from comment #5)
> Hmm, sorry if I don't understand the background for this decision, but it
> seems to me that what we do for contentPrincipal in e10s versus non-e10s is
> different, and we should do the exact same thing in both cases.

This doesn't make sense to me. The content principal is the principal of the document that is in the child process, so it isn't possible to do what the non-e10s case does (ie just return docShell.contentDocument.nodePrincipal).

> In the non-e10s case, if we end up calling nsDocShell::EnsureContentViewer()
> (which roughly matches the case in the e10s code path where we haven't
> loaded the document yet), we try to get the principal using
> GetInheritedPrincipal().  That however shouldn't let you inherit the parent
> docshell's principal as that is a chrome docshell.  So |principal| here
> <http://searchfox.org/mozilla-central/rev/
> 8910ca900f826a9b714607fd23bfa1b37a191eca/docshell/base/nsDocShell.cpp#7963>
> should be null.  This calls nsContentDLF::CreateBlankDocument() which calls
> nsDocument::ResetToURI() with a null principal.  That takes us to this code
> <http://searchfox.org/mozilla-central/rev/
> 8910ca900f826a9b714607fd23bfa1b37a191eca/dom/base/nsDocument.cpp#2137> which
> creates the principal in the non-e10s case.

I don't understand where you're getting this codepath/these codepaths from. In particular, I'm talking about:

https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/browser.xml#510-512
which in non-e10s just returns this.contentDocument.nodePrincipal, so it'll forward whatever the about:blank document's principal is;

and

https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/remote-browser.xml#170-172

which can't do that because e10s, so it uses a stored variable, which we initialize here:

https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/toolkit/content/widgets/remote-browser.xml#374

> Can't we simply call
> nsIScriptSecurityManager.getLoadContextCodebasePrincipal() in the e10s case,
> passing "about:blank" and
> <https://dxr.mozilla.org/mozilla-central/rev/
> 560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/browser.
> xml#267>?

Is there any guarantee that we can actually access the frameloader and its loadContext that early? The code you're pointing to seems to have to deal with a null frameloader... Also, it is my understanding that the docshell lives in the child process - are either of these CPOWs? Because that wouldn't be a good idea, either.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The frameloader lives in the chrome process.  It can have a docshell, or a tabparent, depending on whether the child lives in the content process.

So Ehsan's suggestion looks pretty plausible to me, at least as a thing to try.

Comment 8

[(no longer active)]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to :Ehsan Akhgari from comment #5)
> > Hmm, sorry if I don't understand the background for this decision, but it
> > seems to me that what we do for contentPrincipal in e10s versus non-e10s is
> > different, and we should do the exact same thing in both cases.
> 
> This doesn't make sense to me. The content principal is the principal of the
> document that is in the child process

Correct.

> so it isn't possible to do what the
> non-e10s case does (ie just return docShell.contentDocument.nodePrincipal).

It's totally possible as I tried to describe in comment 5!

> > In the non-e10s case, if we end up calling nsDocShell::EnsureContentViewer()
> > (which roughly matches the case in the e10s code path where we haven't
> > loaded the document yet), we try to get the principal using
> > GetInheritedPrincipal().  That however shouldn't let you inherit the parent
> > docshell's principal as that is a chrome docshell.  So |principal| here
> > <http://searchfox.org/mozilla-central/rev/
> > 8910ca900f826a9b714607fd23bfa1b37a191eca/docshell/base/nsDocShell.cpp#7963>
> > should be null.  This calls nsContentDLF::CreateBlankDocument() which calls
> > nsDocument::ResetToURI() with a null principal.  That takes us to this code
> > <http://searchfox.org/mozilla-central/rev/
> > 8910ca900f826a9b714607fd23bfa1b37a191eca/dom/base/nsDocument.cpp#2137> which
> > creates the principal in the non-e10s case.
> 
> I don't understand where you're getting this codepath/these codepaths from.

I'm describing how we create a principal for the about:blank document in the non-e10s case (and for that matter, for the actual about:blank document created in the child in the e10s case as well.)

> In particular, I'm talking about:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/browser.
> xml#510-512
> which in non-e10s just returns this.contentDocument.nodePrincipal, so it'll
> forward whatever the about:blank document's principal is;

And the about:blank's document's principal is calculated as I described in the quoted text above.  Therefore, |this.contentDocument.nodePrincipal| will return the principal created in <http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/dom/base/nsDocument.cpp#2137>.

> https://dxr.mozilla.org/mozilla-central/rev/
> 560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/remote-
> browser.xml#170-172
> 
> which can't do that because e10s, so it uses a stored variable, which we
> initialize here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/toolkit/content/widgets/remote-
> browser.xml#374

I understand, but the principal we're creating there is bogus, since we don't just create a null principal in the non-e10s case for the about:blank document.

What I'm suggesting is to fix the code in comment 374 to instead of creating the null principal like it does, make it use nsIScriptSecurityManager.getLoadContextCodebasePrincipal() to create a principal for about:blank with the correct OA.

> > Can't we simply call
> > nsIScriptSecurityManager.getLoadContextCodebasePrincipal() in the e10s case,
> > passing "about:blank" and
> > <https://dxr.mozilla.org/mozilla-central/rev/
> > 560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/toolkit/content/widgets/browser.
> > xml#267>?
> 
> Is there any guarantee that we can actually access the frameloader and its
> loadContext that early?

Yes.  These objects can definitely be used before the real content is loaded.

> The code you're pointing to seems to have to deal
> with a null frameloader...

IINM that case deals with the <xul:browser> element not being in the DOM tree.  nsXULElement::BindToTree() creates the frame loader if needed, and nsXULElement::UnbindFromTree() destroys it.

> Also, it is my understanding that the docshell
> lives in the child process - are either of these CPOWs? Because that
> wouldn't be a good idea, either.

As Boris said, the frameloader lives in the chrome process, and so does its load context object.  The load context object is IPDL serializable and can be sent to and accessed in the child process as well.

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1304492 - use frameloader's context to get contentPrincipal,

Review commit: https://reviewboard.mozilla.org/r/81108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81108/

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8794810 [details]
Bug 1304492 - use frameloader's context to get contentPrincipal,

https://reviewboard.mozilla.org/r/81108/#review79710

Tentative r+, but I suspect a try run is in order.

::: toolkit/content/widgets/remote-browser.xml
(Diff revision 1)
> -      <field name="_contentPrincipal">null</field>
> -

With this gone, I think you're going to want to remove [where it's set by RemoteWebProgress](http://searchfox.org/mozilla-central/rev/05ed82e50b45df5aa5a8fad219dece1b56757261/toolkit/modules/RemoteWebProgress.jsm#254).

Comment 11

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #10)
> Comment on attachment 8794810 [details]
> Bug 1304492 - use frameloader's context to get contentPrincipal,
> 
> https://reviewboard.mozilla.org/r/81108/#review79710
> 
> Tentative r+, but I suspect a try run is in order.


Uh, yes, forgot about my link here: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=03218939652c

... which, I think, means we can only do this initially when we know there's no inherited principal, because for URI schemes where principals are inherited, just looking at currentURI and generating a codebase principal is a recipe for problems, e.g. the load here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03218939652c&selectedJob=28058289 .

So, r- from try, I think.

We can still do this for the initial principal, just not all the time.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8794810 [details]
Bug 1304492 - use frameloader's context to get contentPrincipal,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81108/diff/1-2/

Comment 13

[:Gijs (he/him)]

Mike, can you re-review this? I tried to poke mozreview into doing this, but it doesn't seem possible...

Comment 14

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=360d70f6cc48

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8794810 [details]
Bug 1304492 - use frameloader's context to get contentPrincipal,

https://reviewboard.mozilla.org/r/81108/#review79762

Yeah, this looks good. Thanks Gijs!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19e3f7722664
use frameloader's context to get contentPrincipal, r=mconley

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19e3f7722664

Comment 18

[(no longer active)]

Thank Gijs for the fix!