Add attribute to docshell to disable HTML5 media

RESOLVED FIXED in mozilla24

Status

()

enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

BrowserID integration in the browser will use a hidden frame to load the provisioning page from the identity provider (IDP).  We'd like to disable any HTML5 media (ie. <video>/<audio>) from loading/playing in this hidden frame since the user won't be able to control the playback.

Something like nsIDocShell.allowMedia?
Are you also going to block plugins?  Otherwise we're not accomplishing much here.

I'm sure Boris will be able to come up with a much longer list of things you'd want to block.
(In reply to Justin Lebar [:jlebar] from comment #1)
> Are you also going to block plugins?  Otherwise we're not accomplishing much
> here.

Of course, but there is already an attribute for that.

This is what I have (from https://hg.mozilla.org/projects/pine/rev/d4d08c2798e0 ):
allowAuth = false; // TODO: check
allowPlugins = false;
allowImages = false;
allowWindowControl = false;

I just looked through the allow* attributes at https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDocShell#Attributes
Do you want to allow scripts to run in that page?  Stylesheets to load?  XHR?  Websockets?

That said, I think adding allowMedia is a good idea.
(In reply to Boris Zbarsky (:bz) from comment #3)
> Do you want to allow scripts to run in that page?  Stylesheets to load? 
> XHR?  Websockets?

Scripts are essential for this frame because the page needs to do it's own checks if the user has a valid session using whatever means the identity provider already uses (ie. cookies with an XHR, localStorage) and then call the appropriate navigator.id methods.

The spec is at https://github.com/mozilla/id-specs/blob/dev/browserid/index.md#identity-provisioning-flow with some sample code.

I'm not sure if we want to allow WebSockets in this case but it can be handled in a different bug.

We probably don't need stylesheets so I've now disabled them with:
docShell.contentViewer.QueryInterface(Ci.nsIMarkupDocumentViewer).authorStyleDisabled = true;
After fixing this we'll need to remember to also re-enable the relevant sandbox tests:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e7e7255b9b1a
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> After fixing this we'll need to remember to also re-enable the relevant
> sandbox tests:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/e7e7255b9b1a

This bug is currently blocking bug 841495, which (I hope) could solve thumbnail rendering issues on new tab.  Gavin/others, is there someone who could pick this up or provide another solution to the loading issue in bug 841495?
Adding the docShell flag is relatively straightforward, but perhaps roc can advise on how to best have that impact the media element code.
Flags: needinfo?(roc)
I'll attempt this.  It seems like checking the new flag in HTMLMediaElement::LoadResource right before the checking the content policy should work.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Yes, that would work.

You probably want to disable WebAudio based on this flag as well. Add some check to AudioContext...
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> You probably want to disable WebAudio based on this flag as well. Add some
> check to AudioContext...

I haven't figured out the best place for that yet so this patch just deals with media elements.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5e9969295a41
Attachment #728059 - Flags: review?(bzbarsky)
Fix a sessionstore test found from the try push and rev the IDL uuid.
Attachment #728059 - Attachment is obsolete: true
Attachment #728059 - Flags: review?(bzbarsky)
Attachment #728076 - Flags: review?(bzbarsky)
toolkit/identity and toolkit/components/social changes
Attachment #728079 - Flags: review?(gavin.sharp)
Comment on attachment 728076 [details] [diff] [review]
Part 1 v.1.1 Add allowMedia attribute to nsIDocShell and use it with media elements

>+++ b/content/html/content/src/HTMLMediaElement.cpp
>+  nsIPresShell* presShell = OwnerDoc()->GetShell();

This will fail in a display:none iframe.  Maybe add a test for that?

What you should probably do instead is use nsIDocument::GetContainer and QI the result to nsIDocShell.

Worth adding a test for this.

>+    if (docShell) {
>+      bool mediaAllowed = true;
>+      nsresult rv = docShell->GetAllowMedia(&mediaAllowed);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      if (!mediaAllowed) {
>+        return NS_ERROR_FAILURE;

Please mark this attribute as [infallible] in the IDL, and then you will be able to do:

  if (docShell && !docShell->GetAllowMedia()) {
    return NS_ERROR_FAILURE;
  }

>+++ b/docshell/base/nsDocShell.cpp
>+NS_IMETHODIMP nsDocShell::GetAllowMedia(bool * aAllowMedia)
>+    NS_ENSURE_ARG_POINTER(aAllowMedia);

Please don't bother with this.  If someone passes null, they deserve to crash.

>@@ -2916,16 +2931,20 @@ nsDocShell::SetDocLoaderParent(nsDocLoad
>+        if (NS_SUCCEEDED(parentAsDocShell->GetAllowMedia(&value)))
>+        {
>+            SetAllowMedia(value);

And this would become:

  SetAllowMedia(parentAsDocShell->GetAllowMedia());

>@@ -7786,30 +7805,34 @@ nsDocShell::RestoreFromHistory()
>+        bool allowMedia;
>+        childShell->GetAllowMedia(&allowMedia);

And similar here.

r=me with those nits picked.
Attachment #728076 - Flags: review?(bzbarsky) → review+
Comment on attachment 728079 [details] [diff] [review]
Part 2 v.1 - Add consumers of the the allowMedia flag to address TODOs

Android Fennec's _downloadDocument and the add-on SDK's frame/utils.js probably want to use this attribute too. File bugs on them?

I don't understand why the test was disabled on 10.5, but I assume that's no longer relevant since we don't test/build on 10.5 anymore?
Attachment #728079 - Flags: review?(gavin.sharp) → review+
No longer blocks: 841495
Blocks: 870103
Try push: https://tbpl.mozilla.org/?tree=Try&rev=5ae1c4189a19

Last I checked, there were test failures so this needs some work still.
This fixes bz's comments and unbitrots the patch.  SessionStore.jsm has changed such that the changes to it aren't needed anymore, and browser_493467.js is gone.

It feels icky obsoleting other people's patches, but Matt and I have talked about my working on this bug, so I hope it's OK this one time!
Attachment #728076 - Attachment is obsolete: true
Chrome mochitest with a display:none iframe test plus a couple of others.

With all three patches applied: https://tbpl.mozilla.org/?tree=Try&rev=6af47f28df47
Try looks OK, so I'll land this later today.
You forgot Web Audio, it seems...
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> You forgot Web Audio, it seems...

That's bug 879111.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.