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)
Tracking
()
VERIFIED
FIXED
mozilla54
People
(Reporter: asimonca, Assigned: mchiang)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jib
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
[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•8 years ago
|
||
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•8 years 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.
Comment 4•8 years ago
|
||
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.
Updated•8 years ago
|
Rank: 21 → 10
Priority: P2 → P1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mchiang
Comment hidden (mozreview-request) |
Comment 6•8 years 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•8 years 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•8 years 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•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 12•8 years 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.
Reporter | ||
Comment 13•8 years 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•8 years 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)
Updated•8 years ago
|
Flags: needinfo?(jib)
OS: All → Windows
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Updated•8 years ago
|
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
Comment 17•8 years ago
|
||
bugherder uplift |
Comment 18•8 years ago
|
||
bugherder uplift |
Comment 19•8 years ago
|
||
This fix is available in 52.0b9 -- Alex, could you please take a look at it?
Flags: qe-verify+
Flags: needinfo?(alexandru.simonca)
Comment 20•8 years ago
|
||
bugherder uplift |
status-firefox-esr52:
--- → fixed
Reporter | ||
Comment 21•8 years 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)
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•