Closed Bug 1468451 Opened 5 years ago Closed 5 years ago

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


(Core :: WebRTC, defect, P2)




Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed


(Reporter: truber, Assigned: bwc)


(Blocks 1 open bug)


(Keywords: crash, testcase)

Crash Data


(3 files)

Attached 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
    #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
    #3 0x6c24c74aa91d in mozilla::dom::PeerConnectionImplBinding::createTransceiverImpl(JSContext*, JS::Handle<JSObject*>, mozilla::PeerConnectionImpl*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/
    #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/
    #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?
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)
I've verified that the patch fixes the crash.
Comment on attachment 8985381 [details]
Bug 1468451: Add a closed check to addTransceiver. r+jib

::: 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 to fix the spec.
Flags: needinfo?(jib)
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+
Pushed by
Add a closed check to addTransceiver. r+jib r=jib
Closed: 5 years 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?]:


[Has the fix been verified in Nightly?]:


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


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


[Is the change risky?]:


[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]:

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.