Fixing segfault caused by TurnServer.

Fixing segfault caused by TurnServer.

TURN server sockets were being destroyed asynchronously, which could
happen after the TurnServer itself (and even the VirtualSocketServer
used by the sockets) were destroyed.

This is fixed easily by using an AsyncInvoker (to ensure the async
operation doesn't occur after its initiator is destroyed), and keeping
the objects waiting for deletion in a unique_ptr vector.

Committed: https://crrev.com/824f58621315a8b39193e06f960d8ff15f84fc13
Cr-Commit-Position: refs/heads/master@{#13907}

[pthatcher1, 2016-08-23 00:36:55]

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

[pthatcher1, 2016-08-23 00:36:56]

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc
File webrtc/p2p/base/turnserver.cc (right):

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc...
webrtc/p2p/base/turnserver.cc:535: [this] { pending_delete_sockets_.clear(); });
I think the style guide said not to store lambdas.

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

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.h#...
webrtc/p2p/base/turnserver.h:284:
std::vector<std::unique_ptr<rtc::AsyncPacketSocket>> pending_delete_sockets_;
I'd prefer a name like sockets_to_delete.

[Taylor Brandstetter, 2016-08-23 01:11:20]

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc
File webrtc/p2p/base/turnserver.cc (right):

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc...
webrtc/p2p/base/turnserver.cc:535: [this] { pending_delete_sockets_.clear(); });
On 2016/08/23 00:36:55, pthatcher1 wrote:
> I think the style guide said not to store lambdas.

The Chromium style guide says to never store *capturing* lambdas because
non-capturing lambdas used with base::Bind provide added protection:

invoker_.AsyncInvoke<void>(RTC_FROM_HERE, rtc::Thread::Current(),
  base::Bind([] (TurnServer* self) {
    self->pending_delete_sockets_.clear();
  }, this));

But we don't have Chromium's base::Bind. So what would you do here? Is a lambda
basically out of the question, and we have add a 1 line method instead?

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

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.h#...
webrtc/p2p/base/turnserver.h:284:
std::vector<std::unique_ptr<rtc::AsyncPacketSocket>> pending_delete_sockets_;
On 2016/08/23 00:36:55, pthatcher1 wrote:
> I'd prefer a name like sockets_to_delete.

Done.

[pthatcher1, 2016-08-23 01:15:32]

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc
File webrtc/p2p/base/turnserver.cc (right):

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc...
webrtc/p2p/base/turnserver.cc:535: [this] { pending_delete_sockets_.clear(); });
On 2016/08/23 01:11:20, Taylor Brandstetter wrote:
> On 2016/08/23 00:36:55, pthatcher1 wrote:
> > I think the style guide said not to store lambdas.
> 
> The Chromium style guide says to never store *capturing* lambdas because
> non-capturing lambdas used with base::Bind provide added protection:
> 

Ah, thanks for correcting my poor memory.  

> invoker_.AsyncInvoke<void>(RTC_FROM_HERE, rtc::Thread::Current(),
>   base::Bind([] (TurnServer* self) {
>     self->pending_delete_sockets_.clear();
>   }, this));
> 
> But we don't have Chromium's base::Bind. So what would you do here? Is a
lambda
> basically out of the question, and we have add a 1 line method instead?

That's what we've been doing elsewhere.  We could probably be less strict for
code that doesn't go in Chromium (like this one).  But it's probably worth
checking with the rest of the team via webrtc-eng@.

[Taylor Brandstetter, 2016-08-23 23:31:12]

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc
File webrtc/p2p/base/turnserver.cc (right):

https://codereview.webrtc.org/2264343002/diff/1/webrtc/p2p/base/turnserver.cc...
webrtc/p2p/base/turnserver.cc:535: [this] { pending_delete_sockets_.clear(); });
On 2016/08/23 01:15:32, pthatcher1 wrote:
> On 2016/08/23 01:11:20, Taylor Brandstetter wrote:
> > On 2016/08/23 00:36:55, pthatcher1 wrote:
> > > I think the style guide said not to store lambdas.
> > 
> > The Chromium style guide says to never store *capturing* lambdas because
> > non-capturing lambdas used with base::Bind provide added protection:
> > 
> 
> Ah, thanks for correcting my poor memory.  
> 
> > invoker_.AsyncInvoke<void>(RTC_FROM_HERE, rtc::Thread::Current(),
> >   base::Bind([] (TurnServer* self) {
> >     self->pending_delete_sockets_.clear();
> >   }, this));
> > 
> > But we don't have Chromium's base::Bind. So what would you do here? Is a
> lambda
> > basically out of the question, and we have add a 1 line method instead?
> 
> That's what we've been doing elsewhere.  We could probably be less strict for
> code that doesn't go in Chromium (like this one).  But it's probably worth
> checking with the rest of the team via webrtc-eng@.

I'll just bite the bullet, it's only a few extra lines of code.

[skvlad, 2016-08-24 17:18:49]

skvlad@webrtc.org changed reviewers:
+ skvlad@webrtc.org

[skvlad, 2016-08-24 17:18:49]

lgtm

[Taylor Brandstetter, 2016-08-24 18:20:03]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-08-24 18:20:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-24 20:20:42]

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

[commit-bot: I haz the power, 2016-08-24 20:20:44]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Taylor Brandstetter, 2016-08-24 22:05:13]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-08-24 22:05:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-24 22:06:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-24 22:07:06]

Description was changed from

==========
Fixing segfault caused by TurnServer.

TURN server sockets were being destroyed asynchronously, which could
happen after the TurnServer itself (and even the VirtualSocketServer
used by the sockets) were destroyed.

This is fixed easily by using an AsyncInvoker (to ensure the async
operation doesn't occur after its initiator is destroyed), and keeping
the objects waiting for deletion in a unique_ptr vector.
==========

to

==========
Fixing segfault caused by TurnServer.

TURN server sockets were being destroyed asynchronously, which could
happen after the TurnServer itself (and even the VirtualSocketServer
used by the sockets) were destroyed.

This is fixed easily by using an AsyncInvoker (to ensure the async
operation doesn't occur after its initiator is destroyed), and keeping
the objects waiting for deletion in a unique_ptr vector.

Committed: https://crrev.com/824f58621315a8b39193e06f960d8ff15f84fc13
Cr-Commit-Position: refs/heads/master@{#13907}
==========

[commit-bot: I haz the power, 2016-08-24 22:07:07]

Patchset 3 (id:??) landed as
https://crrev.com/824f58621315a8b39193e06f960d8ff15f84fc13
Cr-Commit-Position: refs/heads/master@{#13907}