Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206713004:
Do not create incompatible TurnPort if the server address family is known. (Closed)
DescriptionDo not create incompatible TurnPort if the server address family is known.
In the existing code, if the server address and the local IP family does not
match, we still create a TurnPort and destroy it later.
Instead, we could avoid creating it at the beginning.
This does not affect the client behavior except for the port creation.
BUG=
R=deadbeef@webrtc.org, pthatcher@webrtc.org, zhihuang@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/3d31bd65cf5b582b8a9667c0abbf28cd77c5873d
Patch Set 1 : . #
Total comments: 6
Patch Set 2 : . #Messages
Total messages: 19 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Do not create incompatible TurnPort if the server address family is known. In the existing code, if the server address and the local IP family does not match, we still create a TurnPort and destroy it later. Instead, we could avoid creating it at the beginning. BUG= ========== to ========== Do not create incompatible TurnPort if the server address family is known. In the existing code, if the server address and the local IP family does not match, we still create a TurnPort and destroy it later. Instead, we could avoid creating it at the beginning. This does not affect the client behavior except for the port creation. BUG= ==========
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, zhihuang@webrtc.org
PTAL
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
unit test? https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:1303: if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) { What happens if local_ip_family is AF_UNSPEC and server_ip_family isn't? Is it possible for GetBestIP() to return an address with AF_UNSPEC?
I don't know if this is something that can (or should) be unit tested. Creating a TurnPort and promptly deleting it doesn't have any observable side-effects I can see (I don't think it even creates an extra socket in "non-shared-socket" mode). https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:1302: int local_ip_family = network_->GetBestIP().family(); Can just use ip_.family(). https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:1303: if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) { On 2016/08/03 21:33:08, pthatcher1 wrote: > What happens if local_ip_family is AF_UNSPEC and server_ip_family isn't? Is it > possible for GetBestIP() to return an address with AF_UNSPEC? I agree about this. Even if it's not possible to be AF_UNSPEC right now, a DCHECK would be useful so we can tell if the assumption changes.
It did not create extra socket in the non-shared-socket mode. It does not have observable changes except for avoiding creating a port and soon deleting it (and the logging in the process) as Taylor mentioned. It really just pushes the IsCompatibleAddress check earlier to have less logs like "Port created ..." So it may not be worth adding a test for it. https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:1302: int local_ip_family = network_->GetBestIP().family(); On 2016/08/05 20:31:29, Taylor Brandstetter wrote: > Can just use ip_.family(). Done. https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:1303: if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) { On 2016/08/05 20:31:29, Taylor Brandstetter wrote: > On 2016/08/03 21:33:08, pthatcher1 wrote: > > What happens if local_ip_family is AF_UNSPEC and server_ip_family isn't? Is > it > > possible for GetBestIP() to return an address with AF_UNSPEC? > > I agree about this. Even if it's not possible to be AF_UNSPEC right now, a > DCHECK would be useful so we can tell if the assumption changes. It is possible (although very strange) to have a network with AF_UNSPEC address. Mostly, it will be a NIL address in that case. But if that happens, the Port will be destroyed soon anyway because of the Iscompatible Check later. https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/turnport.cc?... If that happens, even though the TurnPort will fail, it does not necessarily fail the call. For that reason, I think we'd better not have a DCHECK here.
lgtm https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:1303: if (server_ip_family != AF_UNSPEC && server_ip_family != local_ip_family) { On 2016/08/05 22:04:07, honghaiz3 wrote: > On 2016/08/05 20:31:29, Taylor Brandstetter wrote: > > On 2016/08/03 21:33:08, pthatcher1 wrote: > > > What happens if local_ip_family is AF_UNSPEC and server_ip_family isn't? Is > > it > > > possible for GetBestIP() to return an address with AF_UNSPEC? > > > > I agree about this. Even if it's not possible to be AF_UNSPEC right now, a > > DCHECK would be useful so we can tell if the assumption changes. > It is possible (although very strange) to have a network with AF_UNSPEC address. > Mostly, it will be a NIL address in that case. But if that happens, the Port > will be destroyed soon anyway because of the Iscompatible Check later. > https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/turnport.cc?... > If that happens, even though the TurnPort will fail, it does not necessarily > fail the call. For that reason, I think we'd better not have a DCHECK here. > > Ok, makes sense. So we actually do want to treat AF_UNSPEC as mismatching.
On 2016/08/05 22:19:26, Taylor Brandstetter wrote: > lgtm > > https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... > File webrtc/p2p/client/basicportallocator.cc (right): > > https://codereview.webrtc.org/2206713004/diff/20001/webrtc/p2p/client/basicpo... > webrtc/p2p/client/basicportallocator.cc:1303: if (server_ip_family != AF_UNSPEC > && server_ip_family != local_ip_family) { > On 2016/08/05 22:04:07, honghaiz3 wrote: > > On 2016/08/05 20:31:29, Taylor Brandstetter wrote: > > > On 2016/08/03 21:33:08, pthatcher1 wrote: > > > > What happens if local_ip_family is AF_UNSPEC and server_ip_family isn't? > Is > > > it > > > > possible for GetBestIP() to return an address with AF_UNSPEC? > > > > > > I agree about this. Even if it's not possible to be AF_UNSPEC right now, a > > > DCHECK would be useful so we can tell if the assumption changes. > > It is possible (although very strange) to have a network with AF_UNSPEC > address. > > Mostly, it will be a NIL address in that case. But if that happens, the Port > > will be destroyed soon anyway because of the Iscompatible Check later. > > > https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/base/turnport.cc?... > > If that happens, even though the TurnPort will fail, it does not necessarily > > fail the call. For that reason, I think we'd better not have a DCHECK here. > > > > > > Ok, makes sense. So we actually do want to treat AF_UNSPEC as mismatching. lgtm
lgtm
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/105)
Message was sent while issue was closed.
Description was changed from ========== Do not create incompatible TurnPort if the server address family is known. In the existing code, if the server address and the local IP family does not match, we still create a TurnPort and destroy it later. Instead, we could avoid creating it at the beginning. This does not affect the client behavior except for the port creation. BUG= ========== to ========== Do not create incompatible TurnPort if the server address family is known. In the existing code, if the server address and the local IP family does not match, we still create a TurnPort and destroy it later. Instead, we could avoid creating it at the beginning. This does not affect the client behavior except for the port creation. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org, zhihuang@webrtc.org Committed: https://crrev.com/3d31bd65cf5b582b8a9667c0abbf28cd77c5873d Cr-Commit-Position: refs/heads/master@{#13720} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3d31bd65cf5b582b8a9667c0abbf28cd77c5873d Cr-Commit-Position: refs/heads/master@{#13720}
Message was sent while issue was closed.
Description was changed from ========== Do not create incompatible TurnPort if the server address family is known. In the existing code, if the server address and the local IP family does not match, we still create a TurnPort and destroy it later. Instead, we could avoid creating it at the beginning. This does not affect the client behavior except for the port creation. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org, zhihuang@webrtc.org Committed: https://crrev.com/3d31bd65cf5b582b8a9667c0abbf28cd77c5873d Cr-Commit-Position: refs/heads/master@{#13720} ========== to ========== Do not create incompatible TurnPort if the server address family is known. In the existing code, if the server address and the local IP family does not match, we still create a TurnPort and destroy it later. Instead, we could avoid creating it at the beginning. This does not affect the client behavior except for the port creation. BUG= R=deadbeef@webrtc.org, pthatcher@webrtc.org, zhihuang@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3d31bd65cf5b582b8a9667c0a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) manually as 3d31bd65cf5b582b8a9667c0abbf28cd77c5873d (presubmit successful). |
