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

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Jason Smith [:jsmith]]

Attachment: 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.

Comment 1

[Jason Smith [:jsmith]]

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?

Comment 2

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

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.

Comment 3

[Maire Reavy [:mreavy]]

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.

Comment 4

[Maire Reavy [:mreavy]]

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

Comment 5

[Jason Smith [:jsmith]]

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

Comment 6

[Dão Gottwald [:dao]]

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

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

I support Maire's decision.

Comment 8

[Maire Reavy [:mreavy]]

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!

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for now, we'll have to re-evaluate if this isn't landed without regressions before beta 4.

Comment 10

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

Attachment: Add MediaManager function to report what a window is capturing

Comment 11

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

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

Comment 12

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

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

Comment 13

[Dão Gottwald [: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 14

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

Attachment: Add MediaManager function to report what a window is capturing

Comment 15

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

Attachment: Add MediaManager function to report what a window is capturing

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 717628 [details] [diff] [review]
Add MediaManager function to report what a window is capturing

># HG changeset patch
># User Randell Jesup <[e-mail]>
># 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 17

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

Attachment: Add MediaManager function to report what a window is capturing

Comment 18

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

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 19

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 20

[Dão Gottwald [:dao]]

Attachment: front-end part

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.

Comment 21

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

Attachment: Add MediaManager function to report what a window is capturing

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

Comment 22

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

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 23

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

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

dao: good for review

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c22e7998e22

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c22e7998e22

Comment 26

[Justin Dolske [: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.

Comment 27

[Dão Gottwald [:dao]]

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

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

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 29

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

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

[Approval Request Comment]

See request for the backend patch in this bug

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Jason Smith [:jsmith]]

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 33

[Jason Smith [:jsmith]]

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.

Comment 34

[Jason Smith [:jsmith]]

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

Comment 35

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

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.

Comment 36

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

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)

Comment 37

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

More info: Nightly Debug Mac builds appear to work correctly.  I'm spinning an opt build on Linux

Comment 38

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

Attachment: fix backend for GUM a/v notification in opt builds

Comment 39

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

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 40

[Timothy B. Terriberry (:derf)]

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.

Comment 41

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

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

Comment 42

[Timothy B. Terriberry (:derf)]

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

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

Attachment: fix backend for GUM a/v notification in opt builds

Comment 44

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

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 45

[Timothy B. Terriberry (:derf)]

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.

Comment 46

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

Attachment: fix backend for GUM a/v notification in opt builds

Comment 47

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

https://hg.mozilla.org/mozilla-central/rev/937342a52d2e

Comment 48

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

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

Comment 49

[Jason Smith [:jsmith]]

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.

Comment 50

[Jason Smith [:jsmith]]

Marking as verified with one followup.

Comment 51

[Jason Smith [:jsmith]]

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

Comment 52

[Jason Smith [:jsmith]]

Given that bug 847180 actually isn't a regression from this patch, then this patch in full should now be safe to uplift.

Comment 53

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

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 54

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

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 55

[Jason Smith [:jsmith]]

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 56

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

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 57

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 58

[Alex Keybl [:akeybl]]

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

Comment 59

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 60

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

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

Comment 61

[Benjamin Smedberg]

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.

Comment 62

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

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

Comment 63

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

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

Comment 64

[Mihaela Velimiroviciu (:mihaelav)]

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.

Comment 65

[u279076]

Flagging for verification in Firefox 21.

Comment 66

[Mihaela Velimiroviciu (:mihaelav)]

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.