860012 - Finish plumbing PeerConnection error codes

Status: VERIFIED FIXED

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

Description

[Adam Roach [:abr]]

This is a follow-up to Bug 834270 to plumb PeerConnection error callbacks
through to the SIPCC layer so we have better visibility into what causes
the failure callback to be triggered.

Comment 1

[Adam Roach [:abr]]

Attachment: Complete hooking up errors from gsm_sdp to PeerConnectionImpl

Comment 2

[Jason Smith [:jsmith]]

Not blocking, but this definitely a strong want for 22. If this makes it safely to trunk, let's nominate this for Aurora uplift.

Comment 3

[Adam Roach [:abr]]

This patch happens to fix Bug 836391 also, since I was touching the code in question. I'm marking it as blocking that one so we don't forget to close it when this lands.

Comment 4

[Adam Roach [:abr]]

Comment on attachment 735383 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl

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

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c
@@ +1077,5 @@
>   * @pre     None
>   */
>  
>  void cleanSessionData(session_data_t *data)
>  {

For what it's worth, the changes in this function are replacing tabs with spaces. I originally was going to be adding a new field to this structure (and so I needed to free it), but then I realized that I could simply use data->status instead (as the information being sent back is very much in the spirit of how that field is used). But by then, I'd already removed the tabs, and see no reason to revert the change.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3227,5 @@
>          if (strcmp(msg_body.parts[0].body, sdp) != 0) {
>              cc_free_msg_body_parts(&msg_body);
>              ui_set_local_description(evSetLocalDescError, line, call_id,
> +                dcb->caller_id.call_instance_id, strlib_empty(),
> +                PC_INVALID_STATE, "SDP changes are not supported");

Note that this block is commented out -- I just didn't want to leave it with the wrong number of parameters in case we decide this has value.

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 735383 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +182,5 @@
>  
>        case SETLOCALDESC:
>          // TODO: The SDP Parse error list should be copied out and sent up
>          // to the Javascript layer before being cleared here.
>          mPC->ClearSdpParseErrorMessages();

I think you can remove this comment now that you're pushing the errors up.  Also you could move this call to ClearSdpParseErrorMessages up to where you construct the mReason.  Same goes for the next three cases.

::: media/webrtc/signaling/src/sipcc/core/common/ui.c
@@ +1561,2 @@
>  {
> +    char buffer[256];

You could use flex_string_vsprintf for this then you wouldn't have to worry about the size.  See sdp_main.c::sdp_parse_error for an example.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3284,5 @@
>          return (SM_RC_END);
>      }
>      ui_set_local_description(evSetLocalDesc, line, call_id,
> +        dcb->caller_id.call_instance_id, strlib_malloc(local_sdp,-1),
> +        PC_NO_ERROR, 0);

Would it be Mozilla convention to put NULL instead of 0 for a const char * argument?

@@ +3322,5 @@
>      if (!sdpmode) {
>          ui_set_remote_description(evSetRemoteDescError, line, call_id,
>              dcb->caller_id.call_instance_id, strlib_empty(),
> +            PC_INTERNAL_ERROR, "'sdpmode' configuration is false. This should "
> +            "never ever happen. Run for your lives!");

Run for your lives! Run for the hills!
Er, My Lord, they're coming from the hills.
Oh, sorry. Run away from the hills! Run away from the hills! If you see the hills, run the other way!

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 735383 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +124,5 @@
>                                   IPeerConnectionObserver* aObserver)
>        : mPC(aPC),
>          mObserver(aObserver),
> +        mCode(static_cast<PeerConnectionImpl::Error>(aInfo->getStatusCode())),
> +        mReason(aInfo->getStatus()),

This line is causing my signaling_unittests to crash.  It appears that the session_data_t.status is a null pointer instead of an empty string.  Not sure why yet.

Comment 7

[Ethan Hugg [:ehugg]]

This fixed my problem with the signaling_unittest:

--- a/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c
+++ b/media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call_info.c
@@ -305,17 +305,17 @@ cc_int32_t CCAPI_CallInfo_getCallInstanc
  * @param handle - call handle
  * @return call status
  */
 cc_string_t CCAPI_CallInfo_getStatus(cc_callinfo_ref_t handle){
   static const char *fname="CCAPI_CallInfo_getStatus";
   session_data_t *data = (session_data_t *)handle;
   CCAPP_DEBUG(DEB_F_PREFIX"Entering\n", DEB_F_PREFIX_ARGS(SIP_CC_PROV, fname));
 
-  if ( data != NULL){
+  if (data && data->status){
      CCAPP_DEBUG(DEB_F_PREFIX"returned %s\n", DEB_F_PREFIX_ARGS(SIP_CC_PROV, fname), data->status);
      return data->status;
   }
 
   return strlib_empty();
 
 }

Comment 8

[Adam Roach [:abr]]

Attachment: Complete hooking up errors from gsm_sdp to PeerConnectionImpl

Comment 9

[Adam Roach [:abr]]

Comment on attachment 735834 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl

Carrying forward r+ from ehugg

Comment 10

[Adam Roach [:abr]]

https://tbpl.mozilla.org/?tree=Try&rev=b8860ab56327

Comment 11

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58cc5c8a4e01

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58cc5c8a4e01

Comment 13

[Adam Roach [:abr]]

Comment on attachment 735834 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl

[Approval Request Comment]
>Bug caused by (feature/regressing bug #): 
The original implementation (which provided an undocumented integer in error situations) was a strawman, developed prior to the WebRTC specification indicating how errors should be reported. The new error format reflects the current WebRTC spec. Arguably, the defect being fixed here was introduced in Bug 834270, since that bug mad all the errors callbacks opaquely return INTERNAL_ERROR. This half (which provides actual error details) was landed separately from Bug 834270, since our team decided it is more important to have a stable API when we pref on than it is to have full error details plumbed all the way through.

>User impact if declined: 
WebRTC developers will receive only opaque feedback when PeerConnection methods fail (i.e., all error responses will be of the form "INTERNAL_ERROR" without any further useful details).

>Testing completed (on m-c, etc.): 
Landed on both m-i and m-c without regressions, although the patch has been on both trees for a relatively short period of time. 

> Risk to taking this patch (and alternatives if risky): 
Given the straightforward nature of this patch, uplifting it should present virtually no risk. The only potential for problems I can identify is that the buffer management is a little fractured due to the number of shims, thread boundaries, and data structures strings must go through; however, the code is substantially identical to existing, well-tested buffer handling.

>String or IDL/UUID changes made by this patch:
None

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 735834 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl

Approving uplift to get more aurora testing and catch any cases missed on m-c. Also see this is on QA's radar for verification.Will be helpful to get verification on aurora as well once this lands.

Comment 15

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

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

Comment 16

[Jason Smith [:jsmith]]

Haven't gone through every possible flow, but some sanity testing and natural coding mistakes has already shown these errors populating and being quite descriptive. Will mark as verified as such.