Finish plumbing PeerConnection error codes

VERIFIED FIXED in Firefox 22

Status

()

Core
WebRTC: Signaling
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: abr, Assigned: abr)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(firefox22 fixed, firefox23 fixed)

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 735383 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl
Attachment #735383 - Flags: review?(ethanhugg)
Not blocking, but this definitely a strong want for 22. If this makes it safely to trunk, let's nominate this for Aurora uplift.
Whiteboard: [webrtc][blocking-webrtc-][webrtc-uplift]

Updated

5 years ago
Whiteboard: [webrtc][blocking-webrtc-][webrtc-uplift] → [WebRTC][blocking-webrtc-][webrtc-uplift]
(Assignee)

Comment 3

5 years ago
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.
Blocks: 836391
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla22
(Assignee)

Comment 4

5 years ago
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

5 years ago
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!
Attachment #735383 - Flags: review?(ethanhugg) → review+

Comment 6

5 years ago
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

5 years ago
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();
 
 }
(Assignee)

Comment 8

5 years ago
Created attachment 735834 [details] [diff] [review]
Complete hooking up errors from gsm_sdp to PeerConnectionImpl
(Assignee)

Updated

5 years ago
Attachment #735383 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Carrying forward r+ from ehugg
Attachment #735834 - Flags: review+
(Assignee)

Updated

5 years ago
Priority: -- → P1
(Assignee)

Updated

5 years ago
Target Milestone: mozilla22 → mozilla23
https://hg.mozilla.org/mozilla-central/rev/58cc5c8a4e01
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: verifyme
(Assignee)

Comment 13

5 years ago
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
Attachment #735834 - Flags: approval-mozilla-aurora?
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.
Attachment #735834 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3aefe042287
status-firefox22: --- → fixed
status-firefox23: --- → fixed
Whiteboard: [WebRTC][blocking-webrtc-][webrtc-uplift] → [WebRTC][blocking-webrtc-]
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.