Refactor: Simplify WaitableEventWatcher.

Refactor: Simplify WaitableEventWatcher.

This change uses a callback instead of a delegate for specifying what
should be called when a WaitableEvent occurs.

This simplifies the class and gets rid of a workaround internal to the
class to prevent name collision on "Delegate" inner classes.

Tested (linux and windows):
  out/Debug/base_unittests --gtest_filter=*WaitableEventWatcherTest*
  out/Release/ipc_tests with valgrind and leak-check=yes

Previously reverted at:
  https://codereview.chromium.org/12087120/


BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180450

[teravest, 2013-02-01 16:52:24]

This change fixes a memory leak that existed last time around inside
SyncChannel.

It made me a little uncomfortable to add another use of base::Unretained(), but
this makes the behavior consistent with how it was before this change.

[dmichael (off chromium), 2013-02-01 17:27:18]

Can you reference the original CL and/or the revert in your description?

https://codereview.chromium.org/12094106/diff/1/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/12094106/diff/1/ipc/ipc_sync_channel.cc#newco...
ipc/ipc_sync_channel.cc:239: base::Unretained(this));
Okay, so the problem is if you don't use base::Unretained here, SyncContext owns
a member that keeps a reference to ths SyncContext.

Maybe don't keep a member, and instead have a MakeCallback that generates one
instead, for use by the SyncChannel outside. WDYT? See below...

https://codereview.chromium.org/12094106/diff/1/ipc/ipc_sync_channel.cc#newco...
ipc/ipc_sync_channel.cc:530: old_callback =
old_send_done_event_watcher->callback();
If you instead just make the callback here (e.g., by calling
old_send_done_event_watcher->MakeCallback or something), then the context is
only ever owned by the callback on the stack and the WaitableEventWatcher that's
also on the stack.

https://codereview.chromium.org/12094106/diff/1/ipc/ipc_sync_channel.h
File ipc/ipc_sync_channel.h (right):

https://codereview.chromium.org/12094106/diff/1/ipc/ipc_sync_channel.h#newcod...
ipc/ipc_sync_channel.h:194: base::WaitableEventWatcher::EventCallback
shutdown_watcher_callback_;
Why is this called shutdown_watcher_callback_?

It looks like it's really used to indicate a send finished. Maybe
send_done_watcher_callback?

[teravest, 2013-02-01 17:54:15]

Cleaned up the code as you recommended, no leaks with valgrind.

[dmichael (off chromium), 2013-02-01 18:13:23]

lgtm

[brettw, 2013-02-01 18:19:25]

https://codereview.chromium.org/12094106/diff/9001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/12094106/diff/9001/ipc/ipc_sync_channel.cc#ne...
ipc/ipc_sync_channel.cc:358:
base::Bind(&SyncChannel::SyncContext::OnWaitableEventSignaled, this));
Does this have the same leak? It seems like any time you don't use unretained
and add a StartWatching callback on yourself, you set up a circular cycle.

[dmichael (off chromium), 2013-02-01 18:26:28]

https://codereview.chromium.org/12094106/diff/9001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/12094106/diff/9001/ipc/ipc_sync_channel.cc#ne...
ipc/ipc_sync_channel.cc:358:
base::Bind(&SyncChannel::SyncContext::OnWaitableEventSignaled, this));
On 2013/02/01 18:19:25, brettw wrote:
> Does this have the same leak? It seems like any time you don't use unretained
> and add a StartWatching callback on yourself, you set up a circular cycle.
Oh yeah, I think so. In this case, since the watcher is a member and therefore
won't invoke you after you're destroyed, it's probably safe to use
base::Unretained (maybe with a comment)

[teravest, 2013-02-01 18:30:46]

https://codereview.chromium.org/12094106/diff/9001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/12094106/diff/9001/ipc/ipc_sync_channel.cc#ne...
ipc/ipc_sync_channel.cc:358:
base::Bind(&SyncChannel::SyncContext::OnWaitableEventSignaled, this));
On 2013/02/01 18:26:28, dmichael wrote:
> On 2013/02/01 18:19:25, brettw wrote:
> > Does this have the same leak? It seems like any time you don't use
unretained
> > and add a StartWatching callback on yourself, you set up a circular cycle.
> Oh yeah, I think so. In this case, since the watcher is a member and therefore
> won't invoke you after you're destroyed, it's probably safe to use
> base::Unretained (maybe with a comment)

Done.

[brettw, 2013-02-01 19:26:10]

lgtm

[commit-bot: I haz the power, 2013-02-01 19:41:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/12094106/14001

[commit-bot: I haz the power, 2013-02-01 21:41:39]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[tfarina, 2013-02-02 03:17:29]

https://codereview.chromium.org/12094106/diff/14001/base/synchronization/wait...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/12094106/diff/14001/base/synchronization/wait...
base/synchronization/waitable_event_watcher.h:40: //     void
OnWaitableEventSignaled(WaitableEvent* waitable_event) {
you can make this private.

[tfarina, 2013-02-02 03:20:49]

https://codereview.chromium.org/12094106/diff/14001/ipc/ipc_sync_channel.h
File ipc/ipc_sync_channel.h (right):

https://codereview.chromium.org/12094106/diff/14001/ipc/ipc_sync_channel.h#ne...
ipc/ipc_sync_channel.h:182: void OnWaitableEventSignaled(base::WaitableEvent*
arg);
nit: s/arg/event ?

I know it wasn't you that wrote |arg|. The source uses |event| btw ;)

[teravest, 2013-02-04 15:16:37]

https://codereview.chromium.org/12094106/diff/14001/base/synchronization/wait...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/12094106/diff/14001/base/synchronization/wait...
base/synchronization/waitable_event_watcher.h:40: //     void
OnWaitableEventSignaled(WaitableEvent* waitable_event) {
On 2013/02/02 03:17:29, tfarina wrote:
> you can make this private.

Done.

https://codereview.chromium.org/12094106/diff/14001/ipc/ipc_sync_channel.h
File ipc/ipc_sync_channel.h (right):

https://codereview.chromium.org/12094106/diff/14001/ipc/ipc_sync_channel.h#ne...
ipc/ipc_sync_channel.h:182: void OnWaitableEventSignaled(base::WaitableEvent*
arg);
On 2013/02/02 03:20:49, tfarina wrote:
> nit: s/arg/event ?
> 
> I know it wasn't you that wrote |arg|. The source uses |event| btw ;)

Done.

[commit-bot: I haz the power, 2013-02-04 15:17:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/12094106/25001

[commit-bot: I haz the power, 2013-02-04 18:14:29]

Change committed as 180450