Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083803002:
Fix IPv6 support issue. (Closed)
DescriptionFix an issue in IPv6 support.
When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of checking the address family of the local socket against the remote candidate.
BUG=5871
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/f4ae6dc76382309469049dab3b40465289dfa27a
Cr-Commit-Position: refs/heads/master@{#13269}
Patch Set 1 : . #
Total comments: 34
Patch Set 2 : address comments #Patch Set 3 : . #Patch Set 4 : Merge branch 'master' into fix_ipv6_support #
Total comments: 3
Messages
Total messages: 39 (18 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix IPv6 support issue. 1. When resolving IP addresses, using AP_UNSPEC as the address family hint so that it also return IPv6 addresses if they are available. 2. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of check the address family of the local socket against the remote candidate. BUG= ========== to ========== Fix IPv6 support issue. 1. When resolving IP addresses, using AP_UNSPEC as the address family hint so that it also return IPv6 addresses if they are available. 2. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of check the address family of the local socket against the remote candidate. BUG=5871 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
honghaiz@webrtc.org changed reviewers: + andresp@webrtc.org, deadbeef@webrtc.org
PTAL.
Patchset #1 (id:80001) has been deleted
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; Can you make a separate CL for this in case we have to rollback either half of this CL? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:481: const Candidate& remote_candidate) const { This can just be a static function. It doesn't need to be a method of the class. And a more proper name would be "CandidateAddressesCompatible". But you may consider just inlining it since you only call it once. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:592: CreateTurnPort(kTurnUsername, kTurnPassword, cricket::ProtocolAddress( Can we un-DISABLED the test? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:986: // Tests that the local and remote candidate address family should match when family = families https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:987: // a connection is created. Specially if a turn port has an IPv6 address, its Specially -> Specifically, ? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1000: // Create an IPv4 port. Its candidate will match the Turn candidate. TURN https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1008: // Create an IPv6 UDP port. Its candidate won't match the Turn candidate. TURN
On 2016/06/21 05:26:54, pthatcher1 wrote: > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc > File webrtc/base/nethelpers.cc (right): > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... > webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; > Can you make a separate CL for this in case we have to rollback either half of > this CL? > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport.cc:481: const Candidate& remote_candidate) const { > This can just be a static function. It doesn't need to be a method of the > class. > > And a more proper name would be "CandidateAddressesCompatible". > > But you may consider just inlining it since you only call it once. > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > File webrtc/p2p/base/turnport_unittest.cc (right): > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport_unittest.cc:592: CreateTurnPort(kTurnUsername, > kTurnPassword, cricket::ProtocolAddress( > Can we un-DISABLED the test? > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport_unittest.cc:986: // Tests that the local and remote > candidate address family should match when > family = families > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport_unittest.cc:987: // a connection is created. Specially > if a turn port has an IPv6 address, its > Specially -> Specifically, > > ? > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport_unittest.cc:1000: // Create an IPv4 port. Its candidate > will match the Turn candidate. > TURN > > https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... > webrtc/p2p/base/turnport_unittest.cc:1008: // Create an IPv6 UDP port. Its > candidate won't match the Turn candidate. > TURN
https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; On 2016/06/21 05:26:54, pthatcher1 wrote: > Can you make a separate CL for this in case we have to rollback either half of > this CL? I am also not sure if this AF_UNSPEC is that correct. Documentation on behaviour is scarce. I guess since we trigger the resolve from the turnport itself, we do either AF_INET or AF_INET6 depending on the turnport local address family. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:35: class TurnPort : public Port { I noticed every caller of Port::IsCompatibleAddress comes from a place where the port type (udp/tcp/turnport) is known. As so I don't think Port should declare it at all. And if port wants to keep it declared, then turnport should implement it properly.
https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:987: // a connection is created. Specially if a turn port has an IPv6 address, its Can you capitalize TURN consistently here and below? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:991: rtc::ScopedFakeClock clock; I don't know why the fake clock is necessary here. Does this test deal with any timeouts? It doesn't hurt though; I'm just curious. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1001: CreateUdpPort(); Why do you need to create actual remote ports? Couldn't you call CreateConnection with a fake candidate that the test constructs? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1006: EXPECT_TRUE(conn != nullptr); EXPECT_NE(nullptr, conn)? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1014: EXPECT_TRUE(conn == nullptr); EXPECT_EQ(nullptr, conn)?
Patchset #2 (id:120001) has been deleted
PTAL. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; On 2016/06/21 11:36:10, andresp wrote: > On 2016/06/21 05:26:54, pthatcher1 wrote: > > Can you make a separate CL for this in case we have to rollback either half of > > this CL? > > I am also not sure if this AF_UNSPEC is that correct. Documentation on behaviour > is scarce. > > I guess since we trigger the resolve from the turnport itself, we do either > AF_INET or AF_INET6 depending on the turnport local address family. Will do it in a separate CL. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:481: const Candidate& remote_candidate) const { On 2016/06/21 05:26:54, pthatcher1 wrote: > This can just be a static function. It doesn't need to be a method of the > class. > > And a more proper name would be "CandidateAddressesCompatible". > > But you may consider just inlining it since you only call it once. Just inlined it. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:35: class TurnPort : public Port { On 2016/06/21 11:36:10, andresp wrote: > I noticed every caller of Port::IsCompatibleAddress comes from a place where the > port type (udp/tcp/turnport) is known. As so I don't think Port should declare > it at all. > > And if port wants to keep it declared, then turnport should implement it > properly. I think IsCompatibleAddress is useful in matching the local port IP address family vs the family type of the internal address (basically the known public address used by a client to send TURN allocate request) of the turn server. It is also useful when creating a connection with a remote client for non-TURN port. It is a single implementation used by different type of Port. The only exception is that when creating connections in TurnPort, we should not use it. We should check local candidate family vs the remote candidate family. So it makes sense to me to keep its implementation at Port and use it in subclasses of Port. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:592: CreateTurnPort(kTurnUsername, kTurnPassword, cricket::ProtocolAddress( On 2016/06/21 05:26:54, pthatcher1 wrote: > Can we un-DISABLED the test? Will do this in a separate CL, as it was typically because it is flaky, and I would prefer this CL being revert due to re-enabling a flaky test. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:986: // Tests that the local and remote candidate address family should match when On 2016/06/21 05:26:54, pthatcher1 wrote: > family = families Done. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:987: // a connection is created. Specially if a turn port has an IPv6 address, its On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > Can you capitalize TURN consistently here and below? Done. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:991: rtc::ScopedFakeClock clock; On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > I don't know why the fake clock is necessary here. Does this test deal with any > timeouts? It doesn't hurt though; I'm just curious. If we have to wait for some state change with a timeout, it has a chance to be flaky with real clock, right? Although the chance is pretty small here. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1000: // Create an IPv4 port. Its candidate will match the Turn candidate. On 2016/06/21 05:26:54, pthatcher1 wrote: > TURN Done. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1001: CreateUdpPort(); On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > Why do you need to create actual remote ports? Couldn't you call > CreateConnection with a fake candidate that the test constructs? Done. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1006: EXPECT_TRUE(conn != nullptr); On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > EXPECT_NE(nullptr, conn)? Done. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1008: // Create an IPv6 UDP port. Its candidate won't match the Turn candidate. On 2016/06/21 05:26:54, pthatcher1 wrote: > TURN Done. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:1014: EXPECT_TRUE(conn == nullptr); On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > EXPECT_EQ(nullptr, conn)? Done.
https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; Would it make sense to use the family already passed into this function? hints.ai_family = family. The the caller has control by setting the family of address passed into Start. As for AF_UNSPEC, it supposedly works for Linux: http://man7.org/linux/man-pages/man3/getaddrinfo.3.html And for Android, the Bionic code makes two queries: https://android.googlesource.com/platform/bionic/+/7e0bfb511e85834d7c6cb96312... On Windows, it says INET+INET6: https://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx On Mac, it's more complicated: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... But it says " To best support NAT64 networks, it is recommended to resolve all IP address literals with ai_family set to PF_UNSPEC and ai_flags set to AI_DEFAULT." So it seems to be roughly supported. The only one I'm worried about is Mac/iOS. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:35: class TurnPort : public Port { On 2016/06/21 11:36:10, andresp wrote: > I noticed every caller of Port::IsCompatibleAddress comes from a place where the > port type (udp/tcp/turnport) is known. As so I don't think Port should declare > it at all. > > And if port wants to keep it declared, then turnport should implement it > properly. We'd have to duplicate the following logic between tcpport.cc and stunport.cc, but maybe that's OK. if (family == AF_INET6 && (IPIsLinkLocal(ip()) != IPIsLinkLocal(addr.ipaddr()))) { return false; }
https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:991: rtc::ScopedFakeClock clock; On 2016/06/21 18:36:06, honghaiz3 wrote: > On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > > I don't know why the fake clock is necessary here. Does this test deal with > any > > timeouts? It doesn't hurt though; I'm just curious. > > If we have to wait for some state change with a timeout, it has a chance to be > flaky with real clock, right? Although the chance is pretty small here. The general solution in that case is to use a longer timeout. I would suggest changing kTimeout to 10 seconds or more. But if there is a race condition when using a real clock, then the fake clock would make sense.
PTAL. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; On 2016/06/21 18:50:17, pthatcher1 wrote: > Would it make sense to use the family already passed into this function? > > hints.ai_family = family. > > The the caller has control by setting the family of address passed into Start. > > > As for AF_UNSPEC, it supposedly works for Linux: > > http://man7.org/linux/man-pages/man3/getaddrinfo.3.html > > And for Android, the Bionic code makes two queries: > > https://android.googlesource.com/platform/bionic/+/7e0bfb511e85834d7c6cb96312... > > On Windows, it says INET+INET6: > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx > > On Mac, it's more complicated: > > https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... > > But it says " To best support NAT64 networks, it is recommended to resolve > all IP address literals with ai_family set to PF_UNSPEC and ai_flags set to > AI_DEFAULT." > > > So it seems to be roughly supported. The only one I'm worried about is Mac/iOS. > > > > I think it is safer to pass in the family. But the one currently passed is the address family of the named address. I do not think that will have a family type. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:35: class TurnPort : public Port { On 2016/06/21 18:50:18, pthatcher1 wrote: > On 2016/06/21 11:36:10, andresp wrote: > > I noticed every caller of Port::IsCompatibleAddress comes from a place where > the > > port type (udp/tcp/turnport) is known. As so I don't think Port should declare > > it at all. > > > > And if port wants to keep it declared, then turnport should implement it > > properly. > > We'd have to duplicate the following logic between tcpport.cc and stunport.cc, > but maybe that's OK. > > if (family == AF_INET6 && > (IPIsLinkLocal(ip()) != IPIsLinkLocal(addr.ipaddr()))) { > return false; > } For turn port, the generated candidate family is always AF_INET now. So I think it is not necessary to add that check (Even in the future the generated candidate family may be ipv6, it should still be fine not having this check. You mentioned stunport.cc. Do we need to do anything special about stunport here? https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:991: rtc::ScopedFakeClock clock; On 2016/06/21 19:26:48, Taylor Brandstetter wrote: > On 2016/06/21 18:36:06, honghaiz3 wrote: > > On 2016/06/21 17:20:31, Taylor Brandstetter wrote: > > > I don't know why the fake clock is necessary here. Does this test deal with > > any > > > timeouts? It doesn't hurt though; I'm just curious. > > > > If we have to wait for some state change with a timeout, it has a chance to be > > flaky with real clock, right? Although the chance is pretty small here. > > The general solution in that case is to use a longer timeout. I would suggest > changing kTimeout to 10 seconds or more. But if there is a race condition when > using a real clock, then the fake clock would make sense. Removed the fake clock but keep the kTimeout. I think we may consider changing it to 10 second if we see this or similar tests are flaky because 1 second is not enough.
https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.cc File webrtc/base/nethelpers.cc (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/base/nethelpers.c... webrtc/base/nethelpers.cc:48: hints.ai_family = AF_UNSPEC; On 2016/06/21 20:06:49, honghaiz3 wrote: > On 2016/06/21 18:50:17, pthatcher1 wrote: > > Would it make sense to use the family already passed into this function? > > > > hints.ai_family = family. > > > > The the caller has control by setting the family of address passed into Start. > > > > > > As for AF_UNSPEC, it supposedly works for Linux: > > > > http://man7.org/linux/man-pages/man3/getaddrinfo.3.html > > > > And for Android, the Bionic code makes two queries: > > > > > https://android.googlesource.com/platform/bionic/+/7e0bfb511e85834d7c6cb96312... > > > > On Windows, it says INET+INET6: > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx > > > > On Mac, it's more complicated: > > > > > https://developer.apple.com/legacy/library/documentation/Darwin/Reference/Man... > > > > But it says " To best support NAT64 networks, it is recommended to resolve > > all IP address literals with ai_family set to PF_UNSPEC and ai_flags set to > > AI_DEFAULT." > > > > > > So it seems to be roughly supported. The only one I'm worried about is > Mac/iOS. > > > > > > > > > I think it is safer to pass in the family. But the one currently passed is the > address family of the named address. I do not think that will have a family > type. Yeah, the caller would need to call address.SetFamily, which doesn't exist. Perhaps a better option is to require AsyncResolver::Start to pass in an extra parameter that is the family that should be queried. Then the call could choose INET, INET6, or UNSPEC. But then what do we do with UNSPEC? I think for Android, Linux, and Windows, it's safe to use it, according to the docs. And even Mac seems to be clear that they support it, even if in a slightly different way. https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2083803002/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.h:35: class TurnPort : public Port { On 2016/06/21 20:06:49, honghaiz3 wrote: > On 2016/06/21 18:50:18, pthatcher1 wrote: > > On 2016/06/21 11:36:10, andresp wrote: > > > I noticed every caller of Port::IsCompatibleAddress comes from a place where > > the > > > port type (udp/tcp/turnport) is known. As so I don't think Port should > declare > > > it at all. > > > > > > And if port wants to keep it declared, then turnport should implement it > > > properly. > > > > We'd have to duplicate the following logic between tcpport.cc and stunport.cc, > > but maybe that's OK. > > > > if (family == AF_INET6 && > > (IPIsLinkLocal(ip()) != IPIsLinkLocal(addr.ipaddr()))) { > > return false; > > } > > For turn port, the generated candidate family is always AF_INET now. So I think > it is not necessary to add that check (Even in the future the generated > candidate family may be ipv6, it should still be fine not having this check. > > You mentioned stunport.cc. Do we need to do anything special about stunport > here? StunPort and TcpPort both use IsCompatibleAddress and use that logic, so if we remove IsCompatiableAddress (as Andre suggested), we'd need to duplicate that logic in both of those places. In other words, I'm not sure it's worth it to remove IsCompatibleAddress.
lgtm (Assuming you address Taylor's comments in the unit test)
Description was changed from ========== Fix IPv6 support issue. 1. When resolving IP addresses, using AP_UNSPEC as the address family hint so that it also return IPv6 addresses if they are available. 2. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of check the address family of the local socket against the remote candidate. BUG=5871 ========== to ========== Fix an issue in IPv6 support. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of checking the address family of the local socket against the remote candidate. BUG=5871 ==========
lgtm
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083803002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14381)
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2083803002/#ps180001 (title: "Merge branch 'master' into fix_ipv6_support")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2083803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14368)
Description was changed from ========== Fix an issue in IPv6 support. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of checking the address family of the local socket against the remote candidate. BUG=5871 ========== to ========== Fix an issue in IPv6 support. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of checking the address family of the local socket against the remote candidate. BUG=5871 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f4ae6dc76382309469049dab3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) manually as f4ae6dc76382309469049dab3b40465289dfa27a (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Fix an issue in IPv6 support. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of checking the address family of the local socket against the remote candidate. BUG=5871 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f4ae6dc76382309469049dab3... ========== to ========== Fix an issue in IPv6 support. When creating connections on turn port, check whether the local and remote candidates have the same IP address family, instead of checking the address family of the local socket against the remote candidate. BUG=5871 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/f4ae6dc76382309469049dab3b40465289dfa27a Cr-Commit-Position: refs/heads/master@{#13269} ==========
Message was sent while issue was closed.
juberti@google.com changed reviewers: + juberti@google.com
Message was sent while issue was closed.
lgtm Nice catch https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:452: // A TURN port will have two candiates, STUN and TURN. STUN may not Is this still the case? I was under the impression that the STUN candidate is ignored, so this comment is no longer accurate.
Message was sent while issue was closed.
https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:452: // A TURN port will have two candiates, STUN and TURN. STUN may not On 2016/06/23 14:51:18, juberti wrote: > Is this still the case? I was under the impression that the STUN candidate is > ignored, so this comment is no longer accurate. You may be right. I think even if using a TURN server as a STUN server, we create a separate UdpPort object that gathers the STUN candidate.
Message was sent while issue was closed.
https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:452: // A TURN port will have two candiates, STUN and TURN. STUN may not On 2016/06/23 17:09:53, Taylor Brandstetter wrote: > On 2016/06/23 14:51:18, juberti wrote: > > Is this still the case? I was under the impression that the STUN candidate is > > ignored, so this comment is no longer accurate. > > You may be right. I think even if using a TURN server as a STUN server, we > create a separate UdpPort object that gathers the STUN candidate. Agree. I think the TURN port is not collecting STUN candidates. Need to fix this comment.
Message was sent while issue was closed.
https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2083803002/diff/180001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:452: // A TURN port will have two candiates, STUN and TURN. STUN may not On 2016/06/23 17:09:53, Taylor Brandstetter wrote: > On 2016/06/23 14:51:18, juberti wrote: > > Is this still the case? I was under the impression that the STUN candidate is > > ignored, so this comment is no longer accurate. > > You may be right. I think even if using a TURN server as a STUN server, we > create a separate UdpPort object that gathers the STUN candidate. Agree. I think the TURN port is not collecting STUN candidates. Need to fix this comment. |
