1191740 - Add OriginAttributes in TabContext

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

TabContext stores some information for the 'app', like appId, isInBrowserElement, and perhaps the owning mozIApplication.

For FxOS new security model we could expose principal for TabContext.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we can expose the principal, since multiple principals can run inside a child process.

It's not clear to me which properties we will allow to change in a childprocess and which ones we won't. But I think that for now, attaching an OriginAttributes is probably the way to go.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks, Jonas.
Change the subject to add originAttributes.

Comment 3

[Steven Englehardt [:englehardt]]

Attachment: 1191740-tabcontext.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed34414a4ca

I changed all the setters to require an originAttributes dictionary as an argument. I use the suffix string representation for ipc serialization, and store the OwnAppId / IsBrowser in originAttributes, while keeping containingAppId separate. New attributes should require no additional changes as I'll show in the WIP patch in Bug 1195881 (unless we want each attribute to have its own getter in TabContext).

Bobby: Are you the right person to review this?

Comment 4

[Steven Englehardt [:englehardt]]

Yoshi pointed out that I missed a few uses of TabContext in dom/ipc/ContentParent.cpp and uriloader/prefetch/OfflineCacheUpdateParent.cpp.

Two examples are: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp?case=true&from=dom/ipc/ContentParent.cpp#4962 and https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp?case=true&from=dom/ipc/ContentParent.cpp#1009. There are likely other as well. I'll do another search for things using TabContext and post an updated patch.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

also TabParent.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8652092 [details] [diff] [review]
1191740-tabcontext.patch

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

Seems on the right track, but I think if we play our Tetris carefully this code can get significantly simplified, and made less dependent on the specific OriginAttributes that exist today.

::: dom/base/nsFrameLoader.cpp
@@ +2226,5 @@
>  
>    MutableTabContext context;
>    nsCOMPtr<mozIApplication> ownApp = GetOwnApp();
>    nsCOMPtr<mozIApplication> containingApp = GetContainingApp();
> +  OriginAttributes attrs = OriginAttributes();

This looks wrong - don't you intent to

::: dom/ipc/PTabContext.ipdlh
@@ +68,5 @@
>  {
>    PopupIPCTabContext;
>    AppFrameIPCTabContext;
>    BrowserFrameIPCTabContext;
> +  NormalFrameIPCTabContext;

It seems like we should be able to get rid of this union, and just have everything pass OriginAttributes, along with a containing app id (which may be NO_APP_ID). Does that sound right?

::: dom/ipc/TabContext.cpp
@@ +182,5 @@
>    }
>  
>    mInitialized = true;
> +  mOriginAttributes = aOriginAttributes;
> +  mOriginAttributes.mAppId = ownAppId;

I think we should MOZ_RELEASE_ASSERT this and require callers to set the OriginAttributes up properly, here and elsewhere.

Once we do that, can we eliminate all the SetTabContextFor* methods, and let everything be determined by the OriginAttributes passed by the caller?

@@ +187,2 @@
>    mContainingAppId = containingAppId;
>    mOwnApp = aOwnApp;

This member seems redundant now, right? Seems like we should remove it.

::: dom/ipc/TabContext.h
@@ +107,5 @@
> +   * GetOriginAttributes() returns the OriginAttributes of this frame to the
> +   * caller. This is used to store any attribute associated with the frames
> +   * context, such as the AppId.
> +   */
> +  OriginAttributes GetOriginAttributes() const;

Nit: Make this:

OriginAttributes& OriginAttributesRef() const

@@ +132,5 @@
>     * given app.  Either or both apps may be null.
>     */
>    bool SetTabContextForAppFrame(mozIApplication* aOwnApp,
> +                                mozIApplication* aAppFrameOwnerApp,
> +                                OriginAttributes& aOriginAttributes);

Nit: Make these references const (occurs various places within the patch).

@@ +171,5 @@
>     */
>    uint32_t mContainingAppId;
>  
>    /**
> +   * OriginAttributes

Need a more descriptive comment here, i.e. OriginAttributes of the toplevel tab docshell.

Comment 7

[Steven Englehardt [:englehardt]]

Attachment: 1191740-tabcontext.patch

WIP Patch. See comments below.

Comment 8

[Steven Englehardt [:englehardt]]

(In reply to Bobby Holley (PTO through Sept 8) from comment #6)
> ::: dom/base/nsFrameLoader.cpp
> @@ +2226,5 @@
> >  
> >    MutableTabContext context;
> >    nsCOMPtr<mozIApplication> ownApp = GetOwnApp();
> >    nsCOMPtr<mozIApplication> containingApp = GetContainingApp();
> > +  OriginAttributes attrs = OriginAttributes();
> 
> This looks wrong - don't you intent to

I believe you forgot to finish this thought. Not sure what was incorrect here.

> ::: dom/ipc/PTabContext.ipdlh
> @@ +68,5 @@
> >  {
> >    PopupIPCTabContext;
> >    AppFrameIPCTabContext;
> >    BrowserFrameIPCTabContext;
> > +  NormalFrameIPCTabContext;
> 
> It seems like we should be able to get rid of this union, and just have
> everything pass OriginAttributes, along with a containing app id (which may
> be NO_APP_ID). Does that sound right?

I'm not sure we can get rid of it entirely. A TabContext may be created by the parent or by the child. The two pathways are ContentParent::CreateBrowserOrApp [1] and TabChild::ProvideWindowCommon [2]. See also the comment in [3].

ContentParent::CreateBrowserOrApp is called by nsFrameLoader::TryRemoteBrowser [4], which currently constructs the contexts as App, Browser, and Normal frames depending on the attributes. I’ve updated this to construct a single context with originAttributes. This is serialized to FrameIPCTabContext over the process boundary and the Child trusts the attributes and reconstructs the TabContext on the other end of the process boundary. 

TabChild::ProvideWindowCommon constructs PopupIPCTabContext, which contains the openerTabId and an indication if it is a browser element. The leads to several security checks in MaybeInvalidTabContext [5] before the attributes are pulled from the opener the TabContext constructed from them.

Thus it seems to me that it makes sense to keep two separate IPCTabContexts, one for Parent to Child which passes originAttributes and one for Child to Parent which stays as-is. We could just include the opener in FrameIPCTabContext and have it be null for parent to child communication, but I don't think that would make things clearer.

> ::: dom/ipc/TabContext.cpp
> @@ +182,5 @@
> >    }
> >  
> >    mInitialized = true;
> > +  mOriginAttributes = aOriginAttributes;
> > +  mOriginAttributes.mAppId = ownAppId;
> 
> I think we should MOZ_RELEASE_ASSERT this and require callers to set the
> OriginAttributes up properly, here and elsewhere.
> 
> Once we do that, can we eliminate all the SetTabContextFor* methods, and let
> everything be determined by the OriginAttributes passed by the caller?
> @@ +187,2 @@
> >    mContainingAppId = containingAppId;
> >    mOwnApp = aOwnApp;
> 
> This member seems redundant now, right? Seems like we should remove it.

In response to the previous two comments it definitely is possible to remove the various SetTabContextFor* methods. However, I'm not sure it makes sense to remove mOwnApp / mContainingApp. We can derive the App object from the AppId/ContainingAppId, but I imagine it might be better to just cache it for speed reasons as it is used in HasOwnApp, GetOwnApp. Thoughts? If this won't be a problem I can just update those methods to grab the App from the AppId as is done in [6].

Also, if I remove OwnApp as an argument, and just pass the ID, I will no longer have anything to assert (as suggested in the former comment)?

Finally, in my WIP patch above, I'm not sure how we should handle inheritance in the PopupTabContext case. See my comment in [7].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1170
[2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1164
[3] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#455
[4] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2244
[5] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabContext.cpp#247
[6] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabContext.cpp#323
[7] https://bugzilla.mozilla.org/attachment.cgi?id=8656850&action=diff#a/dom/ipc/TabContext.cpp_sec6

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8656850 [details] [diff] [review]
1191740-tabcontext.patch

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

Hey Steven,

This is really excellent work. I'm sorry for the delay on my end, I was PTO and then swamped with very time-sensitive stuff.

The one question I still have is on the appId piece. I think we want the OriginAttributes that we store on TabContext to be identical to the OriginAttributes that live on the Top-Level docshell of that tab, do you agree?

If so, I think we want mOriginAttributes.mAppId to correspond to the value of OwnOrContainingAppId(). If we have app X that creates a browser frame (or some other out-of-process iframe), the containing appId is X and the own appId is NO_APP_ID. In that situation, I believe that the principal of the top-level document should have appId=X. Paul, do I have that right?

::: dom/ipc/PTabContext.ipdlh
@@ +37,2 @@
>  
> +  // The ID of the app containing this app/browser frame.  May be NO_APP_ID.

I'd say "app/browser frame, if applicable."

::: dom/ipc/TabContext.cpp
@@ +261,5 @@
> +      // XXX-senglehardt: not sure if the inheritance is correct here. The
> +      // previous inheritance is described above. If we simply inherit
> +      // originAttirbutes in both cases, mAppId and mInBrowser should still be
> +      // set the same as before. It's not clear that we'll always want the rest
> +      // of the attributes to inherit as well.

I think tabs created via window.open should inherit OriginAttributes in the same way that iframes do. So what we really want here is to call InheritFromDocToChildDocshell, which will be added in bug 1191740, but hasn't landed yet.

If this lands first, copying the attributes is fine, but please add a comment and file a followup bug depending on bug 1191740 and blocking bug 1179985 such that we can fix this up.

Comment 10

[Bobby Holley (:bholley)]

Actually, switching the NI to Jonas, because I'm going to have him SR this patch anyway. But feel free to answer if you get to this first, paul. ;-)

Comment 11

[Bobby Holley (:bholley)]

(and to be clear, the question being asked is the third paragraph of comment 9)

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Bobby Holley (:bholley) from comment #9)
> If so, I think we want mOriginAttributes.mAppId to correspond to the value
> of OwnOrContainingAppId(). If we have app X that creates a browser frame (or
> some other out-of-process iframe), the containing appId is X and the own
> appId is NO_APP_ID. In that situation, I believe that the principal of the
> top-level document should have appId=X. Paul, do I have that right?

I don't quite understand what you are asking.

In short:

If document A, with appid=X, contains a <iframe mozbrowser>, then the inner docshell will have appid=X and isInBrowserElement=true.

If document A, with appid=X, contains an <iframe mozbrowser mozapp="manifest of app Y">, then the inner docshell will have appid=Y and isInBrowserElement=false.

In detail:

FirefoxOS always has the system app as the outermost non-chrome docshell. This docshell has appid=systemAppId and isInBrowserElement=false.

If the system app creates a <iframe mozbrowser remote=true>, then that creates a TabContext in the parent process and a docshell in the childprocess. The doschell will have appid=systemAppId and isInBrowserElement=true.

If the system app creates a <iframe mozbrowser mozapp="manifest of app X" remote=true>, then that creates a TabContext in the parent process and a docshell in the childprocess. The doschell will have appid=X and isInBrowserElement=false.

Currently a document in the child process can't create an out-of-process iframe. But this is in the works. Once that's done I *think* it will have the following behavior:

If a document, with appid=X and isInBrowserElement=false, in a child process it creates an <iframe mozbrowser remote=true>, then it will create a TabContext-like thing in both the child process and in the parent process, as well as a docshell in a sibling process to the child process. The docshell will have appid=X and isInBrowserElement=true.

Either way:
Having the TabContext contain an OriginAttributes which is the same as the topmost docshell inside that TabContext makes sense to me.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #9)
> > If so, I think we want mOriginAttributes.mAppId to correspond to the value
> > of OwnOrContainingAppId(). If we have app X that creates a browser frame (or
> > some other out-of-process iframe), the containing appId is X and the own
> > appId is NO_APP_ID. In that situation, I believe that the principal of the
> > top-level document should have appId=X. Paul, do I have that right?
> 
> I don't quite understand what you are asking.
> 
> In short:
> 
> If document A, with appid=X, contains a <iframe mozbrowser>, then the inner
> docshell will have appid=X and isInBrowserElement=true.
> 
> If document A, with appid=X, contains an <iframe mozbrowser mozapp="manifest
> of app Y">, then the inner docshell will have appid=Y and
> isInBrowserElement=false.


Yes, this is what I was asking, and it sounds like I had it right.

> If the system app creates a <iframe mozbrowser mozapp="manifest of app X"
> remote=true>, then that creates a TabContext in the parent process and a
> docshell in the childprocess. The doschell will have appid=X and
> isInBrowserElement=false.

Except this part. If the iframe is both mozbrowser and mozapp, why is inBrowser=false for the docshell?

> Either way:
> Having the TabContext contain an OriginAttributes which is the same as the
> topmost docshell inside that TabContext makes sense to me.

Great.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Bobby Holley (:bholley) from comment #13)
> > In short:
> > 
> > If document A, with appid=X, contains a <iframe mozbrowser>, then the inner
> > docshell will have appid=X and isInBrowserElement=true.
> > 
> > If document A, with appid=X, contains an <iframe mozbrowser mozapp="manifest
> > of app Y">, then the inner docshell will have appid=Y and
> > isInBrowserElement=false.
[snip]

> > If the system app creates a <iframe mozbrowser mozapp="manifest of app X"
> > remote=true>, then that creates a TabContext in the parent process and a
> > docshell in the childprocess. The doschell will have appid=X and
> > isInBrowserElement=false.
> 
> Except this part. If the iframe is both mozbrowser and mozapp, why is
> inBrowser=false for the docshell?

See example above.

The "mozbrowser" attribute doesn't (unfortunately) mean "set isInBrowserElement=true". It means "this iframe is uses the enhanced API. The document inside this iframe will think it's a toplevel document".

At least I *think* this is the case. If you want to dig deeper you should ping KanRu.

Comment 15

[Bobby Holley (:bholley)]

I see - so it's just an API wart/artifact, and semantically, |mozbrowser mozapp| should just be |mozapp|.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I *think* that is how it is yes.

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

Yes, |mozbrowser mozapp| should just be |mozapp| but it was implemented this way and we didn't bother to change it.

Comment 18

[Tanvi Vyas[:tanvi]]

I don't think Steven has the time to work on this right now, so unassigning.

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

taking

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Bobby Holley (:bholley) from comment #9)
> ::: dom/ipc/TabContext.cpp
> @@ +261,5 @@
> > +      // XXX-senglehardt: not sure if the inheritance is correct here. The
> > +      // previous inheritance is described above. If we simply inherit
> > +      // originAttirbutes in both cases, mAppId and mInBrowser should still be
> > +      // set the same as before. It's not clear that we'll always want the rest
> > +      // of the attributes to inherit as well.
> 
> I think tabs created via window.open should inherit OriginAttributes in the
> same way that iframes do. So what we really want here is to call
> InheritFromDocToChildDocshell, which will be added in bug 1191740, but
> hasn't landed yet.
> 
> If this lands first, copying the attributes is fine, but please add a
> comment and file a followup bug depending on bug 1191740 and blocking bug
> 1179985 such that we can fix this up.

When you say InheritFromDocToChildDocshell in bug 1191740, do you mean in another bug?

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #20)
> When you say InheritFromDocToChildDocshell in bug 1191740, do you mean in
> another bug?
Bug 1209162

Comment 22

[Dave Huseby [:huseby]]

How's this bug going?  Can I help with anything?

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Dave Huseby [:huseby] from comment #22)
> How's this bug going?  Can I help with anything?

I've created the dependency bug 1212250 as asked in comment 9. I should be able to request review today :)

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1. Add originAttributes in TabContext

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2. Factor out nsFrameLoader::GetNewTabContext

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8671182 [details] [diff] [review]
Part 1. Add originAttributes in TabContext

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

r=me with those things fixed, most importantly the fallback to the containing App Id.

::: dom/base/nsFrameLoader.cpp
@@ +298,1 @@
>    }

As mentioned below I think we need an |else| case to fall back to the containing app ID.

::: dom/ipc/PContent.ipdl
@@ +466,5 @@
>      // of ContentParent::CreateBrowserOrApp.
>      //
> +    // When the parent constructs a PBrowser, the child trusts the app token and
> +    // other attributes it receives from the parent.  In that case, the
> +    // contex should be FrameIPCTabContext.

Nit: s/contex/context/

::: dom/ipc/TabContext.cpp
@@ +174,5 @@
>      NS_ENSURE_SUCCESS(rv, false);
>      NS_ENSURE_TRUE(ownAppId != NO_APP_ID, false);
>    }
> +  // Veryify that ownAppId matches mAppId passed in originAttributes
> +  MOZ_RELEASE_ASSERT(aOriginAttributes.mAppId == ownAppId);

I don't think this is quite right. If I understand correctly, if aOwnApp is null but aAppFrameOwnerApp is non-null, then the corresponding docshell's appId (and therefore aOriginAttributes.mAppId) should be the ID of the containing App.

This will also need to be fixed in nsFrameLoader.cpp.

@@ +258,5 @@
>        // Otherwise, we're a new app window and we inherit from our
>        // opener app.
> +
> +      // FIXME bug 1212250 - use InheritFromDocToChildDocshell instead
> +      // of copy attributes directly.

nit: 'instead of copying'

::: dom/ipc/TabContext.h
@@ +105,5 @@
>  
> +  /**
> +   * OriginAttributesRef() returns the OriginAttributes of this frame to the
> +   * caller. This is used to store any attribute associated with the frame's
> +   * context, such as the AppId.

I'd say "associated with the frame's docshell"

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8671184 [details] [diff] [review]
Part 2. Factor out nsFrameLoader::GetNewTabContext

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

r=me with the fixes in part 1.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9c406698782b
https://hg.mozilla.org/integration/b2g-inbound/rev/cde243d57347

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c406698782b
https://hg.mozilla.org/mozilla-central/rev/cde243d57347