Dan Mosedale (:dmosedale, :dmose) Reporter
	Description • 10 years ago

Right now, we're using the microphone / video permissions dialog as an ugly part of our call answering UX.  We need to somehow make it possible for Loop code to to call getUserMedia without those dialogs in order to implement the reasonable call answering UX (see https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming and bug 990678 for that).

A couple of possible solutions I could imagine include giving the Loop about: URIs principals that would allow for this (though see bug 976109 for a glimpse at what that might entail), or exposing a slightly different version of getUserMedia through the mozLoopAPI.

Different solutions would almost certainly produce different estimates, so it'd be good to have some discussion about what's likely to be acceptable to the module owners and security folks (jesup, dveditz, ...) before estimating.

	:shell escalante
	Comment 1 • 10 years ago

Romain T: As a FF browser user, I can answer without having to validate access to my microphone or camera, so that the experience is smoother.
As a FF browser user, I can place calls without having to validate access to my microphone or camera, so that the experience is smoother.

As part of this, we need to back out/remove the workaround in bug 1010325 as per the review comments there.

	Mark Banner (:standard8) Assignee
	Comment 3 • 10 years ago

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming has some UX for this, but it is unclear to me if/what the UX for selecting different devices is going to be.

	:shell escalante
	Comment 4 • 10 years ago

white listing

	Romain Testard [:RT]
	Comment 5 • 10 years ago

Darrin, can you update the UX to allow a user to answer an incoming call with either audio only or audio+video?

	Adam Roach [:abr]
	Comment 6 • 10 years ago

(In reply to Mark Banner (:standard8) from comment #3)
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming has
> some UX for this, but it is unclear to me if/what the UX for selecting
> different devices is going to be.

Let's not confuse access to devices with selection of devices. I know the current doorhanger does so, but they're completely separate issues. We will need some kind of device enumeration to handle selection, but that's a different issue. This one is just about stopping the doorhanger from popping up in the first place.

	Mark Banner (:standard8) Assignee
	Comment 8 • 10 years ago

Comment on attachment 8438028 [details] [diff] [review]
Add 'always allow' behavior for about:loopconversation

Don't forget to remove the previous patch from webrtcui.js.

	Adam Roach [:abr]
	Comment 9 • 10 years ago

Ah, sorry, I didn't mean to assign this to myself -- that's a side effect of using bzexport. The attached patch is a working proof-of-concept, but it needs to have testing added.

	Adam Roach [:abr]
	Comment 10 • 10 years ago

(In reply to Mark Banner (:standard8) from comment #8)
> Don't forget to remove the previous patch from webrtcui.js.

So, I'd like to have a short conversation with mhammond about this -- it seems like removing that code would take a code-path that makes things work and replace it with one that makes certain things break under the right circumstances. That seems to be of questionable value.

	Mark Hammond [:markh] [:mhammond]
	Comment 11 • 10 years ago

(In reply to Adam Roach [:abr] from comment #10)
> (In reply to Mark Banner (:standard8) from comment #8)
> > Don't forget to remove the previous patch from webrtcui.js.
> 
> So, I'd like to have a short conversation with mhammond about this -- it
> seems like removing that code would take a code-path that makes things work
> and replace it with one that makes certain things break under the right
> circumstances. That seems to be of questionable value.

Happy to have that conversation, but you will need to be more specific about those circumstances :)

	Mark Banner (:standard8) Assignee
	Comment 12 • 10 years ago

(In reply to Adam Roach [:abr] from comment #10)
> (In reply to Mark Banner (:standard8) from comment #8)
> > Don't forget to remove the previous patch from webrtcui.js.
> 
> So, I'd like to have a short conversation with mhammond about this -- it
> seems like removing that code would take a code-path that makes things work
> and replace it with one that makes certain things break under the right
> circumstances. That seems to be of questionable value.

We need to chat with Florian as well, as he's the one that requested it in the review comments.

	Florian Quèze [:florian]
	Comment 13 • 10 years ago

(In reply to Adam Roach [:abr] from comment #10)
> (In reply to Mark Banner (:standard8) from comment #8)
> > Don't forget to remove the previous patch from webrtcui.js.
> 
> So, I'd like to have a short conversation with mhammond about this -- it
> seems like removing that code would take a code-path that makes things work
> and replace it with one that makes certain things break under the right
> circumstances. That seems to be of questionable value.

Like mhammond, I would like to know what breaks under what circumstances.

Also FYI, I'm changing that code again in bug 1000253 (see bug 1000253 comment 27 for what I plan to do).

	Adam Roach [:abr]
	Comment 14 • 10 years ago

(In reply to Mark Hammond [:markh] from comment #11)
> (In reply to Adam Roach [:abr] from comment #10)
> > (In reply to Mark Banner (:standard8) from comment #8)
> > > Don't forget to remove the previous patch from webrtcui.js.
> > 
> > So, I'd like to have a short conversation with mhammond about this -- it
> > seems like removing that code would take a code-path that makes things work
> > and replace it with one that makes certain things break under the right
> > circumstances. That seems to be of questionable value.
> 
> Happy to have that conversation, but you will need to be more specific about
> those circumstances :)

Okay, looking at it again, this just fixes things up for about:loop*, which doesn't gain anything. It seems a bit odd that we'd want to leave things in a state where the call fails for all about: URLs, but it's not really that big of a deal. I'm good removing it; and, if someone else needs it for other about: use cases, it can be dealt with at that time.

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> Also FYI, I'm changing that code again in bug 1000253 (see bug 1000253
> comment 27 for what I plan to do).

Thanks. The patch here will have to be tweaked to allow ALWAYS to be returned for about: URIs. I think we'll want to wait for 100253 to land before this patch goes in.

	Mark Banner (:standard8) Assignee
	Comment 15 • 10 years ago

Adding dep on bug 1000253 so we'll get notified when that lands.

(In reply to Mark Banner (:standard8) from comment #3)
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming has
> some UX for this, but it is unclear to me if/what the UX for selecting
> different devices is going to be.

This has been answered elsewhere - bug 1023933 and bug 1023934 will do that.

	Adam Roach [:abr]
	Comment 17 • 10 years ago

Unrotted patch. This can be assigned now. :)

	Mark Banner (:standard8) Assignee
	Comment 18 • 10 years ago

Updated patch:

- Perms are now only stored per session, to avoid stacking up lots of permanently stored urls.
- Included basic unit test for checking the permissions have been set at the xpcshell level.

Todo:

- Work out about webrtcUI.jsm (backing it out doesn't work, discussing with Florian)
- Write mochitest (?) for changes in MediaManager

(I suspect functional testing will be done by a separate bug, once we get it up and running).

	Florian Quèze [:florian]
	Comment 19 • 10 years ago

Comment on attachment 8445209 [details] [diff] [review]
Add 'always allow' behavior for about:loopconversation

Am I understanding correctly that you want all about:loopconversation URLs to be granted device access automatically? If so, wouldn't it be easier to just check the URL in the MediaManager code, and skip the prompt completely (just grant access immediately from the MediaManager code)?

	Adam Roach [:abr]
	Comment 20 • 10 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> Comment on attachment 8445209 [details] [diff] [review]
> Add 'always allow' behavior for about:loopconversation
> 
> Am I understanding correctly that you want all about:loopconversation URLs
> to be granted device access automatically? If so, wouldn't it be easier to
> just check the URL in the MediaManager code, and skip the prompt completely
> (just grant access immediately from the MediaManager code)?

I'm mostly worried about being fragile in the face of other parts of the code calling checkPermissions (or its variations), so I'd prefer to go down the path of adding our URL to the permissions manager.

	Adam Roach [:abr]
	Comment 21 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/aad532ce08e2

	Adam Roach [:abr]
	Comment 22 • 10 years ago

(In reply to Adam Roach [:abr] from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/aad532ce08e2

Gah! Wrong Bug. Sorry, ignore that.

	Florian Quèze [:florian]
	Comment 23 • 10 years ago

(In reply to Adam Roach [:abr] from comment #20)

> I'd prefer to go down
> the path of adding our URL to the permissions manager.

That's exactly the part that worries me as being fragile. I see nothing in http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIPermissionManager.idl that suggests the permission manager would work for URIs that don't have a valid 'host' attribute (the comments near the 'remove' method suggest to me having a 'host' value is kinda assumed).

Actually, looking at the code in nsPermissionManager.cpp, the Add method calls AddFromPrincipal, which calls AddInternal, which will fail if GetHostForPrincipal fails (apparently GetHostForPrincipal will fallback to the principal's origin if it can't find the host, so it probably won't fail).

The other thing that worries me in the approach here is making MediaManager set the 'secure' bit for all about: URLs, when some of them are just redirections to web pages (eg. about:credits).

	Mark Banner (:standard8) Assignee
	Comment 24 • 10 years ago

There's currently discussions going on around bug 1028187 where we're either going to:

- get a host in the uri (somehow)
- and/or we could have attributes on the about redirector that we may then be able to detect in media manager or webrtcUI.

The latter of those would mean that media manager could detect a flag & allow automatically, rather than having to do white-list based analysis everywhere.

	Adam Roach [:abr]
	Comment 26 • 10 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> (In reply to Adam Roach [:abr] from comment #20)
> 
> > I'd prefer to go down
> > the path of adding our URL to the permissions manager.
> 
> That's exactly the part that worries me as being fragile. I see nothing in
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsIPermissionManager.idl that suggests the permission manager would work for
> URIs that don't have a valid 'host' attribute (the comments near the
> 'remove' method suggest to me having a 'host' value is kinda assumed).
> 
> Actually, looking at the code in nsPermissionManager.cpp, the Add method
> calls AddFromPrincipal, which calls AddInternal, which will fail if
> GetHostForPrincipal fails (apparently GetHostForPrincipal will fallback to
> the principal's origin if it can't find the host, so it probably won't fail).
> 
> The other thing that worries me in the approach here is making MediaManager
> set the 'secure' bit for all about: URLs, when some of them are just
> redirections to web pages (eg. about:credits).

Okay, I have some sympathy for these arguments. Just to make sure we're all on the same page, I've attached a working patch that I believe uses the approach you're proposing. I'm find the hard-coding of the Loop URL in here to be a bit jarring, but can't think of a more elegant approach without invoking some really complicated machinery.

Florian: does this look good to you?

	Florian Quèze [:florian]
	Comment 27 • 10 years ago

Attachment 8448098 [details] [diff] looks like what I had in mind, yes.

	Mark Banner (:standard8) Assignee
	Comment 28 • 10 years ago

Updated patch to remove the changes from bug 1010325 that will no longer be needed. I'm currently working on seeing if I can implement a test for this.

	Mark Banner (:standard8) Assignee
	Comment 29 • 10 years ago

Updated patch to include unit tests. The existing tests fail if we do something wrong with the isLoop value, so I don't think we need additional tests for non-about:loopconversation.

	Florian Quèze [:florian]
	Comment 30 • 10 years ago

Comment on attachment 8450162 [details] [diff] [review]
Bypass the video and audio permission prompts for Loop, as Loop will provide its own mechanisms.

Review of attachment 8450162 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mark Banner (:standard8) from comment #29)
> The existing tests fail if we do
> something wrong with the isLoop value, so I don't think we need additional
> tests for non-about:loopconversation.

The test will fail if isLoop ends up being set to false when it should be true. The other case I was concerned about is if isLoop ends up set to true when it should be false (if someday someone changes the media manager code in a way that makes isLoop be true for any about: URL).

It doesn't seem very difficult to register your fake about module for a second url (eg. about:evil) and run similar test code on that second URL.

::: browser/base/content/test/general/browser.ini
@@ +281,5 @@
>  [browser_customize_popupNotification.js]
>  [browser_datareporting_notification.js]
>  run-if = datareporting
>  [browser_devices_get_user_media.js]
>  skip-if = (os == "linux" && debug) || e10s # linux: bug 976544; e10s: Bug ?????? - appears user media notifications only happen in the child and don't make their way to the parent?

Maybe this is a good opportunity to replace the ?????? with the real number? I think this should point to bug 973001.

@@ +283,5 @@
>  run-if = datareporting
>  [browser_devices_get_user_media.js]
>  skip-if = (os == "linux" && debug) || e10s # linux: bug 976544; e10s: Bug ?????? - appears user media notifications only happen in the child and don't make their way to the parent?
> +[browser_devices_get_user_media_special.js]
> +skip-if = e10s # linux: bug 976544; e10s: Bug ?????? - appears user media notifications only happen in the child and don't make their way to the parent?

Drop the linux part of this comment, as you are only disabling for e10s builds. By the way, why is your test not working with e10s?

::: browser/base/content/test/general/browser_devices_get_user_media_special.js
@@ +126,5 @@
> +           Ci.nsIAboutModule.HIDE_FROM_ABOUTABOUT;
> +  }
> +};
> +
> +let factory = {

let factory = XPCOMUtils._getFactory(fakeLoopAboutModule);

::: dom/media/MediaManager.cpp
@@ +1484,5 @@
> +  {
> +    nsCOMPtr<nsIURI> loopURI;
> +    nsresult rv = NS_NewURI(getter_AddRefs(loopURI), "about:loopconversation");
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = docURI->EqualsExceptRef(loopURI, &isLoop);

I'm not a reviewer for this code (I think you want to request review from jesup here), but I think if I wrote this I would have reused the aPrivileged variable here, to avoid having a loop-specific variable in the code when MOZ_LOOP isn't defined.

	Randell Jesup [:jesup] (needinfo me)
	Comment 31 • 10 years ago

Comment on attachment 8450162 [details] [diff] [review]
Bypass the video and audio permission prompts for Loop, as Loop will provide its own mechanisms.

Review of attachment 8450162 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaManager.cpp
@@ +1484,5 @@
> +  {
> +    nsCOMPtr<nsIURI> loopURI;
> +    nsresult rv = NS_NewURI(getter_AddRefs(loopURI), "about:loopconversation");
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = docURI->EqualsExceptRef(loopURI, &isLoop);

Move isLoop inside the ifdef, and add "if (isLoop) { aPrivileged = true; }" per florian

@@ +1493,2 @@
>    // XXX No full support for picture in Desktop yet (needs proper UI)
> +  if (aPrivileged || isLoop ||

remove this change per above

	Mark Banner (:standard8) Assignee
	Comment 32 • 10 years ago

Updated patch to include the comments so far, I'm going to push this to try before I request review on the test changes.

	Adam Roach [:abr]
	Comment 33 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=674f9c0571ff

	Mark Banner (:standard8) Assignee
	Comment 34 • 10 years ago

This has a few extra stability fixes - mainly wait for the green perms icon, before closing streams, as otherwise the test fails intermittently.

New try run is here: https://tbpl.mozilla.org/?tree=Try&rev=5b40e1da53d4 (still a few tests pending).

Requesting review for the non-MediaManager parts.

	Florian Quèze [:florian]
	Comment 35 • 10 years ago

Comment on attachment 8453722 [details] [diff] [review]
Bypass the video and audio permission prompts for Loop, as Loop will provide its own mechanisms.

Review of attachment 8453722 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I only have some trivial comments about the test.

::: browser/base/content/test/general/browser_devices_get_user_media_about_urls.js
@@ +183,5 @@
> +
> +{
> +  desc: "load about:loopconversation",
> +  run: function loadLoopConversation() {
> +    yield loadPage("about:loopconversation");

This isn't really testing anything, just preparing for the about:loopconversation test; is there a reason for having this as a separate item in the gTests array?

@@ +188,5 @@
> +  }
> +},
> +
> +{
> +  // about:loopconversation shouldn't prompt

Merge this comment with the description, so that it's actually printed in the test logs.

@@ +231,5 @@
> +];
> +
> +function test() {
> +  registrar.registerFactory(classIDs[0], "", "@mozilla.org/network/protocol/about;1?what=loopconversation", factory);
> +  registrar.registerFactory(classIDs[1], "", "@mozilla.org/network/protocol/about;1?what=evil", factory);

I think I would merge these registerFactory (and unregisterFactory) calls with the tests themselves; and the classID would become a local variable.

	Mark Banner (:standard8) Assignee
	Comment 36 • 10 years ago

Thanks for the reviews, here's the final version.

	Mark Banner (:standard8) Assignee
	Comment 37 • 10 years ago

Just an update, we're waiting on bug 1037000 before we land this, so that users can still set the microphone by selecting the default in Windows preferences (Loop will get more defaults in other bugs later).

	Mark Banner (:standard8) Assignee
	Comment 38 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/28cffea65648

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/28cffea65648

	u279076
	Comment 40 • 10 years ago

Does this need manual testing or is in-testsuite coverage sufficient?

	u279076
	Comment 41 • 10 years ago

Dan, can you please answer comment 40?

	Mark Banner (:standard8) Assignee
	Comment 42 • 10 years ago

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #40)
> Does this need manual testing or is in-testsuite coverage sufficient?

The in-testsuite coverage is sufficient. Any manual testing of calls would also immediately flag a failure as we'd be prompted for permission on the conversation window when we weren't expecting it.