Destroy a Connection if a CreatePermission request fails.

Destroy a Connection if a CreatePermission request fails.

This means that if a TURN server denies permission for an
unreachable address, we'll no longer ping it fruitlessly.

BUG=webrtc:4917
Committed: https://crrev.com/376e1235c7b602e86afe9f36eb81289e42643718
Cr-Commit-Position: refs/heads/master@{#10789}

[Taylor Brandstetter, 2015-10-22 00:36:53]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2015-10-22 00:36:53]

https://codereview.webrtc.org/1415313004/diff/1/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1415313004/diff/1/webrtc/p2p/base/port.h#newcod...
webrtc/p2p/base/port.h:642: int error_ = 0;
Just style improvements here.

[juberti, 2015-10-26 21:27:24]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2015-10-26 21:27:24]

https://codereview.webrtc.org/1415313004/diff/20001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1415313004/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1349: port_->SignalCreatePermissionResult(port_,
ext_addr_, code);
Rather than using the signal, I think it would be simplest to do something like

port_->DestroyConnection(ext_addr_);

Then you don't need a new TURN connection class.

[juberti, 2015-10-26 21:43:50]

On 2015/10/26 21:27:24, juberti wrote:
>
https://codereview.webrtc.org/1415313004/diff/20001/webrtc/p2p/base/turnport.cc
> File webrtc/p2p/base/turnport.cc (right):
> 
>
https://codereview.webrtc.org/1415313004/diff/20001/webrtc/p2p/base/turnport....
> webrtc/p2p/base/turnport.cc:1349: port_->SignalCreatePermissionResult(port_,
> ext_addr_, code);
> Rather than using the signal, I think it would be simplest to do something
like
> 
> port_->DestroyConnection(ext_addr_);
> 
> Then you don't need a new TURN connection class.

or you could handle it all in this function, if you want, and just do
port_->GetConnection() followed by conn->Destroy().

[pthatcher1, 2015-10-29 14:17:09]

On 2015/10/26 21:43:50, juberti wrote:
> On 2015/10/26 21:27:24, juberti wrote:
> >
>
https://codereview.webrtc.org/1415313004/diff/20001/webrtc/p2p/base/turnport.cc
> > File webrtc/p2p/base/turnport.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1415313004/diff/20001/webrtc/p2p/base/turnport....
> > webrtc/p2p/base/turnport.cc:1349: port_->SignalCreatePermissionResult(port_,
> > ext_addr_, code);
> > Rather than using the signal, I think it would be simplest to do something
> like
> > 
> > port_->DestroyConnection(ext_addr_);
> > 
> > Then you don't need a new TURN connection class.
> 
> or you could handle it all in this function, if you want, and just do
> port_->GetConnection() followed by conn->Destroy().

That sounds like a good improvement to me.  Otherwise, the whole CL looks good.

[Taylor Brandstetter, 2015-11-11 00:17:54]

Alright, I got rid of the TurnConnection class.

Just to note: the purpose of my original design was so that
TurnEntry/TurnPort/Connection wouldn't end up so tightly coupled (directly
calling methods that impact each other's private state). But I guess they're
already so tightly coupled, a little more coupling won't hurt much.

[juberti, 2015-11-23 06:47:05]

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:629: friend class TurnEntry;
Hmm... I don't think TurnEntry should be able to frob private members of Port.
Is there no way to have this friendship be on TurnPort (or removed altogether?)

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1350: Connection* c =
port_->GetConnection(ext_addr_);
To avoid frobbing private state, let's go with the first approach I suggested
earlier, e.g.
call port_->DestroyConnection(ext_addr_), and then TurnPort can call the
protected methods in Port.

[Taylor Brandstetter, 2015-11-23 19:20:18]

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:629: friend class TurnEntry;
On 2015/11/23 06:47:05, juberti wrote:
> Hmm... I don't think TurnEntry should be able to frob private members of Port.
> Is there no way to have this friendship be on TurnPort (or removed
altogether?)

Changed to friendship with TurnPort.

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1350: Connection* c =
port_->GetConnection(ext_addr_);
On 2015/11/23 06:47:05, juberti wrote:
> To avoid frobbing private state, let's go with the first approach I suggested
> earlier, e.g.
> call port_->DestroyConnection(ext_addr_), and then TurnPort can call the
> protected methods in Port.

I'll call the new method "OnCreatePermissionError" instead of
"DestroyConnection", since it needs to set the Connection's state to
"STATE_FAILED" and log a message, so it's more specific than just destroying the
connection.

[pthatcher1, 2015-11-23 23:57:21]

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1350: Connection* c =
port_->GetConnection(ext_addr_);
On 2015/11/23 19:20:18, Taylor Brandstetter wrote:
> On 2015/11/23 06:47:05, juberti wrote:
> > To avoid frobbing private state, let's go with the first approach I
suggested
> > earlier, e.g.
> > call port_->DestroyConnection(ext_addr_), and then TurnPort can call the
> > protected methods in Port.
> 
> I'll call the new method "OnCreatePermissionError" instead of
> "DestroyConnection", since it needs to set the Connection's state to
> "STATE_FAILED" and log a message, so it's more specific than just destroying
the
> connection.
> 

FYI, Taylor and I talked about this.  I suggested making a Fail() method to go
along with Destroy().  I think TurnEntry could call c->Fail() here without being
a friend class.  Fail() == set_state(FAILED); Destroy();

[juberti, 2015-11-24 00:11:26]

On 2015/11/23 23:57:21, pthatcher1 wrote:
>
https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport.cc
> File webrtc/p2p/base/turnport.cc (right):
> 
>
https://codereview.webrtc.org/1415313004/diff/40001/webrtc/p2p/base/turnport....
> webrtc/p2p/base/turnport.cc:1350: Connection* c =
> port_->GetConnection(ext_addr_);
> On 2015/11/23 19:20:18, Taylor Brandstetter wrote:
> > On 2015/11/23 06:47:05, juberti wrote:
> > > To avoid frobbing private state, let's go with the first approach I
> suggested
> > > earlier, e.g.
> > > call port_->DestroyConnection(ext_addr_), and then TurnPort can call the
> > > protected methods in Port.
> > 
> > I'll call the new method "OnCreatePermissionError" instead of
> > "DestroyConnection", since it needs to set the Connection's state to
> > "STATE_FAILED" and log a message, so it's more specific than just destroying
> the
> > connection.
> > 
> 
> FYI, Taylor and I talked about this.  I suggested making a Fail() method to go
> along with Destroy().  I think TurnEntry could call c->Fail() here without
being
> a friend class.  Fail() == set_state(FAILED); Destroy();

That brings up a broader question: should the port destroy the connection, or
just mark it as failed and leave it for the higher layer code to destroy?

I lean toward the latter as the simplest option.

[Taylor Brandstetter, 2015-11-24 00:50:33]

Peter: I did what we talked about, fyi

[juberti, 2015-11-24 00:53:21]

lgtm - I see this parallels how we handle a binding error response.

[pthatcher, 2015-11-24 17:56:47]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2015-11-24 17:56:47]

lgtm

https://codereview.webrtc.org/1415313004/diff/60001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1415313004/diff/60001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:928: c->Destroy();
Why does this need to be friend class?

[Taylor Brandstetter, 2015-11-24 22:35:46]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-11-24 22:35:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415313004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415313004/80001

[commit-bot: I haz the power, 2015-11-24 22:40:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 22:40:37]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1952)

[Taylor Brandstetter, 2015-11-25 00:12:08]

Looks like you guys both LGTM'ed from @google.com accounts

[juberti1, 2015-11-25 01:07:18]

On 2015/11/25 00:12:08, Taylor Brandstetter wrote:
> Looks like you guys both LGTM'ed from @google.com accounts

lgtm

I'm juberti@webrtc.org and I approve this CL

[Taylor Brandstetter, 2015-11-25 16:58:32]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-11-25 16:58:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415313004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415313004/80001

[commit-bot: I haz the power, 2015-11-25 17:00:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-25 17:00:20]

Patchset 5 (id:??) landed as
https://crrev.com/376e1235c7b602e86afe9f36eb81289e42643718
Cr-Commit-Position: refs/heads/master@{#10789}

[pthatcher1, 2015-11-25 23:09:32]

lgtm

For good measure.