Reporting of policy errors via host-offline-reason: part 2b

Convert HeartbeatSender to use Callbacks instead of Listener pattern.

This is a separte, self-contained changelist, but it fits in-between
part 2 and part 3 of a set of changes for reporting of policy errors
via host-offline-reason (which address the issue at crbug.com/410050).
Using Callbacks rather than Listener pattern in HeartbeatSender allows
MinimumHeartbeatSender from part3 to consistently use Callbacks (which
in turn allows a *static* MinimumHeartbeatSender::Create method).

To support this changelist, I introduced MockCallback<Sig> template.
I was not able to find a generic MockCallback in Chromium.  GMock
has MockFunction<Sig>, but connecting it to Chromium's Callback class
is more trouble, then just introducing a new MockCallback<Sig> right here.
In the current changelist I just have MockClosure, but in part3, I will
be extending this to cover MockCallback<R(A1)>.

BUG=410050
Committed: https://crrev.com/ead20f1a85c6cb6f3faf6042bea56126a7078854
Cr-Commit-Position: refs/heads/master@{#305836}

[Łukasz Anforowicz, 2014-11-17 18:16:55]

lukasza@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Łukasz Anforowicz, 2014-11-17 18:16:56]

Lambros,

Could you please review HeartbeatSender refactoring from Listener pattern to
Callbacks (suggested by Renato) that goes in-between part#2 and part#3 of the
code reviews that you've been already working on.

Thanks,

Lukasz

[Lambros, 2014-11-18 00:19:30]

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:292: .Times(0);
nit: Fits on one line.

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:301: false, // <- This is not a
response to offline-reason.
nit: Two spaces before //

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:306: EXPECT_CALL(mock_ack_callback,
Run())
I'm not sure this new code is equivalent to the old test? I don't understand
gmock well enough to be sure, but you have a test in two parts, and you're
changing expectations mid-way through the test.
Depending on how gmock works, your second call to EXPECT_CALL might be a no-op
because of the first call.
Normally, we avoid these sorts of questions by placing all EXPECT_CALLs at the
top of the test function.
Do we have examples in other code where we change mock expectations mid-way
through a test? It seems very dubious to me.
Unless you can convince me this code works as intended, I suggest splitting this
into two tests.

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:307: .Times(1);
nit: Fits on one line.

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:316: .Times(0);
nit: Fits on one line.

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback.h
File remoting/host/mock_callback.h (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:11: #include "base/callback.h"
Could you use base/callback_forward.h instead?

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:24: MOCK_CONST_METHOD0_T(Run, R());
Hmm. This could be tricky to generalize to the n-parameter case? Maybe this
class should be named MockCallback0, in anticipation of this?

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:34: base::WeakPtrFactory<MockCallback<R()>>
weak_factory_;
I'm not convinced we should use WeakPtr in this context. WeakPtr means that the
Run call will become a no-op in the situation where the class is deleted. But in
that case, you will have a potential call that GMock doesn't know about, and
will silently violate expectations. Imagine a test passing because we expected a
mock callback to never be called. But maybe it did actually get called, but the
object got deleted so gmock was never aware of the incorrect call?

I guess what you really want is some kind of assertion (in the gtest failure
sense, not a program crash) that the object doesn't get deleted during a test?
I'm not sure of the right way to do this - maybe needs some wider discussion?

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:160: // HeartbeatSender callbacks
nit: period.
Actually, I'm not sure you need this comment? Also, do these methods need to be
public?

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:1324:
base::Bind(&HostProcess::OnHeartbeatSuccessful, base::Unretained(this)),
Why do we need base::Unretained here? This is a ref-counted object.

[Łukasz Anforowicz, 2014-11-18 01:03:22]

Thanks for the feedback.  I think I addressed most of it.  Please take a look to
see if this is satisfactory - in particular, please:

- Comment on WeakPtr in MockCallback and 1) the idea to enforce no dangling
callbacks in destructor of MockCallback and 2) alternatives (which I don't see
:-( )

- Look at my explanation of why multiple EXPECT_CALL should be fine


Thanks,

Lukasz

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:292: .Times(0);
On 2014/11/18 00:19:29, Lambros wrote:
> nit: Fits on one line.

Done (although I was just following the pattern from SetUp method :-)

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:301: false, // <- This is not a
response to offline-reason.
On 2014/11/18 00:19:29, Lambros wrote:
> nit: Two spaces before //

Done.

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:306: EXPECT_CALL(mock_ack_callback,
Run())
On 2014/11/18 00:19:29, Lambros wrote:
> I'm not sure this new code is equivalent to the old test? I don't understand
> gmock well enough to be sure, but you have a test in two parts, and you're
> changing expectations mid-way through the test.
> Depending on how gmock works, your second call to EXPECT_CALL might be a no-op
> because of the first call.
> Normally, we avoid these sorts of questions by placing all EXPECT_CALLs at the
> top of the test function.
> Do we have examples in other code where we change mock expectations mid-way
> through a test? It seems very dubious to me.
> Unless you can convince me this code works as intended, I suggest splitting
this
> into two tests.
> 

Hmmm... 2 tests would also work.  But would have duplicated code... hmmm... I
think I'll just try the convincing-this-code-works-as-intended path first :-)

https://code.google.com/p/googlemock/wiki/ForDummies (I read that before writing
the code, not when replying to the CR feedback :-) has an explict section titled
"Using Multiple Expectations" and says: "when a mock method is invoked, Google
Mock will search the expectations in the reverse order they are defined, and
stop when an active expectation that matches the arguments is found (you can
think of it as "newer rules override older ones."). If the matching expectation
cannot take any more calls, you will get an upper-bound-violated failure."

I used expectations that override earlier expectations, because I wanted to
disallow calls to ack_callback, until the right moment.  Splitting into 2 tests
is also an option, but this would be a slightly different test (either it won't
directly cover the scenario of response-to-non-error, response-to-error,
got-callback; or it won't be able to verify the response-to-non-error,
verify-got-no-callback-yet, response-to-error).

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:307: .Times(1);
On 2014/11/18 00:19:29, Lambros wrote:
> nit: Fits on one line.

Done.

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:316: .Times(0);
On 2014/11/18 00:19:29, Lambros wrote:
> nit: Fits on one line.

Done.

BTW: This EXPECT_CALL is not really necessary (because the Times(1) on the
previous expectation will already reject further calls), but I put it here
anyway because I think it improves readability of the test code.  But I guess
(in light of your earlier comment about multiple EXPECT_CALLs) one can argue
about the readability either way.

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback.h
File remoting/host/mock_callback.h (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:11: #include "base/callback.h"
On 2014/11/18 00:19:29, Lambros wrote:
> Could you use base/callback_forward.h instead?

Done.  It builds fine.  OTOH, I have a feeling it works only by virtue of
includers of mock_callback.h already including base/callback.h - i.e.
GetCallback method from the template can't be compiled without base/callback.h,
right?

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:24: MOCK_CONST_METHOD0_T(Run, R());
On 2014/11/18 00:19:29, Lambros wrote:
> Hmm. This could be tricky to generalize to the n-parameter case? Maybe this
> class should be named MockCallback0, in anticipation of this?

I am doing this in part#3.  I am not very happy with the amount of code
duplication between templates for different arities, but it definitely works.

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:34: base::WeakPtrFactory<MockCallback<R()>>
weak_factory_;
On 2014/11/18 00:19:29, Lambros wrote:
> I'm not convinced we should use WeakPtr in this context. WeakPtr means that
the
> Run call will become a no-op in the situation where the class is deleted. But
in
> that case, you will have a potential call that GMock doesn't know about, and
> will silently violate expectations. Imagine a test passing because we expected
a
> mock callback to never be called. But maybe it did actually get called, but
the
> object got deleted so gmock was never aware of the incorrect call?
> 
> I guess what you really want is some kind of assertion (in the gtest failure
> sense, not a program crash) that the object doesn't get deleted during a test?
> I'm not sure of the right way to do this - maybe needs some wider discussion?

Yes - this needs more discussion.  I think one way around this is to have an
explicit destructor of MockCallback that does
EXPECT_FALSE(weak_factory_.HasWeakPtrs()).  This would be safest probably. 
OTOH, EXPECT_CALL does allow WillRepeatedly and it can potentially go beyond the
lifetime of MockCallback...

Anyway - this is probably good enough for remoting/host/mock_callback.h, right? 
(agreed that this needs more discussion before we can argue about moving it to
chromium/src/base/mock_callback.h).  If you think this should block part#2b,
then can you suggest which bind_helpers.h we should use here?  ref-counting one
won't work for stack-allocated MockCallbacks.  base::Unretained is yucky.  I
haven't tried ConstRef one but it also shouldn't work / seems yucky wrt lifetime
management of MockCallback vs generated callbacks.

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:160: // HeartbeatSender callbacks
On 2014/11/18 00:19:30, Lambros wrote:
> nit: period.
> Actually, I'm not sure you need this comment? Also, do these methods need to
be
> public?

Yes - good point.  These methods don't need to be public after the changes.

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:1324:
base::Bind(&HostProcess::OnHeartbeatSuccessful, base::Unretained(this)),
On 2014/11/18 00:19:30, Lambros wrote:
> Why do we need base::Unretained here? This is a ref-counted object.

This temporarily follows other references to |this| from this file.  I am
changing to WeakPtr in part#3.

[Lambros, 2014-11-18 02:33:53]

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:306: EXPECT_CALL(mock_ack_callback,
Run())
On 2014/11/18 01:03:22, lukasza wrote:
> On 2014/11/18 00:19:29, Lambros wrote:
> > I'm not sure this new code is equivalent to the old test? I don't understand
> > gmock well enough to be sure, but you have a test in two parts, and you're
> > changing expectations mid-way through the test.
> > Depending on how gmock works, your second call to EXPECT_CALL might be a
no-op
> > because of the first call.
> > Normally, we avoid these sorts of questions by placing all EXPECT_CALLs at
the
> > top of the test function.
> > Do we have examples in other code where we change mock expectations mid-way
> > through a test? It seems very dubious to me.
> > Unless you can convince me this code works as intended, I suggest splitting
> this
> > into two tests.
> > 
> 
> Hmmm... 2 tests would also work.  But would have duplicated code... hmmm... I
> think I'll just try the convincing-this-code-works-as-intended path first :-)
> 
> https://code.google.com/p/googlemock/wiki/ForDummies (I read that before
writing
> the code, not when replying to the CR feedback :-) has an explict section
titled
> "Using Multiple Expectations" and says: "when a mock method is invoked, Google
> Mock will search the expectations in the reverse order they are defined, and
> stop when an active expectation that matches the arguments is found (you can
> think of it as "newer rules override older ones."). If the matching
expectation
> cannot take any more calls, you will get an upper-bound-violated failure."
> 
> I used expectations that override earlier expectations, because I wanted to
> disallow calls to ack_callback, until the right moment.  Splitting into 2
tests
> is also an option, but this would be a slightly different test (either it
won't
> directly cover the scenario of response-to-non-error, response-to-error,
> got-callback; or it won't be able to verify the response-to-non-error,
> verify-got-no-callback-yet, response-to-error).

OK, it looks like you're right, because of the way the EXPECT_CALLs get stacked.

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:316: .Times(0);
On 2014/11/18 01:03:22, lukasza wrote:
> On 2014/11/18 00:19:29, Lambros wrote:
> > nit: Fits on one line.
> 
> Done.
> 
> BTW: This EXPECT_CALL is not really necessary (because the Times(1) on the
> previous expectation will already reject further calls), but I put it here
> anyway because I think it improves readability of the test code.  But I guess
> (in light of your earlier comment about multiple EXPECT_CALLs) one can argue
> about the readability either way.

No it is necessary. Without it, the callback could be triggered exactly once,
but at the wrong time.

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback.h
File remoting/host/mock_callback.h (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:11: #include "base/callback.h"
On 2014/11/18 01:03:22, lukasza wrote:
> On 2014/11/18 00:19:29, Lambros wrote:
> > Could you use base/callback_forward.h instead?
> 
> Done.  It builds fine.  OTOH, I have a feeling it works only by virtue of
> includers of mock_callback.h already including base/callback.h - i.e.
> GetCallback method from the template can't be compiled without
base/callback.h,
> right?

Yes you're right, you do need the full definition of Callback, because
GetCallback() returns a base::Callback object by value. My mistake, sorry!

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:24: MOCK_CONST_METHOD0_T(Run, R());
On 2014/11/18 01:03:22, lukasza wrote:
> On 2014/11/18 00:19:29, Lambros wrote:
> > Hmm. This could be tricky to generalize to the n-parameter case? Maybe this
> > class should be named MockCallback0, in anticipation of this?
> 
> I am doing this in part#3.  I am not very happy with the amount of code
> duplication between templates for different arities, but it definitely works.

You're renaming this in a later CL? Why not do it here? :)

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:34: base::WeakPtrFactory<MockCallback<R()>>
weak_factory_;
On 2014/11/18 01:03:22, lukasza wrote:
> On 2014/11/18 00:19:29, Lambros wrote:
> > I'm not convinced we should use WeakPtr in this context. WeakPtr means that
> the
> > Run call will become a no-op in the situation where the class is deleted.
But
> in
> > that case, you will have a potential call that GMock doesn't know about, and
> > will silently violate expectations. Imagine a test passing because we
expected
> a
> > mock callback to never be called. But maybe it did actually get called, but
> the
> > object got deleted so gmock was never aware of the incorrect call?
> > 
> > I guess what you really want is some kind of assertion (in the gtest failure
> > sense, not a program crash) that the object doesn't get deleted during a
test?
> > I'm not sure of the right way to do this - maybe needs some wider
discussion?
> 
> Yes - this needs more discussion.  I think one way around this is to have an
> explicit destructor of MockCallback that does
> EXPECT_FALSE(weak_factory_.HasWeakPtrs()).  This would be safest probably.
No, this is too strong a condition. A test could legitimately destroy this
object before dropping WeakPtr references. The problem occurs when a callback is
*called* after the object is deleted.

> 
> Anyway - this is probably good enough for remoting/host/mock_callback.h,
right?
No, I still think this code is broken with WeakPtr, though I'm not sure what to
do about it at this stage. Using base::Unretained might actually be preferable
here? A potential use-after-free might be better than having tests falsely
passing?
 
> (agreed that this needs more discussion before we can argue about moving it to
> chromium/src/base/mock_callback.h).  If you think this should block part#2b,
> then can you suggest which bind_helpers.h we should use here?  ref-counting
one
> won't work for stack-allocated MockCallbacks.  base::Unretained is yucky.  I
> haven't tried ConstRef one but it also shouldn't work / seems yucky wrt
lifetime
> management of MockCallback vs generated callbacks.
My vote is to use base::Unretained for now, with a comment explaining that tests
need to guarantee the callback won't trigger after deletion. Let's see if other
reviewers have anything to say?

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:1324:
base::Bind(&HostProcess::OnHeartbeatSuccessful, base::Unretained(this)),
On 2014/11/18 01:03:22, lukasza wrote:
> On 2014/11/18 00:19:30, Lambros wrote:
> > Why do we need base::Unretained here? This is a ref-counted object.
> 
> This temporarily follows other references to |this| from this file.  I am
> changing to WeakPtr in part#3.

I see. I'm curious to know if this still compiles without the base::Unretained?
But OK to keep it for now, since it's used several times in this file already.

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
File remoting/host/heartbeat_sender.h (right):

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
remoting/host/heartbeat_sender.h:10: #include "base/callback.h"
callback_forward.h :)

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
remoting/host/heartbeat_sender_unittest.cc:10: #include "base/memory/weak_ptr.h"
Do we need weak_ptr.h here? Isn't WeakPtr a hidden implementation detail of
MockCallback?

[Łukasz Anforowicz, 2014-11-18 17:30:46]

Thanks for pushing back on avoiding WeakPtr in mock_callback.h;  after thinking
more about it I started to dislike WeakPtr because it only works with
single-threaded (well, sequenced-task-runner-ed to be exact) tests.  I didn't
make any changes yet but maybe we should indeed start with a simple
base::Unretained today, and we can (I think) beef it up later on.  

Can you please take a look at my reply to the WeakPtr comment and let's discuss
it later today.

I think the rest of the feedback is addressed.


-Lukasz

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:316: .Times(0);
On 2014/11/18 02:33:52, Lambros wrote:
> On 2014/11/18 01:03:22, lukasza wrote:
> > On 2014/11/18 00:19:29, Lambros wrote:
> > > nit: Fits on one line.
> > 
> > Done.
> > 
> > BTW: This EXPECT_CALL is not really necessary (because the Times(1) on the
> > previous expectation will already reject further calls), but I put it here
> > anyway because I think it improves readability of the test code.  But I
guess
> > (in light of your earlier comment about multiple EXPECT_CALLs) one can argue
> > about the readability either way.
> 
> No it is necessary. Without it, the callback could be triggered exactly once,
> but at the wrong time.

Oh, right - it is necessary.

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback.h
File remoting/host/mock_callback.h (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:11: #include "base/callback.h"
On 2014/11/18 02:33:52, Lambros wrote:
> On 2014/11/18 01:03:22, lukasza wrote:
> > On 2014/11/18 00:19:29, Lambros wrote:
> > > Could you use base/callback_forward.h instead?
> > 
> > Done.  It builds fine.  OTOH, I have a feeling it works only by virtue of
> > includers of mock_callback.h already including base/callback.h - i.e.
> > GetCallback method from the template can't be compiled without
> base/callback.h,
> > right?
> 
> Yes you're right, you do need the full definition of Callback, because
> GetCallback() returns a base::Callback object by value. My mistake, sorry!

Ok, I'll go back to including base/callback.h

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:24: MOCK_CONST_METHOD0_T(Run, R());
On 2014/11/18 02:33:52, Lambros wrote:
> On 2014/11/18 01:03:22, lukasza wrote:
> > On 2014/11/18 00:19:29, Lambros wrote:
> > > Hmm. This could be tricky to generalize to the n-parameter case? Maybe
this
> > > class should be named MockCallback0, in anticipation of this?
> > 
> > I am doing this in part#3.  I am not very happy with the amount of code
> > duplication between templates for different arities, but it definitely
works.
> 
> You're renaming this in a later CL? Why not do it here? :)

No, not renaming.  In part#3 I am adding an extra template that allows both
MockCallback<R()> (which is enabled by the current changelist / part#2b) as well
as MockCallback<R(A1)> (which is what part#3 enables).  This was MockCallback is
very similar to Callback (i.e. there is no Callback0 or Callback1).

https://codereview.chromium.org/734053003/diff/1/remoting/host/mock_callback....
remoting/host/mock_callback.h:34: base::WeakPtrFactory<MockCallback<R()>>
weak_factory_;
On 2014/11/18 02:33:52, Lambros wrote:
> On 2014/11/18 01:03:22, lukasza wrote:
> > On 2014/11/18 00:19:29, Lambros wrote:
> > > I'm not convinced we should use WeakPtr in this context. WeakPtr means
that
> > the
> > > Run call will become a no-op in the situation where the class is deleted.
> But
> > in
> > > that case, you will have a potential call that GMock doesn't know about,
and
> > > will silently violate expectations. Imagine a test passing because we
> expected
> > a
> > > mock callback to never be called. But maybe it did actually get called,
but
> > the
> > > object got deleted so gmock was never aware of the incorrect call?
> > > 
> > > I guess what you really want is some kind of assertion (in the gtest
failure
> > > sense, not a program crash) that the object doesn't get deleted during a
> test?
> > > I'm not sure of the right way to do this - maybe needs some wider
> discussion?
> > 
> > Yes - this needs more discussion.  I think one way around this is to have an
> > explicit destructor of MockCallback that does
> > EXPECT_FALSE(weak_factory_.HasWeakPtrs()).  This would be safest probably.
> No, this is too strong a condition. A test could legitimately destroy this
> object before dropping WeakPtr references. The problem occurs when a callback
is
> *called* after the object is deleted.
> 
> > 
> > Anyway - this is probably good enough for remoting/host/mock_callback.h,
> right?
> No, I still think this code is broken with WeakPtr, though I'm not sure what
to
> do about it at this stage. Using base::Unretained might actually be preferable
> here? A potential use-after-free might be better than having tests falsely
> passing?
>  
> > (agreed that this needs more discussion before we can argue about moving it
to
> > chromium/src/base/mock_callback.h).  If you think this should block part#2b,
> > then can you suggest which bind_helpers.h we should use here?  ref-counting
> one
> > won't work for stack-allocated MockCallbacks.  base::Unretained is yucky.  I
> > haven't tried ConstRef one but it also shouldn't work / seems yucky wrt
> lifetime
> > management of MockCallback vs generated callbacks.
> My vote is to use base::Unretained for now, with a comment explaining that
tests
> need to guarantee the callback won't trigger after deletion. Let's see if
other
> reviewers have anything to say?

I've been thinking about this a bit more and I am still not sure what is the
best approach here.  Let me try to do a more structured comparison of available
approaches.

Let's try to list features we want and/or issues we want to avoid:

1. Enable explicit checking of HasRemainingCallbacks (this check is important
for ack_or_timeout_reporter_unittest.cc in part#3).  This is easy with WeakPtr
(HasWeakPtrs) and RefCounted (HasOneRef) and possible with Unretained (with
callbacks tgoing hrough extra weakref-like object that is explicitly marked as
dead by destructor of MockCallback).

2. Support multi-threaded test scenarios.  This is not supported by WeakPtr,
easy with RefCountedThreadSafe and possible with Unretained (some kind of
locking might be needed to avoid one thread destroying MockCallback *while*
another thread is calling the generated callback; there is also a question of
what to do in case of reentrancy [when MockCallback is being destroyed from
inside the generated callback]).

3. What happens when callbacks are called after MockCallback is already
destroyed.  Side note - this is similar to a situation where all gMock
expectations are retired - in this case gMock doesn't fail a test, but only
issues a warning
(https://code.google.com/p/googlemock/wiki/CookBook#Ignoring_Uninteresting_Calls).
 With indirection (i.e. calls going through another long-lived object that is
aware of whether MockCallback is alive) this seems possible in all 3 approaches.
 OTOH, this is easiest with RefCounted approach - gMock expectations in this
case can directly influence what happens (i.e. falling back to  default gMock
behavior when MockCallback and expectations are dead);  with other approaches
one would have to use a one-off mechanism through which users could say what
should happen during a callback call when MockCallback is already destroyed.

4. Ease of use.
   With WeakPtr and Unretaied mock callback variable can be declared as:
   MockCallback<void(int)> my_mock_callback;
   With RefCounted, a variable has to be declared as:
   scoped_refptr<MockCallback<void(int)>> my_mock_callback(
       new MockCallback<void(int)>());

I think I am leaning toward Unretained approach right now.  Can we discuss this
later today?

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/734053003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:1324:
base::Bind(&HostProcess::OnHeartbeatSuccessful, base::Unretained(this)),
On 2014/11/18 02:33:52, Lambros wrote:
> On 2014/11/18 01:03:22, lukasza wrote:
> > On 2014/11/18 00:19:30, Lambros wrote:
> > > Why do we need base::Unretained here? This is a ref-counted object.
> > 
> > This temporarily follows other references to |this| from this file.  I am
> > changing to WeakPtr in part#3.
> 
> I see. I'm curious to know if this still compiles without the
base::Unretained?
> But OK to keep it for now, since it's used several times in this file already.

It would compile without base::Unretained and would even work, but would mean
that HeartbeatSender (by holding the callbacks) is extending the lifetime of
HostProcess - this different from current code and I don't want to change it
unnecessarily (although is probably fine as HostProcess explicitly destroys
HeartbeatSender in one of Shutdown methods; but then again HeartbeatSender will
outlive HostProcess after part#3, so I rather won't muck with this
unnecessarily).

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
File remoting/host/heartbeat_sender.h (right):

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
remoting/host/heartbeat_sender.h:10: #include "base/callback.h"
On 2014/11/18 02:33:52, Lambros wrote:
> callback_forward.h :)

What about the base::Closure by-value / not-a-pointer-or-reference fields being
added below?  (I wish Rietveld would show line numbers so I can more easily
refer to other parts of the code being reviewed :-/).

[Lambros, 2014-11-18 18:47:21]

On the CL description:
This is a small self-contained CL. So I think it's better to have the first line
be a description of what the CL does. So you could put as the first line:
"Convert HeartbeatSender to use Callbacks rather than
Listener pattern."

In the rest of the description, you could go on to explain how this CL helps
with the bigger thing you're doing:
"This is part 2b of the work to implement reporting of policy errors via
host-offline-reason..."

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
File remoting/host/heartbeat_sender.h (right):

https://codereview.chromium.org/734053003/diff/20001/remoting/host/heartbeat_...
remoting/host/heartbeat_sender.h:10: #include "base/callback.h"
On 2014/11/18 17:30:45, lukasza wrote:
> On 2014/11/18 02:33:52, Lambros wrote:
> > callback_forward.h :)
> 
> What about the base::Closure by-value / not-a-pointer-or-reference fields
being
> added below?  (I wish Rietveld would show line numbers so I can more easily
> refer to other parts of the code being reviewed :-/).

Yes you're right, didn't see those. I only scanned for "Callback" :)

[Łukasz Anforowicz, 2014-11-18 19:31:43]

Patchset #4 (id:60001) has been deleted

[Łukasz Anforowicz, 2014-11-18 20:31:24]

Switched to base::Unretained in mock_callback.h
(I want to keep mock_callback.h in this changelist simple and therefore limited
to MockClosure only;  OTOH, it might be still be useful to take a peek at
mock_callback.h and mock_callback_unittest.cc in part3 to see where this is
going - https://codereview.chromium.org/719983002/)

Thanks,

Lukasz

[Lambros, 2014-11-18 21:57:56]

lgtm

https://codereview.chromium.org/734053003/diff/80001/remoting/host/heartbeat_...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/80001/remoting/host/heartbeat_...
remoting/host/heartbeat_sender_unittest.cc:10: #include "base/memory/weak_ptr.h"
Do you need weark_ptr.h ?

[Łukasz Anforowicz, 2014-11-18 22:16:54]

Fixed the unnecessary include of weak_ptr.h

https://codereview.chromium.org/734053003/diff/80001/remoting/host/heartbeat_...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/734053003/diff/80001/remoting/host/heartbeat_...
remoting/host/heartbeat_sender_unittest.cc:10: #include "base/memory/weak_ptr.h"
On 2014/11/18 21:57:55, Lambros wrote:
> Do you need weark_ptr.h ?

I don't.  Thanks for pointing this out (I've taken out this include from
mock_callback.h;  I am not sure why I've also had it here;  I'll go through the
overall diff once more).

[Łukasz Anforowicz, 2014-11-26 17:39:45]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2014-11-26 17:40:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/734053003/120001

[commit-bot: I haz the power, 2014-11-26 18:20:00]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-11-26 18:21:18]

Patchset 6 (id:??) landed as
https://crrev.com/ead20f1a85c6cb6f3faf6042bea56126a7078854
Cr-Commit-Position: refs/heads/master@{#305836}