Closed Bug 1330350 Opened 8 years ago Closed 8 years ago

The "mediaDevices.ondevicechange" devices counter is faulty

Categories

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

x86_64
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox52 + verified
firefox-esr52 --- fixed
firefox53 + fixed
firefox54 --- verified

People

(Reporter: asimonca, Assigned: mchiang)

References

Details

Attachments

(1 file)

[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
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.
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: nobody → mchiang
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 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 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.
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
I consider that we should uplift this fix to beta. It's currently blocking our tests for the pre-Release sign off.
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+
This fix is available in 52.0b9 -- Alex, could you please take a look at it?
Flags: qe-verify+
Flags: needinfo?(alexandru.simonca)
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: