Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by honghaiz3 Modified:
4 years, 7 months ago Reviewers:
Taylor Brandstetter, pthatcher1 CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis fixes an issue similar to
https://bugs.chromium.org/p/webrtc/issues/detail?id=3927
where the localhost IP does not match the turn port address.
The issue here is in the TCP port.
BUG=
R=pthatcher@webrtc.org
Committed: https://crrev.com/6705012904e6cbbefce6fbce0a3f615b7aeafd8f
Cr-Commit-Position: refs/heads/master@{#12707}
Patch Set 1 : #Patch Set 2 : minor fix #
Total comments: 6
Patch Set 3 : #Patch Set 4 : Merge with head #
Created: 4 years, 7 months ago
Messages
Total messages: 25 (14 generated)
Description was changed from ========== To allow the TCP connection succeed even if the socket ip address is a localhost. BUG= ========== to ========== This fixes an issue similar to the turn/tcp port issue where the localhost IP does not match the turn port address: https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 BUG= ==========
Description was changed from ========== This fixes an issue similar to the turn/tcp port issue where the localhost IP does not match the turn port address: https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 BUG= ========== to ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address: BUG= ==========
Patchset #1 (id:1) has been deleted
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address: BUG= ========== to ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address. The issue here is in the TCP port. BUG= ==========
Ping for review.
https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport.cc File webrtc/p2p/base/tcpport.cc (right): https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:393: << socket_addr.ToSensitiveString(); This isn't an accurate log. You're saying "to", but giving the "from" address. https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:406: << ". Still allowing it since it's a local host."; it's a local host => it's localhost https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport_u... File webrtc/p2p/base/tcpport_unittest.cc (right): https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport_u... webrtc/p2p/base/tcpport_unittest.cc:44: ss_->SignalSocketCreated.connect(this, &TCPPortTest::OnSocketCreated); Why not just do this in the constructor?
Thanks! PTAL. https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport.cc File webrtc/p2p/base/tcpport.cc (right): https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:393: << socket_addr.ToSensitiveString(); On 2016/05/06 22:47:30, pthatcher1 wrote: > This isn't an accurate log. You're saying "to", but giving the "from" address. Changed to the "to" address. https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport.c... webrtc/p2p/base/tcpport.cc:406: << ". Still allowing it since it's a local host."; On 2016/05/06 22:47:30, pthatcher1 wrote: > it's a local host => it's localhost Done. https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport_u... File webrtc/p2p/base/tcpport_unittest.cc (right): https://codereview.webrtc.org/1914803002/diff/80001/webrtc/p2p/base/tcpport_u... webrtc/p2p/base/tcpport_unittest.cc:44: ss_->SignalSocketCreated.connect(this, &TCPPortTest::OnSocketCreated); On 2016/05/06 22:47:30, pthatcher1 wrote: > Why not just do this in the constructor? Because we don't want to connect this signal for the first two sockets created (when creating the TcpPorts).
lgtm
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1914803002/#ps120001 (title: "Merge with head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914803002/120001
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/13207)
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/1914803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914803002/120001
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/13208)
Description was changed from ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address. The issue here is in the TCP port. BUG= ========== to ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address. The issue here is in the TCP port. BUG= R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6705012904e6cbbefce6fbce0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) manually as 6705012904e6cbbefce6fbce0a3f615b7aeafd8f (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address. The issue here is in the TCP port. BUG= R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6705012904e6cbbefce6fbce0... ========== to ========== This fixes an issue similar to https://bugs.chromium.org/p/webrtc/issues/detail?id=3927 where the localhost IP does not match the turn port address. The issue here is in the TCP port. BUG= R=pthatcher@webrtc.org Committed: https://crrev.com/6705012904e6cbbefce6fbce0a3f615b7aeafd8f Cr-Commit-Position: refs/heads/master@{#12707} ==========
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:120001) has been created in https://codereview.webrtc.org/1979463003/ by tommi@webrtc.org. The reason for reverting is: Speculatively reverting due to failures on the memcheck bot (and possibly other bots): https://build.chromium.org/p/client.webrtc/builders/Linux%20Memcheck/builds/5.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
