The sharing dialog for getUserMedia always reports the last shared set of devices, not the devices actively in use in a tab

VERIFIED FIXED in Firefox 20

Status

()

defect
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jsmith, Assigned: dao)

Tracking

Trunk
Firefox 22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ verified, firefox21+ verified, firefox22+ verified)

Details

(Whiteboard: [getUserMedia][blocking-gum+])

Attachments

(4 attachments, 6 obsolete attachments)

2.73 KB, application/zip
Details
4.73 KB, patch
Dolske
: review+
jesup
: feedback+
Details | Diff | Splinter Review
6.24 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.71 KB, patch
derf
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
Posted file Testcase Zip File
Steps:

1. Load the HTML file in the attached zip
2. Share your camera and mic for video #1
3. Click the green icon - notice it says you are sharing your camera + mic
4. Share your camera for video #2 only
5. Click the green icon

Expected:

The dialog that appears should indicate that your camera and mic are being shared.

Actual:

The dialog shows that your camera is only being shared. This implies that the drop down dialog is only indicating the last set of devices shared within a tab, not the actual list of devices shared in the tab.
Reporter

Updated

6 years ago
Whiteboard: [getUserMedia]
Reporter

Comment 1

6 years ago
My initial read of this bug makes me think this could be a blocker, but I'm going to ask for Monica's input to confirm.

In short, this bug reveals our dialog for showing what devices are being shared is going to fail with multiple different gum calls such as:

* gum video, then gum audio
* gum audio+video, then gum video
* etc...

So the user will only know the last set of devices they chose to share, not the devices that actually being shared within the tab.

Monica - Thoughts on this from the privacy perspective? If we're indicating a lack of truthfulness in multiple gUM usage (camera+mic device integration), how bad is this from the privacy perspective?
Flags: needinfo?(mmc)
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum+]
So this clear:

The testcase here has two separate calls to getUserMedia() within the same tab, and the UI doorhanger from the urlbar only tells us we're sharing whatever was shared in the last call it appears.  Probably the right solution is to indicate the union of all shared streams for that tab/window, and recalculate the union either at display time, or as streams are added and removed.

When the streams are in two different tabs, the doorhangers for each are correct.
I am happy to hear Monica's thoughts here, but as the product manager for the feature, I feel we need to fix this bug before release.  If we're going to support multiple calls to gUM in one tab, we need to be accurate about what user data is being shared in that tab.  For me, this isn't even a close call about what the right thing to do here is: we should block and try to get a fix for it in ASAP.
Flags: needinfo?(mmc)
Perhaps most importantly - who can get us a fix for this bug?
Reporter

Comment 5

6 years ago
(In reply to Maire Reavy [:mreavy] from comment #4)
> Perhaps most importantly - who can get us a fix for this bug?

Since Dao is fixing the other bug, maybe he could help us out on this bug too?
Assignee

Comment 6

6 years ago
(In reply to Randell Jesup [:jesup] from comment #2)
> solution is to indicate the union of all shared streams for that tab/window,
> and recalculate the union either at display time, or as streams are added
> and removed.

Since this list doesn't just grow but can shrink when a site releases access to some but not all devices, we'd need some new nsIMediaManagerService method for the front-end to reliably tell if a site currently has camera access, microphone access or both.
I support Maire's decision.
Assigning to Randell to do the backend pieces of this (i.e.when passed a window, Randell's code will return if it has audio, video or both shared).  Once Randell has the backend piece completed, he'll reassign to Dao for the front-end work.  I'd like to get all the patches for this fix coded, reviewed, and uplifted before the next Beta build, if possible.  I think Randell thinks he can get his piece coded and ready for review later today.

If this plan doesn't make sense to anyone, please speak up.  Thanks!
Assignee: nobody → rjesup
Tracking for now, we'll have to re-evaluate if this isn't landed without regressions before beta 4.
Comment on attachment 717565 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

Backend patch to support solving this bug per request from Dao.  If the interface is a problem from JS, please just indicate what interface/IDL you'd like.  Assumes it's passed an inner-window.

I will remove the test code before committing (and will try to see if I can write a mochitest for it as well; under a separate r?)
Attachment #717565 - Flags: review?(anant)
Attachment #717565 - Flags: feedback?(dolske)
Attachment #717565 - Flags: feedback?(dao)
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum+][webrtc-uplift]
We'll need a matching front-end patch as well, I assume from Dao.
Assignee: rjesup → dao
Assignee

Comment 13

6 years ago
Comment on attachment 717565 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

Does this handle nested/framed windows?
Attachment #717565 - Attachment is obsolete: true
Attachment #717565 - Flags: review?(anant)
Attachment #717565 - Flags: feedback?(dolske)
Attachment #717565 - Flags: feedback?(dao)
Attachment #717627 - Attachment is obsolete: true
Comment on attachment 717628 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

># HG changeset patch
># User Randell Jesup <rjesup@jesup.org>
># Date 1361720456 18000
># Node ID f80a361a1be64b70d9077f09988440e0994553cf
># Parent  3b81696b6d222da9fc18230652545130183309d1
>Bug 843971: Add MediaManager function to report what a window is capturing
>
>diff --git a/dom/media/MediaManager.cpp b/dom/media/MediaManager.cpp
>--- a/dom/media/MediaManager.cpp
>+++ b/dom/media/MediaManager.cpp
>@@ -8,16 +8,17 @@
> #include "nsIDOMFile.h"
> #include "nsIEventTarget.h"
> #include "nsIUUIDGenerator.h"
> #include "nsIScriptGlobalObject.h"
> #include "nsIPopupWindowManager.h"
> #include "nsISupportsArray.h"
> #include "nsIPrefService.h"
> #include "nsIPrefBranch.h"
>+#include "nsIDocShell.h"
> 
> // For PR_snprintf
> #include "prprf.h"
> 
> #include "nsJSUtils.h"
> #include "nsDOMFile.h"
> #include "nsGlobalWindow.h"
> 
>@@ -1292,16 +1293,84 @@ MediaManager::GetActiveMediaCaptureWindo
>     return rv;
> 
>   mActiveWindows.EnumerateRead(WindowsHashToArrayFunc, array);
> 
>   *aArray = array;
>   return NS_OK;
> }
> 
>+nsresult
>+MediaManager::MediaCaptureWindowState(nsIDOMWindow *aWindow, bool *aVideo,
>+                                      bool *aAudio)
* goes with type, nsIDOMWindow* ...


>+{
>+  // We assume we're passed an OuterWindow here
Since it is trivial to support both outer and inner window, we could support both.


>+        nsCOMPtr<nsPIDOMWindow> win = do_GetInterface(item);
>+        if (win && win->GetCurrentInnerWindow()) {
>+          uint64_t windowID = win->GetCurrentInnerWindow()->WindowID();
>+          StreamListeners* listeners = GetActiveWindows()->Get(windowID);
>+          if (listeners) {
So this part should be before the loop. Otherwise you miss to check whether aWindow has listeners.


>-[scriptable, builtinclass, uuid(afe82ff1-2caa-4304-85da-0158a5dee56b)]
>+[scriptable, builtinclass, uuid(2efff6ab-0e3e-4cc4-8f9b-4aaca59a1140)]
> interface nsIMediaManagerService : nsISupports
> {
>+  /* return a array of inner windows that have active captures */
>   readonly attribute nsISupportsArray activeMediaCaptureWindows;
>+
>+  /* Get the capture state for the given window/tab */
>+  void MediaCaptureWindowState(in nsIDOMWindow aWindow, out boolean aVideo, out boolean aAudio);
in .idl method names start with lowercase, so mediaCaptureWindowState
Also, remove "/tab" from the comment and add something about descendant windows.
Attachment #717628 - Flags: review?(bugs) → review-
Attachment #717628 - Attachment is obsolete: true
Comment on attachment 717637 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

Addresses issues; and tested with a quick iframe testcase
Attachment #717637 - Flags: review?(bugs)
Comment on attachment 717637 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

+nsresult
+MediaManager::MediaCaptureWindowState
nsresult -> NS_IMETHODIMP

for (uint32_t i = 0; i < length; i++) {
->
for (uint32_t i = 0; i < length; ++i) {

s/i_end/count/


+  bool CapturingVideo()
+  {
+    if (mVideoSource && mLastEndTimeVideo > 0 && !mFinished) {
+      return true;
+    }
+    return false;
+  }
Maybe just
  return mVideoSource && mLastEndTimeVideo > 0 && !mFinished;

+  bool CapturingAudio()
+  {
+    if (mAudioSource && mLastEndTimeAudio > 0 && !mFinished) {
+      return true;
+    }
+    return false;
+  }
Maybe just
  return mAudioSource && mLastEndTimeAudio > 0 && !mFinished;

This might be a bit nicer if there was public
MediaCaptureWindowState
and then MediaCaptureWindowStateInternal.
Only the public one sets out values by default to false.
Then it calls the internal and internal uses *aVideo and *aAudio everywhere 
and the internal calls the internal method recursively.
That way we wouldn't need temporary audio/video/temp_audio/temp_video nor goto nor break, but just return;

But either way, r=me
Attachment #717637 - Flags: review?(bugs) → review+
Assignee

Comment 20

6 years ago
This doesn't seem to work. I'm getting the recording-device-events notification and activeMediaCaptureWindows contains the new window as expected, but mediaCaptureWindowState returns false for video and audio.
Attachment #718944 - Flags: feedback?(rjesup)
with nits and suggestions from smaug addressed, and with a fix to handle an inner-window being directly passed in
Attachment #717637 - Attachment is obsolete: true
Comment on attachment 719110 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

This works.  It also works with a trivial page with two iframes, each pointing to gum_test.html, and using them to verify the merging (and un-merging) works correctly.
Attachment #719110 - Flags: review?(bugs)
Comment on attachment 718944 [details] [diff] [review]
front-end part

dao: good for review
Attachment #718944 - Flags: feedback?(rjesup) → feedback+
Attachment #719110 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c22e7998e22
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift] → [getUserMedia][blocking-gum+][webrtc-uplift][leave-open]
Target Milestone: --- → Firefox 22
Assignee

Updated

6 years ago
Attachment #718944 - Flags: review?(dolske)
Comment on attachment 718944 [details] [diff] [review]
front-end part

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

::: browser/modules/webrtcUI.jsm
@@ +227,5 @@
> +    captureState = "Camera";
> +  else if (hasAudio.value)
> +    captureState = "Microphone";
> +  else
> +    throw "browser has neither video nor audio access";

Perhaps we should just ignore (console error + return) instead of throwing? If we somehow get into this state, it might be better to try to finish doing the remaining updates.
Attachment #718944 - Flags: review?(dolske) → review+
Assignee

Comment 27

6 years ago
(In reply to Justin Dolske [:Dolske] from comment #26)
> Perhaps we should just ignore (console error + return) instead of throwing?
> If we somehow get into this state, it might be better to try to finish doing
> the remaining updates.

done

https://hg.mozilla.org/integration/mozilla-inbound/rev/da75867527dd
Assignee

Updated

6 years ago
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift][leave-open] → [getUserMedia][blocking-gum+][webrtc-uplift]
Comment on attachment 719110 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Initial landing of the UI feature

User impact if declined: When one tab has multiple streams, what is currently being captured will be mis-reported.  May allow an application to disguise that it's still capturing audio or video, though it should show if it's capturing at all.  It may cause user concern if there are cases where it mis-reports what's being captured, and may cause privacy concerns.

Testing completed (on m-c, etc.): This patch (backend) is on m-c; front-end is on inbound.  Verified so far by me with a page of iframes to gum_test pages.

Risk to taking this patch (and alternatives if risky): Low risk - simply walks the window tree and asks if each is capturing.  All the fallible calls are checked.  Alternative is to not be concerned with it mis-reporting in the unusual case where the same tab captures multiple times or captures audio and video separately instead of with one getUserMedia() call, and document this as a known bug (perhaps relnote). 

String or UUID changes made by this patch: none in this one.
Attachment #719110 - Flags: approval-mozilla-beta?
Attachment #719110 - Flags: approval-mozilla-aurora?
Comment on attachment 718944 [details] [diff] [review]
front-end part

[Approval Request Comment]

See request for the backend patch in this bug
Attachment #718944 - Flags: approval-mozilla-beta?
Attachment #718944 - Flags: approval-mozilla-aurora?
Triage drive-by, will come back around for approval once this has gotten onto central.  We'll want this in beta 3 to get lots of exposure as well as verification before ship.
https://hg.mozilla.org/mozilla-central/rev/da75867527dd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Reporter

Updated

6 years ago
Keywords: verifyme
QA Contact: jsmith
Attachment #718944 - Flags: approval-mozilla-beta?
Attachment #718944 - Flags: approval-mozilla-beta+
Attachment #718944 - Flags: approval-mozilla-aurora?
Attachment #718944 - Flags: approval-mozilla-aurora+
Attachment #719110 - Flags: approval-mozilla-beta?
Attachment #719110 - Flags: approval-mozilla-beta+
Attachment #719110 - Flags: approval-mozilla-aurora?
Attachment #719110 - Flags: approval-mozilla-aurora+
Reporter

Comment 32

6 years ago
These patches totally broke many audio/video gUM requests with the visible UI. Here are such cases that regressed:

* Requesting audio shows no visible indicator at all (no green icon)
* Requesting video and audio shows the visible indicator with camera only
* The original test case in the bug still doesn't work either

Major regressions + original test case doesn't work. Please back this out.
Keywords: verifyme
Reporter

Comment 33

6 years ago
Comment on attachment 718944 [details] [diff] [review]
front-end part

Removing approvals as this causes major regressions with this patch and the original test case doesn't work. So do not uplift this to Aurora and Beta.
Attachment #718944 - Flags: approval-mozilla-beta+
Attachment #718944 - Flags: approval-mozilla-aurora+
Reporter

Updated

6 years ago
Attachment #719110 - Flags: approval-mozilla-beta+
Attachment #719110 - Flags: approval-mozilla-aurora+
Reporter

Comment 34

6 years ago
Flagging checkin-needed to back both patches out. When these patches get backed out, please reopen the bug.
Keywords: checkin-needed
jsmith: I updated from inbound, and tried gum, my gum_iframe patch, and the testcase here.

I see no problems (linux, but there's no platform-dependent code I know of here).

However, on Windows and Mac nightlies from today, I do see it.

Reopening.

I'm guessing that on Mac/Windows the front-end code is passing a different window to the "get me the audio and video".  Unless it's a 32/64-bit thing.... my builds are 64-bits (bit then, so are the Mac builds, so this is unlikely).  I assume xpconnect would do the right thing here with boolean out args anyways.

I'll back the front-end out for now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Weird thing is that Video appears to show correctly; the problems all seem to be audio.

Looking deeper (mediamanger:5 logging in a debug build will help some)
More info: Nightly Debug Mac builds appear to work correctly.  I'm spinning an opt build on Linux
Comment on attachment 720305 [details] [diff] [review]
fix backend for GUM a/v notification in opt builds

Turns out all opt builds were broken.  The cause was a test to see if the media had started (checking the LastEndTime), but for audio since we push, the NotifyPull() support is only for debug builds, and thus it didn't update the LastEndTime.

We don't actually need this, as Activate() tells us we should be recording; we don't need for media to actually start flowing to return that it's enabled.

Removed the test of LastEndTime.
Attachment #720305 - Flags: review?(tterribe)
Comment on attachment 720305 [details] [diff] [review]
fix backend for GUM a/v notification in opt builds

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

This is an improvement, but

A) It's missing thread assertions. AFAICT it's only safe to test mAudioSource and mVideoSource on the main thread (or the MSG thread, but only after Activate has been called).

B) Accessing mFinished races with NotifyFinish on the MSG thread.
Attachment #720305 - Flags: review?(tterribe) → review-
(In reply to Timothy B. Terriberry (:derf) from comment #40)
> Comment on attachment 720305 [details] [diff] [review]
> fix backend for GUM a/v notification in opt builds
> 
> Review of attachment 720305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is an improvement, but
> 
> A) It's missing thread assertions. AFAICT it's only safe to test
> mAudioSource and mVideoSource on the main thread (or the MSG thread, but
> only after Activate has been called).

They're only tested from MainThread.  Added an assert to the master call in MediaManager.cpp

> B) Accessing mFinished races with NotifyFinish on the MSG thread.

NotifyFinished() on MSG thread sets mFinished, and the posts a MEDIA_STOP to the MediaManager thread, which stops the media, and posts GetUserMediaNotificationEvent::STOPPING to the MainThread, and that is what Notify()s the UI to call us and ask what sources are still in use - and test mFinished from MainThread.  So there should be no race. If it was calling us for some other notification while a stream was stopping, you might have a result that we're still capturing (though highly unlikely) but the actual notification for that NotifyFinished is still in progress and would replace that once done (since they'd need to call us again).  You could argue that maybe it would show we'd stopped capturing early, but it would still be after NotifyFinished(), so it isn't early.
(In reply to Randell Jesup [:jesup] from comment #41)
> that once done (since they'd need to call us again).  You could argue that
> maybe it would show we'd stopped capturing early, but it would still be
> after NotifyFinished(), so it isn't early.

Or you might never notice, thanks to the fact that this is undefined behavior. Please actually read the article cdiehl has been linking in every one of the dozens of TSan race bugs he's been filing aginst us: <http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong>. But even if I bought your arguments along these lines before (which I didn't) and even if that article was wrong (I don't think it is) or its concerns vanishingly unlikely to manifest in practice (quite possible... I don't think anyone's done statistics), I would still insist we fix this race if solely because it increases the SNR of automatic tooling like TSan.
Attachment #720305 - Attachment is obsolete: true
Comment on attachment 720319 [details] [diff] [review]
fix backend for GUM a/v notification in opt builds

> I would still insist we fix this race if solely because it increases the SNR of automatic tooling like TSan

That's a strong argument

Added mStopped, mainthread only, set from the last event in that chain (on mainthread).
Attachment #720319 - Flags: review?(tterribe)
Comment on attachment 720319 [details] [diff] [review]
fix backend for GUM a/v notification in opt builds

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

Not quite there yet, but fix the nits and the STARTING/STOPPING thing and I think we're good to go.

::: dom/media/MediaManager.h
@@ +89,3 @@
>    bool CapturingVideo()
>    {
> +    return mVideoSource && !mStopped;

I was actually hoping for thread asserts in these two methods, as this is where the potential race is and is also something someone might try to re-use for some other purpose in the future.

@@ +94,5 @@
>    {
> +    return mAudioSource && !mStopped;
> +  }
> +
> +  void Stopped()

SetStopped() please. It's not clear if Stopped() is a boolean query or not.

@@ +203,5 @@
> +      if (!obs) {
> +        NS_WARNING("Could not get the Observer service for GetUserMedia recording notification.");
> +        return NS_ERROR_FAILURE;
> +      }
> +      if (mStatus) {

STARTING == 0, so if (mStatus) means STOPPING, and thus you just added the Stopped() call to the wrong branch (which was sending the wrong event to NotifyObservers anyway, because clearly the original author was also confused here).

I was going to suggest converting to a switch statement for readability, but all the code except the Stopped() call you just added is common between both branches, with the exception of the name of the event. Just use a static array indexed by the enum to look up that string and get rid of the code duplication. Then you can leave if (mStatus == STOPPING && mListener) { mListener->SetStopped(); } before it.
Attachment #720319 - Flags: review?(tterribe) → review-
Attachment #720319 - Attachment is obsolete: true
Attachment #720327 - Flags: review?(tterribe)
Attachment #720327 - Flags: review?(tterribe) → review+
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Reporter

Updated

6 years ago
Keywords: verifyme
Reporter

Comment 49

6 years ago
Tested on a 3/3 nightly - this is a much better for the most part, but you've still got one regression involving requesting gUM across multiple tabs for concurrent access - the sharing icon doesn't even show on one of the tabs until it's clicked explicitly - http://i.imgur.com/HPECKnc.jpg. I'll file a followup for this.
Reporter

Updated

6 years ago
Depends on: 847180
Reporter

Comment 50

6 years ago
Marking as verified with one followup.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Reporter

Comment 51

6 years ago
Note - Let's hold on uplifting this until the followup bug - bug 847180 is resolved.
Reporter

Updated

6 years ago
No longer depends on: 847180
Reporter

Comment 52

6 years ago
Given that bug 847180 actually isn't a regression from this patch, then this patch in full should now be safe to uplift.
Comment on attachment 718944 [details] [diff] [review]
front-end part

[Approval Request Comment]

Re-nominating now that fix for issue found in test is resolved and tested.
Attachment #718944 - Flags: approval-mozilla-beta?
Attachment #718944 - Flags: approval-mozilla-aurora?
Comment on attachment 719110 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

[Approval Request Comment]

Re-nominating now that fix for issue found in test is resolved and tested.
Attachment #719110 - Flags: approval-mozilla-beta?
Attachment #719110 - Flags: approval-mozilla-aurora?
Reporter

Comment 55

6 years ago
Comment on attachment 720327 [details] [diff] [review]
fix backend for GUM a/v notification in opt builds

You'll also want the followup patch too.
Attachment #720327 - Flags: approval-mozilla-beta?
Attachment #720327 - Flags: approval-mozilla-aurora?
Comment on attachment 720327 [details] [diff] [review]
fix backend for GUM a/v notification in opt builds

jsmith collided with me typing all this... :-)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug

User impact if declined: Can't uplift the other patches in this bug because audio capture would not be shown in the dropdown.  That would mean the "what are your sharing" indications in the URL bar notification (mic or camera or both) would be incorrect if more than one stream is in use by the tab (original reason for this bug and for approval of the other patches).

Testing completed (on m-c, etc.): On m-c.  Tested by Jason.  One existing issue with Notifications already affecting CTP in release is hit; (see comments) which should not block this.

Risk to taking this patch (and alternatives if risky): Very Low.  This patch also happens to fix that the recording notifications (start vs shutdown) had always been backwards (this patch needed to modify that code and the bug would have caused it to break); the UI currently isn't monitoring the value however.

String or UUID changes made by this patch: none
Assignee

Updated

6 years ago
Blocks: 845397
Comment on attachment 718944 [details] [diff] [review]
front-end part

Glad to see the kinks have been mostly caught early and we can go ahead with uplifting this today so it can go into tomorrow' Beta 3 build.
Attachment #718944 - Flags: approval-mozilla-beta?
Attachment #718944 - Flags: approval-mozilla-beta+
Attachment #718944 - Flags: approval-mozilla-aurora?
Attachment #718944 - Flags: approval-mozilla-aurora+
Attachment #719110 - Flags: approval-mozilla-beta?
Attachment #719110 - Flags: approval-mozilla-beta+
Attachment #719110 - Flags: approval-mozilla-aurora?
Attachment #719110 - Flags: approval-mozilla-aurora+
Attachment #720327 - Flags: approval-mozilla-beta?
Attachment #720327 - Flags: approval-mozilla-beta+
Attachment #720327 - Flags: approval-mozilla-aurora?
Attachment #720327 - Flags: approval-mozilla-aurora+
jesup noticed that this has an interface change, so CC'ing bsmedberg and adding ni? for jorgev to help make a ba+ determination
Flags: needinfo?(jorge)
Looks okay to me. It's just an addition to the interface and I see no consumers in the MXR. The only potential breakage would be for binary add-ons, but I don't see this component posing a risk for that.
Flags: needinfo?(jorge)
I really want to try to land this for Beta 3; it's all staged and I've tested it locally.  What else do I need for ba= approval?  (Also, the ba= rules should be spelled out somewhere obvious; no one seems to know what they are (or even really that it exists).
jorge already gave ba approval in comment 59?

Although technically we don't *need* to change the interface here: nsIMediaManager2 would have worked just as well. But it's true that it's extremely unlikely that skype or norton toolbar are using this interface from binary code.
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0

Verified the fix on latest beta (Firefox 20 beta 3) builds.
Flagging for verification in Firefox 21.
Keywords: verifyme
Reporter

Updated

6 years ago
QA Contact: jsmith
Whiteboard: [getUserMedia][blocking-gum+][webrtc-uplift] → [getUserMedia][blocking-gum+]
Mozilla/5.0 (Windows NT 6.2; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20100101 Firefox/21.0

Verified also on Firefox 21 RC 2.

Updated

6 years ago
Depends on: 881331
Component: General → Device Permissions
Depends on: 1102868
You need to log in before you can comment on or make changes to this bug.