Closed Bug 1238160 Opened 8 years ago Closed 8 years ago

Support frames similar to mozbrowser on desktop Firefox

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [multiviewport] [mvp-rdm])

User Story

Desired Features:

* An <html:iframe> element
* Must support OOP content, likely via <iframe remote> attribute
* Must act as a content sandbox suitable for loading general web content, like we have with <iframe mozbrowser>
* Should support the BrowserElement API (from BrowserElement.webidl)
* Must be possible to add such an element to a chrome://*.xhtml page in a content docshell
* Must load appropriate (i.e. global) frame scripts, just like other existing frames
* Must access the same cookie jar, etc. as a <xul:browser> would

Exposure to Content:

* Web content loaded in the frame must not be able to detect any difference between this concept and a <xul:browser>

Attachments

(10 files, 2 obsolete files)

1.15 KB, text/plain
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
mayhemer
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
mayhemer
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
smaug
: review+
Details
58 bytes, text/x-review-board-request
mayhemer
: review+
billm
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
fabrice
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
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
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.
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.
(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?
(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.
Flags: needinfo?(ptheriault)
Flags: needinfo?(kchen)
Flags: needinfo?(bugs)
(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.
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.
(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.
(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.)
(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?
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.
(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).
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).
(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.
(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.
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.
Flags: needinfo?(kchen)
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)
Flags: needinfo?(bugs)
(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.
Summary: Enable mozbrowser frames on desktop Firefox → Support frames similar to mozbrowser on desktop Firefox
User Story: (updated)
User Story: (updated)
(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>?
Attachment #8707557 - Flags: feedback?(ehsan)
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.
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.
(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).
(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 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.
Attachment #8707557 - Flags: feedback?(ehsan) → feedback-
(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.
Flags: needinfo?(ehsan)
(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.  :-)
Flags: needinfo?(ehsan)
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).
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Blocks: 1237360
Whiteboard: [multiviewport]
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.
Flags: needinfo?(jonas)
Flags: needinfo?(ehsan)
Iteration: --- → 47.1 - Feb 8
Flags: qe-verify-
Priority: -- → P1
(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!
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(jonas)
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
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
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/
Attachment #8718600 - Flags: review?(bzbarsky)
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/
Attachment #8718602 - Flags: review?(honzab.moz)
Attachment #8718602 - Flags: review?(bzbarsky)
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/
Attachment #8718603 - Flags: review?(honzab.moz)
Attachment #8718603 - Flags: review?(bugs)
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/
Attachment #8718604 - Flags: review?(wmccloskey)
Attachment #8718604 - Flags: review?(honzab.moz)
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/
Attachment #8718605 - Flags: review?(bugs)
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/
Attachment #8718607 - Flags: review?(bugs)
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/
Attachment #8718608 - Flags: review?(wmccloskey)
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/
Attachment #8718609 - Flags: review?(bzbarsky)
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/
Attachment #8718610 - Flags: review?(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.
Attachment #8718600 - Flags: review?(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
Attachment #8718601 - Flags: review?(bzbarsky) → review+
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.
Attachment #8718600 - Flags: review+
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.
Attachment #8718602 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #8718605 - Flags: review?(bugs)
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-)
Attachment #8718605 - Flags: review-
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.
Attachment #8718609 - Flags: review?(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.
Attachment #8718610 - Flags: review?(bzbarsky) → review+
Comment on attachment 8718603 [details]
MozReview Request: Bug 1238160 - Rename nsILoadContext::GetIsInBrowserElement. r=smaug,mayhemer

https://reviewboard.mozilla.org/r/34615/#review31453
Attachment #8718603 - Flags: review?(bugs) → review+
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?
Attachment #8718607 - Flags: review?(bugs)
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
Attachment #8718602 - Flags: review?(honzab.moz) → review+
(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.
Attachment #8718603 - Flags: review?(honzab.moz) → review+
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
Attachment #8718604 - Flags: review?(honzab.moz)
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 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.
Attachment #8718604 - Flags: review?(wmccloskey) → review+
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.
Attachment #8718608 - Flags: review?(wmccloskey)
:billm requested on IRC that the TabContext comments also be updated to explain where the xul:browser case fits in for each method.
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.
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
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.
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".
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.
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 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 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 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 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 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/
Attachment #8718605 - Flags: review- → review?(bugs)
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/
Attachment #8718607 - Flags: review- → review?(bugs)
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/
Attachment #8718608 - Attachment description: MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm → MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer
Attachment #8718608 - Flags: review?(wmccloskey)
Attachment #8718608 - Flags: review?(honzab.moz)
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 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/
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...
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 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/
Attachment #8718609 - Attachment description: MozReview Request: Bug 1238160 - Add TODOs in non-desktop code paths. r=bz → MozReview Request: Bug 1238160 - Add assertions in non-desktop code paths. r=bz,fabrice
Attachment #8718609 - Flags: review?(fabrice)
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 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
Attachment #8718605 - Flags: review?(bugs) → review+
Comment on attachment 8718608 [details]
MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34623/#review32463
Attachment #8718608 - Flags: review?(honzab.moz) → review+
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
Comment on attachment 8718608 [details]
MozReview Request: Bug 1238160 - Set frame type on TabContext. r=billm,mayhemer

https://reviewboard.mozilla.org/r/34623/#review32797
Attachment #8718608 - Flags: review?(wmccloskey) → review+
Attachment #8718609 - Flags: review?(bzbarsky) → review+
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 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'.
Attachment #8718607 - Flags: review?(bugs) → review+
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
Attachment #8718609 - Flags: review?(fabrice) → review+
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.
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.
(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.
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
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.
(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.
(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.
Flags: needinfo?(ptheriault)
(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.
Congratulations...This is very exciting! Any chance this will make us closer to getting the bug 618354 feature added out of this?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.