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

remoting/host/mock_callback.h

Index: remoting/host/mock_callback.h
diff --git a/remoting/host/mock_callback.h b/remoting/host/mock_callback.h
new file mode 100644
index 0000000000000000000000000000000000000000..3334670ca2d4aeaeac199da0fcc427a98819d64c
--- /dev/null
+++ b/remoting/host/mock_callback.h
@@ -0,0 +1,43 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_MOCK_CALLBACK_H_
+#define REMOTING_HOST_MOCK_CALLBACK_H_
+
+// TODO(lukasza): If possible, move this header file to base/mock_callback.h
+
+#include "base/bind.h"
+#include "base/callback.h"

[Lambros, 2014/11/18 00:19:29]

Could you use base/callback_forward.h instead?

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

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?

[Lambros, 2014/11/18 02:33:52]

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

[Łukasz Anforowicz, 2014/11/18 17:30:45]

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

+#include "base/memory/weak_ptr.h"
+
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace remoting {
+
+template <typename Sig>
+class MockCallback;
+
+template <typename R>
+class MockCallback<R()> {
+ public:
+  MOCK_CONST_METHOD0_T(Run, R());

[Lambros, 2014/11/18 00:19:29]

Hmm. This could be tricky to generalize to the n-parameter case? Maybe this
class should be named MockCallback0, in anticipation of this?

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

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.

[Lambros, 2014/11/18 02:33:52]

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

[Łukasz Anforowicz, 2014/11/18 17:30:45]

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).

+
+  MockCallback() : weak_factory_(this) {
+  }
+
+  base::Callback<R()> GetCallback() {
+    return base::Bind(&MockCallback<R()>::Run, weak_factory_.GetWeakPtr());
+  }
+
+ private:
+  base::WeakPtrFactory<MockCallback<R()>> weak_factory_;

[Lambros, 2014/11/18 00:19:29]

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?

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

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.

[Lambros, 2014/11/18 02:33:52]

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.
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?
 
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?

[Łukasz Anforowicz, 2014/11/18 17:30:45]

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?

+
+  DISALLOW_COPY_AND_ASSIGN(MockCallback);
+};
+
+typedef MockCallback<void(void)> MockClosure;
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_MOCK_CALLBACK_H_