797262 - Update nICEr and nr_socket_prsock to do IPv6

Status: RESOLVED FIXED

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

Comment 2

[Simon]

http://code.google.com/p/webrtc/issues/detail?id=1406 has been updated.
It looks like they have added ipv6 support and will be shipping it soon in Chrome behind a feature flag, before enabling it by default.

Comment 3

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull review -r c79da278963d8a1ab1cd363ed88cb4ea51738291

Comment 4

[Byron Campen [:bwc]]

Work in progress passes ice_unittest, but I doubt it actually works correctly (I have not taught it to not pair IPV4 with IPV6, for example). Just checkpointing for now. Haven't started on IPV6 gathering for windows, either.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 0be03dc2cf73357ecfe76291c7489507f8629392 https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 8e1b1e8c371fbc46ee68bbb48471b1f608ad2909 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 7a18d26df3f40d7ac0a338de70d73b1dd70ea15c https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 7d1cf50efcac59469d553c9b2c405e2232d5b8d0 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 0631740e284c576357a7701798f51f47260df0f6 https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 25e8c8e9de6d5538273ff5ed6bd41127f19fbf49 https://reviewboard-hg.mozilla.org/gecko/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r fae4aadf477a194bcd203e90f96949b4ad9549b9 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: (WIP) IPV6 support in nICEr.

Pull down this commit:

hg pull -r 7d6a8b3b4522e66f1530c45ea9cb71a13dde9600 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: IPV6 support for webrtc

Pull down this commit:

hg pull -r 6101a4b667dd0d6c26738d363a7dcfbca45991f2 https://reviewboard-hg.mozilla.org/gecko/

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: IPV6 support for webrtc

Pull down this commit:

hg pull -r 302a10afb699deaf6e3dc5189c0bfd07cebbbe33 https://reviewboard-hg.mozilla.org/gecko/

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: IPV6 support for webrtc

Pull down this commit:

hg pull -r bd0eda11a32d13c80e8a1cc689f7caa910a1c5b8 https://reviewboard-hg.mozilla.org/gecko/

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56023b911424

Comment 17

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: IPV6 support for webrtc

Pull down this commit:

hg pull -r 90b94d4649bec8f350f2e06d77ea21adce89c9c6 https://reviewboard-hg.mozilla.org/gecko/

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

Unrotted, and fixed some default candidate stuff.

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

/r/6185 - Bug 797262: IPV6 support for webrtc

Pull down this commit:

hg pull -r 6e63d2aa9a409246852994ce73f99272838351de https://reviewboard-hg.mozilla.org/gecko/

Comment 20

[Nils Ohlmeier [:drno]]

Comment on attachment 8584189 [details]
MozReview Request: bz://797262/bwc

https://reviewboard.mozilla.org/r/6183/#review7723

::: media/mtransport/nr_socket_prsock.cpp:430
(Diff revision 14)
> -        r_log(LOG_GENERIC,LOG_CRIT,"Couldn't create socket");
> +        r_log(LOG_GENERIC,LOG_CRIT,"Couldn't create socket, family=%d, err=%d",

"create UDP socket"?

::: media/mtransport/nr_socket_prsock.cpp:437
(Diff revision 14)
> -        r_log(LOG_GENERIC,LOG_CRIT,"Couldn't create socket");
> +        r_log(LOG_GENERIC,LOG_CRIT,"Couldn't create socket, family=%d, err=%d",

"create TCP socket"?

::: media/mtransport/nriceresolver.cpp:167
(Diff revision 14)
> +  switch(resource->address_family) {
> +    case AF_INET:
> +      resolve_flags |= nsIDNSService::RESOLVE_DISABLE_IPV6;
> +      break;
> +    case AF_INET6:
> +      resolve_flags |= nsIDNSService::RESOLVE_DISABLE_IPV4;
> +      break;
> +    default:
> +      ABORT(R_BAD_ARGS);
> +  }
> +
>    if (NS_FAILED(dns_->AsyncResolve(nsAutoCString(resource->domain_name),
> -                                   nsIDNSService::RESOLVE_DISABLE_IPV6, pr,
> +                                   resolve_flags, pr,

If I'm not mistaken you are turning which IP protocol version you use to talk to your DNS server into which kind of DNS records A vs AAAA you are looking for. That sounds wrong to me.

https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNSService.idl#154

If we would restrict what records we are looking for it would need to based on what type of IP connectivity we have.

::: media/mtransport/nrinterfaceprioritizer.cpp:59
(Diff revision 14)
> +    // Prefer IPV6 over IPV4

Hmm I'm not convinced this is a good idea. But I have no better alternative, which is easy to implement.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

> ::: media/mtransport/nrinterfaceprioritizer.cpp:59
> (Diff revision 14)
> > +    // Prefer IPV6 over IPV4
> 
> Hmm I'm not convinced this is a good idea. But I have no better alternative,
> which is easy to implement.

a) what does chrome do?
b) what does Necko/https do?

Comment 22

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/6183/#review7731

> "create UDP socket"?

Sure.

> "create TCP socket"?

Sure.

> If I'm not mistaken you are turning which IP protocol version you use to talk to your DNS server into which kind of DNS records A vs AAAA you are looking for. That sounds wrong to me.
> 
> https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNSService.idl#154
> 
> If we would restrict what records we are looking for it would need to based on what type of IP connectivity we have.

|address_family| refers to what kind of addresses we're trying to look up; since every STUN context (which is bound to exactly one IP address) has its own lookup, each lookup will be either for V4 or V6, but never both.

> Hmm I'm not convinced this is a good idea. But I have no better alternative, which is easy to implement.

I've looked at this many different ways, and it seems like it almost doesn't matter what we do. Here's my thinking (bear in mind that candidate type takes precedence over interface, so this will only matter for comparisons of the same type):

V4 host vs V6 host: Preferring V6 in this case will almost certainly do no harm since it's all local network.
V4 srflx vs V6 srflx: This only happens if you have a NAT that is port-mapping V6 addys. Probably will happen, hopefully is not common.
V4 relay vs V6 relay: Relatively rare (you need to have a NAT that is port mapping V6 addys or blocking UDP in order to need a V6 relay in the first place, _and_ you need a TURN server that has a V6 addy)

FWIW, Chrome prefers V6 addys.

Comment 23

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/6183/#review7735

::: media/mtransport/test/stunserver.cpp:238
(Diff revision 14)
>    nr_local_addr addrs[100];
>    int addr_ct;
>    int r;
> +  int i;
>  
>    r = nr_stun_find_local_addresses(addrs, 100, &addr_ct);

Can we move the 100 into one const?

Comment 24

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/6183/#review7759

> Can we move the 100 into one const?

Sure, why not.

Comment 25

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/6183/#review8981

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1157
(Diff revision 14)
> +        if (!best_cand) {
> +          best_cand = cand;
> +        }
> +        else {
> +          if (best_cand->type < cand->type) {
> +            best_cand = cand;
> +          } else if (best_cand->type == cand->type) {
> +            if (best_cand->priority < cand->priority)
> +              best_cand = cand;
> +          }
> +        }

I think this should be:
if ()
else if ()
else if ()

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:1163
(Diff revision 14)
> +          } else if (best_cand->type == cand->type) {
> +            if (best_cand->priority < cand->priority)
> +              best_cand = cand;

As there is no else part here I would merge the second if with AND into the outer if statement.

Comment 26

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/6183/#review9061

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c:243
(Diff revision 14)
> -    /* We have the component. Now find the "best" candidate, making
> -       use of the fact that more "reliable" candidate types have
> -       higher numbers. So, we sort by type and then priority within
> +    /* If there aren't any IPV4 candidates, try IPV6 */
> +    if((r=nr_ice_component_get_default_candidate(comp, candp, NR_IPV4)) &&
> +       (r=nr_ice_component_get_default_candidate(comp, candp, NR_IPV6))) {

Isn't there a reason why this is the opposite order then the rest?
Everywhere else IPv6 seem to get preferred over IPv4.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:216
(Diff revision 14)
> -int nr_ip4_str_port_to_transport_addr(const char *ip4, UINT2 port, int protocol, nr_transport_addr *addr)
> +int nr_str_port_to_transport_addr(const char *ip, UINT2 port, int protocol, nr_transport_addr *addr_out)
>    {
>      int r,_status;
> -    in_addr_t ip_addr;
> +    struct in_addr addr;
> +    struct in6_addr addr6;
>  
> -    ip_addr=inet_addr(ip4);
> -    if (ip_addr == INADDR_NONE)
> +    if (inet_pton(AF_INET, ip, &addr) == 1) {
> +      if(r=nr_ip4_port_to_transport_addr(ntohl(addr.s_addr),port,IPPROTO_UDP,addr_out))
> +        ABORT(r);
> +    } else if (inet_pton(AF_INET6, ip, &addr6) == 1) {
> +      if(r=nr_ip6_port_to_transport_addr(&addr6,port,IPPROTO_UDP,addr_out))

The input parameter protocol is unused and you over-write it with IPPROTO_UDP for both *_port_to_trnasport_addr() calls?!
This smell like trouble for ICE TCP :-)

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:222
(Diff revision 14)
> -    ip_addr=inet_addr(ip4);
> -    if (ip_addr == INADDR_NONE)
> +    if (inet_pton(AF_INET, ip, &addr) == 1) {
> +      if(r=nr_ip4_port_to_transport_addr(ntohl(addr.s_addr),port,IPPROTO_UDP,addr_out))
> +        ABORT(r);
> +    } else if (inet_pton(AF_INET6, ip, &addr6) == 1) {

Not a big fan of this try and error approach, but I don't have a better solution either.

But to stay consistent can we first try IPv6, before IPv4?

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:223
(Diff revision 14)
> -    if (ip_addr == INADDR_NONE)
> +      if(r=nr_ip4_port_to_transport_addr(ntohl(addr.s_addr),port,IPPROTO_UDP,addr_out))

There is an extra round of network-to-host and then host-to-network inside the nr_ip4_port_to_transport_addr() function. I'm wondering if all callee's are doing network-to-host for the input argument we could avoid the double conversion. But that is probably an optional optimization.

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c:396
(Diff revision 14)
> +int nr_transport_addr_is_link_local(nr_transport_addr *addr)
> +  {
> +    if(addr->ip_version == NR_IPV6){
> +      UINT4* addrTop = (UINT4*)(addr->u.addr6.sin6_addr.s6_addr);
> +      return ((*addrTop & htonl(0xFFC00000)) == htonl(0xFE800000));
> +    }
> +
> +    return(0);

As link local is a pure IPv6 concept wouldn't it make more sense to return an error if this gets called with a non IPv6 addr?

::: media/mtransport/third_party/nICEr/src/net/transport_addr_reg.c:149
(Diff revision 14)
> +          char address[46];

Isn't that length defined somewhere?
If not I think a comment should explain how you got to 46.

Comment 27

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/6183/#review9129

> Isn't there a reason why this is the opposite order then the rest?
> Everywhere else IPv6 seem to get preferred over IPv4.

So here we're selecting the one candidate that is most likely to work for endpoints that don't support ICE. Fewer things support IPV6 than IPV4, so we prefer V4 very strongly here (we also prefer relay candidates here).

> The input parameter protocol is unused and you over-write it with IPPROTO_UDP for both *_port_to_trnasport_addr() calls?!
> This smell like trouble for ICE TCP :-)

I think I can proactively fix this. There's gonna be some bitrot, I'm sure.

> As link local is a pure IPv6 concept wouldn't it make more sense to return an error if this gets called with a non IPv6 addr?

I can add an assert.

> Isn't that length defined somewhere?
> If not I think a comment should explain how you got to 46.

I tried using INET6_ADDRSTRLEN (which is defined to 46 on unixy platforms), but windows didn't have it. I can add a comment.

> Not a big fan of this try and error approach, but I don't have a better solution either.
> 
> But to stay consistent can we first try IPv6, before IPv4?

Since inet_pton is going to be much more expensive for V6 than V4, I did the V4 check first.

Comment 28

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Bug 797262: IPV6 support for webrtc

Comment 29

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Rebased and incorporated feedback.

Comment 30

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/6183/#review9079

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:62
(Diff revision 14)
> -            nextifm = (struct if_msghdr *)next;
> -
> -            if (nextifm->ifm_type != RTM_NEWADDR)
> +#ifdef ANDROID
> +/* Work around an Android NDK < r8c bug */
> +#undef __unused

How about moving this into the new android specific headers?

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:249
(Diff revision 14)
> -   int _status;
> -   int s = socket( AF_INET, SOCK_DGRAM, 0 );
> +  struct ifaddrs* if_addrs=NULL;
> +  struct ifaddrs* if_addr;

It would help if these names better express what they are for, e.g. if_addrs_head or something like that.

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:273
(Diff revision 14)
> -#else
> +          int s = socket( AF_INET, SOCK_DGRAM, 0 );

Remove extra space

::: media/mtransport/third_party/nICEr/src/stun/addrs.c:286
(Diff revision 14)
> -                connection */
> +              *                 connection */

Fix spaces/indentation.

::: media/mtransport/third_party/nrappkit/src/util/util.h:70
(Diff revision 14)
> +int inet_pton(int af, const char *src, void *dst);

The implementation seems to be in a block of:
#if defined(USE_OWN_INET_NTOP) || defined(WIN32)

So I guess this could cause compiler problems, or?

::: media/webrtc/signaling/src/sdp/SdpMediaSection.h:289
(Diff revision 14)
> +    if (mAddr.find('.') != std::string::npos) {
> +      mAddrType = sdp::kIPv4;
> +    }
> +
> +    if (mAddr.find(':') != std::string::npos) {
> +      mAddrType = sdp::kIPv6;
> +    }

If I feed this 1.1.1.1:9 it will become mAddrType kIPv6.
I would suggest an 'else if' and a assert in the 'else' branch to be safe.

Comment 31

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/6183/#review9231

> How about moving this into the new android specific headers?

I think addrs.c is the file that needs this, so it makes sense to keep it here.

> It would help if these names better express what they are for, e.g. if_addrs_head or something like that.

Sure.

> The implementation seems to be in a block of:
> #if defined(USE_OWN_INET_NTOP) || defined(WIN32)
> 
> So I guess this could cause compiler problems, or?

Yeah, we should only enable that impl on WIN32.

> If I feed this 1.1.1.1:9 it will become mAddrType kIPv6.
> I would suggest an 'else if' and a assert in the 'else' branch to be safe.

Actually, there's something worse here. feed::beef:192.168.0.1 (dotted quad notation) will parse as IPV4. Will fix.

Comment 32

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Bug 797262: IPV6 support for webrtc

Comment 33

[Nils Ohlmeier [:drno]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

https://reviewboard.mozilla.org/r/6185/#review9471

Ship It!

Comment 34

[Byron Campen [:bwc]]

I'm going to wait until early 42 to land this.

Comment 35

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Bug 797262: IPV6 support for webrtc r=drno

Comment 36

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Unrot, carry forward r=drno

Comment 37

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Bug 797262: IPV6 support for webrtc r=drno

Comment 38

[Byron Campen [:bwc]]

Try for unrot:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec6683a4c5c

Comment 39

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Carry forward r=drno

Comment 40

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Bug 797262: IPV6 support for webrtc r=drno

Comment 41

[Byron Campen [:bwc]]

Comment on attachment 8616155 [details]
MozReview Request: Bug 797262: IPV6 support for webrtc r=drno

Fixing a crash in ice_unittest when no IPV6 addys are present.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ab1ce9608e

Comment 42

[Byron Campen [:bwc]]

Check back on retriggers

Comment 43

[Byron Campen [:bwc]]

Still waiting on B2G emulator debug to build so our mochitests can run there. Infra is having a hard time with this...

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb822545e34

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfb822545e34

Comment 46

[rajkumaradass]

Is there a constraint as part of this fix to disable IPv6 gathering by default, as we are running into ICE failure in v42 and v43

Comment 47

[Randell Jesup [:jesup] (needinfo me)]

If you're seeing ICE failures, please file a bug (in the same component (Core::WebRTC: Networking), and you can mark it as "Blocking" this bug).  I don't think there's a constraint to disable IPV6

Comment 48

[Byron Campen [:bwc]]

(In reply to rajkumaradass from comment #46)
> Is there a constraint as part of this fix to disable IPv6 gathering by
> default, as we are running into ICE failure in v42 and v43

   There is no pref to disable IPv6. It is possible to detect/strip out IPv6 candidates from content JS, if that helps. If it looks like it might be a bug on our end (even a little bit), please open a bug and we can work through it.