Closed
Bug 1259236
Opened 8 years ago
Closed 8 years ago
FF crashes when using PC.addTrack() with constructed media streams
Categories
(Core :: WebRTC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: lancestout, Assigned: jib, NeedInfo)
References
()
Details
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
bwc
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
bwc
:
review+
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/601.5.17 (KHTML, like Gecko) Version/9.1 Safari/601.5.17 Steps to reproduce: Using FF 45 (and nightly) and with adapter.js loaded: 1) Request user media (providing gumStream) 2) Create new peer connection (pc) 3) Create new media stream (generatedStream) 4) Run generatedStream.addTrack(gumStream.getTracks()[0]) 5) Use pc.addTrack(generatedStream.getTracks()[0], generatedStream) 6) Perform signaling to start peer connection with Chrome (tested with stable) Actual results: Once the peer connection is established, Firefox crashes. Crash report: https://crash-stats.mozilla.com/report/index/bbb92b38-1103-4d32-8115-d96972160323 Only happens from FF to Chrome. FF to FF works. Chrome to FF works. Expected results: Expected peer connection to establish and begin flowing the single track. Why create the intermediary media stream? I was originally experimenting with working around FF requiring the stream parameter in addTrack() by creating one when needed. I did wonder if the intermediate stream was getting garbage collected and causing the issue, but even with keeping a reference I got crashes.
Assignee | ||
Comment 1•8 years ago
|
||
Thanks! This reproduces in this local-loop fiddle as well.
Assignee | ||
Updated•8 years ago
|
Severity: normal → critical
Rank: 16
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Assignee | ||
Comment 2•8 years ago
|
||
So domstream->GetOwnedStream() is nullptr in this case, which makes sense, given that this stream is not one of our legacy "all-in-one source streams". PeerConnectionImpl::AddTrack() needs to go through the track to find the source these days, not the stream. As I read the spec, the DOM stream(s) passed in to addTrack now only count when deciding what streams the remote version of the track should be in on the other side. I don't know the jsep code well enough to know if this means we need to carry both kinds of streams in a refactor, or if we can simply hand jsep the "owned" one.
Comment 3•8 years ago
|
||
We've been requiring for a while now that pc.addTrack must be given the track's original stream for it to work. Bug 1208371 will help in fixing this. We shouldn't crash though, is the current safeguard not enough?
Assignee | ||
Comment 4•8 years ago
|
||
I didn't know we required that. What safeguard? Are you suggesting the API should throw here for now?
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 5•8 years ago
|
||
The problem in AddTrack starts here where the passed-in dom stream is used to create MediaPipelineTransmit: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp?rev=181f18c5d1ad&mark=574-574#566 That constructor calls GetOwnedStream on the arg here, which is nullptr: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h?rev=d1c6e59567a1&mark=398-398#381 And it crashes as soon as stream is referenced, here in MediaPipelineTransmit::AttachToTrack: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp?mark=671-671#657
Assignee | ||
Comment 6•8 years ago
|
||
Is it an option to fish out the original stream from the passed-in track and ignored the passed-in stream for now, or does that just lead to harder to diagnose problems later?
Flags: needinfo?(docfaraday)
Comment 7•8 years ago
|
||
I don't think that will leave nasty subtle problems in its wake, but it could have surprising results for content (ids not lining up?). I feel like throwing an exception with a good error message pointing to the bug might be a better approach, given that we're just straight-up crashing right now.
Flags: needinfo?(docfaraday)
Comment 8•8 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #4) > I didn't know we required that. What safeguard? Are you suggesting the API > should throw here for now? Here: https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#2207 Make it OwnsTrack instead of HasTrack, for now.
Flags: needinfo?(pehrsons)
Comment 9•8 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #6) > Is it an option to fish out the original stream from the passed-in track and > ignored the passed-in stream for now, or does that just lead to harder to > diagnose problems later? That would cause the remote side to group tracks unexpectedly?
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42221/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42221/
Attachment #8734411 -
Flags: review?(docfaraday)
Comment 11•8 years ago
|
||
Comment on attachment 8734411 [details] MozReview Request: Bug 1259236 - throw NotSupportedError on pc.addTrack of track in constructed stream. https://reviewboard.mozilla.org/r/42221/#review38783
Attachment #8734411 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42237/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42237/
Attachment #8734429 -
Flags: review?(docfaraday)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jib
Updated•8 years ago
|
Attachment #8734429 -
Flags: review?(docfaraday) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8734429 [details] MozReview Request: Bug 1259236 - test pc.addTrack of track in constructed stream. https://reviewboard.mozilla.org/r/42237/#review38791 ::: dom/media/tests/mochitest/test_peerConnection_addTrack.html:15 (Diff revision 1) > + function mustThrowWith(msg, reason, f) { > + try { > + f(); > + ok(false, msg + " must throw"); > + } catch (e) { > + is(e.name, reason, msg + " must throw: " + e.message); > + } > + }; Let's unify this and the one in test_peerConnection_promiseSendOnly.html and move it to head.js or something.
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8734429 [details] MozReview Request: Bug 1259236 - test pc.addTrack of track in constructed stream. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42237/diff/1-2/
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6af82dc555 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3e9d751d6b
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bd6af82dc555 https://hg.mozilla.org/mozilla-central/rev/8d3e9d751d6b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Summary: FF crashes when using PC.addTrack() with constructed media streams when connecting to Chrome → FF crashes when using PC.addTrack() with constructed media streams
Comment 17•8 years ago
|
||
I just got a report on this in 46. I think we should uplift to beta 47. jib, could you request?
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8734411 [details] MozReview Request: Bug 1259236 - throw NotSupportedError on pc.addTrack of track in constructed stream. Approval Request Comment [Feature/regressing bug #]: bug 1070216 [User impact if declined]: Crashes when calling var copy = new MediaStream(stream.getTracks()); pc.addTrack(copy.getTracks()[0], copy); [Describe test coverage new/current, TreeHerder]: Landed over a month ago with test. [Risks and why]: Very small risk. Basically adds a nullpointer check, and throws NotSupportError to JS instead. [String/UUID change made/needed]: none
Attachment #8734411 -
Flags: approval-mozilla-beta?
Comment 20•8 years ago
|
||
Too late for 46, though we could consider this for a ride-along on a point release. Carryover, as it also affected 45, so people have needed to work around it already; worst case if we uplift to beta will be retaining that requirement through 46.
Hello lancestout, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(lancestout)
Liz fyi Maire's comment 20. You might want to review this for inclusion in a 46 dot release as a potential ride-along.
Flags: needinfo?(lhenry)
Comment on attachment 8734411 [details] MozReview Request: Bug 1259236 - throw NotSupportedError on pc.addTrack of track in constructed stream. Crash fix, Beta47+
Attachment #8734411 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/648ee3ba5ced https://hg.mozilla.org/releases/mozilla-beta/rev/1e4b52390f6d
Comment 27•8 years ago
|
||
I don't know why my FF is v48.0 but I get this bug :( https://gyazo.com/c5a37fc13fbbf89ceba2d9285b3c8142
Comment 28•8 years ago
|
||
I tried with FF Nightly then it's ok =="
Comment 29•8 years ago
|
||
(In reply to HoangNK from comment #27) > I don't know why my FF is v48.0 but I get this bug :( > > https://gyazo.com/c5a37fc13fbbf89ceba2d9285b3c8142 Without this bug you'd see a crash instead of an error. The one you're seeing now and that got fixed in 49 is bug 1271669.
You need to log in
before you can comment on or make changes to this bug.
Description
•