1192403 - ice_unittest reports error 13 for TCP sockets

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Nils Ohlmeier [:drno]]

(lldb) bt
* thread #2: tid = 0xf3811e, 0x0000000100151b66 ice_unittest`nr_socket_multi_tcp_recvfrom(obj=0x000000010182f57c, buf=0x0000000100df78f0, maxlen=4096, len=0x0000000100df7848, flags=0, from=0x0000000100df7878) + 454 at nr_socket_multi_tcp.c:502, name = 'Socket Thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000100151b66 ice_unittest`nr_socket_multi_tcp_recvfrom(obj=0x000000010182f57c, buf=0x0000000100df78f0, maxlen=4096, len=0x0000000100df7848, flags=0, from=0x0000000100df7878) + 454 at nr_socket_multi_tcp.c:502
  * frame #1: 0x0000000100137ab9 ice_unittest`nr_socket_recvfrom(sock=0x0000000101898d8c, buf=0x0000000100df78f0, maxlen=4096, len=0x0000000100df7848, flags=0, addr=0x0000000100df7878) + 217 at nr_socket.c:98
    frame #2: 0x0000000100002e75 ice_unittest`mozilla::TestStunServer::readable_cb(s=0x000000010185f120, how=0, cb_arg=0x0000000101862350) + 117 at stunserver.cpp:412
    frame #3: 0x00000001000a3272 ice_unittest`mozilla::NrSocketBase::fire_callback(this=0x000000010185f120, how=0) + 178 at nr_socket_prsock.cpp:270
    frame #4: 0x00000001000a37c3 ice_unittest`mozilla::NrSocket::OnSocketReady(this=0x000000010185f120, fd=0x000000010189e9a0, outflags=1) + 83 at nr_socket_prsock.cpp:280
    frame #5: 0x00000001000a384a ice_unittest`non-virtual thunk to mozilla::NrSocket::OnSocketReady(this=0x000000010185f1d0, fd=0x000000010189e9a0, outflags=1) + 58 at nr_socket_prsock.cpp:283
    frame #6: 0x00000001000ff6ef ice_unittest`nsSocketTransportService::DoPollIteration(this=0x0000000101830260, wait=true, pollDuration=0x0000000100df8b50) + 1327 at nsSocketTransportService2.cpp:1086
    frame #7: 0x00000001000febff ice_unittest`nsSocketTransportService::Run(this=0x0000000101830260) + 495 at nsSocketTransportService2.cpp:862
    frame #8: 0x00000001000ffa9c ice_unittest`non-virtual thunk to nsSocketTransportService::Run(this=0x0000000101830278) + 28 at nsSocketTransportService2.cpp:979
    frame #9: 0x00000001001c34e4 ice_unittest`nsThread::ProcessNextEvent(this=0x000000010185e130, aMayWait=true, aResult=0x0000000100df8dbe) + 1876 at nsThread.cpp:867
    frame #10: 0x00000001001976a7 ice_unittest`NS_ProcessNextEvent(aThread=0x000000010185e130, aMayWait=true) + 151 at nsThreadUtils.cpp:277
    frame #11: 0x00000001001c1c3d ice_unittest`nsThread::ThreadFunc(aArg=0x000000010185e130) + 285 at nsThread.cpp:352
    frame #12: 0x0000000100afd251 libnss3.dylib`_pt_root(arg=0x00000001018371a0) + 449 at ptthread.c:212
    frame #13: 0x00007fff81a56268 libsystem_pthread.dylib`_pthread_body + 131
    frame #14: 0x00007fff81a561e5 libsystem_pthread.dylib`_pthread_start + 176
    frame #15: 0x00007fff81a5441d libsystem_pthread.dylib`thread_start + 13

1) I think it is not safe to print the address in line 502 as the struct in an error case has not been initialized
2) In this case the TCP STUN server got the callback, because the client has closed the TCP connection
  2.1) Check if we can identify connection closure early on in the callback and prevent trying to read from a dead connection.
  2.2) Lets see if we can shut down the STUN server earlier (optional if 2.1 get implemented)?

Comment 1

[Nils Ohlmeier [:drno]]

Re-using the TestStunServer::readable_cb for TCP https://dxr.mozilla.org/mozilla-central/source/media/mtransport/test/stunserver.cpp?from=stunserver.cpp&offset=0#572 does not strike me as a brilliant idea.

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1192403: improve ICE TCP error message. r+mjf

Bug 1192403: improve ICE TCP error handling

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8646031 [details]
MozReview Request: Bug 1192403: improve ICE TCP error message. r+mjf

Bug 1192403: improve ICE TCP error handling

Comment 4

[Maire Reavy [:mreavy]]

Nils - where does this stand?  Thanks

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8646031 [details]
MozReview Request: Bug 1192403: improve ICE TCP error message. r+mjf

Bug 1192403: improve ICE TCP error message. r?mjf

Comment 6

[Nils Ohlmeier [:drno]]

I think all of the concerns mentioned in the initial report have been fixed through bug 1206465, except the one error log message in the patch for this bug here.
So lets just land this one line patch and move on.

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8646031 [details]
MozReview Request: Bug 1192403: improve ICE TCP error message. r+mjf

https://reviewboard.mozilla.org/r/15585/#review21363

After the wrap, it should be good.

::: media/mtransport/third_party/nICEr/src/net/nr_socket_multi_tcp.c:489
(Diff revision 3)
> -
> +        r_log(LOG_ICE,LOG_DEBUG,"%s:%d function %s(to:%s) failed with error %d",__FILE__,__LINE__,__FUNCTION__,tcpsock->remote_addr.as_string,r);

I think this should probably be wrapped at 80 col - bwc usually catches me on that too.

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8646031 [details]
MozReview Request: Bug 1192403: improve ICE TCP error message. r+mjf

Bug 1192403: improve ICE TCP error message. r+mjf

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d146bba8beab

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d146bba8beab

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d146bba8beab