1463581 - Stopping a live gUM track doesn't update the aggregated track.enabled state across track clones

Status: VERIFIED FIXED

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

Description

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

As of bug 1299515 we turn off cam/mic on track.enabled = false. However, there's a bug if you're using track.clone(), where this doesn't happen correctly.

E.g. if you do:

    let copy = videoTrack.clone();
    copy.enabled = false;
    videoTrack.stop();

and hold on to `copy`, the camera should mute, its light extinguish, and the browser's blinking red indicator should switch to solid gray. Instead, the light and blinking red indicator stay on.

STR:
 1. Open https://jsfiddle.net/jib1/gphjq1ta/
 2. Give one-time camera permission (leave "remember this decision" unchecked).
 3. Click [Begin], [End], [Begin], [End]

Expected result:
  No second permission prompt, and light goes out after each [End].

Expected result:
  No second permission prompt, and light stays on.

Workaround:
 3. Click [Begin], [Mute video], [End], [Begin], [End]

Comment 1

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

[Tracking Requested - why for this release]: Unfortunate flaw in new 60 privacy feature. Has a workaround though.

Comment 2

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

Typo: the second "Expected result:" should be "Actual result:"

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1463581 - Add browser chrome test.

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

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1463581 - Signal enabled state changes also on UnregisterSink.

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

Comment 5

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

Comment on attachment 8981905 [details]
Bug 1463581 - Signal enabled state changes also on UnregisterSink.

https://reviewboard.mozilla.org/r/247924/#review254288

lgtm

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8981904 [details]
Bug 1463581 - Add browser chrome test.

https://reviewboard.mozilla.org/r/247922/#review254390

::: browser/base/content/test/webrtc/browser_devices_get_user_media_paused.js:42
(Diff revision 1)
> +            .forEach(t => t.stop());
> +    }
> +    if (args.video != null) {
> +      clones.filter(t => t.kind == "video")
> +            .forEach(t => t.stop());
> +    }

should you maybe clear the gClones list in this function?

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8981904 [details]
Bug 1463581 - Add browser chrome test.

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

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8981905 [details]
Bug 1463581 - Signal enabled state changes also on UnregisterSink.

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

Comment 9

[Andreas Pehrson [:pehrsons]]

test-only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=858a8c8f8b2589d918a33307ed146766702b8eb5
with fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8b644da56615604adc85e86613ed9bd991e44f6

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4931c4cb7c74
Add browser chrome test. r=johannh
https://hg.mozilla.org/integration/autoland/rev/d42b5abd0201
Signal enabled state changes also on UnregisterSink. r=jib

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4931c4cb7c74
https://hg.mozilla.org/mozilla-central/rev/d42b5abd0201

Comment 12

[Ryan VanderMeulen [:RyanVM]]

What kind of testing/verification does this need before we consider it for uplift?

Comment 13

[Andreas Pehrson [:pehrsons]]

IMO the added browser chrome test is sufficient because of the simplicity of the fix. If anyone insists on manual verification we should test that
1) the symptoms are gone per the STR in comment 0, and
2) that it didn't regress any existing behavior.

The latter should be covered by the original PI-request for beta which I don't think has taken place yet.

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8981905 [details]
Bug 1463581 - Signal enabled state changes also on UnregisterSink.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
- This can in some cases leave the camera (both hw light and browser chrome) on even though the application disabled it. This can fool users into thinking the application is snooping on them.

User impact if declined:
- See above

Fix Landed on Version:
- 62

Risk to taking this patch (and alternatives if risky):
- Very low

String or UUID changes made by this patch:
- None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.


Approval Request Comment
[Feature/Bug causing the regression]:
- bug 1299515 (60)

[User impact if declined]:
- See above

[Is this code covered by automated tests?]:
- Yes

[Has the fix been verified in Nightly?]:
- I just tested and it looks good

[Needs manual test from QE? If yes, steps to reproduce]:
- No, covered by automation

[List of other uplifts needed for the feature/fix]:
- None

[Is the change risky?]:
- No

[Why is the change risky/not risky?]:
- Trivial fix

[String changes made/needed]:
- None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981905 [details]
Bug 1463581 - Signal enabled state changes also on UnregisterSink.

Makes sure that the hardware indicators reflect the actual state of things. Thanks for including an automated test! Approved for 61.0b12 and ESR 60.1.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/82d7df6894a7
https://hg.mozilla.org/releases/mozilla-beta/rev/32ac9938afd2

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/8953e757ddaa
https://hg.mozilla.org/releases/mozilla-esr60/rev/50cb104b7bd9

Comment 18

[Hani Yacoub, Desktop QA]

Build ID: 20180605220158
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

Verified as fixed on Firefox Nightly 62.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 18.04 x64.

Comment 19

[Hani Yacoub, Desktop QA]

Build ID: 20180607135512
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Verified as fixed on Firefox 61.0b12 and on on Firefox ESR on a build from "https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&fromchange=efbcc205a0d3562691bf62bb0cb55bb7e891e8ff&selectedJob=182024831" on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 18.04 x64.