Fullscreen mode just broke for file:/// urls

RESOLVED FIXED in Firefox 15

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: azakai, Assigned: cpearce)

Tracking

unspecified
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

4.24 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.47 KB, patch
dao
: review+
Details | Diff | Splinter Review
16.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

5 years ago
Thanks for filing.
Assignee: nobody → cpearce
OS: Linux → All
Hardware: x86 → All
I will bet that https://bug746885.bugzilla.mozilla.org/attachment.cgi?id=621920 ends up throwing for a file:// URI or something...
(Assignee)

Comment 3

5 years ago
Yup, that's exactly the problem. :)
Assignee: cpearce → nobody
Component: DOM: Core & HTML → General
Product: Core → Firefox
QA Contact: general → general
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

5 years ago
Created 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.

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.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #622618 - Flags: review?(bugs)
(Assignee)

Comment 7

5 years ago
Created attachment 622619 [details] [diff] [review]
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.
Attachment #622619 - Flags: review?(dao)
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?
(Assignee)

Comment 9

5 years ago
Created attachment 622623 [details] [diff] [review]
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.
Attachment #622619 - Attachment is obsolete: true
Attachment #622619 - Flags: review?(dao)
Attachment #622623 - Flags: review?(dao)

Updated

5 years ago
Attachment #622623 - Flags: review?(dao) → review+
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 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?
(Assignee)

Comment 12

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

Comment 13

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
URI_IS_LOCAL_FILE or something please, not file://
(Assignee)

Comment 15

5 years ago
Created attachment 623994 [details] [diff] [review]
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.
Attachment #622618 - Attachment is obsolete: true
Attachment #622623 - Attachment is obsolete: true
Attachment #622618 - Flags: review?(bugs)
Attachment #623994 - Flags: review?(bugs)
(Assignee)

Comment 16

5 years ago
Created 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

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.
Attachment #623995 - Flags: review?(dao)

Updated

5 years ago
Attachment #623994 - Flags: review?(bugs) → review+
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
(Assignee)

Comment 18

5 years ago
(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?
(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
(Assignee)

Comment 20

5 years ago
Created 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
Attachment #625060 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #623995 - Attachment is obsolete: true
Attachment #623995 - Flags: review?(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?
(Assignee)

Comment 22

5 years ago
Created attachment 625265 [details] [diff] [review]
Patch 2 v4 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host
Attachment #625060 - Attachment is obsolete: true
Attachment #625060 - Flags: review?(dao)
Attachment #625265 - Flags: review?(dao)

Updated

5 years ago
Attachment #625265 - Flags: review?(dao) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 625583 [details] [diff] [review]
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.
Attachment #625583 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Blocks: 756992
(Assignee)

Updated

5 years ago
Blocks: 756993
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.
Attachment #625583 - Flags: review?(bugs) → review+
(Assignee)

Comment 25

5 years ago
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
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/451145d2c3ff
https://hg.mozilla.org/mozilla-central/rev/90564ac34b06
https://hg.mozilla.org/mozilla-central/rev/67cd8131a3d5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.