WebRTC crash [@CheckSTSThread]

RESOLVED FIXED in Firefox 22

Status

()

defect
P2
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: posidron, Assigned: ekr)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla23
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox22+ fixed, firefox23 fixed)

Details

(Whiteboard: [webrtc][blocking-webrtc+][qa-])

Attachments

(3 attachments, 1 obsolete attachment)

Posted file 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.
Posted file 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.
jan-ivar,

can you figure out why this is not being dispatched?
Assignee: nobody → jib
Posted patch Fix thread crash (obsolete) — Splinter Review
* * *
Bug 856433. Fix thread crash--fix
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.
Attachment #731987 - Flags: review?(ethanhugg)
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.
Attachment #731987 - Flags: review?(ethanhugg) → review+
Assignee: jib → ekr
Whiteboard: [webrtc][blocking-webrtc+]
cdiehl, can you retest with ekr's patch?
Flags: needinfo?(cdiehl)
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 on attachment 731987 [details] [diff] [review]
Fix thread crash

This patch is not for this defect but for bug 856848
Attachment #731987 - Attachment is obsolete: true
Assignee: ekr → jib
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.
Flags: needinfo?(cdiehl)
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.
Assignee: jib → ekr
Priority: -- → P2
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][webrtc-uplift]
Targeted to ship for Fx22, tracking to get on rel mgmt radar.
Attachment #738101 - Flags: review?(adam)
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.
Attachment #738101 - Flags: feedback?(cdiehl)
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.
Attachment #738101 - Flags: feedback?(cdiehl)
I saw this only one time but will lookout for it.
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.
Attachment #738101 - Flags: review?(adam) → review+
https://hg.mozilla.org/mozilla-central/rev/3ec3b8034d1e
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+][webrtc-uplift][qa-]
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
Attachment #738101 - Flags: approval-mozilla-aurora?
Attachment #738101 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ae9b2f28dae2
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift][qa-] → [webrtc][blocking-webrtc+][qa-]
Attachment #738101 - Flags: feedback?(cdiehl)
You need to log in before you can comment on or make changes to this bug.