855796 - WebRTC crash [@sdp_attr_fmtp_no_value]

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with m-i changeset: 126594:6da10679d191

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Ethan Hugg [:ehugg]]

Most likely first appeared with the push of patch 2 of Bug 805533

Comment 3

[Ethan Hugg [:ehugg]]

The call vcmOnSdpParseError() should've been dispatched to main thread I think.  Trying a patch for this now.

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: vcmOnSdpParseError should be dispatched to main thread

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 730953 [details] [diff] [review]
vcmOnSdpParseError should be dispatched to main thread


Testcase works for me with this patch and puts the correct message in the nspr log.

Comment 7

[Adam Roach [:abr]]

Comment on attachment 730953 [details] [diff] [review]
vcmOnSdpParseError should be dispatched to main thread

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

r- for unnecessary sync on the cross-thread call.

We're finding increasingly that the use of sync dispatches is problematic. While I don't know of any concrete problems that might arise in this specific instance (as it would require a fair bit of analysis to track this kind of thing down), we *have* seen at least one deadlock occur as the result of hitting our callbacks synchronously. I think we'd be much better off if the thunk strdup'ed its two parameters and then called vcmOnSdpParseError_m async.

We don't lose anything by doing so: the only error we're getting back from the main thread function is an inability to find the PC, which is generally no cause for concern (it simply means that the PC has been cleaned up with events pending, which is perfectly legitimate, if somewhat exceptional). It's probably worth a Warn- or Info-level log message, but I don't think there's value to making sure the caller finds out about it.

I did verify that the current patch prevents the crash I was getting in my mochi tests for bug 834270, so it's definitely on the right path.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2701,5 @@
> + *
> + * This method is called for each parsing error of SDP.  It does not necessarily
> + * mean the SDP read was fatal and can be called many times for the same SDP.
> + *
> + */

Add info to the comment block that this function must run on the main thread.

@@ +2718,4 @@
>   * vcmOnSdpParseError
>   *
>   * This method is called for each parsing error of SDP.  It does not necessarily
>   * mean the SDP read was fatal and can be called many times for the same SDP.

Update to indicate that this is simply a thunk to the main thread.

Comment 8

[Ethan Hugg [:ehugg]]

Attachment: vcmOnSdpParseError should be dispatched to main thread

Comment 9

[Ethan Hugg [:ehugg]]

Comment on attachment 731207 [details] [diff] [review]
vcmOnSdpParseError should be dispatched to main thread

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

Updated with DISPATCH_NORMAL and duped strings.

Comment 10

[Adam Roach [:abr]]

Comment on attachment 731207 [details] [diff] [review]
vcmOnSdpParseError should be dispatched to main thread

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

Looks good to me, although we can leak some small buffers in error situations.

The easy way to make this watertight against leaks would be something like:

> static void vcmOnSdpParseError_m(nsAutoPtr<std::string> peerconnection, nsAutoPtr<std::string> message) {
>   sipcc::PeerConnectionWrapper pc(peerconnection->c_str());
>   ENSURE_PC_NO_RET(pc);
> 
>   pc.impl()->OnSdpParseError(message->c_str());
> }
> 
> int vcmOnSdpParseError(const char *peerconnection, const char *message) {
>   MOZ_ASSERT(peerconnection);
>   MOZ_ASSERT(message);
>   nsAutoPtr<std::string> peerconnectionDuped = new std::string(peerconnection);
>   nsAutoPtr<std::string> messageDuped = new std::string(message);
> ...

Alternately, you could deal directly with C++ strings rather than messing around with the auto pointers, but that would cause the strings to be copied three times rather than just the once.

r+ with nits.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +82,5 @@
>        return errval; \
>      }         \
>    } while(0)
>  
> +#define ENSURE_PC_NO_RET(pc, errval) \

We're not using errval here, so I would suggest removing it.

@@ +2753,5 @@
> +  messageDuped = PL_strdup(message);
> +  MOZ_ASSERT(messageDuped);
> +  if (!messageDuped) {
> +    CSFLogError(logTag, "%s: Unable to propagate sdp parse error messageDuped is null",
> +      __FUNCTION__);

peerconnectionDuped leaks here

@@ +2765,5 @@
> +                   messageDuped),
> +      NS_DISPATCH_NORMAL);
> +
> +  if (!NS_SUCCEEDED(rv)) {
> +    CSFLogError( logTag, "%s(): Could not dispatch to main thread", __FUNCTION__);

peerconnectionDuped and messageDuped leak here

Comment 11

[Adam Roach [:abr]]

Oh, I missed a leak: if the ENSURE_PC_NO_RET fails, then you're going to return without freeing the buffers; and that *will* happen in normal operation. That leak will be hard to fix if you continue to use raw char*. I strongly suggest a move to strings or auto-pointers-to-strings.

Comment 12

[Ethan Hugg [:ehugg]]

Attachment: vcmOnSdpParseError should be dispatched to main thread

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 731275 [details] [diff] [review]
vcmOnSdpParseError should be dispatched to main thread

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

Good points on the possible leaks on error conditions.

This one uses nsAutoPtr<std::string>

bringing forward r+ from abr

Comment 14

[Ethan Hugg [:ehugg]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 17

[Ethan Hugg [:ehugg]]

Comment on attachment 731740 [details] [diff] [review]
Crashtest v1

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

This tests fine this exact problem with fmtp_no_value, but the bug was really about any SDP parsing error would hit this assert.  Do we want to have a test like this that has a collection of SDPs each with a different parsing error, or would that kind of thing go in a different area of the test suite?

Comment 18

[Jason Smith [:jsmith]]

(In reply to Ethan Hugg [:ehugg] from comment #17)
> Comment on attachment 731740 [details] [diff] [review]
> Crashtest v1
> 
> Review of attachment 731740 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This tests fine this exact problem with fmtp_no_value, but the bug was
> really about any SDP parsing error would hit this assert.  Do we want to
> have a test like this that has a collection of SDPs each with a different
> parsing error, or would that kind of thing go in a different area of the
> test suite?

That's probably worthwhile to have a test on as well. Let's file a new bug that focuses more extensively on these tests that would run in a mochitest context.

I'll land this specific test though.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5965e2dd39f9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5965e2dd39f9

Comment 21

[Jason Smith [:jsmith]]

Verified through knowing crashtest didn't blow up in CI + test case being ran locally doesn't blow up.