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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Adam Roach [:abr]]

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

Comment 2

[Christoph Diehl [:posidron]]

It's an ASan enabled debug build with -O2

Comment 3

[Adam Roach [:abr]]

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

Comment 4

[Adam Roach [:abr]]

Attachment: Test case

Comment 5

[Adam Roach [:abr]]

Attachment: Remove ICE state check from get*Description

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 712200 [details] [diff] [review]
Remove ICE state check from get*Description

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

lgtm

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3829e572e6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d3829e572e6

Comment 9

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 10

[Jason Smith [:jsmith]]

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

ack. Small mistake in the patch. One sec.

Comment 11

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 12

[Adam Roach [:abr]]

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.

Comment 13

[Jason Smith [:jsmith]]

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?

Comment 14

[Adam Roach [:abr]]

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

Comment 15

[Jason Smith [:jsmith]]

Clint - What do you think in response to comment 14?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 17

[Adam Roach [:abr]]

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 19

[Jason Smith [:jsmith]]

Attachment: Crashtest v2

Comment 20

[Jason Smith [:jsmith]]

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.

Comment 21

[Adam Roach [:abr]]

Comment on attachment 733072 [details] [diff] [review]
Crashtest v2

Looks good to me.

Comment 22

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c40b2d396c6

Comment 23

[Adam Roach [:abr]]

Reverted and relanded to deal with Unicode BOM in commit message:

http://hg.mozilla.org/integration/mozilla-inbound/rev/3aab0275c08f
http://hg.mozilla.org/integration/mozilla-inbound/rev/000c0c72fa82

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/000c0c72fa82

Comment 25

[cmtalbert]

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