795184 - iframe.mozAllowFullScreen should work across process boundaries

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

iframe.mozAllowFullScreen is currently ignored across process boundaries; content in cross process iframes don't have the mozAllowFullscreen attribute can still go fullscreen.

iframe.mozAllowFullScreen should be percolated across process boundaries.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1: Make iframe.mozallowfullscreen percolate across process boundaries

* Move the code that checks if enclosing iframes have the mozallowfullscreen attribute from nsDocument to nsDocShell.
* Have the BrowserElementChild request value of the parent process' containing iframe's mozallowfullscreen attribute, and stash it in the child's docshell.
* Have the code to check for mozallowfullscreen (now in nsDocShell) check if it's a content boundary, and if so check the stashed value of the parent's mozallowfullscreen attribute. Non content boundaries don't check for a stashed value, and won't stash a value.

Without this patch, anything inside a mozbrowser iframe is able to request fullscreen and enter fullscreen regardless of whether the parent frame contains the mozallowfullscreen attribute.

This requires the following Gaia commit in order for pages viewed in the Gaia browser app to go fullscreen:

https://github.com/cpearce/gaia/commit/f67b7244307056188cd66cfa6e1523d5c3cba6cb

We create iframes in a few other places in Gaia, but none which obviously (to me at least) should or need to have mozallowfullscreen=true on them.

With this patch applied, not having mozallowfullscreen on a mozapp iframe would cause fullscreen requests inside the iframe in the app origin to be denied. But thankfully we set mozallowfullscreen on iframes created for apps in the Gaia window manager [https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L764], so fullscreen requests inside apps in the app origin will still be granted.

(I could have being inside the app origin override the presence of mozallowfullscreen in the parent if you like, I figured someone in B2G should make this decision rather than I).

Comment 2

[Justin Lebar (not reading bugmail)]

cjones, this adds another sync message on BEC startup.  I haven't been following your benchmarking efforts there -- were the sync messages significant?  If so, we could compact these two sync messages into one without much trouble, I think.  But if the sync messages aren't significant, this is cleaner.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 666756 [details] [diff] [review]
Patch v1: Make iframe.mozallowfullscreen percolate across process boundaries

f?cjones re comment 2.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 666756 [details] [diff] [review]
Patch v1: Make iframe.mozallowfullscreen percolate across process boundaries

They did not even begin to show up on the startup timeline.

Comment 5

[Justin Lebar (not reading bugmail)]

>+  /**
>+   * Attribute that determines whether fullscreen is allowed to be entered for
>+   * this subtree of the docshell tree. This is true when all iframes containing
>+   * this docshell have their "mozallowfullscreen" attribute set to "true".
>+   * fullscreenAllowed is only writable at content boundaries, where it is used
>+   * to propagate the value of the cross process parent's iframe's
>+   * "mozallowfullscreen" attribute to the child process. Setting
>+   * fullscreenAllowed on docshells which aren't content boundaries throws an
>+   * exception.
>+   */
>+  attribute boolean fullscreenAllowed;

If you make this [infallible], then you can call GetFullscreenAllowed() off
nsIDocShell, which is nice.  (Perhaps confusingly, [infallible] is a modifier
only on the getter.)

You'd then have to modify your code so that the whole implementation goes
inside nsresult GetFullscreenAllowed(bool* aOut) -- [infallible] will generate
|bool GetFullscreenAllowed()| for you.

Up to you.

>@@ -781,16 +783,17 @@ protected:
>     bool                       mCreated;
>     bool                       mAllowSubframes;
>     bool                       mAllowPlugins;
>     bool                       mAllowJavascript;
>     bool                       mAllowMetaRedirects;
>     bool                       mAllowImages;
>     bool                       mAllowDNSPrefetch;
>     bool                       mAllowWindowControl;
>+    bool                       mFullscreenAllowed;

At the very least, we need a comment here saying that mFullscreenAllowed is
valid only if mAppId != 0 || mIsBrowserFrame.  But it would be a lot clearer to
me if we explicitly encoded this by making mFullscreenAllowed have three
possible states: ALLOWED, NOT_ALLOWED, and UNSET or CHECK_ATTRIBUTES.
Logically, that's what this variable encodes, and explicit is better than
implicit.

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>@@ -2150,16 +2151,85 @@ NS_IMETHODIMP nsDocShell::GetAllowWindow
> }
> 
> NS_IMETHODIMP nsDocShell::SetAllowWindowControl(bool aAllowWindowControl)
> {
>     mAllowWindowControl = aAllowWindowControl;
>     return NS_OK;
> }
> 
>+bool
>+nsDocShell::GetFullscreenAllowed()
>+{
>+    nsCOMPtr<nsIDocShell> docShell = this;
>+    while (docShell) {
>+        // Process boundaries have their mFullscreenAllowed retrieved from their
>+        // corresponding iframe in their parent process upon creation.
>+        if (docShell->GetIsContentBoundary()) {

IsContentBoundary() is not the same as a process boundary.

>+            return static_cast<nsDocShell*>(docShell.get())->mFullscreenAllowed;
>+        }
>+
>+        // For non-process boundaries, check that the enclosing iframe element
>+        // has the mozallowfullscreen attribute set to true. If any ancestor
>+        // iframe does not have mozallowfullscreen=true, then fullscreen is
>+        // prohibited.

ibid.

>+        // If we have no parent then we're the root docshell; no ancestor of the
>+        // original docshell doesn't have a mozallowfullscreen attribute, so
>+        // return true.
>+        nsCOMPtr<nsIDocShellTreeItem> dsti = do_QueryInterface(docShell);
>+        if (!dsti) {
>+            return false;
>+        }

You don't expect this QI to fail, so make it NS_ENSURE_TRUE(dsti, false)?

>+        nsCOMPtr<nsIDocShellTreeItem> parentTreeItem;
>+        dsti->GetParent(getter_AddRefs(parentTreeItem));
>+        if (!parentTreeItem) {
>+            return true;
>+        }
>+        // Otherwise, we have a parent, continue the checking for
>+        // mozFullscreenAllowed in the parent docshell's ancestors.
>+        docShell = do_QueryInterface(parentTreeItem);
>+    }
>+    return true;

It seems simpler to me to write this recursively -- then you don't have to do
the static cast.

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>--- a/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -69,16 +69,18 @@ BrowserElementChild.prototype = {
>   _init: function() {
>     debug("Starting up.");
>     sendAsyncMsg("hello");
> 
>     // Set the docshell's name according to our <iframe>'s name attribute.
>     docShell.QueryInterface(Ci.nsIDocShellTreeItem).name =
>       sendSyncMsg('get-name')[0];
> 
>+    docShell.fullscreenAllowed = sendSyncMsg('get-fullscreen-allowed')[0];
>+
>     BrowserElementPromptService.mapWindowToBrowserElementChild(content, this);
> 
>     docShell.QueryInterface(Ci.nsIWebProgress)
>             .addProgressListener(this._progressListener,
>                                  Ci.nsIWebProgress.NOTIFY_LOCATION |
>                                  Ci.nsIWebProgress.NOTIFY_SECURITY |
>                                  Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
> 
>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js
>@@ -354,16 +355,20 @@ BrowserElementParent.prototype = {
>     if (this._window.document.mozHidden) {
>       this._ownerVisibilityChange();
>     }
>   },
> 
>   _recvGetName: function(data) {
>     return this._frameElement.getAttribute('name');
>   },
>+  
>+  _recvGetFullscreenAllowed: function(data) {
>+    return this._frameElement.getAttribute('mozallowfullscreen');

Do the bool conversion here (i.e., return !!this._frameElement) so we actually
return a bool?

At this point, we have a (perhaps nominal) rule that all testable B2G-related
changes must be accompanied by tests.  I'm not going to r- this patch because
it has no tests, but I would really like some for all this functionality we've
been adding.  We've already regressed it once (albeit mostly intentionally).

r=me with nits addressed.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Requesting blocking since this affect b2g and requires Gaia changes.

Without the changes in this bug there's no way for iframes with cross process content to prohibit their content from entering fullscreen.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is a major security issue.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2: Make iframe.mozallowfullscreen percolate across process boundaries

Updated with review comments addressed. Carrying forward r=jlebar

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pull request for Gaia change at:
https://github.com/mozilla-b2g/gaia/pull/5684

Ideally we should get that merged before we land the gecko change here...

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 668277 [details] [diff] [review]
Patch v2: Make iframe.mozallowfullscreen percolate across process boundaries

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

::: dom/browser-element/BrowserElementParent.js
@@ +361,5 @@
>      return this._frameElement.getAttribute('name');
>    },
> +  
> +  _recvGetFullscreenAllowed: function(data) {
> +    return !!this._frameElement.getAttribute('mozallowfullscreen');

Actually this needs to be hasAttribute() rather than getAttribute(), since the "mozallowfullscreen" attribute is a "boolean attribute", where the presence of the attribute means true, rather than the attributes' value being true (so mozallowfullscreen=false is still supposed to evaluate to true, since it means the mozallowfullscreen attribute is present...).

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed to m-i with s/getAttribute('mozallowfullscreen')/hasAttribute('mozallowfullscreen')/ in BrowserElementParent.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/454595726fd9

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/454595726fd9