822197 - Assertion failure: cb_hdr and crash [@ fim_process_event/FIM_DEBUG]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

./media/webrtc/signaling/src/sipcc/core/gsm/fim.c:538

[...]
if (call_chn == NULL) {                                                                                                     
    FIM_DEBUG(get_debug_string(GSM_DBG1), "FIM", call_id, fname, "no call_chn");
[...]


@rforbes
SEED: 1355734171.18
Fault was detected on test 111

Tested with m-i changeset: 116243:9d6e95e77855

Comment 1

[Ethan Hugg [:ehugg]]

Unfortunately this callstack is not informative enough for me to figure this out.  If you get another crash in this area, please post it.  Thanks.

Comment 2

[Christoph Diehl [:posidron]]

Just a note that this crash still pops up relatively often with m-c changeset:   118049:928550157e6e

Comment 3

[Christoph Diehl [:posidron]]

I am seeing this assertion failure before the crash:

Assertion failure: cb_hdr, at /Users/cdiehl/Code/Mozilla/mc-inbound-asan/media/webrtc/signaling/src/sipcc/core/gsm/fim.c:604

Comment 4

[Ethan Hugg [:ehugg]]

I just got this to happen under the debugger.  It appears to happen when we go over the MAX_CONFIG_LINES limit in SIPCC.  If I hit reload quickly perhaps I'm creating PCs faster than they are being destructed.

Not sure what to do about it yet, whether we even need a maximum number here, whether it could be arbitrarily large, or whether we can make a saner error case when it's hit.

Comment 5

[Christoph Diehl [:posidron]]

(In reply to Ethan Hugg [:ehugg] from comment #4)
> I just got this to happen under the debugger. If I hit reload quickly
> perhaps I'm creating PCs faster than they are being destructed.

That's great. That practice covers with my fuzzing procedures.
Since I added a delay of 3 seconds between each test I am not seeing this crash anymore.

Comment 6

[Ethan Hugg [:ehugg]]

Attachment: Testcase for creating too many PeerConnections

Low-brow testcase that creates and holds many PeerConnection objects and hits this assert every time for me.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Handle creation of more PeerConnections than MAX_LINES

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 702490 [details] [diff] [review]
Handle creation of more PeerConnections than MAX_LINES

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

In this case we are running out of a resource known as the feature control block which are allocated in a fixed array in fsm.c.  This is the simple fix where we handle this with an error instead of a crash.

Also note that after making 45 PeerConnections you will see this

JavaScript error: file:///home/ehugg/mozilla/mozilla-central/obj-ff-dbg/dist/bin/components/PeerConnection.js, line 278: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [IPeerConnection.initialize]

Which is from PeerConnectionMedia::Init/NrIceCtx::StartGathering which is has this maximum limit.  I did not try to fix this since it is handled with a proper error and doesn't crash,  but it appears 45 is our limit for making PCs.

I have also considered changing the fcbs to be either a dynamically expandable array or a linked list so that they would only run out with memory.  I feel that needs its own patch, but let me know if you think that would be a priority.

Comment 9

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

Comment on attachment 702490 [details] [diff] [review]
Handle creation of more PeerConnections than MAX_LINES

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

We will likely need to address the number of slots as well, in a different bug

Comment 10

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/096ee26f5a0d

Comment 11

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

(In reply to Ethan Hugg [:ehugg] from comment #8) 
> Also note that after making 45 PeerConnections you will see this
> 
> JavaScript error:
> file:///home/ehugg/mozilla/mozilla-central/obj-ff-dbg/dist/bin/components/
> PeerConnection.js, line 278: NS_ERROR_FAILURE: Component returned failure
> code: 0x80004005 (NS_ERROR_FAILURE) [IPeerConnection.initialize]

Keep in mind that this is bug 824919 and will hopefully be fixed soon.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/096ee26f5a0d

Comment 13

[Jason Smith [:jsmith]]

Attachment: Crashtest

Comment 14

[Jason Smith [:jsmith]]

Comment on attachment 706855 [details] [diff] [review]
Crashtest

Crashtest for this bug. I've taken Ethan's test case, cleaned some of the code up, and made it a crash test.

Comment 15

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

Comment on attachment 706855 [details] [diff] [review]
Crashtest

>+    var Logger = {
>+      console: function(msg) {
>+        if (window.dump) {
>+          window.dump(msg);
>+        } else if (window.console && window.console.log) {
>+          window.console.log(msg);
>+        }
>+      }
>+    }
[..]
>+
>+    function onFailure(code) {
>+      Logger.console("\n########## Failure: " + code + "\n");
>+    }

I do not think that logging is necessary here to reproduce the crash. It has only put in to the manual testcase to see what's going on.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 706855 [details] [diff] [review]
> Crashtest
> 
> >+    var Logger = {
> >+      console: function(msg) {
> >+        if (window.dump) {
> >+          window.dump(msg);
> >+        } else if (window.console && window.console.log) {
> >+          window.console.log(msg);
> >+        }
> >+      }
> >+    }
> [..]
> >+
> >+    function onFailure(code) {
> >+      Logger.console("\n########## Failure: " + code + "\n");
> >+    }
> 
> I do not think that logging is necessary here to reproduce the crash. It has
> only put in to the manual testcase to see what's going on.

I'm not sure that's right. Take a look at the patch attached here. I'll wait for Ethan's feedback. Clearing feedback- as there isn't a clear indication is correct or not.

Comment 17

[Ethan Hugg [:ehugg]]

Comment on attachment 706855 [details] [diff] [review]
Crashtest

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

Looks good to me, just a couple nits.

::: dom/media/tests/crashtests/822197.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=822197
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Bug 822917 - Many PeerConnections with CreateOffer</title>

The bug number is 822197 - typo.

@@ +22,5 @@
> +    function onCreateOfferSuccess(offer) {
> +    }
> +
> +    function onFailure(code) {
> +      Logger.console("\n########## Failure: " + code + "\n");

As Henrik mentions, logging is only applicable to a manual run.  The loop below should create and hold enough PCs to test the crash.

Comment 18

[Jason Smith [:jsmith]]

Attachment: Crashtest v2

Comment 19

[Jason Smith [:jsmith]]

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

Cleaned up per review comments.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60044584ed1e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60044584ed1e