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.

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?

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.

Perhaps most importantly - who can get us a fix for this bug?

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

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

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?)

We'll need a matching front-end patch as well, I assume from Dao.

Comment on attachment 717565 [details] [diff] [review] Add MediaManager function to report what a window is capturing Does this handle nested/framed windows?

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.

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

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

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.

with nits and suggestions from smaug addressed, and with a fix to handle an inner-window being directly passed in

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.

Comment on attachment 718944 [details] [diff] [review] front-end part dao: good for review

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.

(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

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.

Comment on attachment 718944 [details] [diff] [review] front-end part [Approval Request Comment] See request for the backend patch in this bug

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.

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.

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.

Flagging checkin-needed to back both patches out. When these patches get backed out, please reopen the bug.

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.

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.

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.

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

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

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.

I hate that c++ rule... https://hg.mozilla.org/mozilla-central/rev/763d675d2ab9

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.

Marking as verified with one followup.

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

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.

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.

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.

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

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.

jesup noticed that this has an interface change, so CC'ing bsmedberg and adding ni? for jorgev to help make a ba+ determination

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.

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.

https://hg.mozilla.org/releases/mozilla-beta/rev/56203954a909 https://hg.mozilla.org/releases/mozilla-beta/rev/4e2159c3669c https://hg.mozilla.org/releases/mozilla-beta/rev/870ab128cacd

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ff068be4005 https://hg.mozilla.org/releases/mozilla-aurora/rev/672657b9c774 https://hg.mozilla.org/releases/mozilla-aurora/rev/2044c8923d0a

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.

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.