1060625 - Remove excessive sipcc wrapper code

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Byron Campen [:bwc]]

There are several useless layers of wrapper code around the core functionality we want out of sipcc. We should remove basically all of it. (I think leaving a simple c++ wrapper is sensible, but the rest can go)

Comment 1

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

Proof of concept, just for calling createoffer.

Comment 2

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

Remove wrapper code from createanswer. Refactor and add snark.

Comment 3

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

setlocal and setremote

Comment 4

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

Remove wrapper code for the return path to PeerConnectionImpl from the four basic SDP ops. Lots of more cruft to remove.

Comment 5

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

Some cleanup, cuts down on boilerplate some.

Comment 6

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

addcandidate

Comment 7

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

foundcandidate

Comment 8

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

decruft

Comment 9

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

remotestreamadd

Comment 10

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

add/removestream

Comment 11

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

setpeerconnection

Comment 12

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

The various JSEP ops no longer fire callbacks; the necessary state updates are retrieved from the call object. OnRemoteStreamAdd is the only straggler, I'll probably route that through VcmSIPCCBinding or something and remove the PeerConnectionObserverDispatch for good.

Comment 13

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

Route OnRemoteStreamAdd through VcmSIPCCBinding, plug some leaks, and delete the old wrapper code for JSEP stuff.

Comment 14

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations.

Remove some cruft.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8485982 [details] [diff] [review]
(Proof of concept) Remove sipcc wrapper code around SDP operations.

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

I need to do some formatting cleanup, and make sure we're logging stuff where appropriate, but this is looking close to what we want for this ticket. I don't think I will be able to get us to managing our own state object in time for our work-week, but that's not going to hurt us much I think.

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8485982 [details] [diff] [review]
(Proof of concept) Remove sipcc wrapper code around SDP operations.

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

On the right track for sure.

::: media/webrtc/signaling/include/CC_Call.h
@@ +44,5 @@
> +        virtual fsmdef_states_t getFsmState () const = 0;
> +        virtual std::string fsmStateToString (fsmdef_states_t state) const = 0;
> +
> +        virtual void getErrorString(std::string *error) const = 0;
> +        virtual cc_int32_t getError() const = 0;

These are (almost) all consistently allocated together, but a wrapper would make this easier to work with.

i.e., struct CC_Error {
cc_int32_t code; // see typedef comment
std::string message;
}

That would allow you to remove error state from implementations of this interface.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +47,5 @@
>  #endif
>  
>  extern "C" {
>  #include "ccsdp.h"
> +//#include "vcm.h"

Delete.

@@ +1114,5 @@
> +    for (size_t i = 0; i < pc_stream_table.num_tracks; ++i) {
> +      pc_stream_table.track[i].media_stream_track_id =
> +        sipcc_stream_table->track[i].media_stream_track_id;
> +      // TODO: This was hard-coded to false before, do we even need this member?
> +      pc_stream_table.track[i].video = sipcc_stream_table->track[i].video;

Could you see any value in the commented out stuff that you didn't carry over?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1261,5 @@
> +class TracksAvailableCallback : public DOMMediaStream::OnTracksAvailableCallback
> +{
> +public:
> +  TracksAvailableCallback(DOMMediaStream::TrackTypeHints aTrackTypeHints,
> +                          nsRefPtr<PeerConnectionObserver> aObserver)

Why is this taking a strong reference to the observer?

@@ +1324,5 @@
> +#ifdef MOZILLA_INTERNAL_API
> +    TracksAvailableCallback* tracksAvailableCallback =
> +      new TracksAvailableCallback(mRemoteStreamInfo->mTrackTypeHints, pco);
> +
> +    stream->OnTracksAvailable(tracksAvailableCallback);

Just realized this is a move and not new.  Keeping the comments anyway...

This all looks great, but the API requires that we fire the callback way, way sooner than this.  I think that it's probably better if we just fire OnAddStream here.  The callback is good for OnAddTrack and for setting t=0 and the other cleanup that might not be possible until there is media.

@@ +2103,5 @@
> +
> +  // TODO: What about mid? Is this something that we will choose, or will
> +  // SIPCC choose for us? If the latter, we'll need to make it an outparam or
> +  // something.
> +  std::string mid;

We might want to take over more of this stuff.

@@ +2115,5 @@
> +    CSFLogDebug(logTag, "Passing local candidate to content: %s",
> +                candidate.c_str());
> +    mInternal->mCall->getLocalSdp(&mLocalSDP);
> +    pco->OnIceCandidate(level,
> +                              ObString(mid.c_str()),

=

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2835,5 @@
>  
>      dcb->ice_ufrag = (char *)cpr_malloc(strlen(ufrag) + 1);
> +    if (!dcb->ice_ufrag) {
> +      *error_outparam = strlib_printf( "Failed to allocate string for ICE ufrag");
> +      /* ??? fsmdef_release(fcb, cause, FALSE); */

Looks like a yes to me.  The fact that cause isn't set to something other than CC bothers me a little though:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/include/cc_constants.h#477
?

@@ +2836,5 @@
>      dcb->ice_ufrag = (char *)cpr_malloc(strlen(ufrag) + 1);
> +    if (!dcb->ice_ufrag) {
> +      *error_outparam = strlib_printf( "Failed to allocate string for ICE ufrag");
> +      /* ??? fsmdef_release(fcb, cause, FALSE); */
> +      return (PC_INTERNAL_ERROR);

This is probably fine, but internal errors aren't very helpful in practice.

I'm wondering if you can't thunk this out somehow:

//e.g., report_error(fcb_t*, cause_t, const char*);
return report_error_releasefcb(fcb, CC_CAUSE_OUT_OF_MEM, "Failed to allocate string for ICE ufrag");

::: media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c
@@ +302,5 @@
>      }
>      return (str);
>  }
>  
> +string_t strlib_printf(const char *format, ...) {

It kinda sucks that we're allocating string structs like this, only to have them converted later into std::string (and then nsString).  I can't see any way out of it though.

@@ +306,5 @@
> +string_t strlib_printf(const char *format, ...) {
> +    string_t result = strlib_empty();
> +    va_list args;
> +    flex_string fs;
> +    if (format) {

Is this ever NULL?

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
@@ +15,5 @@
>  #include "CSFAudioControl.h"
> +#include "PeerConnectionImpl.h"
> +#include "PeerConnectionCtx.h"
> +
> +#include "mozilla/ArrayUtils.h" // Pick this up before including sipcc stuff that also includes this within the extern "C"

Really, which one does that and can it be killed?

@@ +143,5 @@
> +fsm_fcb_t* CC_SIPCCCall::getFcb() const
> +{
> +    callid_t call_id = GET_CALL_ID(callHandle);
> +
> +    // TODO: Can we just create and hold our own call state?

Please.  It seems like there is a lot of stuff that is created by that macro that we aren't using too.

@@ +183,5 @@
> +
> +fsmdef_states_t CC_SIPCCCall::getFsmState() const {
> +  fsm_fcb_t *fcb = getFcb();
> +  if (!fcb) {
> +    MOZ_CRASH();

Maybe move this to getFcb() so that there is a guarantee there.  You don't check in other places, and there is at least one return of an error code.  But I think that if an allocate fails like this, there's no point continuing.

@@ +625,5 @@
> +    error_string = NULL;
> +
> +    switch (fcb->state) {
> +      case FSMDEF_S_STABLE:
> +      case FSMDEF_S_HAVE_LOCAL_OFFER: // Why did we allow this?

I think that we need to *not* allow this.  Same for below.

There's a rollback offer that we might want to allow in the have-local-offer state, I think.

@@ +752,5 @@
> +  fsm_fcb_t *fcb = getFcb();
> +
> +  if (!fcb) {
> +    // Can happen if we create too many peerconnections simultaneously
> +    return PC_INTERNAL_ERROR;

What happened to error_string here?  See above regarding fail-and-die on getFcb().

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.h
@@ +76,4 @@
>          CC_SIPCCCallMediaDataPtr  pMediaData;
>          std::string peerconnection;  // The peerconnection handle
> +        fsm_fcb_t* getFcb() const;
> +        string_t local_sdp;

Probably shouldn't switch to underscores_for_names here.

@@ +95,5 @@
> +        virtual void getRemoteSdp(std::string *sdp) const;
> +        virtual fsmdef_states_t getFsmState () const;
> +        virtual std::string fsmStateToString (fsmdef_states_t state) const;
> +        virtual void getErrorString(std::string *error) const;
> +        virtual cc_int32_t getError() const;

Can we typedef cc_int32_t to something (enum?) to help explain it?

Comment 17

[Byron Campen [:bwc]]

Attachment: (Proof of concept) Remove sipcc wrapper code around SDP operations

Unrot for interdiff.

Comment 18

[Byron Campen [:bwc]]

(In reply to Martin Thomson [:mt] from comment #16)
> Comment on attachment 8485982 [details] [diff] [review]
> (Proof of concept) Remove sipcc wrapper code around SDP operations.
> 
> Review of attachment 8485982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On the right track for sure.
> 
> ::: media/webrtc/signaling/include/CC_Call.h
> @@ +44,5 @@
> > +        virtual fsmdef_states_t getFsmState () const = 0;
> > +        virtual std::string fsmStateToString (fsmdef_states_t state) const = 0;
> > +
> > +        virtual void getErrorString(std::string *error) const = 0;
> > +        virtual cc_int32_t getError() const = 0;
> 
> These are (almost) all consistently allocated together, but a wrapper would
> make this easier to work with.
> 
> i.e., struct CC_Error {
> cc_int32_t code; // see typedef comment
> std::string message;
> }
> 
> That would allow you to remove error state from implementations of this
> interface.
> 

   I actually tried exactly this earlier on, and just wasn't happy with it, since putting the error code in an outparam makes it easier to inadvertently fail to report an error, necessitating guard code (setting the outparam error code to PC_INTERNAL_ERROR and defaulting the error string to something sensible, which requires some extra code).

> @@ +1114,5 @@
> > +    for (size_t i = 0; i < pc_stream_table.num_tracks; ++i) {
> > +      pc_stream_table.track[i].media_stream_track_id =
> > +        sipcc_stream_table->track[i].media_stream_track_id;
> > +      // TODO: This was hard-coded to false before, do we even need this member?
> > +      pc_stream_table.track[i].video = sipcc_stream_table->track[i].video;
> 
> Could you see any value in the commented out stuff that you didn't carry
> over?
> 

   Not really, the code I have does everything that commented out block did and more.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1261,5 @@
> > +class TracksAvailableCallback : public DOMMediaStream::OnTracksAvailableCallback
> > +{
> > +public:
> > +  TracksAvailableCallback(DOMMediaStream::TrackTypeHints aTrackTypeHints,
> > +                          nsRefPtr<PeerConnectionObserver> aObserver)
> 
> Why is this taking a strong reference to the observer?
> 
> @@ +1324,5 @@
> > +#ifdef MOZILLA_INTERNAL_API
> > +    TracksAvailableCallback* tracksAvailableCallback =
> > +      new TracksAvailableCallback(mRemoteStreamInfo->mTrackTypeHints, pco);
> > +
> > +    stream->OnTracksAvailable(tracksAvailableCallback);
> 
> Just realized this is a move and not new.  Keeping the comments anyway...
> 
> This all looks great, but the API requires that we fire the callback way,
> way sooner than this.  I think that it's probably better if we just fire
> OnAddStream here.  The callback is good for OnAddTrack and for setting t=0
> and the other cleanup that might not be possible until there is media.
> 

   Should this be a separate ticket?

> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +2835,5 @@
> >  
> >      dcb->ice_ufrag = (char *)cpr_malloc(strlen(ufrag) + 1);
> > +    if (!dcb->ice_ufrag) {
> > +      *error_outparam = strlib_printf( "Failed to allocate string for ICE ufrag");
> > +      /* ??? fsmdef_release(fcb, cause, FALSE); */
> 
> Looks like a yes to me.  The fact that cause isn't set to something other
> than CC bothers me a little though:
> 
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> sipcc/include/cc_constants.h#477
> ?
> 

   Yeah, it looks kinda derpy, but it will all be going away in bug 1065020.

> @@ +2836,5 @@
> >      dcb->ice_ufrag = (char *)cpr_malloc(strlen(ufrag) + 1);
> > +    if (!dcb->ice_ufrag) {
> > +      *error_outparam = strlib_printf( "Failed to allocate string for ICE ufrag");
> > +      /* ??? fsmdef_release(fcb, cause, FALSE); */
> > +      return (PC_INTERNAL_ERROR);
> 
> This is probably fine, but internal errors aren't very helpful in practice.
> 
> I'm wondering if you can't thunk this out somehow:
> 
> //e.g., report_error(fcb_t*, cause_t, const char*);
> return report_error_releasefcb(fcb, CC_CAUSE_OUT_OF_MEM, "Failed to allocate
> string for ICE ufrag");
> 

   I thought about this, but when we're managing our own state the release stuff will be going away anyhow.

> ::: media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c
> @@ +302,5 @@
> >      }
> >      return (str);
> >  }
> >  
> > +string_t strlib_printf(const char *format, ...) {
> 
> It kinda sucks that we're allocating string structs like this, only to have
> them converted later into std::string (and then nsString).  I can't see any
> way out of it though.
> 

   Not without converting some of sipcc into c++ code, sadly.

> @@ +306,5 @@
> > +string_t strlib_printf(const char *format, ...) {
> > +    string_t result = strlib_empty();
> > +    va_list args;
> > +    flex_string fs;
> > +    if (format) {
> 
> Is this ever NULL?
> 

   It is in this patch, but I've removed all such instances in my queue. The code that this came from had the check here, though.

> ::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
> @@ +15,5 @@
> >  #include "CSFAudioControl.h"
> > +#include "PeerConnectionImpl.h"
> > +#include "PeerConnectionCtx.h"
> > +
> > +#include "mozilla/ArrayUtils.h" // Pick this up before including sipcc stuff that also includes this within the extern "C"
> 
> Really, which one does that and can it be killed?
> 

   In several places, so we can set up static asserts that ensure that the various enums and their arrays of string representations are the same size.

> @@ +143,5 @@
> > +fsm_fcb_t* CC_SIPCCCall::getFcb() const
> > +{
> > +    callid_t call_id = GET_CALL_ID(callHandle);
> > +
> > +    // TODO: Can we just create and hold our own call state?
> 
> Please.  It seems like there is a lot of stuff that is created by that macro
> that we aren't using too.
> 

   I can create a bug for this then and put the number in the comment. It will probably not be trivial.

> @@ +183,5 @@
> > +
> > +fsmdef_states_t CC_SIPCCCall::getFsmState() const {
> > +  fsm_fcb_t *fcb = getFcb();
> > +  if (!fcb) {
> > +    MOZ_CRASH();
> 
> Maybe move this to getFcb() so that there is a guarantee there.  You don't
> check in other places, and there is at least one return of an error code. 
> But I think that if an allocate fails like this, there's no point continuing.
> 

   Actually, this can fail if we have too many PeerConnections (see setPeerConnection). It might also be possible to trigger this if content tries to do stuff after creation pf the PC fails in this way. I will add checking just in case.

> @@ +625,5 @@
> > +    error_string = NULL;
> > +
> > +    switch (fcb->state) {
> > +      case FSMDEF_S_STABLE:
> > +      case FSMDEF_S_HAVE_LOCAL_OFFER: // Why did we allow this?
> 
> I think that we need to *not* allow this.  Same for below.
> 
> There's a rollback offer that we might want to allow in the have-local-offer
> state, I think.
> 

   I'd be very surprised if that worked. I will trim down these switch statements to something that makes sense to me.

> @@ +752,5 @@
> > +  fsm_fcb_t *fcb = getFcb();
> > +
> > +  if (!fcb) {
> > +    // Can happen if we create too many peerconnections simultaneously
> > +    return PC_INTERNAL_ERROR;
> 
> What happened to error_string here?  See above regarding fail-and-die on
> getFcb().
> 

   Yeah, I'm going to fix that up, and create a common boilerplate function to handle these checks and such.

> @@ +95,5 @@
> > +        virtual void getRemoteSdp(std::string *sdp) const;
> > +        virtual fsmdef_states_t getFsmState () const;
> > +        virtual std::string fsmStateToString (fsmdef_states_t state) const;
> > +        virtual void getErrorString(std::string *error) const;
> > +        virtual cc_int32_t getError() const;
> 
> Can we typedef cc_int32_t to something (enum?) to help explain it?

   Yeah, one of the things I worked on yesterday.

Comment 19

[Byron Campen [:bwc]]

Attachment: Remove sipcc wrapper code around SDP operations

Incorporate feedback.

Comment 20

[Byron Campen [:bwc]]

Comment on attachment 8486667 [details] [diff] [review]
Remove sipcc wrapper code around SDP operations

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

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ -524,5 @@
>                    device_event_getname(aDeviceEvent));
>    }
>  }
>  
> -void PeerConnectionCtx::onCallEvent(ccapi_call_event_e aCallEvent,

This function is a no-op now, like the others (in header file)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h
@@ +21,5 @@
>  
>  #include "StaticPtr.h"
>  #include "PeerConnectionImpl.h"
>  #include "mozIGeckoMediaPluginService.h"
> +#include "nsIRunnable.h"

include-what-you-use fix

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -252,5 @@
> -        mFsmStateStr(aInfo->fsmStateToString(mFsmState)) {
> -    if (mCallState == REMOTESTREAMADD) {
> -      MediaStreamTable *streams = nullptr;
> -      streams = aInfo->getMediaStreams();
> -      mRemoteStream = mPC->media()->GetRemoteStream(streams->media_stream_id);

This line went to line 1319, but the rest is gone.

@@ -266,5 @@
> -
> -  ~PeerConnectionObserverDispatch(){}
> -
> -#ifdef MOZILLA_INTERNAL_API
> -  class TracksAvailableCallback : public DOMMediaStream::OnTracksAvailableCallback

This has been moved to to line 1273.

@@ -321,5 @@
> -        mReason += " | SDP Parsing Error: " + *i;
> -      }
> -      if (errors.size()) {
> -        mCode = PeerConnectionImpl::kInvalidSessionDescription;
> -      }

This concatenation logic moved to line 1040.

@@ -344,5 @@
> -      mPC->SetSignalingState_m(static_cast<PCImplSignalingState>(mFsmState - offset));
> -    } else {
> -      CSFLogError(logTag, ": **** UNHANDLED SIGNALING STATE : %d (%s)",
> -                  mFsmState, mFsmStateStr.c_str());
> -    }

This stuff (including the comment above) moved to line 1945.

@@ -346,5 @@
> -      CSFLogError(logTag, ": **** UNHANDLED SIGNALING STATE : %d (%s)",
> -                  mFsmState, mFsmStateStr.c_str());
> -    }
> -
> -    JSErrorResult rv;

All of the stuff in this switch statement (apart from REMOTESTREAMADD and FOUNDICECANDIDATE) have moved into their respective functions (eg; createOffer).

@@ -409,5 @@
> -        mObserver->OnAddIceCandidateError(mCode, ObString(mReason.c_str()), rv);
> -        break;
> -
> -      case FOUNDICECANDIDATE:
> -        {

Since we do not have to shoehorn all of the data into a single string in a callback from sipcc, we don't need any of this parse code.

@@ -444,5 @@
> -            CSFLogDebug(logTag, "Passing local candidate to content: %s",
> -                        candidate.c_str());
> -            mObserver->OnIceCandidate(level_long & 0xffff,
> -                                      ObString(mid.c_str()),
> -                                      ObString(candidate.c_str()), rv);

We've put this stuff in FoundICECandidate

@@ -449,5 @@
> -        }
> -        break;
> -      case REMOTESTREAMADD:
> -        {
> -          DOMMediaStream* stream = nullptr;

This has all moved to OnRemoteStreamAdd.

@@ -473,5 @@
> -          break;
> -        }
> -
> -      case UPDATELOCALDESC:
> -        /* No action necessary */

Removed.

@@ -478,5 @@
> -        break;
> -
> -      default:
> -        CSFLogError(logTag, ": **** UNHANDLED CALL STATE : %d (%s)",
> -                    mCallState, mStateStr.c_str());

Removed

@@ -1949,5 @@
>  
>  void
> -PeerConnectionImpl::onCallEvent(const OnCallEventArgs& args)
> -{
> -  const ccapi_call_event_e &aCallEvent = args.mCallEvent;

Pretty much all of this is gone.

@@ -1973,5 @@
> -
> -  switch (event) {
> -    case SETLOCALDESCSUCCESS:
> -    case UPDATELOCALDESC:
> -      mLocalSDP = aInfo->getSDP();

We set this in SetLocalDescription and FoundIceCandidate

@@ -1978,5 @@
> -      break;
> -
> -    case SETREMOTEDESCSUCCESS:
> -    case ADDICECANDIDATE:
> -      mRemoteSDP = aInfo->getSDP();

We set this in SetRemoteDescription and AddIceCandidate

::: media/webrtc/signaling/src/sipcc/core/ccapp/CCProvider.h
@@ -107,5 @@
> -    unsigned int  media_stream_track_id;
> -    unsigned int  media_stream_id;
> -    cc_media_options_t* cc_options;
> -    string_t      candidate;
> -    Timecard *    timecard;

We don't use this struct anymore, so remove our modifications.

::: media/webrtc/signaling/src/sipcc/core/gsm/fim.c
@@ -140,5 @@
>      }
>  }
>  
>  
> -static fim_icb_t *

We export this now, so we can exfiltrate the fcb.

@@ -160,5 @@
>  
>      return call_chn;
>  }
>  
> -static fim_icb_t *

We export this now, so we can exfiltrate the fcb.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +163,5 @@
>  {
>  /* FSMDEF_S_IDLE ------------------------------------------------------------ */
>      {
>      /* CC_MSG_SETUP            */ fsmdef_ev_idle_setup,       // New incoming
> +    /* CC_MSG_SETUP_ACK        */ NULL,

All of the other state tables use NULL for invalid states; we added this fsmdef_ev_default business so we could cause error callbacks to come back at us, but since we don't have these callbacks anymore, we go back to the normal way of doing this.

@@ -197,4 @@
>      /* CC_MSG_DIALSTRING       */ fsmdef_ev_idle_dialstring,  // new outgoing
> -    /* CC_MSG_MWI              */ fsmdef_ev_default,
> -    /* CC_MSG_SESSION_AUDIT    */ fsmdef_ev_session_audit,
> -    /* CC_MSG_CREATEOFFER      */ fsmdef_ev_default,

These msg types no longer exist, so state table has been pruned.

@@ +2855,5 @@
>  
>      if (vcm_res) {
> +        FSM_DEBUG_SM(DEB_F_PREFIX"vcmGetDtlsIdentity returned an error", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +        *error_outparam = strlib_printf("Failed to get DTLS identity");
> +        fsmdef_release(fcb, cause, FALSE);

We actually didn't release here before. Seems sensible though.

@@ +2997,5 @@
>          FSM_DEBUG_SM(DEB_F_PREFIX"vcmGetDtlsIdentity returned an error", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +
> +        *error_outparam = strlib_printf("Could not get DTLS identity");
> +
> +        fsmdef_release(fcb, cause, FALSE);

We actually did not release here before. Seems like a sensible thing to do though.

@@ +3095,5 @@
>          fsm_change_state(fcb, __LINE__, FSMDEF_S_CLOSED);
> +        *error_outparam = strlib_printf(
> +            "'sdpmode' configuration is false. This should never ever happen. "
> +            "Run for your lives!");
> +        fsmdef_release(fcb, cause, FALSE);

We weren't releasing before, but we should have been.

@@ +3103,5 @@
>      if (!dcb->sdp) {
> +        *error_outparam = strlib_printf(
> +            "Setting of local SDP before calling createOffer or createAnswer "
> +            "is not currently supported.");
> +        fsmdef_release(fcb, cause, FALSE);

We weren't calling release here before, should we really let this slide?

@@ +3148,5 @@
>          if (cause != CC_CAUSE_OK) {
> +            *error_outparam = strlib_printf(
> +                "Could not configure local ICE state from SDP; cause = %s",
> +                cc_cause_name(cause));
> +            fsmdef_release(fcb, cause, FALSE);

We weren't releasing here, but we probably should have been.

@@ +3169,2 @@
>                  cc_cause_name(cause));
> +            fsmdef_release(fcb, cause, FALSE);

Same here.

@@ +3189,4 @@
>          }
> +        *error_outparam = strlib_printf(
> +            "Provisional answers are not yet supported");
> +        fsmdef_release(fcb, cause, FALSE);

Maybe we should let this slide. I'll remove this release.

@@ +3195,5 @@
>      default:
> +        *error_outparam = strlib_printf(
> +            "Unknown session description type: %d",
> +            action);
> +        fsmdef_release(fcb, cause, FALSE);

This seems like a fatal error to me.

@@ +3204,5 @@
>      local_sdp = sipsdp_write_to_buf(dcb->sdp->src_sdp, &local_sdp_len);
>      if (!local_sdp) {
> +        *error_outparam = strlib_printf(
> +            "Could not encode local SDP for local description");
> +        fsmdef_release(fcb, cause, FALSE);

This seems like a fatal error to me.

@@ +3252,5 @@
>          fsm_change_state(fcb, __LINE__, FSMDEF_S_CLOSED);
> +        *error_outparam = strlib_printf(
> +            "'sdpmode' configuration is false. This should never ever happen. "
> +            "Run for your lives!");
> +        fsmdef_release(fcb, cause, FALSE);

Fatal.

@@ +3306,5 @@
> +            *error_outparam = strlib_printf(
> +                "Could not process offer SDP; cause = %s",
> +                cc_cause_name(cause));
> +
> +            fsmdef_release(fcb, cause, FALSE);

Seems fatal to me, but maybe we should let slide in case they have another SDP they want to try?

@@ +3350,5 @@
>          if (cause != CC_CAUSE_OK) {
> +            *error_outparam = strlib_printf(
> +                "ICE attributes missing; cause = %s",
> +                cc_cause_name(cause));
> +            fsmdef_release(fcb, cause, FALSE);

Seems fatal to me, but maybe we should let content try another SDP?

@@ +3417,4 @@
>          }
> +        *error_outparam = strlib_printf(
> +            "Provisional answers are not yet supported");
> +        fsmdef_release(fcb, cause, FALSE);

I'll remove this.

@@ +3684,1 @@
>          FSM_DEBUG_SM(DEB_F_PREFIX"failure setting ice candidate.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));

Whoops, missing return. Will fix.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +779,5 @@
> +                              cc_feature_t *msg,
> +                              string_t *error_outparam);
> +pc_error fsmdef_addstream(fsm_fcb_t *fcb,
> +                              cc_feature_t *msg,
> +                              string_t *error_outparam);

I'll reindent these.

::: media/webrtc/signaling/src/sipcc/core/includes/uiapi.h
@@ -49,4 @@
>      evMaxEvent
>  } call_events;
>  
> -/* These values must be kept in sync with the equivalent values in:

Moved to cc_constants.h

::: media/webrtc/signaling/src/sipcc/core/src-common/string_lib.c
@@ +303,5 @@
>      return (str);
>  }
>  
> +string_t strlib_printf(const char *format, ...) {
> +    string_t result = strlib_empty();

This function was declared in string_lib.h, but not implemented. I grabbed this code from post_message_helper (which I deleted).

Comment 21

[Martin Thomson [:mt:]]

Comment on attachment 8486667 [details] [diff] [review]
Remove sipcc wrapper code around SDP operations

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

As we discussed, there are a lot of cases where we free the fcb struct, but there are also a few

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1259,5 @@
> +                        "a show-stopper, since whether we incorporate "
> +                        "candidates into the SDP doesn't really matter since "
> +                        "we're full trickle)",
> +                __FUNCTION__, mHandle.c_str(), error_string.c_str());
> +    pco->OnAddIceCandidateError(error, ObString(error_string.c_str()), rv);

So the impact of this is that the remote SDP isn't updated properly.  Will we still use the candidate?

Reporting an error like this generally needs to be fatal.  But if we still use the candidate, then it's definitely not fatal, just crappy.  And if we don't use the candidate, it's ICE, so it might continue to work.

If we want to fix this properly, there's probably a spec issue to resolve.

@@ +1493,5 @@
> +      mInternal->mCall->getErrorString(&error_string);
> +      CSFLogError(logTag, "%s: (video) pc = %s, error = %s",
> +                  __FUNCTION__, mHandle.c_str(), error_string.c_str());
> +      return NS_ERROR_FAILURE;
> +    }

This code duplication seems ripe for refactoring.

@@ +2132,5 @@
> +
> +  // TODO: What about mid? Is this something that we will choose, or will
> +  // SIPCC choose for us? If the latter, we'll need to make it an outparam or
> +  // something.
> +  std::string mid;

I suspect that we should be choosing mid somewhere.  Not here specifically, but we do want to make a mapping for the m= sections.  (I believe that Adam's partial o/a draft has some requirements we'd want to adhere to.)

Let's try to map something out ahead of next week.

::: media/webrtc/signaling/src/sipcc/core/ccapp/CCProvider.h
@@ -107,5 @@
> -    unsigned int  media_stream_track_id;
> -    unsigned int  media_stream_id;
> -    cc_media_options_t* cc_options;
> -    string_t      candidate;
> -    Timecard *    timecard;

Ooo, that statement implies all sorts of destructive goodness.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2855,5 @@
>  
>      if (vcm_res) {
> +        FSM_DEBUG_SM(DEB_F_PREFIX"vcmGetDtlsIdentity returned an error", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +        *error_outparam = strlib_printf("Failed to get DTLS identity");
> +        fsmdef_release(fcb, cause, FALSE);

Good thing that it never failed before then.

@@ +3417,4 @@
>          }
> +        *error_outparam = strlib_printf(
> +            "Provisional answers are not yet supported");
> +        fsmdef_release(fcb, cause, FALSE);

We can probably treat PRANSWER as ANSWER, yes.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
@@ +157,5 @@
> +    // options).
> +    fsm_fcb_t *fcb = (fsm_fcb_t*) call_chn->next_icb // FSM_TYPE_CNF
> +                                          ->next_icb // FSM_TYPE_B2BCNF
> +                                          ->next_icb // FSM_TYPE_XFR
> +                                          ->next_icb->cb; // FSM_TYPE_DEF

Guhhhhhhh.

Comment 22

[Byron Campen [:bwc]]

Attachment: Remove sipcc wrapper code around SDP operations

Incorporate review feedback, and remove a bad error log.

Comment 23

[Byron Campen [:bwc]]

Comment on attachment 8486851 [details] [diff] [review]
Remove sipcc wrapper code around SDP operations

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

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8486667&action=interdiff&newid=8486851&headers=1
Try: https://tbpl.mozilla.org/?tree=Try&rev=58d8f2d0ff90

This update fixes the following things (substantial enough that I'd like to get a second look):
* OnIceCandidate was jumping ahead of OnSetLocalDescriptionSuccess because of the setTimeout(0) in PeerConnection.js, I've reused some code to give these an extra trip through the pump.
* When JSEP ops encountered a fatal error, we weren't going to CLOSED, we were instead creating a new call object.
* In some failure cases we were leaking the ufrag and pwd down in sipcc, moved the cleanup code to where it should have been all along.
* Some non-fatal errors are now fatal, and vice-versa, as discussed earlier.
* Added a missing return statement.
* Fixed some indentation.
* Removed an error log when strlib_free is handed a null ptr (this should just silently no-op if it bothers to check at all).

Comment 24

[Byron Campen [:bwc]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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