Seeing duplicate candidate priorities when using both STUN and TURN servers

RESOLVED FIXED in Firefox 48

Status

()

Core
WebRTC: Networking
P3
normal
Rank:
38
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

4 years ago
Assignee: nobody → docfaraday
(Assignee)

Comment 1

4 years ago
Created attachment 827104 [details] [diff] [review]
(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)
(Assignee)

Comment 2

4 years ago
Created attachment 827115 [details] [diff] [review]
(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.
(Assignee)

Updated

4 years ago
Attachment #827104 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f22921615961
(Assignee)

Updated

4 years ago
Attachment #827115 - Flags: feedback?(ekr)

Comment 4

4 years ago
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
Attachment #827115 - Flags: feedback?(ekr) → feedback+
backlog: --- → webRTC+
Rank: 38
Priority: -- → P3
Created attachment 8730942 [details]
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/
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.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 7

2 years ago
I'm pretty sure it was ready, but let me kick off a try run and give it a closer look tomorrow.
Flags: needinfo?(docfaraday)

Updated

2 years ago
Blocks: 1188391
(Assignee)

Comment 8

2 years ago
Created 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

Review commit: https://reviewboard.mozilla.org/r/42483/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42483/
Attachment #8734796 - Flags: review?(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
Attachment #8734796 - Flags: review?(drno) → review+

Comment 10

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f87d50f97577
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.