899485 - Have SDP handling return sensible cause codes

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Adam Roach [:abr]]

Currently, most errors that percolate up from SDP generation and
negotiation result in the rather opaque response code CC_CAUSE_ERROR,
which is rendered to web developers as "cause = ERR" in many WebRTC
error callbacks.

To aid in troubleshooting and web developer debugging, these various
failurse should be distinctive, informative error codes.

Comment 1

[Adam Roach [:abr]]

Attachment: Have SDP handling return sensible cause codes

This patch removes CC_CAUSE_ERROR from gsm_sdp.c almost entierly,
replacing it with more meaningful error responses, most of which
are new. It also updates the two duelling cause-code-to-string
conversion functions, both of which had become pretty badly out
of sync with the cause codes themselves.

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 783026 [details] [diff] [review]
Have SDP handling return sensible cause codes

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

In the new function gsmsdp_check_ice_attributes_exist() there are two returns of CC_CAUSE_ERROR which I think should be now CC_CAUSE_NO_ICE_ATTRIBUTES.  You probably started this patch before that one was in.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1600,5 @@
> +            return result == SDP_INVALID_SDP_PTR ?
> +                       CC_CAUSE_INVALID_SDP_POINTER :
> +                   result == SDP_INVALID_PARAMETER ?
> +                       CC_CAUSE_BAD_ICE_ATTRIBUTE :
> +                   CC_CAUSE_ERROR;

I would indent or paren this to make it a bit clearer.

@@ +6227,5 @@
> +    if ( dcb_p->sdp == NULL || dcb_p->sdp->src_sdp == NULL ) {
> +        cause = gsmsdp_init_local_sdp(dcb_p->peerconnection, &(dcb_p->sdp));
> +        if ( cause != CC_CAUSE_OK) {
> +          return cause;
> +      }

I think this brace should be under the if

Comment 3

[Adam Roach [:abr]]

Attachment: Have SDP handling return sensible cause codes

Comment 4

[Adam Roach [:abr]]

Comment on attachment 783634 [details] [diff] [review]
Have SDP handling return sensible cause codes

Carrying forward r+ from ehugg

Comment 5

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e039d1a5523a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e039d1a5523a