iframe.mozAllowFullScreen should work across process boundaries

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
mozilla18
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 666756 [details] [diff] [review]
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).
Attachment #666756 - Flags: review?(justin.lebar+bug)
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 on attachment 666756 [details] [diff] [review]
Patch v1: Make iframe.mozallowfullscreen percolate across process boundaries

f?cjones re comment 2.
Attachment #666756 - Flags: feedback?(jones.chris.g)
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.
Attachment #666756 - Flags: feedback?(jones.chris.g) → feedback+
>+  /**
>+   * 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.
Attachment #666756 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 6

6 years ago
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.
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
This is a major security issue.
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
(Assignee)

Comment 8

6 years ago
Created attachment 668277 [details] [diff] [review]
Patch v2: Make iframe.mozallowfullscreen percolate across process boundaries

Updated with review comments addressed. Carrying forward r=jlebar
Attachment #666756 - Attachment is obsolete: true
Attachment #668277 - Flags: review+
(Assignee)

Comment 9

6 years ago
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...
(Assignee)

Comment 10

6 years ago
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...).
(Assignee)

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/454595726fd9
https://hg.mozilla.org/mozilla-central/rev/454595726fd9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.