1423515 - Failed to send a devicechange event for permanent permission case (no live stream)

Status: RESOLVED FIXED

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

Description

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

STRs:

1. Open the 1st tab and go to https://jsfiddle.net/Lqo4paed/ press gUM! and grant permanent permission
2. Open the 2nd tab and go to the same page.
3. Close the 1st tab.
4. Plug in or out an external webcam.

Expected behavior: there should be a devicechange event 
Actual behavior: there is no devicechange event

Comment 1

[Andreas Pehrson [:pehrsons]]

We think the reason we haven't seen this on try is because we've re-used an old MediaManager instance where the devicechange callback setup was already properly done.

An orthogonal but related question is why the MediaManager and/or MediaDevices objects were not GCed after previous tests and whether we can fix that.

Comment 2

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

Attachment: Bug 1423515 - register callback while creating the real backend.

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

Comment 3

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

Attachment: Bug 1423515 - move test_ondevicechange.html to the beginning place.

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

Comment 4

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

try with both patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f2f87aced0160aaf1cfb4a976b8fb407eb95b1&selectedJob=150145779

try with this patch (Bug 1423515 - move test_ondevicechange.html to the beginning place):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0362f057a3c33ddb32437921747355eff94d722&selectedJob=150146745

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8934931 [details]
Bug 1423515 - register callback while creating the real backend.

https://reviewboard.mozilla.org/r/205858/#review211424

r+ if my assumption is true and you add that comment. Thanks!

::: dom/media/MediaManager.cpp:2026
(Diff revision 1)
>  {
>    bool fakeDeviceChangeEventOn = mPrefs.mFakeDeviceChangeEventOn;
>    MediaManager::PostTask(NewTaskFrom([fakeDeviceChangeEventOn]() {
>      MediaManager* manager = MediaManager::GetIfExists();
>      MOZ_RELEASE_ASSERT(manager); // Must exist while media thread is alive
> +    manager->GetBackend(0);

I assume this is needed in case persistent permission is given but no gUM() or enumeration() has created the real backend yet.

The r+ only stands if this assumption is true.

Please put in a comment explaining this because without an explanation this call looks erratic.

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8934932 [details]
Bug 1423515 - move test_ondevicechange.html to the beginning place.

https://reviewboard.mozilla.org/r/205860/#review211426

::: dom/media/tests/mochitest/mochitest.ini:27
(Diff revision 1)
> +# MediaManager and/or MediaDevices objects were not GCed after
> +# previous tests, which make this test unable to detect bug 1423515

I'd like the below info to be part of the comment in some form:

> A MediaManager instance is re-used from previous tests, and so it can inherit the setup of "devicechange" callbacks, masking issues such as bug 1423515 that are then only found by running the test on its own.

::: dom/media/tests/mochitest/mochitest.ini:28
(Diff revision 1)
>    !/dom/media/test/bug461281.ogg
>    !/dom/media/test/seek.webm
>    !/dom/media/test/gizmo.mp4
>  
> +# MediaManager and/or MediaDevices objects were not GCed after
> +# previous tests, which make this test unable to detect bug 1423515

s/this test/test_ondevicechange.html/

::: dom/media/tests/mochitest/mochitest.ini:29
(Diff revision 1)
>    !/dom/media/test/seek.webm
>    !/dom/media/test/gizmo.mp4
>  
> +# MediaManager and/or MediaDevices objects were not GCed after
> +# previous tests, which make this test unable to detect bug 1423515
> +# Before we find out a way to clean up backends when they're no longer used,

s/out //

::: dom/media/tests/mochitest/mochitest.ini:31
(Diff revision 1)
> +[test_ondevicechange.html]
> +skip-if = os == 'android'

I think a newline after this would be appropriate to separate it from the others.

Comment 7

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

Comment on attachment 8934931 [details]
Bug 1423515 - register callback while creating the real backend.

https://reviewboard.mozilla.org/r/205858/#review211450

::: dom/media/MediaManager.cpp:2026
(Diff revision 1)
>  {
>    bool fakeDeviceChangeEventOn = mPrefs.mFakeDeviceChangeEventOn;
>    MediaManager::PostTask(NewTaskFrom([fakeDeviceChangeEventOn]() {
>      MediaManager* manager = MediaManager::GetIfExists();
>      MOZ_RELEASE_ASSERT(manager); // Must exist while media thread is alive
> +    manager->GetBackend(0);

Yes, the assumption is correct. Will put a comment here.

Comment 8

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

It seems we cannot change the test execution order now.
So I filed a new bug 1423790 to clean up the backend which is no longer used and cancel the patch.

Comment 9

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

Comment on attachment 8934931 [details]
Bug 1423515 - register callback while creating the real backend.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8afdf2b8201
register callback while creating the real backend. r=pehrsons

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

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