Closed
Bug 933986
Opened 11 years ago
Closed 8 years ago
Seeing duplicate candidate priorities when using both STUN and TURN servers
Categories
(Core :: WebRTC: Networking, defect, P3)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: bwc, Assigned: bwc)
References
Details
Attachments
(3 files, 1 obsolete file)
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•11 years ago
|
Assignee: nobody → docfaraday
Assignee | ||
Comment 1•11 years ago
|
||
Strawman patch (breaks source compat a tiny bit)
Assignee | ||
Comment 2•11 years ago
|
||
Landed a hunk in the wrong place. Fixed now.
Assignee | ||
Updated•11 years ago
|
Attachment #827104 -
Attachment is obsolete: true
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f22921615961
Assignee | ||
Updated•11 years ago
|
Attachment #827115 -
Flags: feedback?(ekr)
Comment 4•10 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+
Updated•9 years ago
|
backlog: --- → webRTC+
Rank: 38
Priority: -- → P3
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40253/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40253/
Comment 6•8 years ago
|
||
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•8 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)
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f87d50f97577
Status: NEW → RESOLVED
Closed: 8 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.
Description
•