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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Smith [:jsmith]]

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.

Comment 1

[Jason Smith [:jsmith]]

Apparently this also reproduces for me with using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=779039#c0.

Comment 3

[Anant Narayanan [:anant]]

Attachment: Deny request if no devices were found - v1

Comment 4

[Anant Narayanan [:anant]]

Attachment: Deny request if no devices were found - v1

Comment 5

[Dão Gottwald [: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?

Comment 6

[Anant Narayanan [:anant]]

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.

Comment 7

[Dão Gottwald [:dao]]

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

[Dão Gottwald [:dao]]

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.

Comment 9

[Anant Narayanan [:anant]]

(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).

Comment 10

[Anant Narayanan [:anant]]

Attachment: Deny request if no devices were found - v2

Comment 11

[Dão Gottwald [:dao]]

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

Comment 12

[Anant Narayanan [:anant]]

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

Comment 13

[Dão Gottwald [:dao]]

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

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which bug caused this regression?

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: GetUserMediaDevices: Consider no devices available an error

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Dao: which way do you want to go of the two alternatives?  I'm guessing the second alternative is a little simpler.

Comment 19

[Anant Narayanan [: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.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/988de3eea4ce

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/988de3eea4ce

Comment 22

[Jason Smith [:jsmith]]

Can't automate this until we have bug 815002.

Comment 23

[Jason Smith [:jsmith]]

Needs bug 827145 to test. Will verify on that bug then.

Comment 24

[Wamiq Hafeez]

Thanks