Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(72)

Issue 2068263003: Do not delete a connection in the turn port with permission error or refresh error. (Closed)

Created:
4 years, 6 months ago by honghaiz3
Modified:
4 years, 6 months ago
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.

Description

Do not delete a connection in the turn port with permission error, refresh error, or binding error. Even if those error happened, the connection may still be able to receive packets for a while. If we delete the connections, all packets arriving will be dropped. BUG=webrtc:6007 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/3d77deb29c15bfb8f794ef3413837a0ec0f0c131 Cr-Commit-Position: refs/heads/master@{#13262}

Patch Set 1 : . #

Total comments: 17

Patch Set 2 : . #

Total comments: 14

Patch Set 3 : Address comments #

Total comments: 2

Patch Set 4 : Address Taylor's comment and merge #

Total comments: 1

Patch Set 5 : Change comments #

Patch Set 6 : Merge with head #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -63 lines) Patch
M webrtc/base/virtualsocketserver.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M webrtc/p2p/base/p2ptransportchannel.cc View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/p2p/base/p2ptransportchannel_unittest.cc View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments Download
M webrtc/p2p/base/port.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/p2p/base/port.cc View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/p2p/base/turnport.h View 1 2 3 3 chunks +8 lines, -5 lines 0 comments Download
M webrtc/p2p/base/turnport.cc View 1 2 3 9 chunks +29 lines, -22 lines 3 comments Download
M webrtc/p2p/base/turnport_unittest.cc View 1 2 11 chunks +72 lines, -35 lines 0 comments Download

Messages

Total messages: 34 (17 generated)
honghaiz3
4 years, 6 months ago (2016-06-16 00:47:34 UTC) #9
honghaiz3
https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptransportchannel_unittest.cc File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptransportchannel_unittest.cc#newcode2080 webrtc/p2p/base/p2ptransportchannel_unittest.cc:2080: EXPECT_EQ(conn2, FindNextPingableConnectionAndPingIt(&ch)); I only moved this test up so ...
4 years, 6 months ago (2016-06-16 00:59:58 UTC) #12
pthatcher1
https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptransportchannel_unittest.cc File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptransportchannel_unittest.cc#newcode2108 webrtc/p2p/base/p2ptransportchannel_unittest.cc:2108: cricket::P2PTransportChannel ch("do not ping failed connection", 1, &pa); do ...
4 years, 6 months ago (2016-06-16 22:34:21 UTC) #13
honghaiz3
PTAL. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptransportchannel_unittest.cc File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/p2ptransportchannel_unittest.cc#newcode2108 webrtc/p2p/base/p2ptransportchannel_unittest.cc:2108: cricket::P2PTransportChannel ch("do not ping failed connection", 1, &pa); ...
4 years, 6 months ago (2016-06-16 23:22:54 UTC) #14
pthatcher1
https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc#newcode564 webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is ...
4 years, 6 months ago (2016-06-21 06:58:26 UTC) #15
Taylor Brandstetter
https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/140001/webrtc/p2p/base/turnport.cc#newcode564 webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port is ...
4 years, 6 months ago (2016-06-21 18:48:08 UTC) #16
honghaiz3
PTAL. https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/120001/webrtc/p2p/base/turnport.cc#newcode564 webrtc/p2p/base/turnport.cc:564: << "Received TURN message while the Turn port ...
4 years, 6 months ago (2016-06-22 01:50:41 UTC) #19
Taylor Brandstetter
lgtm with nit https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport.h#newcode208 webrtc/p2p/base/turnport.h:208: void handleRefreshError(); Nit: why handleRefreshError and ...
4 years, 6 months ago (2016-06-22 16:49:54 UTC) #20
honghaiz3
https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport.h File webrtc/p2p/base/turnport.h (right): https://codereview.webrtc.org/2068263003/diff/200001/webrtc/p2p/base/turnport.h#newcode208 webrtc/p2p/base/turnport.h:208: void handleRefreshError(); On 2016/06/22 16:49:54, Taylor Brandstetter wrote: > ...
4 years, 6 months ago (2016-06-22 18:01:48 UTC) #21
pthatcher1
lgtm, with comment nit https://codereview.webrtc.org/2068263003/diff/220001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2068263003/diff/220001/webrtc/p2p/base/port.h#newcode527 webrtc/p2p/base/port.h:527: // It will not be ...
4 years, 6 months ago (2016-06-22 18:43:34 UTC) #22
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/3d77deb29c15bfb8f794ef3413837a0ec0f0c131 Cr-Commit-Position: refs/heads/master@{#13262}
4 years, 6 months ago (2016-06-22 23:02:01 UTC) #27
honghaiz3
A revert of this CL (patchset #6 id:260001) has been created in https://codereview.webrtc.org/2090833002/ by honghaiz@webrtc.org. ...
4 years, 6 months ago (2016-06-22 23:17:11 UTC) #28
juberti
https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (left): https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc#oldcode1528 webrtc/p2p/base/turnport.cc:1528: port_->DestroyConnection(ext_addr_); This also seems like an error that should ...
4 years, 6 months ago (2016-06-23 21:07:29 UTC) #30
pthatcher1
https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc#newcode1503 webrtc/p2p/base/turnport.cc:1503: bool found = port_->FailAndPruneConnection(ext_addr_); On 2016/06/23 21:07:29, juberti wrote: ...
4 years, 6 months ago (2016-06-23 21:29:23 UTC) #31
juberti
On 2016/06/23 21:29:23, pthatcher1 wrote: > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc > File webrtc/p2p/base/turnport.cc (right): > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc#newcode1503 > ...
4 years, 6 months ago (2016-06-23 21:34:50 UTC) #32
juberti
On 2016/06/23 21:34:50, juberti wrote: > On 2016/06/23 21:29:23, pthatcher1 wrote: > > > https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc ...
4 years, 6 months ago (2016-06-23 21:35:43 UTC) #33
honghaiz3
4 years, 6 months ago (2016-06-24 01:07:09 UTC) #34
Message was sent while issue was closed.
On 2016/06/23 21:35:43, juberti wrote:
> On 2016/06/23 21:34:50, juberti wrote:
> > On 2016/06/23 21:29:23, pthatcher1 wrote:
> > >
> >
>
https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport.cc
> > > File webrtc/p2p/base/turnport.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2068263003/diff/260001/webrtc/p2p/base/turnport...
> > > webrtc/p2p/base/turnport.cc:1503: bool found =
> > > port_->FailAndPruneConnection(ext_addr_);
> > > On 2016/06/23 21:07:29, juberti wrote:
> > > > Shouldn't we destroy the connection if we can't create a permission for
> it?
> > We
> > > > shouldn't be able to receive data if the permission fails.
> > > 
> > > Not if the TURN server doesn't check for CreatePermission, right?
> > 
> > I guess my point is that this seems like such a gross failure that trying to
> > survive in this case seems hopeless.
> 
> or, put a different way, trying to workaround these gross failures may mask
the
> underlying problem.
I choose to error on the side of keeping the connection for a while but marking
it as failed and pruned. So basically it won't have chance to send ping or data
except for potentially receiving data. It won't mask the underlying problem
because transport channel should kick off ICE restart or regathering as if the
connections are not there. 
Having said that, if indeed there is absolutely no hope that the server won't
send back data in those case, perhaps we should just destroy the connection (as
we do before).

Powered by Google App Engine
This is Rietveld 408576698