1238160 - Support frames similar to mozbrowser on desktop Firefox

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I recently started a thread about possibly enabling mozbrowser frames for use on desktop Firefox by enabling the pref "dom.mozBrowserFramesEnabled".[1]

Mossop has indicated to me on IRC that it makes sense to move forward, assuming it's safe to do so.

The main areas I'm aware of for investigation are:

# Security / Exposure to Content

As bholley pointed out on the list, mozbrowser is different than most things in desktop Firefox today, since the security model relies on permissions, not just "are you chrome?". Looking at the webidl[2], even after the pref is enabled, all features correctly check at least the "browser" permission.

My initial investigation suggests it's correctly hidden from web content. A page with system principal is allowed[3] to see the APIs, but regular content can't. Even if web content sets "mozbrowser", the APIs are not defined at all, so it appears to be hidden from regular content correctly.

sicking added that it might be worth adding automated tests that checks that <iframe mozbrowser> gets no extra API, for example by enumerating the properties on it and comparing to a plain <iframe>.  Also worth testing that <iframe mozbrowser remote src="same-origin-page.html"> can be reached into and doesn't run
out-of-process.

# e10s

In some initial testing, <iframe mozbrowser remote> is correctly remoted to the child process (assuming you have flipped the pref). Basic functionality appears to work great, but I have not tested it thoroughly yet.

billm says someone should audit the frameloader/TabParent code to make sure everything works the way we expect.  

billm also says we'll need to make sure that our frame scripts are being loaded in the right places.  Occasionally people to load a frame script using the global message manager and assume that it applies only to each tab.  We already have getGroupMessageManager for this purpose, but we might have to do some more work there.

[1]: https://groups.google.com/forum/#!msg/firefox-dev/pBNWfPgQopI/9bYyJa-6CgAJ
[2]: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/BrowserElement.webidl
[3]: https://dxr.mozilla.org/mozilla-central/rev/3f780f4b14acab29b16bc4141a44180a8af9dd08/extensions/cookie/nsPermissionManager.cpp#2059

Comment 1

[(no longer active)]

Please specify which exact features you need to use.  dom.mozBrowserFramesEnabled controls more than just the mozbrowser APIs.  See for example <https://dxr.mozilla.org/mozilla-central/source/dom/webidl/AudioChannelManager.webidl#34>.

Also, loading stuff in such iframes may change what the web content loaded inside it can observe/do by changing what the isInBrowserElement member of the principal <https://dxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#295> returns, which is not OK on desktop.

Comment 2

[Bobby Holley (:bholley)]

Yeah. It seems like what J Ryan wants is just a more modern and less-xul-y way to load web content in a child process than the existing <browser> setup. However, mozbrowser is largely about allowing web content to load other web content in a sandbox, which isn't what we want here.

So it seems like the way forward here is probably to refactor the machinery to a similar-but-separate primitive to be used by chrome.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Ehsan Akhgari from comment #1)
> Please specify which exact features you need to use. 
> dom.mozBrowserFramesEnabled controls more than just the mozbrowser APIs. 
> See for example
> <https://dxr.mozilla.org/mozilla-central/source/dom/webidl/
> AudioChannelManager.webidl#34>.

My main desire is to load web content in the child process without XUL, as :bholley points out.  I am not interested in AudioChannelManager, etc.  The APIs I would want access to are those listed in BrowserElement.webidl.

> Also, loading stuff in such iframes may change what the web content loaded
> inside it can observe/do by changing what the isInBrowserElement member of
> the principal
> <https://dxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#295>
> returns, which is not OK on desktop.

Okay, I was not aware of this part.  Certainly I don't want it to be observable to web content.

(In reply to Bobby Holley (busy) from comment #2)
> Yeah. It seems like what J Ryan wants is just a more modern and less-xul-y
> way to load web content in a child process than the existing <browser>
> setup. However, mozbrowser is largely about allowing web content to load
> other web content in a sandbox, which isn't what we want here.

Yes, that's right.  I want to load web content in a child process by using an iframe that is part of a chrome page.

> So it seems like the way forward here is probably to refactor the machinery
> to a similar-but-separate primitive to be used by chrome.

Who can help discuss what this might look like?  Are we saying potentially a separate attribute (not mozbrowser) with different, more specific characteristics?

Comment 4

[Bobby Holley (:bholley)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Who can help discuss what this might look like?  Are we saying potentially a
> separate attribute (not mozbrowser) with different, more specific
> characteristics?

Some combination of KanRu, smaug, and pauljt might be able to get you started.

Comment 5

[(no longer active)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> (In reply to :Ehsan Akhgari from comment #1)
> > Please specify which exact features you need to use. 
> > dom.mozBrowserFramesEnabled controls more than just the mozbrowser APIs. 
> > See for example
> > <https://dxr.mozilla.org/mozilla-central/source/dom/webidl/
> > AudioChannelManager.webidl#34>.
> 
> My main desire is to load web content in the child process without XUL, as
> :bholley points out.  I am not interested in AudioChannelManager, etc.  The
> APIs I would want access to are those listed in BrowserElement.webidl.

OK, then someone needs to implement what you want.  As things stand, <iframe mozbrowser> was built for a very different use case.  (Parts of the implementation can be shared of course, but it's not as simple as toggling a pref.)

> > So it seems like the way forward here is probably to refactor the machinery
> > to a similar-but-separate primitive to be used by chrome.
> 
> Who can help discuss what this might look like?  Are we saying potentially a
> separate attribute (not mozbrowser) with different, more specific
> characteristics?

The syntax can be anything really, we just can't reuse mozbrowser.

Comment 6

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

As long as you set mozbrowser attribute (without also setting the mozapps attribute), the contained page should behave just like a normal page. I.e. it should look identical to being opened in a <xul:browser>.

The only thing that is affected is which cookie jar it's using.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> As long as you set mozbrowser attribute (without also setting the mozapps
> attribute), the contained page should behave just like a normal page. I.e.
> it should look identical to being opened in a <xul:browser>.
> 
> The only thing that is affected is which cookie jar it's using.

Is there really nothing else that checks inBrowser and does something special?

That aside, getting separate cookies etc is a major behavior difference from using a xul:browser, and it's not immediately clear to me that we want that. Especially since you'd end up sharing cookies with any other chrome code also using the same mechanism, since we don't have the appId to segregate them.

Comment 8

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> As long as you set mozbrowser attribute (without also setting the mozapps
> attribute), the contained page should behave just like a normal page. I.e.
> it should look identical to being opened in a <xul:browser>.
> 
> The only thing that is affected is which cookie jar it's using.

It has implications beyond the cookie jar.  For example it affects how we store permissions <https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1781> (and perhaps elsewhere where we look at the OriginAttributes.)

Comment 9

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #8)
> (In reply to Jonas Sicking (:sicking) from comment #6)
> > As long as you set mozbrowser attribute (without also setting the mozapps
> > attribute), the contained page should behave just like a normal page. I.e.
> > it should look identical to being opened in a <xul:browser>.
> > 
> > The only thing that is affected is which cookie jar it's using.
> 
> It has implications beyond the cookie jar.

By "cookie jar", sicking is imprecisely referring to "all of the things that we divide into separate buckets based on OriginAttributes".

> For example it affects how we
> store permissions
> <https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#1781>

Is there any actual special behavior there for inBrowser, other than the aforementioned segregation of things with non-equal OriginAttributes?

Comment 10

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

The whole goal of <iframe mozbrowser> is to create an context that presents a normal web runtime. So there really shouldn't be.

The only thing that I know of is that we have some weirdness regarding window.open() when appid != 0.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Bobby Holley (busy) from comment #7)
> That aside, getting separate cookies etc is a major behavior difference from
> using a xul:browser, and it's not immediately clear to me that we want that.
> Especially since you'd end up sharing cookies with any other chrome code
> also using the same mechanism, since we don't have the appId to segregate
> them.

Yes, ideally for my use case it would be same cookie jar, etc. as you'd get with <xul:browser>, so the page would see the same storage if it were in <xul:browser> vs. <iframe mozbrowser> (or whatever alternate we construct here).

Comment 12

[Bobby Holley (:bholley)]

Right. So it sounds like you want something that uses most of the machinery of iframe mozbrowser but doesn't mess with the OriginAttributes and doesn't accidentally enable any other APIs.

That is most likely possible, but someone would need to dive into the code and do it (and someone would need to review it).

Comment 13

[(no longer active)]

(In reply to Bobby Holley (busy) from comment #12)
> Right. So it sounds like you want something that uses most of the machinery
> of iframe mozbrowser but doesn't mess with the OriginAttributes and doesn't
> accidentally enable any other APIs.

Yeah that seems like what we want here.  In theory it should be "easy" if you fake a false return value from GetIsInBrowserElement(), but I haven't investigated in any detail beyond that.

Comment 14

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #13)
> (In reply to Bobby Holley (busy) from comment #12)
> > Right. So it sounds like you want something that uses most of the machinery
> > of iframe mozbrowser but doesn't mess with the OriginAttributes and doesn't
> > accidentally enable any other APIs.
> 
> Yeah that seems like what we want here.  In theory it should be "easy" if
> you fake a false return value from GetIsInBrowserElement(), but I haven't
> investigated in any detail beyond that.

Which one? I hope you don't mean the GetIsInBrowserElement on nsIPrincipal. We should just make sure that the code that munges the OriginAttributes to add inBrowser=true doesn't run.

Comment 15

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

You could implement <iframe remote> (just for chrome) without dependency on mozbrowser but you will be unable to use mozbrowser APIs.

Like Jonas said the mozbrowser iframe should load web content normally as long as you don't use any mozapp attribute. Content loaded by mozbrowser will use different data jar from <xul:browser>. Maybe we could use a new attribute to cancel the inBrowser flag set by mozbrowser attribute.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Well, then GetIsBrowserOrApp and such in DocShell would become rather odd methods. At least naming would need to be changed. 

But, do we need html:iframe in an HTML document where the iframe's content document is loaded OOP, or do we need that + BrowserElement API? Sounds like the latter.
What should happen for frame scripts?  Should we get the usual frame scripts which xul:browser has loaded too so that various browser features actually work? or would remote html:iframe be totally separate thing from xul:browsers and it would be up to the creator of iframe to load the needed scripts? (but then, what all web features wouldn't quite work there... we have lots of frame scripts)

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Olli Pettay [:smaug] from comment #16)
> But, do we need html:iframe in an HTML document where the iframe's content
> document is loaded OOP, or do we need that + BrowserElement API? Sounds like
> the latter.

Yes, I think the latter with both OOP and BrowserElement API.

> What should happen for frame scripts?  Should we get the usual frame scripts
> which xul:browser has loaded too so that various browser features actually
> work? or would remote html:iframe be totally separate thing from
> xul:browsers and it would be up to the creator of iframe to load the needed
> scripts? (but then, what all web features wouldn't quite work there... we
> have lots of frame scripts)

<iframe mozbrowser remote> already functions in a reasonable way with frame scripts:

* Global mm scripts *are* loaded (like chrome://global/content/browser-content.js)
* Browser mm group scripts *are not* loaded (like chrome://browser/content/content.js)
  * This is because there is no "messagemanagergroup" attribute set

These existing policies seem fine to me.  I think whatever <iframe> syntax we craft here can follow along with this same policy.

There is no "messagemanagergroup" concept for <iframe>s currently, but I don't think that matters initially.  I would expect the creator of the <iframe> to load additional frame scripts if they are needed.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Compare APIs in iframe vs. mozbrowser

(In reply to :Ehsan Akhgari from comment #1)
> Also, loading stuff in such iframes may change what the web content loaded
> inside it can observe/do by changing what the isInBrowserElement member of
> the principal
> <https://dxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#295>
> returns, which is not OK on desktop.

Ehsan, I've attached a test that compares the APIs accessible from window, document, and navigator in <iframe> vs. <iframe mozbrowser>.  Unless I've done something wrong, they expose the same properties on all of these objects.  Of course, there are many other APIs, so this is just a rough sanity check.

Is there a specific API you can reference as an example of something you're worried about that would get exposed to content in <iframe mozbrowser>?

Comment 19

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

I don't understand where the perception that the inner page would see a different API comes from.

The way that B2G renders web pages from the normal web is using <iframe mozbrowser>, and the whole point of B2G was to create a web-based OS. So we certainly did everything we could to render webpages normally.

The only thing we did different was that we used cookie jars as a security mechanism. This didn't affect B2G the same way since we never used <xul:browser> and so all pages still ended up in the same cookie jar.

We also put limitations on window.open since we didn't want users to have to juggle a large number of windows on a small screen with little room for tab management.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

It might be useful to check which all frame scripts are loaded to xul:browser remote=true (in case they are used for tabs) and which get loaded to mozbrowsers when enabled.
Right now everything expects xul:browser handling, so we might be somewhat random scripts to mozbrowser. So some things might work, some might not.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: List of frame scripts loaded by xul:browser vs. iframe mozbrowser

(In reply to Olli Pettay [:smaug] from comment #20)
> It might be useful to check which all frame scripts are loaded to
> xul:browser remote=true (in case they are used for tabs) and which get
> loaded to mozbrowsers when enabled.
> Right now everything expects xul:browser handling, so we might be somewhat
> random scripts to mozbrowser. So some things might work, some might not.

I actually collected those lists to prepare my earlier reply in comment 17. :)

This file lists the frame scripts each one currently loads, as well as how they were added (which message manager, etc).

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #15)
> You could implement <iframe remote> (just for chrome) without dependency on
> mozbrowser but you will be unable to use mozbrowser APIs.

How about <iframe mozbrowser="clean share-cookie-jar" remote> ? I was told "remote" is something eventually we want to get rid of...

(obvious the naming should be improved)

Comment 23

[(no longer active)]

Comment on attachment 8707557 [details] [diff] [review]
Compare APIs in iframe vs. mozbrowser

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Created attachment 8707557 [details] [diff] [review]
> Compare APIs in iframe vs. mozbrowser
> 
> (In reply to :Ehsan Akhgari from comment #1)
> > Also, loading stuff in such iframes may change what the web content loaded
> > inside it can observe/do by changing what the isInBrowserElement member of
> > the principal
> > <https://dxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#295>
> > returns, which is not OK on desktop.
> 
> Ehsan, I've attached a test that compares the APIs accessible from window,
> document, and navigator in <iframe> vs. <iframe mozbrowser>.  Unless I've
> done something wrong, they expose the same properties on all of these
> objects.  Of course, there are many other APIs, so this is just a rough
> sanity check.
> 
> Is there a specific API you can reference as an example of something you're
> worried about that would get exposed to content in <iframe mozbrowser>?

Sorry, please see comment 8 (aka Jonas' definition of "cookie jar" which basically means some of the persistent data we store on behalf of the website, including but not limited to HTTP cookies.)  There is no difference in the *APIs* that the page can observe.  As such, I don't think there's any point in taking this test.

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Ehsan Akhgari from comment #23)
> Sorry, please see comment 8 (aka Jonas' definition of "cookie jar" which
> basically means some of the persistent data we store on behalf of the
> website, including but not limited to HTTP cookies.)  There is no difference
> in the *APIs* that the page can observe.  As such, I don't think there's any
> point in taking this test.

Right, I am aware of the cookie jar distinction.  It seemed like you and perhaps others thought there *were also* API exposure differences, in addition to the cookie jar difference.  Anyway, with your reply here it seems clear enough that you now agree there is no concern about exposed APIs.

Ehsan, I will likely start attempting a patch to access the "regular" cookie jar, like <xul:browser> does, through some kind of extra attribute syntax.  Aside from the cookie jars, does it seem safe to enable existing mozbrowser frames on desktop?  You would get separate storage until the cookie jar issue is resolved, but the cookie jar part could be solved separately from whether mozbrowser frames can be used at all.

We could potentially break up dom.mozBrowserFramesEnabled into several prefs, so that on desktop only BrowserElement.webidl APIs are enabled and Firefox OS can still have all the others, like AudioChannelManager.webidl.

Comment 25

[(no longer active)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> (In reply to :Ehsan Akhgari from comment #23)
> > Sorry, please see comment 8 (aka Jonas' definition of "cookie jar" which
> > basically means some of the persistent data we store on behalf of the
> > website, including but not limited to HTTP cookies.)  There is no difference
> > in the *APIs* that the page can observe.  As such, I don't think there's any
> > point in taking this test.
> 
> Right, I am aware of the cookie jar distinction.  It seemed like you and
> perhaps others thought there *were also* API exposure differences, in
> addition to the cookie jar difference.  Anyway, with your reply here it
> seems clear enough that you now agree there is no concern about exposed APIs.

I never thought that.  :-)  Not sure where that idea originates from...

> Ehsan, I will likely start attempting a patch to access the "regular" cookie
> jar, like <xul:browser> does, through some kind of extra attribute syntax. 
> Aside from the cookie jars, does it seem safe to enable existing mozbrowser
> frames on desktop?  You would get separate storage until the cookie jar
> issue is resolved, but the cookie jar part could be solved separately from
> whether mozbrowser frames can be used at all.

Are you familiar with the OriginAttributes setup?  What we want to do is to make sure that you'd get the same OriginAttributes as you would in a xul:browser.  And you would need to create a different pref with a different way of signaling this new kind of iframe, and the usual due diligence to make sure that iframe isn't changed as far Web content is concerned.

> We could potentially break up dom.mozBrowserFramesEnabled into several
> prefs, so that on desktop only BrowserElement.webidl APIs are enabled and
> Firefox OS can still have all the others, like AudioChannelManager.webidl.

I personally think it is easier to start with a new pref instead of trying to salvage dom.mozBrowserFramesEnabled.  But the pref issue is the easiest thing to deal with here.  :-)

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

After talking more with Ehsan on IRC, it sounds like he would be comfortable with enabling dom.mozBrowserFramesEnabled on desktop after the following:

1. Investigate all the things enabled by dom.mozBrowserFramesEnabled
2. Add a test (if there isn't one already) to verify that <iframe mozbrowser> is equivalent to <iframe> in web content
3. Investigate the consumers of GetIsInBrowserElement

There's of course the cookie jar difference still, but that could be solved separately from just enabling mozbrowser frames (though they would be confusing to use on desktop until this is resolved).

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Before I go too far down the rabbit hole of OriginAttributes changes, it would be good to agree on a name for this concept of "<iframe mozbrowser> that uses the same cookie jar as <xul:browser>".  

After talking with :bholley, we thought of the term "isolated" to describe the *current* <iframe mozbrowser> origins.  So, the new kind we'd be adding here is an "iframe mozbrowser without isolation" or <iframe mozbrowser isolated=false>.

:ehsan / :sicking, does this terminology and syntax seem okay to you?

If so, part of my work would be to rename things like nsIPrincipal::GetIsInBrowserElement to GetIsInIsolatedBrowserElement, since only the isolated ones would have the separate mInBrowser = true origin attribute.

Comment 28

[(no longer active)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #27)
> Before I go too far down the rabbit hole of OriginAttributes changes, it
> would be good to agree on a name for this concept of "<iframe mozbrowser>
> that uses the same cookie jar as <xul:browser>".  
> 
> After talking with :bholley, we thought of the term "isolated" to describe
> the *current* <iframe mozbrowser> origins.  So, the new kind we'd be adding
> here is an "iframe mozbrowser without isolation" or <iframe mozbrowser
> isolated=false>.
> 
> :ehsan / :sicking, does this terminology and syntax seem okay to you?

Yeah, but I don't feel strongly about the syntax anyways.

> If so, part of my work would be to rename things like
> nsIPrincipal::GetIsInBrowserElement to GetIsInIsolatedBrowserElement, since
> only the isolated ones would have the separate mInBrowser = true origin
> attribute.

Good idea!

Comment 29

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

I've always used the term "cookie jar". So what this would be is "<iframe mozbrowser> which uses the normal cookie jar" or some such.

I believe the Sec Engineering team is using the term "context", so "<iframe mozbrowser> which uses the normal context".

A technical term might be "<iframe mozbrowser> which uses the same OriginAttributes".

All that said, I really don't care about the name. In practice I don't think any of the names mentioned so far will be understood by the community in general. So expect to have to explain this multiple times.

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Both :billm and :ehsan asked for some investigation here:

* Audit nsFrameLoader and TabContext code for mozbrowser on desktop
* Investigate all the things enabled by dom.mozBrowserFramesEnabled
* Investigate the consumers of GetIsInBrowserElement

I have completed my audit of these things along with various Principal, DocShell, and TabContext APIs related to whether things are browser elements, apps, and what their OriginAttributes are, as these are all related to the "non-isolated" <iframe mozbrowser>.

The audit details are available at:

https://docs.google.com/document/d/1rHVOFhDtko8adpgXxaPvYs6qhAEIdvARVuciVDnKIbk/edit

Here is a summary of the changes I plan to make to support desktop mozbrowser and non-isolated mode:

1. Enable dom.mozBrowserFramesEnabled on desktop with tests to verify no content exposure
2. Ensure various existing tests for mozbrowser run on desktop
3. Rename Principal::GetIsInBrowserElement to GetIsInIsolatedBrowserElement and mInBrowser to mInIsolatedBrowser
4. Rename LoadContext::GetIsInBrowserElement to GetIsInIsolatedBrowserElement
5. Rename TabContext::IsBrowserElement to IsIsolatedBrowserElement
6. Add nsDocShell::Is(In)?IsolatedBrowserElement based on OriginAttributes and use for callers checking isolation instead of frame type
7. Change nsFrameLoader::GetNewTabContext to only set mInIsolatedBrowser when isolated
8. Change nsDocShell::GetOriginAttributes to only set mInIsolatedBrowser when isolated
9. Change callers that check Principal::GetIsInBrowserElement but really want "is browser frame" to DocShell versions that check frame type
10. Change TabContext::IsBrowserOrApp to check frame type instead of OriginAttributes
11. Add new tests for the non-isolated case, ensure it has no content exposure
12. Add TODOs where app permissions are validated today (but disabled on desktop) which would need future updates to properly handle isolation state

Comment 31

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Test mozbrowser APIs to ensure no content exposure. r=bz

Check the various mozbrowser APIs to ensure they are allowed when you have
browser permission and blocked when you don't (like regular web content).

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

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Enable mozbrowser frames on desktop. r=bz

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

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Rename OriginAttributes.mInBrowser and associated methods. r=bz,mayhemer

This change renames OriginAttributes.mInBrowser and
nsIPrincipal::GetIsInBrowserElement to add "Isolated".  Other methods that pass
these values around also have name changes.

Tokens such as "inBrowser" have previously been serialized into cache keys, used
as DB column names, stored in app registries, etc.  No changes are made to any
serialization formats.  Only runtime method and variable names are updated.

No behavior changes are made in this patch, so some renamed methods may have
have nonsensical implementations.  These are corrected in subsequent patches
focused on behavior.

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

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Rename nsILoadContext::GetIsInBrowserElement. r=smaug,mayhemer

This change renames nsILoadContext::GetIsInBrowserElement to add "Isolated".
Other methods that pass these values around also have name changes.

Tokens such as "isInBrowserElement" have previously been serialized into cache
keys, used as DB column names, stored in app registries, etc.  No changes are
made to any serialization formats.  Only runtime method and variable names are
updated.

No behavior changes are made in this patch, so some renamed methods may have
have nonsensical implementations.  These are corrected in subsequent patches
focused on behavior.

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

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Rename TabContext::IsBrowserElement. r=billm,mayhemer

This change renames TabContext::IsBrowserElement to add "Isolated".  Other
methods that pass these values around also have name changes.

No behavior changes are made in this patch, so some renamed methods may have
have nonsensical implementations.  These are corrected in subsequent patches
focused on behavior.

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

Comment 36

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Set tab context's isolation from frame attr. r=smaug

Adds nsFrameLoader::OwnerIsIsolatedBrowserFrame which checks the isolated
attribute of browser frames, if present.

This is used to set isolation in nsFrameLoader::GetNewTabContext only when true.

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

Comment 37

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Set docshell isolation mode. r=smaug

Adds nsDocShell::GetIsIsolatedBrowserElement, which parallels
GetIsInIsolatedBrowserElement, but only checks the immediate docshell.

Adds nsDocShell::SetIsInIsolatedBrowserElement for the frame loader and tab
child to set the isolation state.

nsDocShell methods related to browser elements (and their callers) are updated
to use GetIs(In)?IsolatedBrowserElement when checking isolation / origins and
GetIsBrowserElement when checking frame types.

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

Comment 38

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

Adds TabContext::IsBrowserElement which is set by the frame loader for all
browser frames.  This is in contrast to its previous implementation, which has
since been renamed IsIsolatedBrowserElement, since it checks isolated state in
OriginAttributes.

TabContext methods related to browser elements (and their callers) are updated
to use IsIsolatedBrowserElement when check isolation / origins and
IsBrowserElement when checking frame types.

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

Comment 39

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice

Several code paths try to ask the principal if it's in a browser element, but
the principal now only knows about *isolated* browser elements.  All such code
paths are currently unused on desktop.  Comments are added to each case to note
they would need to be updated if isolation disabling is brought to such
platforms.

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

Comment 40

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1238160 - Test frame principal when toggling isolation. r=bz

Test frame principals in different configurations to verify the new isolated
attribute works as expected.

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718600 [details]
MozReview Request: Bug 1238160 - Test mozbrowser APIs to ensure no content exposure. r=bz

https://reviewboard.mozilla.org/r/34609/#review31407

::: dom/base/test/test_mozbrowser_apis_allowed.html:26
(Diff revision 1)
> +    [ "stop" ],
> +    [ "purgeHistory" ],

This is missing "download".  Should have it here, right?  We want to at least test that it exists in the enabled case, and that it does NOT exist in the other case.

::: dom/base/test/test_mozbrowser_apis_allowed.html:32
(Diff revision 1)
> +    [ "getContentDimensions" ],
> +    [ "findAll", "example", "case-insensitive" ],

Also missing setInputMethodActive, setNFCFocus.

I guess those depend on other permissions, so we'd want to test that they are not present in both cases.

::: dom/base/test/test_mozbrowser_apis_allowed.html:35
(Diff revision 1)
> +    [ "clearMatch" ],
> +    [ "getStructuredData" ],

And missing executeScript, right?

Again, we'd want to make sure it's not exposed in both cases.

::: dom/base/test/test_mozbrowser_apis_blocked.html:14
(Diff revision 1)
> +  const METHODS = [

I would really prefer it if we shared this array across the two tests (say in a JS file that both load).

::: dom/base/test/test_mozbrowser_apis_blocked.html:49
(Diff revision 1)
> +  function once(target, eventName, useCapture = false) {

And the rest of this infra can be shared between the tests too, right?

::: dom/base/test/test_mozbrowser_apis_blocked.html:99
(Diff revision 1)
> +        frame[name](...args);

No, this, this isn't the right test.  We want to make sure the API is not exposed, not that it throws.

What you want to do is:

  ok(!(name in frame), ...)
  
and lose the try/catch altogether, I think.

Same for the attribute case: we want to check that the property is missing, not just that its value is undefined.

r=me with the above addressed.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718601 [details]
MozReview Request: Bug 1238160 - Enable mozbrowser frames on desktop. r=bz

https://reviewboard.mozilla.org/r/34611/#review31423

r=me

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718600 [details]
MozReview Request: Bug 1238160 - Test mozbrowser APIs to ensure no content exposure. r=bz

https://reviewboard.mozilla.org/r/34609/#review31425

Argh.  I _meant_ to mark r+, once the issues are addressed.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718602 [details]
MozReview Request: Bug 1238160 - Rename OriginAttributes.mInBrowser and associated methods. r=bz,mayhemer

https://reviewboard.mozilla.org/r/34613/#review31433

In the commit message, "may have have" should just be "may have".

::: caps/nsIPrincipal.idl:308
(Diff revision 1)
> +     * <iframe mozbrowser isolated="false"> does not count as isolated since

So this isolated="false" thing is not a thing yet, right?  It's going to be added in a later diff?

The general way boolean attributes work in HTML is that attr being set means true.  Having foo="false" mean false is a XUL-ism, and I don't think we should be adding those in HTML.

So we would want the attribute to have a name that implies non-isolation when set, and have its absence mean isolation.

r=me with those fixed.

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34613/#review31433

> So this isolated="false" thing is not a thing yet, right?  It's going to be added in a later diff?
> 
> The general way boolean attributes work in HTML is that attr being set means true.  Having foo="false" mean false is a XUL-ism, and I don't think we should be adding those in HTML.
> 
> So we would want the attribute to have a name that implies non-isolation when set, and have its absence mean isolation.

Right, it's not yet a thing.  It's added in the later patch called "Set tab context's isolation from frame attr", currently set for review by smaug.  (I wasn't really sure how to choose reviewers for some this without making things really confusing...)

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718605 [details]
MozReview Request: Bug 1238160 - Set tab context's isolation from frame attr. r=smaug

https://reviewboard.mozilla.org/r/34619/#review31443

::: browser/app/profile/firefox.js:1654
(Diff revision 1)
> +pref("dom.browserFrameIsolation.disablingPossible", true);

So I don't think we need any this kind of pref.

::: dom/html/nsGenericHTMLFrameElement.cpp:573
(Diff revision 1)
> +nsGenericHTMLFrameElement::GetIsolated(bool *aOut)

So I would always return true here if element's NodePrincipal() isn't chrome principal.

And just drop all the permission handling

I would prefer if the isolation state was store somewhere, perhaps in frameloader, since changing the attribute doesn't in fact change the state.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718605 [details]
MozReview Request: Bug 1238160 - Set tab context's isolation from frame attr. r=smaug

So I think this patch can be quite a bit simpler.

(and mozreview is silly by not allowing r-)

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718609 [details]
MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice

https://reviewboard.mozilla.org/r/34625/#review31445

I would much prefer it if the relevant codepaths runtime aborted, or at least asserted, if run on a platform where disabling isolation is supported.  That would ensure that they get updated as needed.  As things stand, it's not clear how someone enabling disabling of isolation on some platform where these codepaths are hit would know to update them.

Comment 49

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/34625/#review31447

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718610 [details]
MozReview Request: Bug 1238160 - Test frame principal when toggling isolation. r=bz

https://reviewboard.mozilla.org/r/34627/#review31449

r=me subject to my comments about boolean HTML attributes above.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718603 [details]
MozReview Request: Bug 1238160 - Rename nsILoadContext::GetIsInBrowserElement. r=smaug,mayhemer

https://reviewboard.mozilla.org/r/34615/#review31453

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718607 [details]
MozReview Request: Bug 1238160 - Set docshell isolation mode. r=smaug

https://reviewboard.mozilla.org/r/34621/#review31457

I think I'd like to see that GetIsBrowserElement handling clarified.

::: dom/base/nsFrameLoader.cpp:1204
(Diff revision 1)
> -      otherDocshell->GetIsBrowserElement() ||
> +      otherDocshell->GetIsIsolatedBrowserElement() ||

hmm, so don't we want to remove GetIsBrowserElement()?
Otherwise it is error prone to check either
IsBrowserElement or IsIsolatedBrowserElement. One will easily forget there are them both.
Could we have IsNonIsolatedBrowserElement() and IsIsolatedBrowserElement() or something like that?

...expect... looks like nsDocShell::GetIsBrowserElement is called only by frameloader, so could we just remove it?

Comment 53

[Honza Bambas (:mayhemer)]

Comment on attachment 8718602 [details]
MozReview Request: Bug 1238160 - Rename OriginAttributes.mInBrowser and associated methods. r=bz,mayhemer

https://reviewboard.mozilla.org/r/34613/#review31671

Comment 54

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #53)
> Comment on attachment 8718602 [details]
> MozReview Request: Bug 1238160 - Rename OriginAttributes.mInBrowser and
> associated methods. r=bz,mayhemer
> 
> https://reviewboard.mozilla.org/r/34613/#review31671

Only for /dom/storage, /network, /uriloader/prefetch.

Comment 55

[Honza Bambas (:mayhemer)]

Comment on attachment 8718603 [details]
MozReview Request: Bug 1238160 - Rename nsILoadContext::GetIsInBrowserElement. r=smaug,mayhemer

https://reviewboard.mozilla.org/r/34615/#review31701

for /netwerk part

Comment 56

[Honza Bambas (:mayhemer)]

Comment on attachment 8718604 [details]
MozReview Request: Bug 1238160 - Rename TabContext::IsBrowserElement. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34617/#review31705

for /dom/network and /netwerk

Comment 57

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8718604 [details]
MozReview Request: Bug 1238160 - Rename TabContext::IsBrowserElement. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34617/#review31841

::: dom/ipc/PTabContext.ipdlh:16
(Diff revision 1)
> -// If isBrowserElement is false, this PopupIPCTabContext corresponds to an app
> -// frame, and the frame's app-id and app-frame-owner-app-id will be equal to the
> -// opener's values.
> +// If isIsolatedBrowserElement is false, this PopupIPCTabContext corresponds to
> +// an app frame, and the frame's app-id and app-frame-owner-app-id will be equal
> +// to the opener's values.

This comment doesn't seem right to me. Can you confirm that isIsolatedBrowserElement is also be false in the e10s case, in which case we're not in an app frame?

::: dom/ipc/TabContext.h:45
(Diff revision 1)
> -   * Does this TabContext correspond to a mozbrowser?  (<iframe mozbrowser
> +   * Does this TabContext correspond to a mozbrowser?

Can you change to "Does this TabContext correspond to an isolated mozbrowser?". That seems more correct according to my understanding.

::: dom/ipc/TabContext.h:63
(Diff revision 1)
>    bool IsBrowserOrApp() const;

Could this be renamed to IsIsolated? I feel like that would be clearer. Am I right that IsBrowserOrApp() returns false for a normal XUL browser element (or for a non-isolated mozbrowser in the new world)?

Relatedly, should we consider apps to be isolated? I feel like that makes sense.

Comment 58

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8718608 [details]
MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34623/#review31851

We talked on IRC and decided that we need a further renaming to IsMozBrowserElement/IsIsolatedMozBrowserElement. Also, this will be easier to review if it's squashed with the renaming patch.

Comment 59

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:billm requested on IRC that the TabContext comments also be updated to explain where the xul:browser case fits in for each method.

Comment 60

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34609/#review31407

> This is missing "download".  Should have it here, right?  We want to at least test that it exists in the enabled case, and that it does NOT exist in the other case.

Yes, I've added this and it is allowed / blocked based on browser permission like the others.

> Also missing setInputMethodActive, setNFCFocus.
> 
> I guess those depend on other permissions, so we'd want to test that they are not present in both cases.

Yes, added those, which are blocked in both runs due to missing more permissions.

> And missing executeScript, right?
> 
> Again, we'd want to make sure it's not exposed in both cases.

Yes, added this, which is blocked in both runs due to missing more permissions.

> I would really prefer it if we shared this array across the two tests (say in a JS file that both load).

Moved array and test infra to shared file.

> No, this, this isn't the right test.  We want to make sure the API is not exposed, not that it throws.
> 
> What you want to do is:
> 
>   ok(!(name in frame), ...)
>   
> and lose the try/catch altogether, I think.
> 
> Same for the attribute case: we want to check that the property is missing, not just that its value is undefined.

Thanks, I've updated the test as you described.

Comment 61

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34617/#review31841

> This comment doesn't seem right to me. Can you confirm that isIsolatedBrowserElement is also be false in the e10s case, in which case we're not in an app frame?

This was confused partly by the rename patch, but I've also updated to document the `<xul:browser>` case.

> Can you change to "Does this TabContext correspond to an isolated mozbrowser?". That seems more correct according to my understanding.

I think this was confusing because of the rename patch.  Please re-check in the revised, squashed version.

> Could this be renamed to IsIsolated? I feel like that would be clearer. Am I right that IsBrowserOrApp() returns false for a normal XUL browser element (or for a non-isolated mozbrowser in the new world)?
> 
> Relatedly, should we consider apps to be isolated? I feel like that makes sense.

(I have renamed this to `IsMozBrowserOrApp()` since it is false for `<xul:browser>`.)  This is not the same as isolation.  I believe this is another confusion from the separate rename patch.  Please re-check in the new version.  We do want some concept based on frame type only, not isolation.  Here are existing callers that want to know only about the frame type: https://dxr.mozilla.org/mozilla-central/search?q=IsBrowserOrApp%28%29+-regexp%3AGet%28Really%29%3FIsBrowserOrApp&redirect=false&case=true

Comment 62

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34619/#review31443

> So I don't think we need any this kind of pref.

Okay, I've removed the pref.

> So I would always return true here if element's NodePrincipal() isn't chrome principal.
> 
> And just drop all the permission handling
> 
> I would prefer if the isolation state was store somewhere, perhaps in frameloader, since changing the attribute doesn't in fact change the state.

Okay, I have changed to only check for chrome principal.  I left the check as a method on the frame element, since that seemed to parallel the other browser status checks, but can move it you'd still prefer it.

Comment 63

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34613/#review31433

Fixed commit message.

> Right, it's not yet a thing.  It's added in the later patch called "Set tab context's isolation from frame attr", currently set for review by smaug.  (I wasn't really sure how to choose reviewers for some this without making things really confusing...)

I changed the attribute name to "noisolation".

Comment 64

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34621/#review31457

> hmm, so don't we want to remove GetIsBrowserElement()?
> Otherwise it is error prone to check either
> IsBrowserElement or IsIsolatedBrowserElement. One will easily forget there are them both.
> Could we have IsNonIsolatedBrowserElement() and IsIsolatedBrowserElement() or something like that?
> 
> ...expect... looks like nsDocShell::GetIsBrowserElement is called only by frameloader, so could we just remove it?

Yes, `DS::GetIsBrowserElement` can be removed, since the frame loader case is using the isolated version now.  I'll remove it.

Comment 65

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34625/#review31445

I thought about this for a while, but I did not see a good way to add an assert.  It seems like we just don't have enough information from the principal alone.  I at least added a warning when apps are used that we are now assuming `inIsolatedMozBrowser` is the same as `mozbrowser frame`.

If you have ideas on how we could make the checks stronger, I am happy to do so!

Comment 66

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718600 [details]
MozReview Request: Bug 1238160 - Test mozbrowser APIs to ensure no content exposure. r=bz

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

Comment 67

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718601 [details]
MozReview Request: Bug 1238160 - Enable mozbrowser frames on desktop. r=bz

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

Comment 68

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718602 [details]
MozReview Request: Bug 1238160 - Rename OriginAttributes.mInBrowser and associated methods. r=bz,mayhemer

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

Comment 69

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718603 [details]
MozReview Request: Bug 1238160 - Rename nsILoadContext::GetIsInBrowserElement. r=smaug,mayhemer

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

Comment 70

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718605 [details]
MozReview Request: Bug 1238160 - Set tab context's isolation from frame attr. r=smaug

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

Comment 71

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718607 [details]
MozReview Request: Bug 1238160 - Set docshell isolation mode. r=smaug

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

Comment 72

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718608 [details]
MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

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

Comment 73

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718609 [details]
MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice

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

Comment 74

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718610 [details]
MozReview Request: Bug 1238160 - Test frame principal when toggling isolation. r=bz

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

Comment 75

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/34625/#review31445

I was thinking that we could assert that isolation disabling is not supported on platforms where apps are used.  Those are the two things that can't coexist, right?   Neither of those things depends on the principal at all, afaict...

Comment 76

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34625/#review31445

Okay, I've updated patch to use assertions.  I had to limit assertions to MOZ_B2G only, since it's currently still possible to have an app on desktop (pending bug 1238079).

Comment 77

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718609 [details]
MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice

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

Comment 78

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8718610 [details]
MozReview Request: Bug 1238160 - Test frame principal when toggling isolation. r=bz

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

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718605 [details]
MozReview Request: Bug 1238160 - Set tab context's isolation from frame attr. r=smaug

https://reviewboard.mozilla.org/r/34619/#review32361

Comment 80

[Honza Bambas (:mayhemer)]

Comment on attachment 8718608 [details]
MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34623/#review32463

Comment 81

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8718608 [details]
MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34623/#review32797

Comment 82

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718609 [details]
MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice

https://reviewboard.mozilla.org/r/34625/#review33265

::: dom/apps/AppsUtils.jsm:349
(Diff revision 3)
> +    for (let id in aApps) {
> +      return true;
> +    }
> +    return false;

Is this better than Object.getOwnPropertyNames(aApps).length > 0?  I guess either one is ok; this one is a bit more mysterious.

::: dom/base/nsFrameLoader.cpp:1657
(Diff revision 3)
> +    return true;

Should this be "return false"?  Seems like it should, so the assert code doesn't change behavior...

r=me

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718607 [details]
MozReview Request: Bug 1238160 - Set docshell isolation mode. r=smaug

https://reviewboard.mozilla.org/r/34621/#review32371

::: docshell/base/nsDocShell.cpp:6120
(Diff revision 2)
> -    if (!docshell->GetIsBrowserOrApp()) {
> +    if (!docshell->GetIsMozBrowserOrApp()) {

Please test that we set active state properly even in non-isolated mozbrowser frames. Just some manual test or so.

::: docshell/base/nsDocShell.cpp:13924
(Diff revision 2)
> +{

Could we MOZ_ASSERT here that if mIsInIsolatedMozBrowser is true, mFrameType is eFrameTypeBrowser

::: docshell/base/nsDocShell.cpp:13935
(Diff revision 2)
> +  *aIsInIsolatedMozBrowserElement = result;

similar assertion here, but use GetInheritedFrameType() and not mFrameType

::: docshell/base/nsDocShell.cpp:14004
(Diff revision 2)
>    }

I think I'd prefer something like
attrs.mInIsolatedMozBrowser = mIsInIsolatedMozBrowser;
MOZ_ASSERT(!mIsInIsolatedMozBrowse || mFrameType == eFrameTypeBrowser);
without the 'if'.

Comment 84

[[:fabrice] Fabrice Desré]

Comment on attachment 8718609 [details]
MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice

https://reviewboard.mozilla.org/r/34625/#review33817

Comment 85

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34625/#review33265

> Is this better than Object.getOwnPropertyNames(aApps).length > 0?  I guess either one is ok; this one is a bit more mysterious.

Your version does seem more readable. :) I have changed to that.

> Should this be "return false"?  Seems like it should, so the assert code doesn't change behavior...

Yeah, that does seems simpler to follow.  The default case today where isloation is still enabled has already exited the function by the earlier return above.  I'll add some more comments to the various if {} blocks here.

Comment 86

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/34621/#review32371

> Please test that we set active state properly even in non-isolated mozbrowser frames. Just some manual test or so.

It appears to work correctly in my manual testing.

> Could we MOZ_ASSERT here that if mIsInIsolatedMozBrowser is true, mFrameType is eFrameTypeBrowser

Okay, assert added.

> similar assertion here, but use GetInheritedFrameType() and not mFrameType

Okay, assert added.

> I think I'd prefer something like
> attrs.mInIsolatedMozBrowser = mIsInIsolatedMozBrowser;
> MOZ_ASSERT(!mIsInIsolatedMozBrowse || mFrameType == eFrameTypeBrowser);
> without the 'if'.

Okay, I've made these changes.

Comment 87

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 88

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #86)
> > Could we MOZ_ASSERT here that if mIsInIsolatedMozBrowser is true, mFrameType is eFrameTypeBrowser
> 
> Okay, assert added.
> 
> > similar assertion here, but use GetInheritedFrameType() and not mFrameType
> 
> Okay, assert added.
> 
> > I think I'd prefer something like
> > attrs.mInIsolatedMozBrowser = mIsInIsolatedMozBrowser;
> > MOZ_ASSERT(!mIsInIsolatedMozBrowse || mFrameType == eFrameTypeBrowser);
> > without the 'if'.
> 
> Okay, I've made these changes.

Actually, I've remembered that we can't add all of these assertions.  Here's an example scenario:

<iframe mozbrowser>: eFrameTypeBrowser, mIsInIsolatedMozBrowser == true
  <iframe>:          eFrameTypeRegular, mIsInIsolatedMozBrowser == true

mIsInIsolatedMozBrowser is passed along to regular <iframes> inside of the <iframe mozbrowser>, so it's not correct to say mIsInIsolatedMozBrowser can only be true if the current frame is eFrameTypeBrowser.

The assertion in GetIsInIsolatedMozBrowserElement() should still be valid, since you should always be *inside* a browser frame if mIsInIsolatedMozBrowser is true.  I'll remove the others.

Comment 89

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 90

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d40b29bc4527
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b296adf9b33
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a21a014436
https://hg.mozilla.org/integration/mozilla-inbound/rev/267ab6361fa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b3d7cbff15
https://hg.mozilla.org/integration/mozilla-inbound/rev/981fee6f681f
https://hg.mozilla.org/integration/mozilla-inbound/rev/76944f0d24ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/80f7b29a27f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/14eddac51e70

Comment 91

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d40b29bc4527
https://hg.mozilla.org/mozilla-central/rev/9b296adf9b33
https://hg.mozilla.org/mozilla-central/rev/b2a21a014436
https://hg.mozilla.org/mozilla-central/rev/267ab6361fa1
https://hg.mozilla.org/mozilla-central/rev/86b3d7cbff15
https://hg.mozilla.org/mozilla-central/rev/981fee6f681f
https://hg.mozilla.org/mozilla-central/rev/76944f0d24ab
https://hg.mozilla.org/mozilla-central/rev/80f7b29a27f1
https://hg.mozilla.org/mozilla-central/rev/14eddac51e70

Comment 92

[Chris Mills [:cmills]]

I've moved the Browser API pages back into the general web APIs section, and out of the Firefox OS-only APIs section:

https://developer.mozilla.org/en-US/docs/Web/API/Browser_API
https://developer.mozilla.org/en-US/docs/Web/API/Using_the_Browser_API

I've also updated the wording to show that it is available in Fx 47 chrome code too.

I've also updated the descriptions and compat info on HTMLIFrameElement, and added a note to the 47 release notes.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement
https://developer.mozilla.org/en-US/Firefox/Releases/47#Others

Does this look ok? Am I missing anything?

Note: Yes, this still isn't really a _web_ API, but I've put it in Web/API/ for now because we don't really have anywhere to put APIs that aren't web APIs, aren't Firefox OS-specific, and aren't add-on/devtool-specific. If we just shoved it randomly under Gecko/, say, it would be much harder to find.

Comment 93

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #92)
> Does this look ok? Am I missing anything?

Looks good to me!  Thanks Chris.

Comment 94

[(no longer active)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #92)
> Note: Yes, this still isn't really a _web_ API, but I've put it in Web/API/
> for now because we don't really have anywhere to put APIs that aren't web
> APIs, aren't Firefox OS-specific, and aren't add-on/devtool-specific. If we
> just shoved it randomly under Gecko/, say, it would be much harder to find.

I think that's better than having it be called a Web API.  I think not confusing Web developers is more important than our own convenience here.  Google search is usually very good at finding MDN contact anyway.

Comment 95

[Chris Mills [:cmills]]

(In reply to :Ehsan Akhgari from comment #94)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #92)
> > Note: Yes, this still isn't really a _web_ API, but I've put it in Web/API/
> > for now because we don't really have anywhere to put APIs that aren't web
> > APIs, aren't Firefox OS-specific, and aren't add-on/devtool-specific. If we
> > just shoved it randomly under Gecko/, say, it would be much harder to find.
> 
> I think that's better than having it be called a Web API.  I think not
> confusing Web developers is more important than our own convenience here. 
> Google search is usually very good at finding MDN contact anyway.

I think you have a point; to fix this, I've moved it under a new API listing, here:

https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/Chrome/API/Browser_API

We'll add more content here as appropriate.

Comment 96

[Brett Zamir]

Congratulations...This is very exciting! Any chance this will make us closer to getting the bug 618354 feature added out of this?