Commit aa39e919 authored by Byron Campen [:bwc]'s avatar Byron Campen [:bwc] Committed by bcampen@mozilla.com
Browse files

Bug 1954423: Make sure we finish checking peer nominated pairs. r=mjf 

This contains three changes.

1. When nomination happens, do not cancel higher priority pairs that have been
peer nominated but not fully nominated yet (because our own checks have not
succeeded). If our response to the peer nomination on a pair like this has been
received, the other end is already using it.

2. Allow CANCELED pairs to be revived by the triggered check code, similar
to IN_PROGRESS pairs.

3. Allow the triggered check code to run again on a pair if that pair just
received a peer nomination, and has a priority high enough that it could end up
being selected.

Differential Revision: https://phabricator.services.mozilla.com/D242720
parent 0d43637d

dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_candidate_pair.c

+10 −7
Changes for dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_candidate_pair.c: 10 added lines, 7 removed lines.
Original line number Diff line number Diff line
@@ -454,14 +454,11 @@ static int nr_ice_candidate_copy_for_triggered_check(nr_ice_cand_pair *pair) 
    return(_status);

}



int nr_ice_candidate_pair_do_triggered_check(nr_ice_peer_ctx *pctx, nr_ice_cand_pair *pair)

int nr_ice_candidate_pair_do_triggered_check(nr_ice_peer_ctx *pctx, nr_ice_cand_pair *pair, int force)

  {

    int r,_status;



    if(pair->state==NR_ICE_PAIR_STATE_CANCELLED) {

      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND_PAIR(%s): Ignoring matching but canceled pair",pctx->label,pair->codeword);

      return(0);

    } else if(pair->state==NR_ICE_PAIR_STATE_SUCCEEDED) {

    if(pair->state==NR_ICE_PAIR_STATE_SUCCEEDED) {

      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND_PAIR(%s): No new trigger check for succeeded pair",pctx->label,pair->codeword);

      return(0);

    } else if (pair->local->stream->obsolete) {
@@ -472,8 +469,11 @@ int nr_ice_candidate_pair_do_triggered_check(nr_ice_peer_ctx *pctx, nr_ice_cand_ 
      return (0);

    }



    /* Do not run this logic more than once on a given pair */

    if(!pair->triggered){

    /* Do not run this logic more than once on a given pair (|force| is set

     * when the check we received has USE-CANDIDATE for this pair for the first

     * time, and this pair is a higher priority than anything that has been

     * nominated so far) */

    if(!pair->triggered || force){

      r_log(LOG_ICE,LOG_INFO,"ICE-PEER(%s)/CAND-PAIR(%s): triggered check on %s",pctx->label,pair->codeword,pair->as_string);



      pair->triggered=1;
@@ -492,6 +492,9 @@ int nr_ice_candidate_pair_do_triggered_check(nr_ice_peer_ctx *pctx, nr_ice_cand_ 
          r_log(LOG_ICE,LOG_INFO,"ICE-PEER(%s)/CAND-PAIR(%s): Inserting pair to trigger check queue: %s",pctx->label,pair->codeword,pair->as_string);

          nr_ice_candidate_pair_trigger_check_append(&pair->remote->stream->trigger_check_queue,pair);

          break;

        case NR_ICE_PAIR_STATE_CANCELLED:

          r_log(LOG_ICE,LOG_INFO,"ICE-PEER(%s)/CAND-PAIR(%s): received STUN check on cancelled pair, resurrecting: %s",pctx->label,pair->codeword,pair->as_string);

          /* fall through */

        case NR_ICE_PAIR_STATE_IN_PROGRESS:

          /* Instead of trying to maintain two stun contexts on the same pair,

           * and handling heterogenous responses and error conditions, we instead

dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_candidate_pair.h

+1 −1
Changes for dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_candidate_pair.h: 1 added line, 1 removed line.
Original line number Diff line number Diff line
@@ -87,7 +87,7 @@ void nr_ice_candidate_pair_set_state(nr_ice_peer_ctx *pctx, nr_ice_cand_pair *pa 
void nr_ice_candidate_pair_dump_state(nr_ice_cand_pair *pair, int log_level);

void nr_ice_candidate_pair_cancel(nr_ice_peer_ctx *pctx,nr_ice_cand_pair *pair, int move_to_wait_state);

int nr_ice_candidate_pair_select(nr_ice_cand_pair *pair);

int nr_ice_candidate_pair_do_triggered_check(nr_ice_peer_ctx *pctx, nr_ice_cand_pair *pair);

int nr_ice_candidate_pair_do_triggered_check(nr_ice_peer_ctx *pctx, nr_ice_cand_pair *pair, int force);

void nr_ice_candidate_pair_insert(nr_ice_cand_pair_head *head,nr_ice_cand_pair *pair);

int nr_ice_candidate_pair_trigger_check_append(nr_ice_cand_pair_head *head,nr_ice_cand_pair *pair);

void nr_ice_candidate_pair_restart_stun_nominated_cb(NR_SOCKET s, int how, void *cb_arg);

dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_component.c

+37 −17
Changes for dom/media/webrtc/transport/third_party/nICEr/src/ice/ice_component.c: 37 added lines, 17 removed lines.
Original line number Diff line number Diff line
@@ -805,12 +805,10 @@ static int nr_ice_component_pair_matches_check(nr_ice_component *comp, nr_ice_ca 
    return(1);

  }



static int nr_ice_component_handle_triggered_check(nr_ice_component *comp, nr_ice_cand_pair *pair, nr_stun_server_request *req, int *error)

static int nr_ice_component_handle_use_candidate(nr_ice_component *comp, nr_ice_cand_pair *pair, int *error)

  {

    nr_stun_message *sreq=req->request;

    int r=0,_status;



    if(nr_stun_message_has_attribute(sreq,NR_STUN_ATTR_USE_CANDIDATE,0)){

    if(comp->stream->pctx->controlling){

      r_log(LOG_ICE,LOG_WARNING,"ICE-PEER(%s)/CAND_PAIR(%s): Peer sent USE-CANDIDATE but is controlled",comp->stream->pctx->label, pair->codeword);

    }
@@ -827,15 +825,6 @@ static int nr_ice_component_handle_triggered_check(nr_ice_component *comp, nr_ic 
        }

      }

    }

    }



    /* Note: the RFC says to trigger first and then nominate. But in that case

     * the canceled trigger pair would get nominated and the cloned trigger pair

     * would not get the nomination status cloned with it.*/

    if(r=nr_ice_candidate_pair_do_triggered_check(comp->stream->pctx,pair)) {

      *error=(r==R_NO_MEMORY)?500:400;

      ABORT(r);

    }



    _status=0;

  abort:
@@ -905,9 +894,26 @@ static int nr_ice_component_process_incoming_check(nr_ice_component *comp, nr_tr 
       * we are willing to handle multiple matches here. */

      if(nr_ice_component_pair_matches_check(comp, pair, local_addr, req)){

        r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND_PAIR(%s): Found a matching pair for received check: %s",comp->stream->pctx->label,pair->codeword,pair->as_string);

        if(r=nr_ice_component_handle_triggered_check(comp, pair, req, error))

        int peer_nominated = pair->peer_nominated;

        if(nr_stun_message_has_attribute(req->request,NR_STUN_ATTR_USE_CANDIDATE,0)){

          if(r=nr_ice_component_handle_use_candidate(comp, pair, error)) {

            ABORT(r);

          }

        }



        int new_peer_nomination = !peer_nominated && pair->peer_nominated;

        int might_select = !comp->nominated ||

          (comp->nominated->priority < pair->priority);

        int force = new_peer_nomination && might_select;



        /* Note: the RFC says to trigger first and then nominate. But in that

         * case the canceled trigger pair would get nominated and the cloned

         * trigger pair would not get the nomination status cloned with it.*/

        if(!found_valid && (r=nr_ice_candidate_pair_do_triggered_check(comp->stream->pctx, pair, force))) {

          *error=(r==R_NO_MEMORY)?500:400;

          ABORT(r);

        ++found_valid;

        }

        found_valid=1;

      }

      pair=TAILQ_NEXT(pair,check_queue_entry);

    }
@@ -969,10 +975,18 @@ static int nr_ice_component_process_incoming_check(nr_ice_component *comp, nr_tr 
      TAILQ_INSERT_TAIL(&comp->candidates,pcand,entry_comp);

      pcand=0;



      if(nr_stun_message_has_attribute(req->request,NR_STUN_ATTR_USE_CANDIDATE,0)){

        if(r=nr_ice_component_handle_use_candidate(comp, pair, error)) {

          ABORT(r);

        }

      }



      /* Finally start the trigger check if needed */

      if(r=nr_ice_component_handle_triggered_check(comp, pair, req, error))

      if(r=nr_ice_candidate_pair_do_triggered_check(comp->stream->pctx, pair, 0)) {

        *error=(r==R_NO_MEMORY)?500:400;

        ABORT(r);

      }

    }



    _status=0;

  abort:
@@ -1540,10 +1554,15 @@ int nr_ice_component_nominated_pair(nr_ice_component *comp, nr_ice_cand_pair *pa 
    r_log(LOG_ICE,LOG_INFO,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s): cancelling all pairs but %s",comp->stream->pctx->label,comp->stream->label,comp->component_id,pair->codeword,pair->as_string);



    /* Cancel checks in WAITING and FROZEN per ICE S 8.1.2 */

    /* DO NOT CANCEL HIGHER PRIORITY PEER NOMINATED PAIRS!!! If a pair has been

     * peer nominated, we _must_ pursue it to completion, because if this is

     * the highest priority working pair from the peer's perspective, this is

     * the one it will use! This is a spec bug. */

    p2=TAILQ_FIRST(&comp->stream->trigger_check_queue);

    while(p2){

      if((p2 != pair) &&

         (p2->remote->component->component_id == comp->component_id)) {

         (p2->remote->component->component_id == comp->component_id) &&

         !(p2->peer_nominated && (p2->priority > pair->priority))) {

        assert(p2->state == NR_ICE_PAIR_STATE_WAITING ||

               p2->state == NR_ICE_PAIR_STATE_CANCELLED);

        r_log(LOG_ICE,LOG_INFO,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s): cancelling FROZEN/WAITING pair %s in trigger check queue because CAND-PAIR(%s) was nominated.",comp->stream->pctx->label,comp->stream->label,comp->component_id,p2->codeword,p2->as_string,pair->codeword);
@@ -1558,7 +1577,8 @@ int nr_ice_component_nominated_pair(nr_ice_component *comp, nr_ice_cand_pair *pa 
      if((p2 != pair) &&

         (p2->remote->component->component_id == comp->component_id) &&

         ((p2->state == NR_ICE_PAIR_STATE_FROZEN) ||

          (p2->state == NR_ICE_PAIR_STATE_WAITING))) {

          (p2->state == NR_ICE_PAIR_STATE_WAITING)) &&

         !(p2->peer_nominated && (p2->priority > pair->priority))) {

        r_log(LOG_ICE,LOG_INFO,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s): cancelling FROZEN/WAITING pair %s because CAND-PAIR(%s) was nominated.",comp->stream->pctx->label,comp->stream->label,comp->component_id,p2->codeword,p2->as_string,pair->codeword);



        nr_ice_candidate_pair_cancel(pair->pctx,p2,0);