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

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: 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.

Comment 2

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

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).

Comment 3

[Eric Rescorla (:ekr)]

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

Comment 4

[Maire Reavy [:mreavy]]

Marking this as fixed because we believe this is bug 820671, which was fixed after this report.

Comment 5

[Christoph Diehl [:posidron]]

Attachment: callstack-after-patches-landed

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

Comment 6

[Eric Rescorla (:ekr)]

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

Comment 7

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

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?

Comment 8

[Eric Rescorla (:ekr)]

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?

Comment 9

[Christoph Diehl [:posidron]]

> 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.

Comment 10

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

Attachment: Media pointer check after NS_DISPATCH_SYNC

Added patch in any case. Can't hurt given the sync dispatch.

Comment 11

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

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?

Comment 12

[Eric Rescorla (:ekr)]

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.

Comment 13

[Eric Rescorla (:ekr)]

I don't think you need to release the flow explicitly, though. It's in a refptr, so it will just auto-release, right?

Comment 14

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

Attachment: Media pointer check after NS_DISPATCH_SYNC. r=rjesup

Agree. Changed to return NULL in shutdown situation.

Comment 15

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

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

Comment 16

[Phil Ringnalda (:philor)]

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