Matthew N. [:MattN] Assignee
	Description • 12 years ago

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?

	Justin Lebar (not reading bugmail)
	Comment 1 • 12 years ago

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.

	Matthew N. [:MattN] Assignee
	Comment 2 • 12 years ago

(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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

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.

	Matthew N. [:MattN] Assignee
	Comment 4 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 12 years ago

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

	Jennifer Morrow [:Boriss] (UX)
	Comment 6 • 11 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 11 years ago

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

	Matthew N. [:MattN] Assignee
	Comment 8 • 11 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 11 years ago

Yes, that would work.

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

	Matthew N. [:MattN] Assignee
	Comment 10 • 11 years ago

(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

	Matthew N. [:MattN] Assignee
	Comment 11 • 11 years ago

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

	Matthew N. [:MattN] Assignee
	Comment 12 • 11 years ago

toolkit/identity and toolkit/components/social changes

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 11 years ago

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?

	Matthew N. [:MattN] Assignee
	Comment 15 • 11 years ago

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

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

	Drew Willcoxon :adw
	Comment 16 • 11 years ago

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!

	Drew Willcoxon :adw
	Comment 17 • 11 years ago

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

	Drew Willcoxon :adw
	Comment 18 • 11 years ago

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

	Drew Willcoxon :adw
	Comment 19 • 11 years ago

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

	(no longer active)
	Comment 20 • 11 years ago

You forgot Web Audio, it seems...

	Matthew N. [:MattN] Assignee
	Comment 21 • 11 years ago

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

That's bug 879111.

	Ed Morley [:emorley]
	Comment 22 • 11 years ago

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

	Jean-Yves Perrier [:teoli]
	Comment 23 • 10 years ago

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