759964 - Add attribute to docshell to disable HTML5 media

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement)

Description

[Matthew N. [:MattN]]

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?

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[Matthew N. [:MattN]]

(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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Matthew N. [:MattN]]

(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;

Comment 5

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

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

Comment 6

[Jennifer Morrow [:Boriss] (UX)]

(In reply to :Gavin Sharp (use [e-mail] 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?

Comment 7

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

Adding the docShell flag is relatively straightforward, but perhaps roc can advise on how to best have that impact the media element code.

Comment 8

[Matthew N. [:MattN]]

I'll attempt this.  It seems like checking the new flag in HTMLMediaElement::LoadResource right before the checking the content policy should work.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, that would work.

You probably want to disable WebAudio based on this flag as well. Add some check to AudioContext...

Comment 10

[Matthew N. [:MattN]]

Attachment: Part 1 v.1 Add allowMedia attribute to nsIDocShell and use it with media elements

(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

Comment 11

[Matthew N. [:MattN]]

Attachment: Part 1 v.1.1 Add allowMedia attribute to nsIDocShell and use it with media elements

Fix a sessionstore test found from the try push and rev the IDL uuid.

Comment 12

[Matthew N. [:MattN]]

Attachment: Part 2 v.1 - Add consumers of the the allowMedia flag to address TODOs

toolkit/identity and toolkit/components/social changes

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

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

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?

Comment 15

[Matthew N. [:MattN]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5ae1c4189a19

Last I checked, there were test failures so this needs some work still.

Comment 16

[Drew Willcoxon :adw]

Attachment: Part 1 v.1.2 - nsIDocShell.allowMedia

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!

Comment 17

[Drew Willcoxon :adw]

Attachment: Part 3 - Tests

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

Comment 18

[Drew Willcoxon :adw]

Try looks OK, so I'll land this later today.

Comment 19

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce02e3335eb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8830e3fe79ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/6970e918f924

Comment 20

[(no longer active)]

You forgot Web Audio, it seems...

Comment 21

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> You forgot Web Audio, it seems...

That's bug 879111.

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ce02e3335eb7
https://hg.mozilla.org/mozilla-central/rev/8830e3fe79ff
https://hg.mozilla.org/mozilla-central/rev/6970e918f924

Comment 23

[Jean-Yves Perrier [:teoli]]

Added nsIDocShell.allowMedia in:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDocShell
and a comment in:
https://developer.mozilla.org/en-US/Firefox/Releases/24