|
|
Created:
4 years, 7 months ago by honghaiz3 Modified:
4 years, 6 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. |
DescriptionCreate a new connection if a candidate reuses an address
If the remote side sends a candidate with the same address and port with an existing candidate,
but with a new ufrag and pwd, the local client will create a new connection from it
and destroy the old connection with the same remote address.
BUG=webrtc:5915
Committed: https://crrev.com/36f50e8e4e3e947ad8db1179e358b11431a8f079
Cr-Commit-Position: refs/heads/master@{#13000}
Patch Set 1 : #Patch Set 2 : Sync with head #
Total comments: 16
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 2
Patch Set 5 : address comments #
Created: 4 years, 6 months ago
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Create a new connection if a candidate has a dup address If the remote side sends a candidate with the same address and port but a new ufrag and pwd, we will create a new connection from it and destroy the old connection with the same remote address. BUG= ========== to ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, we will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, we will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ========== to ========== Create a new connection if a candidate reuses an address If the remote side sends an candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Create a new connection if a candidate reuses an address If the remote side sends an candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ========== to ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ==========
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
PTAL.
Thanks for making a change so quickly. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:885: PortInterface* origin_port) { I don't quite understand why you split these methods into two, since both of them don't create a connection under certain circumstances. So how do you decide to put the logic in one vs. the other? https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:888: // Don't create connection if this is a candidate we received in a create connection => create a caonnection https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:890: if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_) {}s please https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:225: PortInterface* origin_port); Can you leave a comment explaining when we create one and when we don't (the "maybe" part)? https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2487: } I think we also need a test that if we receive a STUN ping with the new ufrag/pwd, we will process it correctly. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:273: void Port::AddConnection(Connection* conn) { If we're going to destroy existing connections when we add ones, we should rename this AddOrReplaceConnection, and comment that the decision of whether to add or replace is based on remote address. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:283: ret.first->second = conn; Instead of putting logic in OnConnectionDestroyed to check if it's the connection we're looking for, why not just ret.first->SignalDestroyed.disconnect(this); Before destroying it?
Patchset #3 (id:100001) has been deleted
PTAL. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:885: PortInterface* origin_port) { On 2016/05/27 18:42:54, pthatcher1 wrote: > I don't quite understand why you split these methods into two, since both of > them don't create a connection under certain circumstances. So how do you > decide to put the logic in one vs. the other? I thought it may improve readability. Since it caused confusion, I just put them together in one method. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:888: // Don't create connection if this is a candidate we received in a On 2016/05/27 18:42:54, pthatcher1 wrote: > create connection => create a caonnection Done. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:890: if (origin == PortInterface::ORIGIN_MESSAGE && incoming_only_) On 2016/05/27 18:42:54, pthatcher1 wrote: > {}s please Done. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.h:225: PortInterface* origin_port); On 2016/05/27 18:42:54, pthatcher1 wrote: > Can you leave a comment explaining when we create one and when we don't (the > "maybe" part)? Removed the maybe method. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2487: } On 2016/05/27 18:42:54, pthatcher1 wrote: > I think we also need a test that if we receive a STUN ping with the new > ufrag/pwd, we will process it correctly. I tried to do it but found that it is kind of unnecessary and it would be complicated to do so. The ping test essentially checks 1) Whether GetConnectionTo() finds the right connection, 2) Whether the ufrag in the remote candidate matches the one in the stun ping request. Both of them have already been tested here. The operation of combining these two are actually in each subclass of Port. Trying to test the combined operation needs to expose methods in the subclasses and some extra hackings, which may not be really worth it. WDYT? https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:273: void Port::AddConnection(Connection* conn) { On 2016/05/27 18:42:54, pthatcher1 wrote: > If we're going to destroy existing connections when we add ones, we should > rename this AddOrReplaceConnection, and comment that the decision of whether to > add or replace is based on remote address. Done. https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:283: ret.first->second = conn; On 2016/05/27 18:42:54, pthatcher1 wrote: > Instead of putting logic in OnConnectionDestroyed to check if it's the > connection we're looking for, why not just > > ret.first->SignalDestroyed.disconnect(this); > > Before destroying it? That would also do. But in the OnConnectionDestroyed method, deleting a connection without checking whether the connection in the map is actually the one destroyed has a potential risk. It is perhaps slightly safer to check it there. I could also disconnect here and also do a check there although one of them is really not necessary. WDYT?
https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:283: ret.first->second = conn; On 2016/05/31 18:57:31, honghaiz3 wrote: > On 2016/05/27 18:42:54, pthatcher1 wrote: > > Instead of putting logic in OnConnectionDestroyed to check if it's the > > connection we're looking for, why not just > > > > ret.first->SignalDestroyed.disconnect(this); > > > > Before destroying it? > That would also do. > But in the OnConnectionDestroyed method, deleting a connection without checking > whether the connection in the map is actually the one destroyed has a potential > risk. It is perhaps slightly safer to check it there. > I could also disconnect here and also do a check there although one of them is > really not necessary. > WDYT? I think the check is overkill. We don't have the check now, and it's been fine. I think the disconnect from the signal is all we need. https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection); I think this should also be AddOrReplaceConnectionk, since that's what it does. https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u, conn2->remote_candidate().generation()); Can't we right here inject a STUN ping into the connection and verify that the last ping received was that one?
Patchset #4 (id:140001) has been deleted
PTAL. https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection); On 2016/05/31 20:09:29, pthatcher1 wrote: > I think this should also be AddOrReplaceConnectionk, since that's what it does. It might look strange to use AddOrRepalceConnection here if you look at the implementation of the method. We are not replacing anything directly in the method. It will get replaced simply because the old connection (on the same remote address) was destroyed and removed from the list. WDYT? https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u, conn2->remote_candidate().generation()); On 2016/05/31 20:09:29, pthatcher1 wrote: > Can't we right here inject a STUN ping into the connection and verify that the > last ping received was that one? Added per your suggestion, although I still feel it is a little strange to include that here. All it (the faked ping message) does is to construct a Ping message using the info from the connection except for the remote ufrag. But since we have already checked that the remote ufrag is the same as the one in the connection remote candidate, it is actually using the info from the connection completely to check whether the connection can handle a ping request. Perhaps, it can be tested independently that a connection can handle a ping request properly. But that kind of tests are already there in port_unittest file.
lgtm https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection); On 2016/05/31 23:25:23, honghaiz3 wrote: > On 2016/05/31 20:09:29, pthatcher1 wrote: > > I think this should also be AddOrReplaceConnectionk, since that's what it > does. > It might look strange to use AddOrRepalceConnection here if you look at the > implementation of the method. We are not replacing anything directly in the > method. It will get replaced simply because the old connection (on the same > remote address) was destroyed and removed from the list. > WDYT? Ah, you're completely right. Please keep it AddConnection. https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u, conn2->remote_candidate().generation()); On 2016/05/31 23:25:23, honghaiz3 wrote: > On 2016/05/31 20:09:29, pthatcher1 wrote: > > Can't we right here inject a STUN ping into the connection and verify that the > > last ping received was that one? > > Added per your suggestion, although I still feel it is a little strange to > include that here. > All it (the faked ping message) does is to construct a Ping message using the > info from the connection except for the remote ufrag. But since we have already > checked that the remote ufrag is the same as the one in the connection remote > candidate, it is actually using the info from the connection completely to check > whether the connection can handle a ping request. > Perhaps, it can be tested independently that a connection can handle a ping > request properly. > But that kind of tests are already there in port_unittest file. I agree it's not optimal, but I still think it's better than nothing. https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1987: void ReceivedPingOnConnection(cricket::Connection* conn, Should call this ReceivePingOnConnection or maybe just ReceivePing
https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2018693002/diff/80001/webrtc/p2p/base/port.cc#n... webrtc/p2p/base/port.cc:283: ret.first->second = conn; On 2016/05/31 20:09:29, pthatcher1 wrote: > On 2016/05/31 18:57:31, honghaiz3 wrote: > > On 2016/05/27 18:42:54, pthatcher1 wrote: > > > Instead of putting logic in OnConnectionDestroyed to check if it's the > > > connection we're looking for, why not just > > > > > > ret.first->SignalDestroyed.disconnect(this); > > > > > > Before destroying it? > > That would also do. > > But in the OnConnectionDestroyed method, deleting a connection without > checking > > whether the connection in the map is actually the one destroyed has a > potential > > risk. It is perhaps slightly safer to check it there. > > I could also disconnect here and also do a check there although one of them is > > really not necessary. > > WDYT? > > I think the check is overkill. We don't have the check now, and it's been fine. > I think the disconnect from the signal is all we need. Done. You are right. We only hit the replace-connection very rarely. https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:874: AddConnection(connection); On 2016/06/01 01:57:04, pthatcher1 wrote: > On 2016/05/31 23:25:23, honghaiz3 wrote: > > On 2016/05/31 20:09:29, pthatcher1 wrote: > > > I think this should also be AddOrReplaceConnectionk, since that's what it > > does. > > It might look strange to use AddOrRepalceConnection here if you look at the > > implementation of the method. We are not replacing anything directly in the > > method. It will get replaced simply because the old connection (on the same > > remote address) was destroyed and removed from the list. > > WDYT? > > Ah, you're completely right. Please keep it AddConnection. Acknowledged. https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2486: EXPECT_EQ(1u, conn2->remote_candidate().generation()); On 2016/06/01 01:57:04, pthatcher1 wrote: > On 2016/05/31 23:25:23, honghaiz3 wrote: > > On 2016/05/31 20:09:29, pthatcher1 wrote: > > > Can't we right here inject a STUN ping into the connection and verify that > the > > > last ping received was that one? > > > > Added per your suggestion, although I still feel it is a little strange to > > include that here. > > All it (the faked ping message) does is to construct a Ping message using the > > info from the connection except for the remote ufrag. But since we have > already > > checked that the remote ufrag is the same as the one in the connection remote > > candidate, it is actually using the info from the connection completely to > check > > whether the connection can handle a ping request. > > Perhaps, it can be tested independently that a connection can handle a ping > > request properly. > > But that kind of tests are already there in port_unittest file. > > > > I agree it's not optimal, but I still think it's better than nothing. Acknowledged. https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2018693002/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1987: void ReceivedPingOnConnection(cricket::Connection* conn, On 2016/06/01 01:57:04, pthatcher1 wrote: > Should call this ReceivePingOnConnection or maybe just ReceivePing Done.
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/2018693002/#ps180001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018693002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018693002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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/2018693002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018693002/180001
Message was sent while issue was closed.
Description was changed from ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ========== to ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 ========== to ========== Create a new connection if a candidate reuses an address If the remote side sends a candidate with the same address and port with an existing candidate, but with a new ufrag and pwd, the local client will create a new connection from it and destroy the old connection with the same remote address. BUG=webrtc:5915 Committed: https://crrev.com/36f50e8e4e3e947ad8db1179e358b11431a8f079 Cr-Commit-Position: refs/heads/master@{#13000} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/36f50e8e4e3e947ad8db1179e358b11431a8f079 Cr-Commit-Position: refs/heads/master@{#13000} |