Closed Bug 866514 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- disabled
firefox22 + fixed
firefox23 + verified

People

(Reporter: jsmith, Assigned: roc)

References

Details

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

Attachments

(5 files, 1 obsolete file)

Attached file 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.
Blocks: 834835
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked]
Blocks: 834837
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.
Attached patch leaky fix (obsolete) — Splinter Review
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.
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+
Flags: in-testsuite?
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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]
lgtm on 5/9 nightly build
Status: RESOLVED → VERIFIED
Keywords: verifyme
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+
I suspect the test doesn't need separate review from roc
Comment on attachment 745738 [details] [diff] [review]
Basic gUM Media Stream Track Mochitests

Yeah, only one review is needed.
Attachment #745738 - Flags: review?(roc)
Keywords: checkin-needed
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?
Attachment #745738 - Flags: approval-mozilla-beta?
Attachment #745738 - Flags: approval-mozilla-beta+
Attachment #745738 - Flags: approval-mozilla-aurora?
Attachment #745738 - Flags: approval-mozilla-aurora+
checkin-needed to land the tests on aurora and beta
Keywords: checkin-needed
No longer blocks: 870002
Depends on: 870002
Patch auto-verified by landing in comment 24.
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome][webrtc-uplift] → [WebRTC][blocking-webrtc+][qa-automation-blocked][parity-chrome]
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]
I think you want disabled in this case.
No longer blocks: 945334
You need to log in before you can comment on or make changes to this bug.