856433 - WebRTC crash [@CheckSTSThread]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

This crash occured while fuzzing the SDP.

Before the crash we see:

ICE(PC:): Error parsing attribute: candidate:0 2 UDP 2113601790 192.168.178.20 50769 typ 
Assertion failure: on, at /Users/cdiehl/dev/repos/mozilla/mozilla-webvtt/obj-ff64-asan-opt/media/mtransport/build/nr_timer.cpp:120
ASAN:SIGSEGV

Tested with m-i changeset: 126761:c99f1fd792db and the applied patch of bug 786235.

Comment 1

[Christoph Diehl [:posidron]]

Attachment: NSPR_LOG

The parsing error seems not to be related to the crash. The used SDP which is printed in the NSPR_LOG does not trigger this crash.

Comment 2

[Eric Rescorla (:ekr)]

jan-ivar,

can you figure out why this is not being dispatched?

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Fix thread crash

* * *
Bug 856433. Fix thread crash--fix

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 731987 [details] [diff] [review]
Fix thread crash

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

I wasn't able to reproduce exactly this issue, but I believe
that the general problem is that it's possible to have
a PCMedia with bogus thread pointers (hence no dispatches).
This patch fixes that. With just the unit test changes,
we get a crash, though at a different site.

Note: this patch will probably not successfully apply without
the TURN patches.

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 731987 [details] [diff] [review]
Fix thread crash

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

lgtm

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +528,5 @@
>  
>  class SignalingAgent {
>   public:
> +  SignalingAgent() : pc(nullptr) {
> +    cfg_.addStunServer("23.21.150.121", 3478);

I know this was already in here, but it wouldn't hurt to say here where this server is.

@@ +597,5 @@
>    void Close()
>    {
> +    if (pc) {
> +      cout << "Close" << endl;
> +      

trailing whitespace.

Comment 6

[Maire Reavy [:mreavy]]

cdiehl, can you retest with ekr's patch?

Comment 7

[Eric Rescorla (:ekr)]

The patch in c4 is actually a separate defect. The problem here is that
vcmSetIceMediaParams_m and vcmSetIceSessionParams_m call nICEr on the
main thread and in fact need to dispatch (asynchronously) to the 
signaling thread.

JIB: can you produce a test which replicates this and supply a fix?

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 731987 [details] [diff] [review]
Fix thread crash

This patch is not for this defect but for bug 856848

Comment 9

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

Cliff notes from IRC: The bug here seems to be that vcmSetIceMediaParams is dispatching to the main thread rather than the STS thread:

From http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp#883 :

  short vcmSetIceMediaParams(const char *peerconnection,
                             int level,
                             char *ufrag,
                             char *pwd,
                             char **candidates,
                             int candidate_ct)
  {
    short ret;

    mozilla::SyncRunnable::DispatchToThread(VcmSIPCCBinding::getMainThread(),
        WrapRunnableNMRet(&vcmSetIceMediaParams_m,

We're only seeing the CheckSTS() assert now because we finally pushed a check in nr_timer. 

The assert was provoked by a bogus candidate as seen in the attached log:

  "candidate:0 2 UDP 2113601790 192.168.178.20 50769 typ"

It is missing " host" or something after "typ" on the end, which is why parsing fails and gets into its failure path that involves timers and the CheckSTS() assert.

Ekr says the call to ParseAttributes needs to be asynchronously dispatched onto the STS thread. This hasn't caused a problem so far because ICE is in an idle state at this point.

Comment 10

[Eric Rescorla (:ekr)]

jib: you can't just dispatch to the STS thread because then you won't have a PCImpl handle.

Rather vcmSetIceParams_m needs to dispatch to the STS thread.

Comment 11

[Jason Smith [:jsmith]]

Targeted to ship for Fx22, tracking to get on rel mgmt radar.

Comment 12

[Eric Rescorla (:ekr)]

Attachment: Allow cancellation of nonexistent nrappkit timers off the STS thread

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 738101 [details] [diff] [review]
Allow cancellation of nonexistent nrappkit timers off the STS thread

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

cdiehl: can you verify that this no longer crashes? you will need the patch for bug 854085 that I just landed on m-i.

Comment 14

[Eric Rescorla (:ekr)]

Comment on attachment 738101 [details] [diff] [review]
Allow cancellation of nonexistent nrappkit timers off the STS thread

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

cdiehl: can you verify that this no longer crashes? you will need the patch for bug 854085 that I just landed on m-i.

Comment 15

[Christoph Diehl [:posidron]]

I saw this only one time but will lookout for it.

Comment 16

[Adam Roach [:abr]]

Comment on attachment 738101 [details] [diff] [review]
Allow cancellation of nonexistent nrappkit timers off the STS thread

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

Looks good to me.

Comment 17

[Eric Rescorla (:ekr)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec3b8034d1e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ec3b8034d1e

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 738101 [details] [diff] [review]
Allow cancellation of nonexistent nrappkit timers off the STS thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Occasional crashes

Testing completed (on m-c, etc.): on m-c, tested with c++ unit test in local CI

Risk to taking this patch (and alternatives if risky): No risk; checks for a null pointer before checking the thread.

String or IDL/UUID changes made by this patch: none

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ae9b2f28dae2