40 bytes, text/x-review-board-request
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
Component: General → WebRTC
Product: Firefox → Core
Worth adding: this crash happens 100% of the time.
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
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");
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.
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 ) 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.  http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_bug825703.html
Created attachment 8692716 [details] MozReview Request: Bug 1227781 - Fix crash with bogus STUN parameters Bug 1227781 - Fix crash with bogus STUN parameters
Attachment #8692716 - Flags: review?(docfaraday)
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/
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 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+
Assignee: nobody → ekr
backlog: --- → webrtc/webaudio+
Priority: -- → P1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
You need to log in before you can comment on or make changes to this bug.