Closed Bug 855796 Opened 7 years ago Closed 7 years ago

WebRTC crash [@sdp_attr_fmtp_no_value]

Categories

(Core :: WebRTC: Signaling, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla22

People

(Reporter: posidron, Assigned: ehugg)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [webrtc][blocking-webrtc+])

Attachments

(4 files, 2 obsolete files)

Attached file testcase
Tested with m-i changeset: 126594:6da10679d191
Attached file callstack
Most likely first appeared with the push of patch 2 of Bug 805533
The call vcmOnSdpParseError() should've been dispatched to main thread I think.  Trying a patch for this now.
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.
Attachment #730953 - Flags: review?(adam)
Assignee: nobody → ethanhugg
Duplicate of this bug: 855880
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.
Attachment #730953 - Flags: review?(adam) → review-
Blocks: 834270
Attachment #730953 - Attachment is obsolete: true
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.
Attachment #731207 - Flags: review?(adam)
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
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.
Attachment #731207 - Flags: review?(adam) → review+
Whiteboard: [webrtc][blocking-webrtc+]
Attachment #731207 - Attachment is obsolete: true
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
Attachment #731275 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c3fccc05f7b5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Flags: in-testsuite?
Keywords: verifyme
Attachment #731740 - Flags: review?(ethanhugg)
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?
Attachment #731740 - Flags: review?(ethanhugg) → review+
(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.
Verified through knowing crashtest didn't blow up in CI + test case being ran locally doesn't blow up.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 873791
Depends on: 875640
You need to log in before you can comment on or make changes to this bug.