Closed Bug 836349 Opened 11 years ago Closed 11 years ago

WebRTC Assertion failure: !assert_ice_ready || (mIceState != kIceGathering), at media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1075 and crash [@ sipcc::PeerConnectionImpl::CheckApiState()]

Categories

(Core :: WebRTC, defect, P1)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: posidron, Assigned: abr)

References

Details

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

Crash Data

Attachments

(4 files, 2 obsolete files)

Attached file callstack
This happens while fuzzing the JSEP and it seems only to occur with optimized builds. It's possible that a testcase might follow up on this bug.

Tested with m-c changeset: 120354:2cc710018b14 and -O2
Crash Signature: [@ sipcc::PeerConnectionImpl::CheckApiState()]
Summary: WebRTC Assertion failure: !assert_ice_ready || (mIceState != kIceGathering), at media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1075 → WebRTC Assertion failure: !assert_ice_ready || (mIceState != kIceGathering), at media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1075 and crash [@ sipcc::PeerConnectionImpl::CheckApiState()]
Assignee: nobody → adam
Whiteboard: [webrtc][blocking-webrtc+]
(In reply to Christoph Diehl [:cdiehl] from comment #0)
> Created attachment 708177 [details]
> callstack
> 
> This happens while fuzzing the JSEP and it seems only to occur with
> optimized builds.

I'm a little flummoxed. How are you hitting a PR_ASSERT with an optimized build? I thought the asserts were compiled out for optimized builds...
It's an ASan enabled debug build with -O2
Ah, okay. That makes sense. Thanks. Given that fact, are we sure it makes sense to block preffing on for this bug? Absent this assert, it looks like the code would simply error out...
Blocks: 796463
Priority: -- → P1
Attached file Test case
Keywords: testcase
Attachment #712200 - Flags: review?(ekr)
Comment on attachment 712200 [details] [diff] [review]
Remove ICE state check from get*Description

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

lgtm
Attachment #712200 - Flags: review?(ekr) → review+
Attachment #712200 - Flags: checkin?(rjesup)
https://hg.mozilla.org/mozilla-central/rev/4d3829e572e6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]
Attachment #712200 - Flags: checkin?(rjesup) → checkin+
Attached patch Crashtest v1 (obsolete) — Splinter Review
Attachment #731746 - Flags: review?(adam)
Comment on attachment 731746 [details] [diff] [review]
Crashtest v1

ack. Small mistake in the patch. One sec.
Attachment #731746 - Flags: review?(adam)
Attachment #731746 - Attachment is obsolete: true
Attached patch Crashtest v1 (obsolete) — Splinter Review
Attachment #731750 - Flags: review?(adam)
Comment on attachment 731750 [details] [diff] [review]
Crashtest v1

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

This looks like it should evoke the crash we originally saw. You might double-check with someone who knows more about how the test harness behaves whether it's okay to use alert() in a crashtest.
Attachment #731750 - Flags: review?(adam) → review+
Comment on attachment 731750 [details] [diff] [review]
Crashtest v1

Asked clint about this - nope, we can't use alert in a crash test.

I'm guessing the alert is irrelevant to the test itself, right? What can I safely modify here to still generate the crash?
Attachment #731750 - Flags: review-
(In reply to Jason Smith [:jsmith] from comment #13)
> I'm guessing the alert is irrelevant to the test itself, right? What can I
> safely modify here to still generate the crash?

The mochi tests use info(), but I'm guessing that's probably not available -- I think it's part of the mochi framework (which, AFAIK, isn't used by the crash tests). I suspect dump() would be okay, but I would also run that by clint to see if he thinks it's kosher.
Clint - What do you think in response to comment 14?
Flags: needinfo?(ctalbert)
Comment on attachment 731750 [details] [diff] [review]
Crashtest v1

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

::: dom/media/tests/crashtests/836349.html
@@ +10,5 @@
> +    function crash()
> +    {
> +      var pc = new mozRTCPeerConnection();
> +      var ld = pc.localDescription;
> +      alert (ld.sdp);

The issue here is most likely the access to pc.localDescription.sdp which is causing the crash. The alert shouldn't play a role here. So it should be enough to assign it's value to a new variable like `var sdp = pc.localDescription.sdp`. You should try an older Firefox build to verify that. If it crashes we are fine.
(In reply to Henrik Skupin (:whimboo) from comment #16)
> Comment on attachment 731750 [details] [diff] [review]
> Crashtest v1
> 
> Review of attachment 731750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/crashtests/836349.html
> @@ +10,5 @@
> > +    function crash()
> > +    {
> > +      var pc = new mozRTCPeerConnection();
> > +      var ld = pc.localDescription;
> > +      alert (ld.sdp);
> 
> The issue here is most likely the access to pc.localDescription.sdp which is
> causing the crash. The alert shouldn't play a role here. So it should be
> enough to assign it's value to a new variable like `var sdp =
> pc.localDescription.sdp`. You should try an older Firefox build to verify
> that. If it crashes we are fine.

The reason I like the idea of doing something with a clearly visible side effect here (like printing the value to the console) is that this is the *kind* of thing that a good compiler can prune (and I would assume that modern javascript interpreters do enough look-ahead to enable this kind of optimization, although I'm certainly not an expert in the area). So, even if our existing javascript implementation doesn't presently detect that a series of unused variable assignments optimizes to a no-op, there's no guarantee that it won't in the future.
Makes sense. Haven't thought about that. So dump() is a good idea then, given that we will also see it in the log for further investigation if something is wrong.
Flags: needinfo?(ctalbert)
Attachment #731750 - Attachment is obsolete: true
Comment on attachment 733072 [details] [diff] [review]
Crashtest v2

Execution path is pretty much the same, so I think we're okay changing to dump here.
Attachment #733072 - Flags: review?(adam)
Comment on attachment 733072 [details] [diff] [review]
Crashtest v2

Looks good to me.
Attachment #733072 - Flags: review?(adam) → review+
Keywords: checkin-needed
(In reply to Jason Smith [:jsmith] from comment #15)
> Clint - What do you think in response to comment 14?

dump is fine.  Sorry I didn't see this sooner.
You need to log in before you can comment on or make changes to this bug.