Duplicated priorities for IPv6 ICE candidates

RESOLVED FIXED in Firefox 43

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: drno, Assigned: bwc)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
On my office machine I get multiple IPv6 addresses assigned. This results in test failures of:
- IceConnectTest.TestPollCandPairsAfterConnect
- IceConnectTest.TestPollCandPairsDuringConnect

Probably this caused by priorities per interface being re-used for multiple IP addresses on the same interface:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#470
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_candidate.c#461
(Reporter)

Updated

4 years ago
Rank: 25
I think if we start using nrinterfaceprioritizer.cpp on OS X and Windows, this will fix this problem, even if none of the prioritization based on vpn, estimated speed, etc worked at all.

That said, the non-nr_interface_prioritizer code is busted. We are simply handling the preference bump for IPV6 wrong, and there is no logic that ensures that addresses on a multi-homed interface will get distinct priorities.
backlog: --- → webRTC+
(Reporter)

Comment 2

4 years ago
I think even nrinterfaceprioritizer won't help much if this code is being used: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nrinterfaceprioritizer.cpp?from=nrinterfaceprioritizer.cpp#64

Would it help if we simply count the IPs per interface (in nrinterfaceprioritizer)?
Because I think trying to really come up with some smart logic which tries to sort different IPs on a given interface, e.g. prefer temp IPv6 over static etc, is just too complicated and will have corner cases which fail again.
It will sort addrs from the same interface arbitrarily, yes, but it still assigns a unique priority to everything.
Comment on attachment 8656193 [details]
MozReview Request: Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.

This fixes these tests for me on OS X.
Attachment #8656193 - Flags: feedback?(drno)
(Reporter)

Comment 6

4 years ago
https://reviewboard.mozilla.org/r/18105/#review16257

On 10.10 I had to rebase it on top of current mozilla central, because I got strange linker errors in some memory related code.
Looks good to me. Fixes the ice unit test on my machine.
(Reporter)

Updated

4 years ago
Attachment #8656193 - Flags: feedback?(drno) → feedback+
Assignee: nobody → docfaraday
Comment on attachment 8656193 [details]
MozReview Request: Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.

Bug 1186590: Enable interface prioritizer on all platforms.
Attachment #8656193 - Flags: feedback+ → review?(drno)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8656193 [details]
MozReview Request: Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.

https://reviewboard.mozilla.org/r/18107/#review16391

LGTM
Attachment #8656193 - Flags: review?(drno) → review+
Comment on attachment 8656193 [details]
MozReview Request: Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.

Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.
Attachment #8656193 - Attachment description: MozReview Request: Bug 1186590: Enable interface prioritizer on all platforms. → MozReview Request: Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.
Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.
Comment on attachment 8657306 [details]
MozReview Request: Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.

Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.
Attachment #8657306 - Flags: review?(drno)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8657306 [details]
MozReview Request: Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.

https://reviewboard.mozilla.org/r/18353/#review16825

LGTM

::: media/mtransport/nrinterfaceprioritizer.cpp:119
(Diff revision 2)
> +  static const std::vector<std::string>& interface_preference_list()
> +  {
> +    static std::vector<std::string> list(build_interface_preference_list());
> +    return list;
> +  }
> +
> +  static std::vector<std::string> build_interface_preference_list()
> +  {
> +    std::vector<std::string> result;
> +    result.push_back("rl0");
> +    result.push_back("wi0");
> +    result.push_back("en0");
> +    result.push_back("en1");
> +    result.push_back("en2");
> +    result.push_back("en3");
> +    result.push_back("eth0");
> +    result.push_back("eth1");
> +    result.push_back("eth2");
> +    result.push_back("em1");
> +    result.push_back("em0");
> +    result.push_back("ppp");
> +    result.push_back("ppp0");
> +    result.push_back("vmnet1");
> +    result.push_back("vmnet0");
> +    result.push_back("vmnet3");
> +    result.push_back("vmnet4");
> +    result.push_back("vmnet5");
> +    result.push_back("vmnet6");
> +    result.push_back("vmnet7");
> +    result.push_back("vmnet8");
> +    result.push_back("virbr0");
> +    result.push_back("wlan0");
> +    result.push_back("lo0");
> +    return result;
> +  }

Is there a reason you build the list every time this gets called?
Couldn't we simply store the list as private variable initialized in Init()?
Attachment #8657306 - Flags: review?(drno) → review+
https://reviewboard.mozilla.org/r/18353/#review16825

> Is there a reason you build the list every time this gets called?
> Couldn't we simply store the list as private variable initialized in Init()?

I did this as a function-scoped static to avoid doing it for every LocalAddress that is created, and also to avoid doing it if webrtc is never used. The builder function is just there to help interface_preference_list build the list the first time it is called.
Check back on try
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c197bcf7d9a5
https://hg.mozilla.org/mozilla-central/rev/5745780785dd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.