WebRTC crash [@sipcc::PeerConnectionMedia::AddTransportFlow]

RESOLVED FIXED in mozilla20

Status

()

defect
P1
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: posidron, Assigned: ekr)

Tracking

(Blocks 1 bug, {crash})

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC] [blocking-webrtc+] [qa-], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

7 years ago
Posted file callstack
This happened during fuzzing the SDP after the "big lock" patch landed. The signature looks new to me.

Tested with m-i changeset: 115674:740c8dd81b04
Reporter

Comment 1

7 years ago
Posted file SDP
This is the last known SDP for that crash. Not sure how helpful this though. Since I can't reproduce it with the same packet.
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc+]
I would think this is bug 820671, but it obviously got past earlier pc->media() calls in VcmCreateTransportFlow before getting to AddTransportFlow.  Perhaps a combination of bug 820671 and bug 792175.  ekr, what do you think?  If so, this should be closed as it's not repeatable (and if we're wrong it should recur).
Assignee: nobody → ekr
Assignee

Comment 3

7 years ago
Ostensibly this is post 792175. That said, it's on the main thread and so is the destruction of the media object, so I think it probably is 820671. I would close and see if it comes back
Marking this as fixed because we believe this is bug 820671, which was fixed after this report.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Reporter

Comment 5

7 years ago
void AddTransportFlow(int aIndex, bool aRtcp,
                        mozilla::RefPtr<mozilla::TransportFlow> aFlow) {
    int index_inner = aIndex * 2 + (aRtcp ? 1 : 0);

    mTransportFlows[index_inner] = aFlow; // <- Crash
  }


Tested with m-c changeset: 117036:f5ed2691d901
Reporter

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 6

7 years ago
I notice that there is an NS_DISPATCH_SYNC in this function. We may be getting reentrancy there.

A lame check would be to add a check for media() before the AddTransportFlow to see if it has
been destroyed. A better improvement would be to restructure things so that the SYNC comes
at the end or better yet is an ASYNC
pc->media() returns an nsRefPtr here, and it's operator->() does an

  NS_PRECONDITION(mRawPtr != 0,
                  "You can't dereference a NULL nsRefPtr with operator->().");

so wouldn't we see that message in the logs if the media was destroyed at this point?
Assignee

Comment 8

7 years ago
We might... But all I see here is a callstack, so I don't know. Based on reading the code, it appears that it's at least possible for this precondition not to cause a crash, but just a log message.

cdiehl: can we get the complete log?
Flags: needinfo?(cdiehl)
Reporter

Comment 9

7 years ago
> cdiehl: can we get the complete log?

Since the crash is not reproducible with a single test - not so easily. I am not seeing the crash right now on a regular basis but if I do, then I will try to manage to get a log.
Flags: needinfo?(cdiehl)
Added patch in any case. Can't hurt given the sync dispatch.
Attachment #697926 - Flags: review?(rjesup)
Comment on attachment 697926 [details] [diff] [review]
Media pointer check after NS_DISPATCH_SYNC

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

check with ekr about if it makes sense to return NULL in this case.  Otherwise, r+ (and r+ if you add flow = nullptr; if ekr says so)

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2669,5 @@
>        return NULL;
>      }
> +    if (pc->media().get()) { // test because of NS_DISPATCH_SYNC above
> +      pc->media()->AddTransportFlow(level, rtcp, flow);
> +    }

Should we release the flow and return NULL in this case?
Attachment #697926 - Flags: review?(rjesup) → review+
Flags: needinfo?(ekr)
Assignee

Comment 12

7 years ago
Comment on attachment 697926 [details] [diff] [review]
Media pointer check after NS_DISPATCH_SYNC

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2669,5 @@
>        return NULL;
>      }
> +    if (pc->media().get()) { // test because of NS_DISPATCH_SYNC above
> +      pc->media()->AddTransportFlow(level, rtcp, flow);
> +    }

IMO yes. The way you get into this state is if you have closed the PC and at this point you want the vcm code to fail.
Flags: needinfo?(ekr)
Assignee

Comment 13

7 years ago
I don't think you need to release the flow explicitly, though. It's in a refptr, so it will just auto-release, right?
Agree. Changed to return NULL in shutdown situation.
Attachment #697926 - Attachment is obsolete: true
Attachment #698210 - Flags: review+
Attachment #698210 - Flags: checkin?(rjesup)
Attachment #698210 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/eaede79d4d42
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+] [qa-]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.