829856 - WebRTC libc++abi.dylib abort [@sipcc::PeerConnectionImpl::ConvertIceConfiguration]

Status: VERIFIED INVALID

(Core :: WebRTC, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

To reproduce load the provided testcase and then click the button.

libc++abi.dylib: terminate called throwing an exception

Program received signal SIGABRT, Aborted.
0x00007fff95368212 in __pthread_kill ()
gdb $ frame 11
#11 0x000000010458a482 in sipcc::PeerConnectionImpl::ConvertIceConfiguration (aSrc=@0x7fff5fbf6070, aDst=0x7fff5fbf5d78, aCx=0x11903fa60) at PeerConnectionImpl.cpp:334
334         url = url.substr(scheme.length()+1);
gdb $ list
329                                            JS_ValueToString(aCx, jsUrl))).get();
330       string scheme = url.substr(0, url.find(':'));
331       transform(scheme.begin(), scheme.end(), scheme.begin(), ::tolower);
332       if (scheme == "stun" || scheme == "stuns" || scheme == "turn")
333       {
334         url = url.substr(scheme.length()+1);
335         int port = (scheme == "stuns")? 5349 : 3478;
336         int pos;
337         if ((pos = url.find('@')) != url.npos) {
338           url = url.substr(pos+1);
gdb $ p scheme
$1 = {
  _M_dataplus = {
    <std::allocator<char>> = {
      <__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, 
    members of std::basic_string<char>::_Alloc_hider: 
    _M_p = 0x14cb07a28 "turn"
  }, 
  static npos = 18446744073709551615
}
gdb $ bt
#0  0x00007fff95368212 in __pthread_kill ()
#1  0x00007fff88c5baf4 in pthread_kill ()
#2  0x00007fff88c9fdce in abort ()
#3  0x00007fff8e0a8a17 in abort_message ()
#4  0x00007fff8e0a63c6 in default_terminate ()
#5  0x00007fff8d4bd887 in _objc_terminate ()
#6  0x00007fff8e0a63f5 in safe_handler_caller ()
#7  0x00007fff8e0a6450 in std::terminate ()
#8  0x00007fff8e0a75b7 in __cxa_throw ()
#9  0x00007fff932cd41b in std::__throw_out_of_range ()
#10 0x00007fff932f6fd4 in std::string::substr ()
#11 0x000000010458a482 in sipcc::PeerConnectionImpl::ConvertIceConfiguration (aSrc=@0x7fff5fbf6070, aDst=0x7fff5fbf5d78, aCx=0x11903fa60) at PeerConnectionImpl.cpp:334
#12 0x000000010458a915 in sipcc::PeerConnectionImpl::Initialize (this=0x14cb06b00, aObserver=0x14cb07050, aWindow=0x122ca9c20, aIceConfiguration=@0x7fff5fbf6070, aThread=0x100121c50, aCx=0x11903fa60) at PeerConnectionImpl.cpp:401
#13 0x0000000103904d19 in NS_InvokeByIndex_P (that=0x14cb06b00, methodIndex=3, paramCount=5, params=0x7fff5fbf6040) at /Users/cdiehl/Code/Mozilla/mc-debug/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
#14 0x0000000102ae26d4 in CallMethodHelper::Invoke (this=0x7fff5fbf6000) at /Users/cdiehl/Code/Mozilla/mc-debug/js/xpconnect/src/XPCWrappedNative.cpp:3085
#15 0x0000000102ae0a2c in CallMethodHelper::Call (this=0x7fff5fbf6000) at /Users/cdiehl/Code/Mozilla/mc-debug/js/xpconnect/src/XPCWrappedNative.cpp:2419



Tested with m-c changeset: 118520:44dcffe8792b and with the following additional patches:

https://bugzilla.mozilla.org/attachment.cgi?id=698334
https://bugzilla.mozilla.org/attachment.cgi?id=699413

Comment 1

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

Crash was in uncommitted patch in Bug 825703. Patch fixed.

Comment 2

[Jason Smith [:jsmith]]

Not getting a crash on a 1/25 build. Verified. Strangely getting an error fired back with no contents....that's umm...strange. I'll file a bug.

Comment 3

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

> Not getting a crash on a 1/25 build. Verified. Strangely getting an error fired
> back with no contents....that's umm...strange. I'll file a bug.

The error you get is:

  Error: RTCPeerConnection constructor passed invalid RTCConfiguration - iceServers
     [] property not present

Your testcase.html uses:
    var v1 = new window.mozRTCPeerConnection([{url:'turn'}]);

while the correct syntax is (now):
    var v1 = new window.mozRTCPeerConnection({ iceServers: [{url:'turn:0.0.0.0'}] });

It worked before because I got the syntax wrong at first in bug 825703. I fixed it in bug 834463 which landed yesterday.

The fact that errors from PeerConnection.js show as blank in Web Console is a long-standing bug I noticed as well (the easiest way to reproduce is to disable peerconnection in about:config and do a call). We should definitely file a bug for that.

I wrapped your call in try{ ...} catch(e) { console.log(e);} to see the error.

Comment 4

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

I see you filed bug 834933. I'll comment there as well.

Comment 5

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 6

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

Comment on attachment 731751 [details] [diff] [review]
Crashtest v1

Review of attachment 731751 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/crashtests/829856.html
@@ +7,5 @@
> +  <meta charset="utf-8">
> +  <title>Bug 829856</title>
> +  <script type="application/javascript">
> +    function start() {
> +        var v0 = new window.mozRTCPeerConnection();

FWIW the above line isn't necessary, as the plain parsing error was on the line below (but see overall comment below).

@@ +8,5 @@
> +  <title>Bug 829856</title>
> +  <script type="application/javascript">
> +    function start() {
> +        var v0 = new window.mozRTCPeerConnection();
> +        var v1 = new window.mozRTCPeerConnection([{url:'turn'}]);

The code looks fine, but overall, I don't think we need this crashtest for hand-rolled URL parsing code we never ended up using and never landed (the hand-rolled parsing code you see in the comment crashed after comparing the word 'turn' and assuming there was a character after it). After review, we went a different direction, reusing large parts of our existing nsURI parsing code instead, which is presumably robust and not for us to test. That's what we landed.

Again, I recommend closing this bug as invalid, since it's unusual to file reports on uncommitted code downloaded from a work-in-progress patch in Bugzilla. Marking r- until I hear a reason otherwise.

Comment 7

[Jason Smith [:jsmith]]

Okay, sounds good. I'll move this to invalid then.