863833 - WebRTC assertion failure: stream->created and abort [@gsmsdp_add_remote_track]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

The trigger is on line 30:

    pc2.setRemoteDescription(pc1_offer, step3, onFailure);
    window.location.reload();

Tested with m-i changeset: 129149:9c124a12d219

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

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

This callstack doesn't include the signature you list:  gsmsdp_add_remote_track

Comment 3

[Christoph Diehl [:posidron]]

It does, please scroll down to Thread:  29

Comment 4

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

gsmsdp_negotiate_media_lines() knows that the create() failed, but is calling gsm_add_remote_track() anyways.

pc_stream_id is -1, cause = 25, created_media_stream = false;

Over to Ethan

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: Signaling, add track only when add stream successful

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 739831 [details] [diff] [review]
Signaling, add track only when add stream successful

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

I was unable to replicate this even on the exact M-I rev on OSX 10.8 and letting it run for 10 mins.  I created this patch based on Randell's analysis.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Signaling, add track only when add stream successful

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 739841 [details] [diff] [review]
Signaling, add track only when add stream successful

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

Second thought we should do it this way in order to not unnecessarily break multi-track streams.

Comment 9

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

Comment on attachment 739841 [details] [diff] [review]
Signaling, add track only when add stream successful

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4646,5 @@
> +                                result = gsmsdp_add_remote_track(0, i, dcb_p, media);
> +                                MOZ_ASSERT(result);  /* TODO([e-mail]) add real
> +                                                       error checking, but this
> +                                                       "can't fail" */
> +                          }

It looks like we'll fall through (near the end of the routine) into code that assumes the track was added/etc; it doesn't check cause or return early with an error.

If you look at it and say it's ok, then r+.  Otherwise r- for now.

Comment 10

[Ethan Hugg [:ehugg]]

I'll take a look and see if there's a reason that we don't return early when cause = CC_CAUSE_NO_MEDIA.

Comment 11

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

Taking since Ethan is on vacation.  Blocking+, though I could be persuaded otherwise probably, but I want to fix and uplift it as it's a crash.

Comment 12

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

Attachment: Signaling, add track only when add stream successful

Comment 13

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

Comment on attachment 741254 [details] [diff] [review]
Signaling, add track only when add stream successful

After looking, it's totally appropriate to return the error immediately

Comment 14

[Adam Roach [:abr]]

Comment on attachment 741254 [details] [diff] [review]
Signaling, add track only when add stream successful

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

This fixes the issue without making existing behavior worse. r+

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4625,5 @@
>                                                                dcb_p->call_id,
>                                                                media,
>                                                                &pc_stream_id);
>                                if (lsm_rc) {
> +                                return (CC_CAUSE_NO_MEDIA);

I'm not 100% convinced that the logic is right here, but it's no more wrong that it was before. The issue is that a failure to add video would prevent establishment of an audio-only session. That's probably okay, since such failures should be extremely infrequent.

@@ +4646,5 @@
> +                                result = gsmsdp_add_remote_track(0, i, dcb_p, media);
> +                                MOZ_ASSERT(result);  /* TODO([e-mail]) add real
> +                                                       error checking, but this
> +                                                       "can't fail" */
> +                          }

We're out past 80 columns here, but I'm not sure it's realistic to aggressively wrap things when we're indented so far. This function is in desperate need of a functional decomposition for readability's sake, but that's not the subject of this bug.

If you feel like wrapping this to fit, I think it would be a slight improvement. It's perfectly understandable to want to leave it as-is.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/93f79ae43c2a

Ethan - any comments?  I think if we failed to add a track, generally bad things for this call are going on and we don't know exactly what (and typically, the peerconnection is being torn down already).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93f79ae43c2a

Comment 17

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

Comment on attachment 741254 [details] [diff] [review]
Signaling, add track only when add stream successful

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Minimal - it appears it will fail adding the track and continue.  It's possible there would be follow-on issues, but it's not obvious that anything bad would happen.  Biggest impact would be on fuzzing testing (crashing due to asserts when this case is hit.)

Testing completed (on m-c, etc.): on m-c

Risk to taking this patch (and alternatives if risky): very low risk - fails the entire operation instead (and other fail-the-operation cases already exist, so it shouldn't be a new path).

String or IDL/UUID changes made by this patch: none

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 741254 [details] [diff] [review]
Signaling, add track only when add stream successful

Please be on the lookout for any regressions this may have & let QA know if some testing may help here.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf17ffb21f5e

Comment 20

[Ethan Hugg [:ehugg]]

This seems OK.  I made a build that returned CC_CAUSE_NO_MEDIA all of the time and it sanely called the error callback of SetRemoteDescription which is appropriate for an error from lsm_add_remote_stream.