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)

45 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: lancestout, Assigned: jib, NeedInfo)

References

()

Details

Attachments

(2 files)

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.
Thanks! This reproduces in this local-loop fiddle as well.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → critical
Rank: 16
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
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.
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?
I didn't know we required that. What safeguard? Are you suggesting the API should throw here for now?
Flags: needinfo?(pehrsons)
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
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)
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)
(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)
(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?
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: nobody → jib
Attachment #8734429 - Flags: review?(docfaraday) → review+
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.
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/
https://hg.mozilla.org/mozilla-central/rev/bd6af82dc555
https://hg.mozilla.org/mozilla-central/rev/8d3e9d751d6b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
I just got a report on this in 46. I think we should uplift to beta 47. jib, could you request?
Flags: needinfo?(jib)
Ugh, yeah should have done that long ago...
Flags: needinfo?(jib)
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?
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+
I think this can wait till 47.
Flags: needinfo?(lhenry)
See Also: → 1271669
I don't know why my FF is v48.0 but I get this bug :(

https://gyazo.com/c5a37fc13fbbf89ceba2d9285b3c8142
I tried with FF Nightly then it's ok =="
(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.