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

VERIFIED FIXED in Firefox 22

Status

()

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jsmith, Assigned: roc)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 disabled, firefox22+ fixed, firefox23+ verified)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][qa-automation-blocked][parity-chrome])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 742827 [details]
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.
(Reporter)

Updated

6 years ago
Blocks: 834835
(Reporter)

Updated

6 years ago
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked]
(Reporter)

Updated

6 years ago
Blocks: 834837
(Reporter)

Updated

6 years ago
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked] → [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome]
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.
Assignee: nobody → roc
status-firefox21: --- → disabled
status-firefox22: --- → affected
status-firefox23: --- → affected
tracking-firefox22: ? → +
tracking-firefox23: ? → +
Created attachment 744114 [details] [diff] [review]
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.
Created attachment 745009 [details] [diff] [review]
Part 1: Add DOMMediaStream::OnTracksAvailableCallback
Attachment #745009 - Flags: review?(rjesup)
Created attachment 745010 [details] [diff] [review]
Part 2: Delay delivering getUserMedia stream result until the DOM object has asynchronously acquired the desired tracks
Attachment #744114 - Attachment is obsolete: true
Attachment #745010 - Flags: review?(rjesup)
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.
Attachment #745009 - Flags: review?(rjesup) → review+
Attachment #745010 - Flags: review?(rjesup) → review+
(Reporter)

Updated

6 years ago
Flags: in-testsuite?
(Reporter)

Comment 8

6 years ago
Created attachment 745738 [details] [diff] [review]
Basic gUM Media Stream Track Mochitests
(Reporter)

Comment 9

6 years ago
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.
Attachment #745738 - Flags: review?(roc)
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 :-(
https://hg.mozilla.org/mozilla-central/rev/67d21aabfb75
https://hg.mozilla.org/mozilla-central/rev/531c57d87cf4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox23: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Reporter)

Comment 13

6 years ago
Roc - When you are ready, can you take a look at the mochitests included here?
Keywords: verifyme
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome] → [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome][webrtc-uplift]
(Reporter)

Comment 14

6 years ago
lgtm on 5/9 nightly build
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
Keywords: verifyme
Blocks: 870002
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
Attachment #745009 - Flags: approval-mozilla-aurora?
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.
Attachment #745009 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #745009 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #745738 - Flags: review+
I suspect the test doesn't need separate review from roc
(Reporter)

Comment 18

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

Yeah, only one review is needed.
Attachment #745738 - Flags: review?(roc)
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
(Reporter)

Comment 19

6 years ago
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
Attachment #745738 - Flags: approval-mozilla-beta?
Attachment #745738 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f22b1e69ac
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Attachment #745738 - Flags: approval-mozilla-beta?
Attachment #745738 - Flags: approval-mozilla-beta+
Attachment #745738 - Flags: approval-mozilla-aurora?
Attachment #745738 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 22

6 years ago
checkin-needed to land the tests on aurora and beta
Keywords: checkin-needed
(Reporter)

Updated

6 years ago
No longer blocks: 870002
Depends on: 870002
Keywords: checkin-needed
(Reporter)

Comment 25

6 years ago
Patch auto-verified by landing in comment 24.
status-firefox22: fixed → verified
(Reporter)

Updated

6 years ago
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome][webrtc-uplift] → [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome]
Created attachment 756994 [details] [diff] [review]
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 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
Attachment #756994 - Flags: approval-mozilla-beta?
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!
Attachment #756994 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome] → [WebRTC][blocking-webrtc-][qa-automation-blocked][parity-chrome]
(Reporter)

Comment 30

6 years ago
I think you want disabled in this case.
status-firefox22: wontfix → disabled
https://hg.mozilla.org/releases/mozilla-beta/rev/f53c82908920
Relanded since this is now proven not the be the cause of bug 870002
status-firefox22: disabled → fixed
Blocks: 945334
No longer blocks: 945334
You need to log in before you can comment on or make changes to this bug.