866514 - getAudioTracks and getVideoTracks do not return the correct amount of tracks not too long after the media stream is given in JS

Status: VERIFIED FIXED

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

Description

[Jason Smith [:jsmith]]

Attachment: gUM Test Case

STR:

1. Load the attached test case
2. Inspect the console log

Expected:

Upon immediately getting access to the stream from the success callback in gUM, I'd expect that calling getAudioTracks or getVideoTracks should return the correct amount of tracks.

Example - video only request should get 1 video track, 0 audio tracks.

Actual:

If you call getAudioTracks or getVideoTracks early in the timeframe when the stream is returned to JS, you will get the incorrect amount of tracks returned.

I've seen this issue with the simple gUM test case attached and upon analyzing the various stream properties in PeerConnection while a handshake takes place.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is a bit of a problem because all stream setup is asynchronous.

I think we might need to delay returning the MediaStream from the gUM callback until at least one MediaStreamTrack has appeared in its DOMMediaStream.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: leaky fix

This works.

However, it does lead to the possibility of leaks due to a cycle involving the GetUserMediaRunnable, the TracksAvailableCallback, and the DOMMediaStream. The cycle will be removed as long as the required tracks eventually arrive, but if they don't, we'll leak :-(.

The obvious way to fix that is to make GetUserMediaRunnable and TracksAvailableCallback participate in cycle collection. But currently no WebRTC code participates in cycle collection. I think it probably should, but someone has to make the call.

This is hard to resolve in other ways. For one thing, I think the success and error getUserMediaCallbacks, since they wrap script objects, could also be involved in unexpected leaks though JS. Basically through the magic of JS expandos, any DOM/WebIDL object can have a path of strong references to any other DOM/WebIDL object.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Add DOMMediaStream::OnTracksAvailableCallback

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Delay delivering getUserMedia stream result until the DOM object has asynchronously acquired the desired tracks

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to manually keep the DOMMediaStream alive until the DOM tracks have been added, or it will be GCed and destroyed under us. But then there's a possibility of leaks if the DOM tracks are never added. So I jumped through some hoops to try to eliminate all possibility of leaks, by only waiting for the DOM tracks to be added if we can be really sure they will be added.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4609a4efeb39
https://hg.mozilla.org/integration/mozilla-inbound/rev/f110d0270f8e

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5972785237b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a081cfffeed0

for thread safety assertions on Windows XP and 7:

https://tbpl.mozilla.org/php/getParsedLog.php?id=22605157&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22604913&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22605215&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22604946&tree=Mozilla-Inbound

and test failures on OSX:

https://tbpl.mozilla.org/php/getParsedLog.php?id=22605176&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22606107&tree=Mozilla-Inbound

Comment 8

[Jason Smith [:jsmith]]

Attachment: Basic gUM Media Stream Track Mochitests

Comment 9

[Jason Smith [:jsmith]]

Comment on attachment 745738 [details] [diff] [review]
Basic gUM Media Stream Track Mochitests

Here's some basic gUM mochitests to cover your patches up with some automation for media stream tracks.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

All the failures in comment #7 are the result of these patches.

The thread-safety assertions should be easy to fix.

The test failure is because a new stream returned by getUserMedia no longer has a currentTime of 0. E.g. if the video track is added after the audio track, we may have "played" some stream content while it was in the audio-only state. One way to fix this is to record the currentTime when all tracks were added and we returned the stream from getUserMedia, and subtract that from the currentTime we report via the DOM API. I'll try that.

Sorry this is taking so long to land :-(

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67d21aabfb75

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/67d21aabfb75
https://hg.mozilla.org/mozilla-central/rev/531c57d87cf4

Comment 13

[Jason Smith [:jsmith]]

Roc - When you are ready, can you take a look at the mochitests included here?

Comment 14

[Jason Smith [:jsmith]]

lgtm on 5/9 nightly build

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 745009 [details] [diff] [review]
Part 1: Add DOMMediaStream::OnTracksAvailableCallback

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 834835 (already uplifted)

User impact if declined: Crashes if GC runs after certain operations; or requiring backout of an important compatibility feature (bug 834835)

Testing completed (on m-c, etc.): On MC.  One regression (bug 870002) from either this bug, bug 8663224 or bug 868406 (all of which need uplift except bug 868406).  The regression is windows-only, and refuses to produce a minidump; we're planning on taking a test machine offline to run the tests in a way that we can grab a dump.  The regression only affects use of this new feature, and it has not been seen yet except on the test servers.

Risk to taking this patch (and alternatives if risky): moderate - the risk is involved in resolving bug 870002 or deciding/finding-out that it's very hard to hit in practice (as opposed to tests).  We can also back out this bug 866514 if it's found to be the cause, or back out all of these and bug 834835, though that would be a painful decision as it would push developers to use "back-door"/non-standard ways to figure out if a stream has video, or they'd write code that works in Chrome but not in Firefox.

String or IDL/UUID changes made by this patch: none

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 745009 [details] [diff] [review]
Part 1: Add DOMMediaStream::OnTracksAvailableCallback

This is going to be heading to Beta now (if approved) since Merge Day has completed.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

I suspect the test doesn't need separate review from roc

Comment 18

[Jason Smith [:jsmith]]

Comment on attachment 745738 [details] [diff] [review]
Basic gUM Media Stream Track Mochitests

Yeah, only one review is needed.

Comment 19

[Jason Smith [:jsmith]]

Comment on attachment 745738 [details] [diff] [review]
Basic gUM Media Stream Track Mochitests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: No automated tests for this patch.
Testing completed (on m-c, etc.): Tests pass locally.
Risk to taking this patch (and alternatives if risky): None - this is a test only patch.
String or IDL/UUID changes made by this patch: None

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f22b1e69ac

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/61d42001197a
https://hg.mozilla.org/releases/mozilla-beta/rev/30b3a3038c02

We'll see if bug 870002 rears its ugly head on beta now.

Comment 22

[Jason Smith [:jsmith]]

checkin-needed to land the tests on aurora and beta

Comment 23

[Ed Morley [:emorley]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/154a0b681893
https://hg.mozilla.org/releases/mozilla-beta/rev/8f3fb4aa0107

Comment 25

[Jason Smith [:jsmith]]

Patch auto-verified by landing in comment 24.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Attachment: backout on beta due to bug 870002

Beta backout patch since we won't be able to solve this for Beta 4 barring a miracle.  Try at  https://tbpl.mozilla.org/?tree=Try&rev=a652c78969d2

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 756994 [details] [diff] [review]
backout on beta due to bug 870002

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug

User impact if declined: intermittent mochitest crashes, but getUserMedia tracks will appear in the stream before the success callback.  If this backout lands, applications which depend on the tracks being available may be negatively affected until we can fix it.

Testing completed (on m-c, etc.): Try submitted for backout.  We didn't see failures on Beta until this landed.

Risk to taking this patch (and alternatives if risky): minimal as we ran without it on beta for a while.  I'm pinging Kairo and the stability people to see if we believe we may be hitting bug 870002 in the field and not getting or connecting the reports to this bug (given the lack of stack traceback, this seems possible).

String or IDL/UUID changes made by this patch: none

Comment 28

[Alex Keybl [:akeybl]]

Comment on attachment 756994 [details] [diff] [review]
backout on beta due to bug 870002

Thanks for ensuring this backout gets in for the final 3 betas!

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d9444230262

Comment 30

[Jason Smith [:jsmith]]

I think you want disabled in this case.

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f53c82908920
Relanded since this is now proven not the be the cause of bug 870002