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
Assignee

Comment 1

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

Comment 3

4 years ago
It will sort addrs from the same interface arbitrarily, yes, but it still assigns a unique priority to everything.
Assignee

Comment 4

4 years ago
Bug 1186590: Enable interface prioritizer on all platforms.
Assignee

Comment 5

4 years ago
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

Updated

4 years ago
Assignee: nobody → docfaraday
Assignee

Comment 7

4 years ago
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+
Assignee

Comment 9

4 years ago
Ok, the next thing I will do is teach nrinterfaceprioritizer some of the stuff here:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nricectx.cpp#407

So the fallback logic here works a little better:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nrinterfaceprioritizer.cpp?case=true&from=nrinterfaceprioritizer.cpp#64
Assignee

Comment 10

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

Comment 11

4 years ago
Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.
Assignee

Comment 12

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.

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+
Assignee

Comment 14

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

Comment 15

4 years ago
Check back on try
Flags: needinfo?(docfaraday)
Assignee

Updated

4 years ago
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.