933986 - Seeing duplicate candidate priorities when using both STUN and TURN servers

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P3)

Description

[Byron Campen [:bwc]]

The following chunks of code are both run on each candidate when TURN is enabled:

      /* And a srvrflx candidate for each STUN server */
      for(j=0;j<ctx->stun_server_ct;j++){
        if(r=nr_ice_candidate_create(ctx,component,
          isock,sock,SERVER_REFLEXIVE,
          &ctx->stun_servers[j],component->component_id,&cand))
          ABORT(r);
        TAILQ_INSERT_TAIL(&component->candidates,cand,entry_comp);
        component->candidate_ct++;
        cand=0;
      }


      /* And both a srvrflx and relayed candidate for each TURN server */
      for(j=0;j<ctx->turn_server_ct;j++){
        nr_socket *turn_sock;
        nr_ice_candidate *srvflx_cand;

        /* srvrflx */
        if(r=nr_ice_candidate_create(ctx,component,
          isock,sock,SERVER_REFLEXIVE,
          &ctx->turn_servers[j].turn_server,component->component_id,&cand))
          ABORT(r);
        cand->state=NR_ICE_CAND_STATE_INITIALIZING; /* Don't start */
        cand->done_cb=nr_ice_initialize_finished_cb;
        cand->cb_arg=cand;

...
}

Inside nr_ice_candidate_compute_priority, a component of the candidate priority is computed as follows:

      case SERVER_REFLEXIVE:
        if(r=NR_reg_get_uchar(NR_ICE_REG_PREF_TYPE_SRV_RFLX,&type_preference))
          ABORT(r);
        stun_priority=255-cand->stun_server->index;
        break;

Since the array of stun servers and the array of turn servers are separate, stun_server->index is not going to be unique, causing duplicated candidate priorities.

Comment 1

[Byron Campen [:bwc]]

Attachment: (WIP) Switch over from index to an id, and ensure uniqueness when feeding into the candidate priority calculation.

Strawman patch (breaks source compat a tiny bit)

Comment 2

[Byron Campen [:bwc]]

Attachment: (WIP) Switch over from index to an id, and ensure uniqueness when feeding into the candidate priority calculation.

Landed a hunk in the wrong place. Fixed now.

Comment 3

[Byron Campen [:bwc]]

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

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 827115 [details] [diff] [review]
(WIP) Switch over from index to an id, and ensure uniqueness when feeding into the candidate priority calculation.

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

This looks plausible

Comment 5

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 933986. (WIP) Switch over from index to an id, and ensure uniqueness when feeding into the candidate priority calculation.

Review commit: https://reviewboard.mozilla.org/r/40253/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40253/

Comment 6

[Nils Ohlmeier [:drno]]

I rebased this old patch, because I think we want it to avoid candidate dupes. I'm seeing lots of "duplicate priority" errors in Hello ICE error logs. So I think this could help prevent these.

Byron, what is left to land this? I would give it r+ and land it like it is.

Comment 7

[Byron Campen [:bwc]]

I'm pretty sure it was ready, but let me kick off a try run and give it a closer look tomorrow.

Comment 8

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 933986. Switch over from index to an id, and ensure uniqueness when feeding into the candidate priority calculation. r?drno

Review commit: https://reviewboard.mozilla.org/r/42483/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42483/

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8734796 [details]
MozReview Request: Bug 933986. Switch over from index to an id, and ensure uniqueness when feeding into the candidate priority calculation. r?drno

https://reviewboard.mozilla.org/r/42483/#review39007

Comment 10

[Pulsebot]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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