753546 - Fullscreen mode just broke for file:/// urls

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alon Zakai (:azakai)]

On today's nightly, there is a new prompt for fullscreen. It works on web content but on file:// urls it fails, pressing allow has no effect (the prompt stays up). No errors in web console.

Judging by the timeline, perhaps related to bug 746885?

For a testcase, you can for example download

  http://syntensity.com/static/bb/client.html
  http://syntensity.com/static/bb/client.data

into some dir, and browse with a file:// url to client.html

Comment 1

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

Thanks for filing.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I will bet that https://bug746885.bugzilla.mozilla.org/attachment.cgi?id=621920 ends up throwing for a file:// URI or something...

Comment 3

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

Yup, that's exactly the problem. :)

Comment 4

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

We also need to handle other schemas, for example this data URI:
data:text/html,<div id="d"><button onclick="document.getElementById('d').mozRequestFullScreen();">press me</button></div>

Comment 5

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

Hmm... So when a site wants to lock the pointer but fullscreen has not yet been approved, we'll set a listener waiting on "perm-changed" notification which resumes the pointer lock request once the site's domain's had the "fullscreen" permission added/denied for its host.

So is it safe to just assume documents whose principal's uri's host is null are allowed/approved to go fullscreen? Then we can not show the approval UI and just approve pointer lock in that case.

Comment 6

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

Attachment: Patch 1 v1: Assume documents with a null principal's uri's host are approved for fullscreen and thus pointer lock.

Content changes; when requesting pointer lock, check if the document's principal's uri's host is null, and if so assume it's a local file or local data uri, and assume it's approved for fullscreen.

Comment 7

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

Attachment: Patch 2 v1: Don't show fullscreen approval UI for documents with a null principal's uri's host

/browser changes; Don't show approval UI when loading a document whose principal's uri has a null host, since we can't add permissions to that host anyway.

We'll still show the auto-hiding entered fullscreen warning.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

There are nsIURIs (those implemented by nsSimpleURI, e.g. for data: URIs) for which getting .host throws. Doesn't this front-end code need to take that into account?

Comment 9

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

Attachment: Patch 2 v2: Don't show fullscreen approval UI for documents with a null principal's uri's host

Ok, wrap the read of uri.host in try/catch.

Comment 10

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

Comment on attachment 622618 [details] [diff] [review]
Patch 1 v1: Assume documents with a null principal's uri's host are approved for fullscreen and thus pointer lock.

This looks a bit scary.
Couldn't we limit file:// permission to a directory or some such?
Also, I'd prefer whitelisting protocols, not allow all without host.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 622623 [details] [diff] [review]
Patch 2 v2: Don't show fullscreen approval UI for documents with a null principal's uri's host

This seems to auto-approve fullscreen for data: URIs. Why is that safe? I'm not sure I understand why "no host" in general means "safe to approve", either - maybe you can make that assumption for file:/// URIs, but that isn't necessarily the only case where "host" will return null.

And what does "locally hosted data: URI" mean?

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #10)
> Couldn't we limit file:// permission to a directory or some such?

We could add that capability to the permission manager. Perhaps it's better to do it on an exact file basis, rather than directory basis; it doesn't make sense to grant a privilege to the whole of the user's Downloads folder for example...

> Also, I'd prefer whitelisting protocols, not allow all without host.

Ok. Probably better to only enable permanent permission grants to file:// and principal URIs with a host then (i.e. on everything that the permission manager can/will handle), and require explicit approval every time in all other cases..

Comment 13

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> Comment on attachment 622623 [details] [diff] [review]
> Patch 2 v2: Don't show fullscreen approval UI for documents with a null
> principal's uri's host
> 
> This seems to auto-approve fullscreen for data: URIs. Why is that safe? I'm
> not sure I understand why "no host" in general means "safe to approve",
> either - maybe you can make that assumption for file:/// URIs, but that
> isn't necessarily the only case where "host" will return null.
> 
> And what does "locally hosted data: URI" mean?

If a data URI's is linked to from some webpage, its principal inherits the domain of that webpage, i.e. it's still tainted with the domain.

Whereas a data uri entered by the user into the address bar has a null principal (IIRC). This is what I mean by "locally hosted data URI", probably not the clearest name sorry.

Comment 14

[Boris Zbarsky [:bzbarsky]]

URI_IS_LOCAL_FILE or something please, not file://

Comment 15

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

Attachment: Patch 1 v2 - Make nsDocument listen for 'fullscreen-approved' notification rather than 'perm-changed'

I'm not keen on modifying the permission manager to handle file URIs, since then everywhere we use the permission manager we will spontaneously start granting permissions to file URIs, and that may not be expected or a good thing.

So instead, require all fullscreen requests from documents with a URI without a host to be approved upon every request. This will be slightly inconvenient for developers, but not for users.

To do this, I change the pending pointer lock code to listen for a "fullscreen-approved" notification to be dispatched with a subject which is the document which requested fullscreen. This then signifies that the fullscreen request has been approved.

We remove the "fullscreen-approved" observer when we exit fullscreen, so notifications can't arrive after we've left fullscreen.

We still use the permission manager to store whether a URI with a host has already been granted fullscreen, so Gecko knows whether subsequent fullscreen requests have been pre-approved.

Comment 16

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

Attachment: Patch 2 v2 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host

Don't allow fullscreen approval decision to be remembered for document's which have a URI that the permission manager can't handle. These document's require approval upon each fullscreen request.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 623995 [details] [diff] [review]
Patch 2 v2 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host

>+  getDisplayHost: function(uri) {
>+    if (uri.scheme == "file")
>+      return uri.path;
>+    let utils = {};
>+    Cu.import("resource://gre/modules/DownloadUtils.jsm", utils);
>+    return utils.DownloadUtils.getURIHost(uri.spec)[0];
>+  },

file URIs are often rather long. Can we just hide the "... is now fullscreen" label for them?

>+    let displayHost = this.getDisplayHost(this.fullscreenDoc.nodePrincipal.URI);

>+      host = this.fullscreenDoc.nodePrincipal.URI.host;

>+      Services.perms.testPermission(this.fullscreenDoc.nodePrincipal.URI, "fullscreen") == Services.perms.ALLOW_ACTION;

assign this.fullscreenDoc.nodePrincipal.URI to a locale variable

Comment 18

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

(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 623995 [details] [diff] [review]
> Patch 2 v2 - Don't enable fullscreen approval decision to be remembered for
> document's whose URI doesn't have a host

> file URIs are often rather long. Can we just hide the "... is now
> fullscreen" label for them?

Just so I'm clear, do you mean: hide the "$domain has entered fullscreen" label completely for files, so we don't display "$file_path is now fullscreen" on the approval UI? Or did you mean remove the "is now fullscreen" suffix from the string?

Comment 19

[Dão Gottwald [:dao]]

(In reply to Chris Pearce (:cpearce) from comment #18)
> (In reply to Dão Gottwald [:dao] from comment #17)
> > Comment on attachment 623995 [details] [diff] [review]
> > Patch 2 v2 - Don't enable fullscreen approval decision to be remembered for
> > document's whose URI doesn't have a host
> 
> > file URIs are often rather long. Can we just hide the "... is now
> > fullscreen" label for them?
> 
> Just so I'm clear, do you mean: hide the "$domain has entered fullscreen"
> label completely for files, so we don't display "$file_path is now
> fullscreen" on the approval UI?

yes

Comment 20

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

Attachment: Patch 2 v3 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 625060 [details] [diff] [review]
Patch 2 v3 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host

>     // Note: the warning box can be non-null if the warning box from the previous request
>     // wasn't hidden before another request was made.
>     if (!this.warningBox) {
>       this.warningBox = document.getElementById("full-screen-warning-container");
>       // Add a listener to clean up state after the warning is hidden.
>       this.warningBox.addEventListener("transitionend", this);
>-      this.warningBox.removeAttribute("hidden");
>     }

>+    this.warningBox.removeAttribute("hidden");

This change is unrelated to this bug, isn't it?

Comment 22

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

Attachment: Patch 2 v4 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host

Comment 23

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

Attachment: Patch 3 v1: fullscreen approved flag (for pointer lock in local files)

Sorry, I discovered that I'd missed a case: pointer lock in a local file. I was still checking for the permission grant in requestPointerLock (d'oh!), so we're still broken for local files since they can't have permissions granted in the permission manager.

So I need to add a flag to nsDocument to denote when fullscreen is approved, and have all fullscreen documents observe the "fullscreen-approved" notification (not just observing when waiting on pending pointer lock requests) an use that to denote whether we've been approved for fullscreen and pointer lock.

This is based on top of the previous two patches.

Comment 24

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

Comment on attachment 625583 [details] [diff] [review]
Patch 3 v1: fullscreen approved flag (for pointer lock in local files)


>+nsDocument::AddFullscreenApprovedObserver()
>+{
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIObserver> obs(do_QueryInterface(static_cast<nsIDocument*>(this)));
>+  NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE);
'this' is nsDocument , and nsDocument is nsIObserver so QI shouldn't be needed.
You could pass 'this' to AddObserver


>+nsDocument::RemoveFullscreenApprovedObserver()
>+{
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIObserver> obs(do_QueryInterface(static_cast<nsIDocument*>(this)));
>+  NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE);
>+
>+  nsresult res = os->RemoveObserver(obs, "fullscreen-approved");
>+  NS_ENSURE_SUCCESS(res, res);

same here.

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/451145d2c3ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/90564ac34b06
https://hg.mozilla.org/integration/mozilla-inbound/rev/67cd8131a3d5

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/451145d2c3ff
https://hg.mozilla.org/mozilla-central/rev/90564ac34b06
https://hg.mozilla.org/mozilla-central/rev/67cd8131a3d5