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

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
critical
Rank:
15
RESOLVED FIXED
11 months ago
4 months ago

People

(Reporter: truber, Assigned: bwc)

Tracking

(Blocks 2 bugs, {crash, testcase})

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

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

Details

(crash signature)

Attachments

(3 attachments)

Posted file 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
Flags: in-testsuite?
Reporter

Updated

11 months ago
Crash Signature: [@ mozilla::PeerConnectionMedia::AddTransceiver] [@ operator! | mozilla::PeerConnectionMedia::AddTransceiver]
Byron, can you please take a look at this?
Assignee: nobody → docfaraday
Rank: 15
Flags: needinfo?(docfaraday)
Priority: -- → P2
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?
Flags: needinfo?(docfaraday) → needinfo?(jib)
Comment hidden (mozreview-request)
I've verified that the patch fixes the crash.

Comment 6

11 months ago
mozreview-review
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.
Attachment #8985381 - Flags: review?(jib) → review+
Filed issue https://github.com/w3c/webrtc-pc/issues/1891 to fix the spec.
Flags: needinfo?(jib)
Comment hidden (mozreview-request)
Byron is this ready to land?
Flags: needinfo?(docfaraday)
I believe so, but I'll have to put it in phabricator. Maybe autoland failed to land it or something.
Flags: needinfo?(docfaraday)
MozReview-Commit-ID: DMmorKFz5EL
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.
Attachment #9010324 - Flags: review+

Comment 13

8 months ago
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83cf62d8cd25
Add a closed check to addTransceiver. r+jib r=jib

Comment 14

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83cf62d8cd25
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Byron, do you think that this can be safely uplifted to 63 beta? Thanks
Flags: needinfo?(docfaraday)
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.
Flags: needinfo?(docfaraday)
Attachment #9010324 - Flags: approval-mozilla-beta?
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!
Attachment #9010324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Should we land this testcase as a crashtest?
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.