918054 - getUserMedia audio can be requested while a call is active, but shouldn't be

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Jason Smith [:jsmith]]

Build: 9/17/2013 Master
Device: Buri

STR

1. Start a call with a different person
2. Hit the home button & go to the browser
3. Go to http://mozilla.github.io/webrtc-landing/gum_test.html
4. Select Audio

Expected

A HARDWARE_UNAVAILABLE error should fire, as a higher priority application has access to the microphone.

Actual

A permission prompt appears allowing access to the microphone. Granting access turns on microphone use for that site while the call is active. This means talking into your microphone at this point sends audio to the media stream at that website and the other person in the call at the same time.

Comment 1

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

I assume the "call" referred to in STR #1 is a phone call to the other person, not a webrtc call.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #1)
> I assume the "call" referred to in STR #1 is a phone call to the other
> person, not a webrtc call.

Yup - calling via the dialer app to another person on a different phone.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Are we sure web and phone should be mutually exclusive here? What's the precedent?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> Are we sure web and phone should be mutually exclusive here? What's the
> precedent?

FWIW - I filed this bug per a discussion I had with Paul, so let me needinfo Paul for his views on the impact of this bug.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Allowing recording of phone calls sounds like both a useful feature, and a privacy risk. I can't to either partner requirements or privacy guidelines (or even law?). A quick skim of android and iphone apps and there are many that provide this functionality though, so maybe this is a valuable feature.

But after talking with SC last week, I thought that regardless of what we want to do here, that currently it isn't actually possible for both webrtc and phone to use the microphone at the same time. So at the very least this wont do what the user is expecting.

If we are planning to allow recording of phone calls, please outline the privacy controls planned to prevent bugging of calls. Could an app start recording a call without user interaction? 

A visual notification doesn't seem very strong here if you have the phone up to your ear. But maybe the user always needs to click a prompt to start audio recording. I have read the spec but the current implementation doesn't match that.

Comment 6

[Ivan Tsay (:ITsay)]

Need to further discuss with UX and legal team on the actual behavior. If the time is short for fixing it. We probably can just not allow to get user media when there is a call

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Jason asked me to provide further guidance here - my opinion is that if our indicators were stronger, then I can see this being a safe and valid use case (assuming we completely mitigate the risk of accidentally recording your own call). 

There are probably legal concerns here too - according to my extensive legal knowledge [1] call recording is considering wire-tapping in some jurisdictions (rly australia!?!). That said, answering machines record phone calls and this could be a valid legitimate use case for this API. 

So my opinion here is that this needs to be blocked until we completely the risk of accidentally being recorded. Other bugs (e.g. bug 917359, bug 917544) may address this risk partially, but last time I checked, there was no notification which shows the user which app is doing the recording. I can't see a bug for that so I will raise one since to me that is probably the most important control.




[1] http://en.wikipedia.org/wiki/Telephone_recording_laws

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Just to clarify by "that this needs to be blocked" I mean we should not allow GUM and calls simultaneously until we address the risk of accidental recording.

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I did a experiment of doing recording during a phone call. The media recorder can only record the voice from microphone but not the remote side.
After discussing with @slee, we can monitor the phone state in MediaManager like what we did in bug 904025. However, I would like to move the audio input handling from MediaManager to AudioManager on Firefox OS in the long run.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP - prevent-gum-while-in-call.patch

We can reject GetUserMedia by checking the call status in MediaManager. However, we'll need a way to query the call status while initializing MediaManger.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Right now there is no API for conent process to query the phone state immediately.
We may have another way to achieve thing....

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP2- prevent-gum-while-in-call.patch

According to comment 11, we cannot obtain the correct call state while initializing MediaManager in content process. Therefore, I move the check logic to ContentPermissionPrompt.js, which is running in chrome process. However, taking this approach means the error message will be "Permission Denied" instead of "HARDWARE_UNAVAILABLE".

Comment 13

[Jason Smith [:jsmith]]

(In reply to Shih-Chiang Chien [:schien] from comment #12)
> Created attachment 812595 [details] [diff] [review]
> WIP2- prevent-gum-while-in-call.patch
> 
> According to comment 11, we cannot obtain the correct call state while
> initializing MediaManager in content process. Therefore, I move the check
> logic to ContentPermissionPrompt.js, which is running in chrome process.
> However, taking this approach means the error message will be "Permission
> Denied" instead of "HARDWARE_UNAVAILABLE".

If you can get me a try build, then I can provide feedback here.

Comment 14

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

Comment on attachment 812595 [details] [diff] [review]
WIP2- prevent-gum-while-in-call.patch

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

I can live with the error - not ideal, but ok

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 812595 [details] [diff] [review]
WIP2- prevent-gum-while-in-call.patch

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

::: b2g/components/ContentPermissionPrompt.js
@@ +28,5 @@
>  
>  var permissionManager = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
>  var secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);
>  
> +var permissionSpecificChecker = {};

s/var/let

@@ +121,5 @@
>      request.cancel();
>      return true;
>    },
>  
> +  handledSpecific: function handledSpecific(request) {

s/request/aRequest

Also we should find a better name for this function...

@@ +146,5 @@
>  
> +    if (this.handledSpecific(request)) {
> +      return;
> +    }
> +

merge these two as:
 if (this.handleExistingPermission(request) ||
     this.handledSpecific(request))

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: prevent-gum-while-in-call.patch

Update according to feedback comment.

@jsmith, please use the following build for feedback. Thanks.
https://tbpl.mozilla.org/?tree=Try&rev=6f49da1901bd

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 812947 [details] [diff] [review]
prevent-gum-while-in-call.patch

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

r=me with comments addressed

::: b2g/components/ContentPermissionPrompt.js
@@ +126,5 @@
> +    if (permissionSpecificChecker.hasOwnProperty(request.type)) {
> +      return permissionSpecificChecker[request.type](request);
> +    } else {
> +      return false;
> +    }

return permissionSpecificChecker.hasOwnProperty(request.type) 
         ? permissionSpecificChecker[request.type](request)
         : false;

@@ +136,5 @@
>        request.allow();
>        return true;
>      }
>  
> +    if (this.handledByApp(request) || 

nit: trailing whitespace

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: prevent-gum-while-in-call.patch

Update patch according to review comments, carry r+.

@jsmith, you may use the try build for feedback.
https://tbpl.mozilla.org/?tree=Try&rev=a4c299d4e981

Comment 19

[Jason Smith [:jsmith]]

Comment on attachment 813077 [details] [diff] [review]
prevent-gum-while-in-call.patch

Tested on your most recent try build - lgtm. I got permission denied when I try to access gUM audio while within a call. gUM audio worked after the call was complete, so it didn't cause any immediate regressions. Also checked to make sure that making a call to the phone with gUM audio active releases the stream - that still works as well.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1bf11c407d09

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1bf11c407d09

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b4152d366c3

Comment 23

[Jason Smith [:jsmith]]

Already verified pre-landing, so marking as verified.