Crash creating RTCPeerConnection

RESOLVED FIXED in Firefox 45

Status

()

P1
critical
Rank:
15
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sheppy, Assigned: ekr)

Tracking

({crash})

unspecified
mozilla45
Unspecified
Mac OS X
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-0590142f-92d8-4348-8333-2eccf2151125.
=============================================================

My WebRTC sample is crashing while attempting to open an RTCPeerConnection. Calling out from Firefox crashes with NS_UNEXPECTED in adapter.js line 78. Calling out from Chrome works.

Calling into Firefox (from Chrome) crashes at the same place. Calling into Chrome works.

Chrome <--> Chrome calls work great.
Crash is in new code added in 44 http://hg.mozilla.org/mozilla-central/rev/0d841bed4209
Blocks: 1207824
Component: General → WebRTC
Product: Firefox → Core
(Reporter)

Comment 2

3 years ago
Worth adding: this crash happens 100% of the time.
(Reporter)

Comment 4

3 years ago
If I back up to Firefox 41, the crash does not occur. However, I still get NS_UNEXPECTED in the web console.

Browser Console shows an NS_ERROR_FAILURE on  PeerConnection.js:389:0.

That code is:

    // Add a reference to the PeerConnection to global list (before init).
    _globalPCList.addPC(this);

    this._impl.initialize(this._observer, this._win, rtcConfig,   <<<--- this line here
                          Services.tm.currentThread);
    this._initIdp();
    _globalPCList.notifyLifecycleObservers(this, "initialized");
(Reporter)

Comment 5

3 years ago
The problem appears to be that I followed a guide that showed including the username in the TURN server URL: "webrtc@<ipaddress>". The sample also included the username field in the options, so I assumed it was cool to do both. Guess not.

Removing the username from the URL fixes both the NS_UNEXPECTED and the crash on both Firefox 41 and 44.
(Assignee)

Comment 6

3 years ago
OK, the underlying problem is that RecordEndOfTelemetry assumes that mJsepSession is nonzero and the PC ctor is failing before it's constructed.
Here's a fiddle with Eric's input that causes the constructor to fail: https://jsfiddle.net/6ny7vbab/
Looks like we have no test coverage for a failing PeerConnection c++ constructor.

Most bad input is caught (and tested [1]) in PeerConnection.js, so almost because of this we have no test coverage for late errors in the c++ constructor code. We should probably consider leaving a rare known bad input unchecked in JS, so we can write a test using it to, and catch future regressions like this.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_bug825703.html
(Assignee)

Comment 9

3 years ago
Created attachment 8692716 [details]
MozReview Request: Bug 1227781 - Fix crash with bogus STUN parameters

Bug 1227781 - Fix crash with bogus STUN parameters
(Assignee)

Updated

3 years ago
Attachment #8692716 - Flags: review?(docfaraday)
(Assignee)

Comment 10

3 years ago
Comment on attachment 8692716 [details]
MozReview Request: Bug 1227781 - Fix crash with bogus STUN parameters

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26315/diff/1-2/
(Assignee)

Comment 11

3 years ago
Byron,

There are actually two defects here:

1. Failure to initialize mThread
2. Trying to send telemetry even when mJsepSession is NULL

The first is an assert, which is probably why the original report
doesn't show it. This patch includes a test as well as a fix for both
defects. I'm not that excited about conditionally compiling the second
mThread assignment, but because either Initialize() function can be called,
it was that or double assign.

Comment 12

3 years ago
Comment on attachment 8692716 [details]
MozReview Request: Bug 1227781 - Fix crash with bogus STUN parameters

https://reviewboard.mozilla.org/r/26315/#review23921

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:833
(Diff revision 1)
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aThread);
> +  mThread = do_QueryInterface(aThread);

Should we be doing this stuff in the c'tor instead? We aren't doing any error handling with it.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2501
(Diff revision 1)
> +  if (!mJsepSession)
> +    return;

Braces here.
Attachment #8692716 - Flags: review?(docfaraday) → review+

Updated

3 years ago
Assignee: nobody → ekr
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20a38236eb39
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

3 years ago
Depends on: 1231196

Comment 15

3 years ago
Created attachment 8696796 [details]
MozReview Request: Bug 1227781: Fix init of PeerConnectionImpl::mThread for unit-tests and similar.

Bug 1227781: Fix init of PeerConnectionImpl::mThread for unit-tests and similar.

Updated

3 years ago
Attachment #8696796 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.