Closed Bug 1250122 Opened 4 years ago Closed 4 years ago

Regression: WebRTC video broken

Categories

(Firefox for Android :: General, defect)

47 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox46 --- unaffected
firefox47 blocking verified
firefox48 --- verified
fennec 47+ ---

People

(Reporter: TeoVermesan, Assigned: gcp)

Details

(Keywords: regression)

Attachments

(2 files)

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
Flags: needinfo?(gpascutto)
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.
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
[Tracking Requested - why for this release]: regression
tracking-fennec: --- → ?
Critical regression, tracked for Fx47 as a blocker.
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?
Attachment #8722579 - Flags: review?(mark.finkle)
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?
Attachment #8722579 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/417bac059dab26a1283843a00a041aebdb2f2813
Bug 1250122 - Record granting permissions for the Camera on Android. r=mfinkle
(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.
https://hg.mozilla.org/mozilla-central/rev/417bac059dab
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Verified as fixed on latest Nightly on Samsung Galaxy S5 (Android 5.0)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → 47+
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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
Attachment #8727382 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/db12e253eb84
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 47 → Firefox 48
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)
You need to log in before you can comment on or make changes to this bug.