Closed
Bug 1015486
Opened 10 years ago
Closed 10 years ago
Desktop client needs to bypass the media device permission prompt -punch hole for unrestricted getUserMedia using loop
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla33
People
(Reporter: dmosedale, Assigned: standard8)
References
()
Details
(Whiteboard: [p=2][must fix for mvp, addresses bug 1010325 comments][gecko])
User Story
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.
Attachments
(1 file, 8 obsolete files)
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.
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.
Summary: punch hole for unrestricted getUserMedia using loop → Desktop client needs to bypass the media device permission prompt -punch hole for unrestricted getUserMedia using loop
Updated•10 years ago
|
User Story: (updated)
Assignee | ||
Updated•10 years ago
|
User Story: (updated)
Whiteboard: [p=?] → [p=?][must fix for mvp, addresses bug 1010325 comments]
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → mozilla33
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.
Flags: needinfo?(dhenein)
Comment 4•10 years ago
|
||
white listing
Whiteboard: [p=?][must fix for mvp, addresses bug 1010325 comments] → [p=2][must fix for mvp, addresses bug 1010325 comments]
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?
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.
Comment 7•10 years ago
|
||
Updated•10 years ago
|
Assignee: nobody → adam
Status: NEW → ASSIGNED
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.
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.
Assignee: adam → nobody
Status: ASSIGNED → NEW
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.
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 :)
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.
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).
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.
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.
Depends on: 1000253
Flags: needinfo?(dhenein)
Whiteboard: [p=2][must fix for mvp, addresses bug 1010325 comments] → [p=2][must fix for mvp, addresses bug 1010325 comments][gecko]
Comment 16•10 years ago
|
||
Updated•10 years ago
|
Attachment #8438028 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → standard8
Target Milestone: mozilla33 → 33 Sprint 2- 7/7
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).
Assignee | ||
Updated•10 years ago
|
Attachment #8442172 -
Attachment is obsolete: true
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)?
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.
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.
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).
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.
Comment 25•10 years ago
|
||
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?
Flags: needinfo?(florian)
Comment 27•10 years ago
|
||
Attachment 8448098 [details] [diff] looks like what I had in mind, yes.
Flags: needinfo?(florian)
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.
Attachment #8445209 -
Attachment is obsolete: true
Attachment #8448098 -
Attachment is obsolete: true
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.
Attachment #8449402 -
Attachment is obsolete: true
Attachment #8450162 -
Flags: review?(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.
Attachment #8450162 -
Flags: review?(florian) → feedback+
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
Attachment #8450162 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Target Milestone: 33 Sprint 2- 7/7 → 33 Sprint 3- 7/21
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.
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.
Attachment #8450162 -
Attachment is obsolete: true
Attachment #8452990 -
Attachment is obsolete: true
Attachment #8453722 -
Flags: review?(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.
Attachment #8453722 -
Flags: review?(florian) → review+
Assignee | ||
Comment 36•10 years ago
|
||
Thanks for the reviews, here's the final version.
Attachment #8453722 -
Attachment is obsolete: true
Attachment #8453758 -
Flags: review+
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).
Depends on: 1037000
Assignee | ||
Comment 38•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/28cffea65648
Target Milestone: 33 Sprint 3- 7/21 → mozilla33
Comment 39•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/28cffea65648
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 40•10 years ago
|
||
Does this need manual testing or is in-testsuite coverage sufficient?
Flags: qe-verify?
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.
Flags: needinfo?(dmose)
You need to log in
before you can comment on or make changes to this bug.
Description
•