1250122 - Regression: WebRTC video broken

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. Go to http://mozilla.github.io/webrtc-landing/gum_test.html
2. Tap "Audio & Video"
3. Accept access to the device's built-in camera and built-in microphone

Expected results:
- The video is playing from the built-in camera and sound is heard from the device speakers

Actual results:
- "SourceUnavailableError: Failed to allocate videosource" error message is displayed.

Note:
-regression
good build: 17-02
bad build: 18-02
pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=15621f98b53b1994c7ae2e2703a6e50203c5304c&tochange=1150ac4755c7bb35df4fc7504f6f1b6c257f400e

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Fallout from bug 1177242. At a guess, Firefox for Android has its own version of

https://hg.mozilla.org/mozilla-central/diff/c5d6c3e00c91/browser/modules/webrtcUI.jsm

which probably needs a similar fix. I thought I tested this and concluded it worked, but obviously I was wrong.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Critical regression, tracked for Fx47 as a blocker.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: Record granting permissions for the Camera on Android

The logic is a bit simpler/different from:
https://hg.mozilla.org/mozilla-central/diff/c5d6c3e00c91/browser/modules/webrtcUI.jsm

because it looks like Android doesn't remember permissions at all?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8722579 [details] [diff] [review]
Record granting permissions for the Camera on Android

>diff --git a/mobile/android/chrome/content/WebrtcUI.js b/mobile/android/chrome/content/WebrtcUI.js

>         if (audioDevices[audioId])
>           allowedDevices.AppendElement(audioDevices[audioId]);
> 
>+
>         let videoId = 0;

Remove the extra linebreak

>         if (inputs && inputs.videoSource != undefined)
>           videoId = inputs.videoSource;
>-        if (videoDevices[videoId])
>+        if (videoDevices[videoId]) {
>           allowedDevices.AppendElement(videoDevices[videoId]);
>+          let perms = Services.perms;
>+          perms.add(aUri, "camera", perms.EXPIRE_SESSION);

Services.perms.add(...)  works too

Yeah, I think WesJ wanted to add "rememebering" at some point, but we never did. Maybe you could file that so we could track it for later?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/417bac059dab26a1283843a00a041aebdb2f2813
Bug 1250122 - Record granting permissions for the Camera on Android. r=mfinkle

Comment 7

[Gian-Carlo Pascutto [:gcp]]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Services.perms.add(...)  works too

Yes, but then it needs to be repeated for the constat/enum value later in that line.
 
> Yeah, I think WesJ wanted to add "rememebering" at some point, but we never
> did. Maybe you could file that so we could track it for later?

Filed bug 1250872.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/417bac059dab

Comment 9

[u421692]

Verified as fixed on latest Nightly on Samsung Galaxy S5 (Android 5.0)

Comment 10

[Gian-Carlo Pascutto [:gcp]]

mbanner observed while investigating bug 1249365 that this is actually completely broken.

>perms.add(aUri, "camera", perms.EXPIRE_SESSION);

Should've been perms.add(aUri, "camera", perms.ALLOW_ACTION, perms.EXPIRE_SESSION);

This bug means that we'll accidentally store camera permissions for sites permanently.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: MozReview Request: Bug 1250122 - Fix number of arguments to nsIPermissionManager::Add. r?mfinkle

Review commit: https://reviewboard.mozilla.org/r/38467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38467/

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8727382 [details]
MozReview Request: Bug 1250122 - Fix number of arguments to nsIPermissionManager::Add. r?mfinkle

https://reviewboard.mozilla.org/r/38467/#review35027

Sorry I didn't catch this the first time

Comment 13

[Pulsebot]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db12e253eb84

Comment 15

[u549602]

Verified as fixed on the 48.0b1 build. This issue was tested on a Nexus 6P (Android 7.0), ZTE Grand X (Android 4.0.4) and on a Sony Xperia Z2 (Android 5.0.2)