1468451 - Crash near null [@ mozilla::PeerConnectionMedia::AddTransceiver]

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes a crash near null in m-c 20180612-49efc43b1438.

==31727==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000e0 (pc 0x6c24c4d00d0c bp 0x72470593dc90 sp 0x72470593db20 T0)
==31727==The signal is caused by a READ memory access.
==31727==Hint: address points to the zero page.
    #0 0x6c24c4d00d0b in operator! /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h
    #1 0x6c24c4d00d0b in mozilla::PeerConnectionMedia::AddTransceiver(mozilla::JsepTransceiver*, mozilla::dom::MediaStreamTrack&, mozilla::dom::MediaStreamTrack*, RefPtr<mozilla::TransceiverImpl>*) /builds/worker/workspace/build/src
/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1150
    #2 0x6c24c4d01956 in mozilla::PeerConnectionImpl::CreateTransceiverImpl(nsTSubstring<char16_t> const&, mozilla::dom::MediaStreamTrack*, mozilla::ErrorResult&) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peercon
nection/PeerConnectionImpl.cpp:1205:17
    #3 0x6c24c74aa91d in mozilla::dom::PeerConnectionImplBinding::createTransceiverImpl(JSContext*, JS::Handle<JSObject*>, mozilla::PeerConnectionImpl*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/
bindings/PeerConnectionImplBinding.cpp:353:62
    #4 0x6c24c959f349 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/
build/src/dom/bindings/BindingUtils.cpp:3285:13
    #5 0x6c24d0d889ab in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:274:15

Comment 1

[Nils Ohlmeier [:drno]]

Byron, can you please take a look at this?

Comment 2

[Byron Campen [:bwc]]

Looks like we aren't checking whether the PC is closed inside addTransceiver, and we're ending up in the weeds. Surprisingly, the spec doesn't require this check, but it probably should. jib?

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b39eff88c4bfbf7b5c9a608e6ba4f98122d530fd

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1468451: Add a closed check to addTransceiver. r+jib

Review commit: https://reviewboard.mozilla.org/r/251002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251002/

Comment 5

[Byron Campen [:bwc]]

I've verified that the patch fixes the crash.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8985381 [details]
Bug 1468451: Add a closed check to addTransceiver. r+jib

https://reviewboard.mozilla.org/r/251002/#review257446

::: dom/media/PeerConnection.js:1210
(Diff revision 1)
>    _addTransceiverNoEvents(sendTrackOrKind, init) {
> +    this._checkClosed();

Right fix, but one nit.

This helper is called from 3 places.

I think it'd be easier to reason about the code if we only called _checkClosed() in the addTransceiver() function rather than inside this internal helper function.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Filed issue https://github.com/w3c/webrtc-pc/issues/1891 to fix the spec.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8985381 [details]
Bug 1468451: Add a closed check to addTransceiver. r+jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251002/diff/1-2/

Comment 9

[Nils Ohlmeier [:drno]]

Byron is this ready to land?

Comment 10

[Byron Campen [:bwc]]

I believe so, but I'll have to put it in phabricator. Maybe autoland failed to land it or something.

Comment 11

[Byron Campen [:bwc]]

Attachment: Bug 1468451: Add a closed check to addTransceiver. r+jib

MozReview-Commit-ID: DMmorKFz5EL

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9010324 [details]
Bug 1468451: Add a closed check to addTransceiver. r+jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83cf62d8cd25
Add a closed check to addTransceiver. r+jib r=jib

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/83cf62d8cd25

Comment 15

[Pascal Chevrel:pascalc]

Byron, do you think that this can be safely uplifted to 63 beta? Thanks

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 9010324 [details]
Bug 1468451: Add a closed check to addTransceiver. r+jib

Approval Request Comment
[Feature/Bug causing the regression]:

   Bug 1290948

[User impact if declined]:

   Safe tab crashes can be caused by JS pretty reliably.

[Is this code covered by automated tests?]:

   No.

[Has the fix been verified in Nightly?]:

   Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

   No.

[List of other uplifts needed for the feature/fix]:

   None.

[Is the change risky?]:

   No.

[Why is the change risky/not risky?]:

   We're just adding a small check that will cause addTransceiver to be a no-op when the PC is closed.

[String changes made/needed]:

   None.

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9010324 [details]
Bug 1468451: Add a closed check to addTransceiver. r+jib

Low risk patch for stability, uplift approved for 63 beta 9. Thanks!

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fed75bc2166e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Should we land this testcase as a crashtest?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5711d0125d
Add crashtest. r=me

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5d5711d0125d