Last Comment Bug 753546 - Fullscreen mode just broke for file:/// urls
: Fullscreen mode just broke for file:/// urls
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Chris Pearce (:cpearce)
:
:
Mentors:
Depends on:
Blocks: 756992 756993
  Show dependency treegraph
 
Reported: 2012-05-09 14:33 PDT by Alon Zakai (:azakai)
Modified: 2012-05-22 06:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 v1: Assume documents with a null principal's uri's host are approved for fullscreen and thus pointer lock. (2.20 KB, patch)
2012-05-09 21:02 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v1: Don't show fullscreen approval UI for documents with a null principal's uri's host (1.50 KB, patch)
2012-05-09 21:04 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v2: Don't show fullscreen approval UI for documents with a null principal's uri's host (1.57 KB, patch)
2012-05-09 21:29 PDT, Chris Pearce (:cpearce)
dao+bmo: review+
Details | Diff | Splinter Review
Patch 1 v2 - Make nsDocument listen for 'fullscreen-approved' notification rather than 'perm-changed' (4.24 KB, patch)
2012-05-15 04:17 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch 2 v2 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host (8.88 KB, patch)
2012-05-15 04:20 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v3 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host (9.89 KB, patch)
2012-05-18 04:17 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch 2 v4 - Don't enable fullscreen approval decision to be remembered for document's whose URI doesn't have a host (9.47 KB, patch)
2012-05-18 14:29 PDT, Chris Pearce (:cpearce)
dao+bmo: review+
Details | Diff | Splinter Review
Patch 3 v1: fullscreen approved flag (for pointer lock in local files) (16.54 KB, patch)
2012-05-21 02:32 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review

Description Alon Zakai (:azakai) 2012-05-09 14:33:11 PDT
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) 2012-05-09 15:45:01 PDT
Thanks for filing.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-05-09 18:14:43 PDT
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) 2012-05-09 19:14:15 PDT
Yup, that's exactly the problem. :)
Comment 4 Chris Pearce (:cpearce) 2012-05-09 19:17:48 PDT
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) 2012-05-09 19:56:51 PDT
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) 2012-05-09 21:02:42 PDT
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.
Comment 7 Chris Pearce (:cpearce) 2012-05-09 21:04:31 PDT
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.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-09 21:16:59 PDT
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) 2012-05-09 21:29:27 PDT
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.
Comment 10 Olli Pettay [:smaug] 2012-05-10 08:10:47 PDT
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: gavin@gavinsharp.com] 2012-05-10 08:25:37 PDT
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) 2012-05-10 17:44:49 PDT
(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) 2012-05-10 17:47:46 PDT
(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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-05-10 17:49:21 PDT
URI_IS_LOCAL_FILE or something please, not file://
Comment 15 Chris Pearce (:cpearce) 2012-05-15 04:17:59 PDT
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.
Comment 16 Chris Pearce (:cpearce) 2012-05-15 04:20:05 PDT
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.
Comment 17 Dão Gottwald [:dao] 2012-05-17 07:29:38 PDT
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) 2012-05-18 02:05:21 PDT
(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] 2012-05-18 02:51:52 PDT
(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) 2012-05-18 04:17:36 PDT
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
Comment 21 Dão Gottwald [:dao] 2012-05-18 04:42:59 PDT
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) 2012-05-18 14:29:32 PDT
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
Comment 23 Chris Pearce (:cpearce) 2012-05-21 02:32:40 PDT
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.
Comment 24 Olli Pettay [:smaug] 2012-05-21 05:39:54 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.