1227781 - Crash creating RTCPeerConnection

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Eric Shepherd [:sheppy]]

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.

Comment 1

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

Crash is in new code added in 44 http://hg.mozilla.org/mozilla-central/rev/0d841bed4209

Comment 2

[Eric Shepherd [:sheppy]]

Worth adding: this crash happens 100% of the time.

Comment 3

[Eric Shepherd [:sheppy]]

A couple other crash log links with way less sucky stackst:

https://crash-stats.mozilla.com/report/index/14f65bfa-995d-444c-822c-c7cb22151124
https://crash-stats.mozilla.com/report/index/e6db389e-c737-4d78-82dd-748112151124

Comment 4

[Eric Shepherd [:sheppy]]

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");

Comment 5

[Eric Shepherd [:sheppy]]

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.

Comment 6

[Eric Rescorla (:ekr)]

OK, the underlying problem is that RecordEndOfTelemetry assumes that mJsepSession is nonzero and the PC ctor is failing before it's constructed.

Comment 7

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

Here's a fiddle with Eric's input that causes the constructor to fail: https://jsfiddle.net/6ny7vbab/

Comment 8

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

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

Comment 9

[Eric Rescorla (:ekr)]

Attachment: MozReview Request: Bug 1227781 - Fix crash with bogus STUN parameters

Bug 1227781 - Fix crash with bogus STUN parameters

Comment 10

[Eric Rescorla (:ekr)]

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/

Comment 11

[Eric Rescorla (:ekr)]

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

[Byron Campen [:bwc]]

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.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20a38236eb39

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20a38236eb39

Comment 15

[Byron Campen [:bwc]]

Attachment: 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.