1186590 - Duplicated priorities for IPv6 ICE candidates

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

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

Comment 1

[Byron Campen [:bwc]]

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.

Comment 2

[Nils Ohlmeier [:drno]]

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.

Comment 3

[Byron Campen [:bwc]]

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

Comment 4

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1186590 - Part 1: Enable interface prioritizer on all platforms.

Bug 1186590: Enable interface prioritizer on all platforms.

Comment 5

[Byron Campen [:bwc]]

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.

Comment 6

[Nils Ohlmeier [:drno]]

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.

Comment 7

[Byron Campen [:bwc]]

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.

Comment 8

[Nils Ohlmeier [:drno]]

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

Comment 9

[Byron Campen [:bwc]]

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

Comment 10

[Byron Campen [:bwc]]

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.

Comment 11

[Byron Campen [:bwc]]

Attachment: 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.

Comment 12

[Byron Campen [:bwc]]

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.

Comment 13

[Nils Ohlmeier [:drno]]

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()?

Comment 14

[Byron Campen [:bwc]]

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.

Comment 15

[Byron Campen [:bwc]]

Check back on try

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c197bcf7d9a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5745780785dd

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c197bcf7d9a5
https://hg.mozilla.org/mozilla-central/rev/5745780785dd