1330350 - The "mediaDevices.ondevicechange" devices counter is faulty

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Alexandru Simonca [:asimonca], Release Desktop QA]

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

Comment 1

[Alex Chronopoulos [:achronop]]

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!

Comment 2

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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.

Comment 3

[Alexandru Simonca [:asimonca], Release Desktop QA]

Also happens on Windows 7.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 5

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1330350 - add holding mechanism to fix faulty device counter;

Review commit: https://reviewboard.mozilla.org/r/107472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107472/

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 8

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

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 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8830748 [details]
Bug 1330350 - add holding mechanism to fix faulty device counter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107472/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7cb0a69d237
add holding mechanism to fix faulty device counter; r=jib

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Alexandru Simonca [:asimonca], Release Desktop QA]

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.

Comment 13

[Alexandru Simonca [:asimonca], Release Desktop QA]

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

Comment 14

[Alexandru Simonca [:asimonca], Release Desktop QA]

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.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f668fe84b2b5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eecf3e2efa04

Comment 19

[Andrei Vaida [:avaida]]

This fix is available in 52.0b9 -- Alex, could you please take a look at it?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/eecf3e2efa04

Comment 21

[Alexandru Simonca [:asimonca], Release Desktop QA]

This issue is VERIFIED FIXED in Beta 52.0b9 on Windows 10 x64, Mac OS X 10.11.6 and Ubunutu 16.04 x64.