The "mediaDevices.ondevicechange" devices counter is faulty

VERIFIED FIXED in Firefox 52

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
10
VERIFIED FIXED
11 months ago
9 months ago

People

(Reporter: asimonca, Assigned: mchiang)

Tracking

Trunk
mozilla54
x86_64
Windows
Points:
---

Firefox Tracking Flags

(firefox52+ verified, firefox-esr52 fixed, firefox53+ fixed, firefox54 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
[Note]
2 media devices were used for testing this: 
- Microsoft LifeCam HD-3000 (webcam with microphone)
- Plantronics headset with microphone

[Affected versions]:
- Nightly 53.0a1 (id: 20170106030204)
- Aurora 52.0a2 (id: 20170111004018)

[Affected platforms]:
- Windows 10 x64

[Steps to reproduce]:
1. Go to https://jsfiddle.net/jib1/Lqo4paed/
2. Press "gUM!" and "Share the selected device" on the doorhanger.
3. Press "Enumerate!". 4 devices are found: webcam videoinput, default microphone (virtual OS webcam mic), desktop microphone (webcam mic) & microphone (headset microphone).
4. Disconnect the camera. This causes 2 of the 4 devices to be disconnected and 2 "device change!" messages appear.
5. Press "Enumerate!" again. 2 devices are found. default: Microphone (virtual OS headset mic) & Microphone (headset mic).
6. Plug the camera back in.

[Expected result]:
- 2 "device change!" messages should appear to inform the user that 2 new devices were connected in addition to the other 2.

[Actual result]:
- 6-7 "device change!" messages are appear.

[Regression range]:
- This is not a regression. It is a new feature.
I get the same error. I do not have it on OSX. My output:

5 devices.
videoinput: Integrated Camera id=6srVT/dweh9z8s76ICeSp4/xa6mMNr7NdQ8OZOuEOB4= group=
videoinput: Logitech HD Pro Webcam C920 id=pP/77+nind4g5hvPhpAfTWeCg5/3dR+C03oRwSTnJ2Q= group=
audioinput: default: Microphone (HD Pro Webcam C920) id=NEYEiHZpIvkYC3MMVi6YWM+74GXq/io+opvXP9ufexQ= group=
audioinput: Internal Microphone (Conexant 20672 SmartAudio HD) id=AgCXAn2uBzUgWIZ6FHSqzZ/hFRGQCbQZLncJ1j8RWp0= group=
audioinput: Microphone (HD Pro Webcam C920) id=Z/ugxP5X0esT5Jhxmw6rRgNW1T7auVl+c5ZMEkdUR5w= group=
device change!
device change!
3 devices.
videoinput: Integrated Camera id=6srVT/dweh9z8s76ICeSp4/xa6mMNr7NdQ8OZOuEOB4= group=
audioinput: default: Internal Microphone (Conexant 20672 SmartAudio HD) id=q96wYTLsksiIKs0rd6kLXjYCMjJDiyB/7Qr41CFL904= group=
audioinput: Internal Microphone (Conexant 20672 SmartAudio HD) id=AgCXAn2uBzUgWIZ6FHSqzZ/hFRGQCbQZLncJ1j8RWp0= group=
device change!
device change!
device change!
device change!
device change!
device change!
device change!
Rank: 21
Priority: -- → P2
(Assignee)

Comment 2

10 months ago
http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/media/webrtc/trunk/webrtc/modules/video_capture/windows/device_info_ds.cc#63

The above snippet is the most upstream. The WM_DEVICECHANGE event is sent from Windows system through the callback function WndProc we provided.

Not sure why so many events are sent from Windows system while plug in a device.
(Reporter)

Comment 3

10 months ago
Also happens on Windows 7.
OS: Windows 10 → All
The spec allows for fuzzing of these events for privacy reasons, so regardless of cause, we can probably solve this by adding, say, a 0.5 second delay.
Rank: 21 → 10
Priority: P2 → P1
(Assignee)

Updated

10 months ago
Assignee: nobody → mchiang
Comment hidden (mozreview-request)

Comment 6

10 months ago
mozreview-review
Comment on attachment 8830748 [details]
Bug 1330350 - add holding mechanism to fix faulty device counter;

https://reviewboard.mozilla.org/r/107472/#review108680

Lgtm with a few nits.

::: dom/media/MediaDevices.h:32
(Diff revision 1)
>                            ,public DeviceChangeCallback
>  {
>  public:
>    explicit MediaDevices(nsPIDOMWindowInner* aWindow) :
> -    DOMEventTargetHelper(aWindow) {}
> +    DOMEventTargetHelper(aWindow) {
> +    mFuzzTimer = do_CreateInstance(NS_TIMER_CONTRACTID);

Maybe defer creation of timer until first-use? The only reason to define it at construction time would be to make it invariant, and it does not appear to be.

::: dom/media/MediaDevices.cpp:18
(Diff revision 1)
>  #include "nsIScriptGlobalObject.h"
>  #include "nsIPermissionManager.h"
>  #include "nsPIDOMWindow.h"
>  #include "nsQueryObject.h"
>  
> +#define FUZZ_TIMEOUT_IN_MS 1000

Maybe DEVICECHANGE_HOLD_TIME_IN_MS since we're not actually fuzzing (maybe a different bug)?

Also, did you find half a second wasn't sufficient?

::: dom/media/MediaDevices.cpp:34
(Diff revision 1)
> +
> +  NS_DECL_ISUPPORTS
> +
> +  NS_IMETHOD Notify(nsITimer* aTimer) final
> +  {
> +    if (mMediaDevices) {

No need to test. mMediaDevices is invariant.

::: dom/media/MediaDevices.cpp:220
(Diff revision 1)
>    if (NS_FAILED(rv))
>      return;
>  
>    if (!(MediaManager::Get()->IsActivelyCapturingOrHasAPermission(GetOwner()->WindowID()) ||
>      Preferences::GetBool("media.navigator.permission.disabled", false))) {
>      return;
>    }
>  
> -  DispatchTrustedEvent(NS_LITERAL_STRING("devicechange"));
> +  if (mFuzzTimer)
> +  {

Just looking at this and the two returns right above this new code, and thinking that we have a lot of silent failure here. We should probably LOG something here if ever any of these three things fail.
Attachment #8830748 - Flags: review?(jib) → review+

Comment 7

10 months ago
mozreview-review-reply
Comment on attachment 8830748 [details]
Bug 1330350 - add holding mechanism to fix faulty device counter;

https://reviewboard.mozilla.org/r/107472/#review108680

> Just looking at this and the two returns right above this new code, and thinking that we have a lot of silent failure here. We should probably LOG something here if ever any of these three things fail.

Sorry two out of three. Ignore my comment wrt "media.navigator.permission.disabled" since that's not a failure obviously.
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
Comment on attachment 8830748 [details]
Bug 1330350 - add holding mechanism to fix faulty device counter;

https://reviewboard.mozilla.org/r/107472/#review108680

> Maybe DEVICECHANGE_HOLD_TIME_IN_MS since we're not actually fuzzing (maybe a different bug)?
> 
> Also, did you find half a second wasn't sufficient?

Yes, 0.5 second is not sufficient for Mac.
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 10

10 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7cb0a69d237
add holding mechanism to fix faulty device counter; r=jib
Keywords: checkin-needed

Comment 11

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7cb0a69d237
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Reporter)

Comment 12

10 months ago
This issue is VERIFIED FIXED on Windows 10 x64, Windows 7 x64, Mac OS 10.12.3 and Ubuntu 14.04 x64 for Firefox Nightly 54.0a1 (id: 20170130030205). The event is only triggered once upon connecting / disconnecting cameras.
status-firefox54: fixed → verified
(Reporter)

Comment 13

9 months ago
I consider that we should uplift this fix to beta. It's currently blocking our tests for the pre-Release sign off.
(Reporter)

Comment 14

9 months ago
Jib, do you think we'll get this in tomorrow's Beta 9? It's the last chance we have to test it before the RC.
Flags: needinfo?(jib)
Flags: needinfo?(jib)
OS: All → Windows
Comment on attachment 8830748 [details]
Bug 1330350 - add holding mechanism to fix faulty device counter;

Approval Request Comment
[Feature/Bug causing the regression]: New feature in 52 (bug 1300468) 
[User impact if declined]: 

Web sites trying out this new feature in 52 may see up to ~7 devicechange events fire where one is expected, whenever a USB camera is disconnected or plugged in on Windows.

[Is this code covered by automated tests?]:

We have coverage of the code [1] even though we don't simulate firing multiple events, which is something that came from the Windows backend. The remedy, a timer-delay, is arguably code-covered even without firing more than one event.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_ondevicechange.html

[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:

Yes. STRs in comment 0, with the modification that we only produce and want a single event fired, even for cameras with mics.

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.

[Why is the change risky/not risky?]:

New feature. Patch is very low risk, because it modifies a new feature in lazy-loaded navigator.mediaDevices, and moreover won't run except on web pages that not only touch navigator.mediaDevices, but register a "devicechange" event listener on it (the new feature). Thus, any fallout should be limited to a narrow subset of the WebRTC-using sites that try out this brand new feature. 

The patch adds a timer with a 1 second delay window after which only a single event is fired, where before multiple events might have fired.

[String changes made/needed]: None
Attachment #8830748 - Flags: approval-mozilla-beta?
Attachment #8830748 - Flags: approval-mozilla-aurora?
Comment on attachment 8830748 [details]
Bug 1330350 - add holding mechanism to fix faulty device counter;

We should retest, but yes let's try to get this into the build today. It may not land until the RC build next Monday (in which case we would need to uplift to m-r)
Attachment #8830748 - Flags: approval-mozilla-beta?
Attachment #8830748 - Flags: approval-mozilla-beta+
Attachment #8830748 - Flags: approval-mozilla-aurora?
Attachment #8830748 - Flags: approval-mozilla-aurora+
tracking-firefox52: --- → +
tracking-firefox53: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/f668fe84b2b5
status-firefox53: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/eecf3e2efa04
status-firefox52: affected → fixed
This fix is available in 52.0b9 -- Alex, could you please take a look at it?
Flags: qe-verify+
Flags: needinfo?(alexandru.simonca)
https://hg.mozilla.org/releases/mozilla-esr52/rev/eecf3e2efa04
status-firefox-esr52: --- → fixed
(Reporter)

Comment 21

9 months ago
This issue is VERIFIED FIXED in Beta 52.0b9 on Windows 10 x64, Mac OS X 10.11.6 and Ubunutu 16.04 x64.
Flags: needinfo?(alexandru.simonca)
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.