Closed Bug 802656 Opened 7 years ago Closed 7 years ago

Trying to request access to the camera or mic in gUM with no/locked camera/mic support - no error is fired back

Categories

(Core :: WebRTC, defect, P2)

19 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jsmith, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [getUserMedia] [blocking-gum+] [qa-])

Attachments

(1 file, 3 obsolete files)

Steps:

1. Call gUM with either video: true, audio: true, or video: true, audio: true with no camera and mic support

Expected:

I should get an error callback with NO_DEVICES_FOUND.

Actual:

No error callback is fired.
Whiteboard: [getUserMedia]
Summary: Trying to request access to the camera or mic in gUM - no error is fired back → Trying to request access to the camera or mic in gUM with no camera/mic support - no error is fired back
Keywords: regression
Whiteboard: [getUserMedia] → [getUserMedia] [blocking-gum+]
Apparently this also reproduces for me with using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=779039#c0.
Summary: Trying to request access to the camera or mic in gUM with no camera/mic support - no error is fired back → Trying to request access to the camera or mic in gUM with no/locked camera/mic support - no error is fired back
Priority: -- → P2
Duplicate of this bug: 806437
Assignee: nobody → anant
Status: NEW → ASSIGNED
Attachment #678335 - Flags: review?(dao)
Attachment #678335 - Attachment is obsolete: true
Attachment #678335 - Flags: review?(dao)
Attachment #678336 - Flags: review?(dao)
Comment on attachment 678336 [details] [diff] [review]
Deny request if no devices were found - v1

How feasible would it be for the backend to just bypass the UI invocation in this case, rather than letting UI code deny the request?
I'm going to add that code separately, but state may also change in between the messaging so it's better to handle this case in the UI code as well for completeness.
(In reply to Anant Narayanan [:anant] from comment #6)
> I'm going to add that code separately, but state may also change in between
> the messaging

How exactly would the state change? Note that nsIObserverService notifications are synchronous.
Comment on attachment 678336 [details] [diff] [review]
Deny request if no devices were found - v1

Also, comment 0 suggests that we don't want the PERMISSION_DENIED error for this.
Attachment #678336 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #7)
> How exactly would the state change? Note that nsIObserverService
> notifications are synchronous.

But camera acquisition is not (and is done off the main thread). It is definitely possible for another tab to have grabbed the camera after we checked in C++ but not being available when JS enumerates.

Besides, it is just wrong for the UI to simply return without sending back an observer notification when it receives one.

We do not have a "NO_DEVICES_FOUND" error anywhere in the spec at the moment. It think PERMISSION_DENIED works just as well, atleast better than doing that nothing at all, though I agree we might want a better error name (perhaps HARDWARE_UNAVAILABLE).
Attachment #678336 - Attachment is obsolete: true
Attachment #678447 - Flags: review?(rjesup)
Attachment #678447 - Flags: review?(dao)
Attachment #678447 - Flags: review?(rjesup) → review+
(In reply to Anant Narayanan [:anant] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > How exactly would the state change? Note that nsIObserverService
> > notifications are synchronous.
> 
> But camera acquisition is not (and is done off the main thread). It is
> definitely possible for another tab to have grabbed the camera after we
> checked in C++ but not being available when JS enumerates.

In my experience the turnaround time is so short that it's impossible for the user to do anything in the meantime, so it seems like we're talking about an edge case here. A more realistic case would be the list of devices changing between the popup appearing the first time (it can be dismissed and re-opened multiple times) and the user granting the request. Your patch doesn't handle this, which seems inconsistent if we really care about this problem.

It's also news to me that the same source can't be shared twice concurrently. Could this be made work on our side?

> Besides, it is just wrong for the UI to simply return without sending back
> an observer notification when it receives one.

There's no actual contract requiring this.
(In reply to Dão Gottwald [:dao] from comment #11)
> It's also news to me that the same source can't be shared twice
> concurrently. Could this be made work on our side?

Yes, but we couldn't solve the resulting privacy/UX issues satisfactorily so we've decided to disable that functionality for now.

> > Besides, it is just wrong for the UI to simply return without sending back
> > an observer notification when it receives one.
> 
> There's no actual contract requiring this.

Of course there is. If there is no corresponding message for an outgoing observer notification from the backend the gUM call will just hang. It is the UI's responsibility to send back a reasonable response for every incoming.

Honestly, I don't see what the problem is here.
(In reply to Anant Narayanan [:anant] from comment #12)
> > It's also news to me that the same source can't be shared twice
> > concurrently. Could this be made work on our side?
> 
> Yes, but we couldn't solve the resulting privacy/UX issues satisfactorily so
> we've decided to disable that functionality for now.

Ok, if that's just the interim solution, then I don't think the UI should worry about it especially if the proposed workaround only covers an edge case while leaving other more interesting cases unaddressed. (The privacy/UX implications aren't clear to me, by the way.)

> > > Besides, it is just wrong for the UI to simply return without sending back
> > > an observer notification when it receives one.
> > 
> > There's no actual contract requiring this.
> 
> Of course there is. If there is no corresponding message for an outgoing
> observer notification from the backend the gUM call will just hang. It is
> the UI's responsibility to send back a reasonable response for every
> incoming.

Popup notifications don't work like this. Users can generally leave them unanswered infinitely. Secondly, if the backend doesn't invoke the UI in the first place, sending a response in this case becomes moot. (I think the UI code should throw an exception rather than just returning, though.)

> Honestly, I don't see what the problem is here.

I don't see the point of adding this code. We should just take the backend fix which you said you're gonna add anyway.
Assignee: anant → rjesup
Duplicate of this bug: 823083
Which bug caused this regression?
Henrik: Probably regressed when we did a major rewrite to make this non-synchronous.

Dao: I have a problem with Anant's proposed backend fix to avoid invoking the UI if there are no devices to choose from.

Right now we get the device list in response to a request (GetUserMediaDevices) from the UI.  We'd have to do this before dispatching the UI request.

Longer term, we need to support the usecases of "darn, I forgot to plug in my headphones/webcam" without cancelling/erroring out and re-invoking.  That means allowing incomplete/empty lists, and allowing use to notify the UI if the device changes (hotplug events).

So, the backend fix proposed will be wrong in that case and would need to be backed out anyways and go back to GetUserMediaDevices() or equivalent.  So, my preference would be (for now) return an error or denial if the list is empty.  Alternatively, I could return an error from GetUserMediaDevices if you prefer, but I'm not 100% certain that case is fully handled (and a denial returned) - it didn't look like it.

I'll attach a patch that returns an error (the alternative solution); as mentioned I suspect this needs fixes in the JSM to complete.
Dao: which way do you want to go of the two alternatives?  I'm guessing the second alternative is a little simpler.
Flags: needinfo?(dao)
Attachment #694849 - Flags: review?(anant)
Comment on attachment 694849 [details] [diff] [review]
GetUserMediaDevices: Consider no devices available an error

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

We should probably file a bug for the hotplug feature.
Attachment #694849 - Flags: review?(anant) → review+
Attachment #678447 - Attachment is obsolete: true
Attachment #678447 - Flags: review?(dao)
https://hg.mozilla.org/mozilla-central/rev/988de3eea4ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
Blocks: 827145
Blocks: 827146
Can't automate this until we have bug 815002.
Depends on: 815002
Flags: in-testsuite-
Needs bug 827145 to test. Will verify on that bug then.
Keywords: verifyme
Whiteboard: [getUserMedia] [blocking-gum+] → [getUserMedia] [blocking-gum+] [qa-]
Flags: needinfo?(dao)

Thanks

You need to log in before you can comment on or make changes to this bug.