Reporting of policy errors via host-offline-reason: part 3

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
index ca3feb6d06a415f1853b363e7b089f8b0a03ad7a..8f6bedfa44f06ef5b4e86dc0ba1e3ab6516556b8 100644
--- a/remoting/host/mock_callback.h
+++ b/remoting/host/mock_callback.h
@@ -10,11 +10,69 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
 
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace remoting {
 
+// MockCallback<Sig> can be used to mock Callback<Sig> from base/callback.h
+//
+// Basic usage example:
+//
+//   MockCallback<void(int)> mock_callback;
+//   EXPECT_CALL(mock_callback, Run(123)).Times(2);
+//   base::Callback<void(int)> callback = mock_callback.GetCallback();

[Wez, 2014/12/03 19:45:49]

Is it important that GetCallback() is only called after EXPECT_CALL()
expectations have been set?

How many times can GetCallback() be called on MockCallback?

If it can be called multiple times then what are the rules around the
expectations being matched?

nit: MockCallback seems a misleading name, since it's not a mock callback in
itself, but seemingly a factory for mocked callbacks?

[Łukasz Anforowicz, 2014/12/03 22:00:44]

Sort of.  EXPECT_CALL is not thread-safe wrt Callback::Run.  This is sort of
covered already in "Thread safety notes".  I've also added a comment next to the
example.
Can be called multiple times.  All Callbacks retrieved in this way are subject
to gMock expectations set on MockCalback::Run method.  I've added a comment
where I try to explain this.
Hmmm... interesting.  Yes - maybe this should be renamed to MockCallbackFactory.

+//
+//   // Use |callback| in the remainder of the test.
+//   // GMock will enforce the expectations from above.
+//
+// HasRemainingCallbacks usage example:

[Wez, 2014/12/03 19:45:49]

HasRemainingCallbacks has not been introduced yet, so there is no context for
the reader - perhaps you mean "Example of verifying that callback instances were
correctly freed"?

It seems that this capability is the only reason we need this class - test can
otherwise just EXPECT_CALL on test fixture methods and then Bind() to those to
get callbacks - is it actually useful enough to justify this extra complexity,
given that we already have tools like ASan and TSan to trap threading/memory
issues?

[Łukasz Anforowicz, 2014/12/03 22:00:44]

Good point.  I am not sure about this
- Some tests (i.e. ack_or_timeout_reporter_unittest.cc) don't have a test
fixture, just test functions.  I also don't quite like the aesthetics of having
to declare mocked Run in the test fixture (kind of globally) and using them in
individual tests (although this particular aesthetics concern is easily
addressed by the next item below).  
- Some tests decided to introduce MockCallback class (with the mocked method
similar to the one you are suggesting to be embedded in a test fixture).  This
works but it seemed nice at one point to let the tests avoid having to declare a
one-off class in every test that needs to mock a callback. 
- When other tests introduced separate MockCallback classes, they sometimes
unnecessarily exposed ref-counting to the test
(i.e. see:
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/test_he...)

But other than that, you are correct - this functionality can be achieved in
other ways with some extra class and/or mocked-method declarations next to test
suites (except for HasRemainingCallbacks).  BTW: HasRemainingCallbacks can be
done outside of Callback by having test maintain its own WeakPtrFactory (to an
instance of whatever class declares a mocked Run), passing a weak_ptr to
base::Bind and asserting weak_ptr_factory.HasWeakPtrs().

To help decide, I got rid of mock_callback.h in a separate patchset - the test
code has to do a bit more in this case, but only slightly.  If you think
patchset#10 (with mock_callback.h) doesn't have any advantage over patchset#11
(without mock_callback.h) then let's get rid of mock_callback.h (especially
since it would probably receive similar feedback when trying to move that to a
chromium-wide location [which was my intention]). 

+//
+//   MockCallback<int()> mock_callback;
+//   EXPECT_CALL(mock_callback, Run()).WillOnce(Return(123));
+//   HypotheticalFunctionUnderTest(mock_callback.GetCallback());
+//   EXPECT_FALSE(mock_callback.HasRemainingCallbacks())
+//      << "Verify that FunctionUnderTest didn't store the callback "
+//      << "and therefore callers of FunctionUnderTest don't have to "
+//      << "worry about lifetime of arguments bound to the callback";

[Wez, 2014/12/03 19:45:49]

You need to be very clear on the threading semantics for a check like this,
otherwise you may get a test that passes HasRemainingCallbacks() because thread
timings happen to tear things down at the right times, but then it fails
mysteriously now and again when the timings change and the object lives a little
longer on some background thread.

[Łukasz Anforowicz, 2014/12/03 22:00:44]

I don't understand:
- HasRemainingCallbacks is thread-safe (by virtue of atomic/interlocked
operations via RefCountedThreadSafe<T>).
- HasRemainingCallbacks is subject to race conditions.  This is sort of obvious.
 What would I say in a comment to document this?

+//
+// Thread safety notes:
+//
+//   Calling Run on a Callback returned from MockCallback::GetCallback
+//   - Is *not* thread-safe wrt setting default actions and expectations (via
+//     ON_CALL and EXPECT_CALL gMock macros).

[Wez, 2014/12/03 19:45:49]

This is not very clear; you mean that the call expectations must have been set
before any thread calls Run()? Or something else?

[Łukasz Anforowicz, 2014/12/03 22:00:44]

One cannot call EXPECT_CALL and Run in parallel.  It is ok to have a sequence of
calls EXPECT_CALL, Run, EXPECT_CALL (overrides previous expectations), Run.

+//   - Is thread-safe wrt other Run calls and destroying MockCallback.

[Wez, 2014/12/03 19:45:49]

What are the semantics if a callback still exists when the MockCallback is
destroyed?

The comment on GetCallback() explicitly states that the caller is responsible
for guaranteeing that nothing calls Run() on the callback after the MockCallback
has been destroyed?

[Łukasz Anforowicz, 2014/12/03 22:00:44]

This is documented in "What happens when Callback::Run is called after
MockCallbackFactory is destroyed" comment.
Ooops... this is a stale comment.  I'll fix this.

+//
+//   This is similar to the guarantees described at
+//   https://code.google.com/p/googlemock/wiki/CookBook#Using_Google_Mock_and_Threads
+//   except that the guarantees hold on all platforms.
+//
+// What happens when Callback::Run is called after MockCallback is destroyed:

[Wez, 2014/12/03 19:45:49]

This contradicts the GetCallback comment, though.

[Łukasz Anforowicz, 2014/12/03 22:00:44]

Acknowledged.

+//
+//   - gTest EXPECT failure will be triggered
+//   - gMock's default return value will be returned - see:
+//     https://code.google.com/p/googlemock/wiki/CookBook#Setting_the_Default_Value_for_a_Return_Type
+
+// Implementation notes:
+//
+// WeakPtr-like code
+// -----------------
+//
+// Relationship between MockCallback and MockCallbackTracker is similar
+// to the relationship between WeakPtrFactory<T> and WeakPtr<T>.  We don't
+// use WeakPtrFactory<T> and WeakPtr<T>, because:

[Wez, 2014/12/03 19:45:49]

This comment is very confusing; it's not clear what similarities they share,
especially since you then go on to describe differences in threading
limitations, at least one of which is fundamental to what WeakPtr is.

MockCallbackTracker also hasn't been mentioned in any of the preceding text, so
it's confusing to introduce it here without any reference to its purpose.

+//  - WeakPtrs can only bind to methods without return values
+//    (enforced by a static assert in bind_internal.h:402).
+//  - WeakPtr impose task-specific, thread-safety-restrictions that are
+//    not present in Callback that MockCallback is mocking.
+//  - For MockCallback we don't necessarily want the behavior where Callbacks
+//    bound to WeakPtr are translated into no-ops, before the bound function
+//    gets to execute.  In MockCallback case we want the test to have control of
+//    what happens in this case (i.e. gtest EXPECT, rather than a no-op;
+//    note that we cannot use gtest ASSERT for non-void-returning Run method).
+
 template <typename Sig>
 class MockCallback;
 
@@ -23,16 +81,114 @@ class MockCallback<R()> {
  public:
   MOCK_CONST_METHOD0_T(Run, R());
 
-  MockCallback() {
+  MockCallback() : mock_callback_tracker_(new MockCallbackTracker(this)) {}
+
+  ~MockCallback() { mock_callback_tracker_->MarkMockCallbackAsDead(); }
+
+  // Caller of GetCallback has to guarantee that the returned callback
+  // will not be run after |this| is destroyed.
+  base::Callback<R()> GetCallback() const {
+    return base::Bind(&MockCallbackTracker::Run, mock_callback_tracker_);
+  }
+
+  // Returns |true| if callbacks returned by |GetCallback| have not been
+  // destroyed yet.
+  bool HasRemainingCallbacks() const {
+    bool is_tracker_field_the_only_ref = mock_callback_tracker_->HasOneRef();
+    return !is_tracker_field_the_only_ref;
   }
 
+ private:
+  class MockCallbackTracker
+      : public base::RefCountedThreadSafe<MockCallbackTracker> {
+   public:
+    MockCallbackTracker(MockCallback* mock_callback)
+        : mock_callback_(mock_callback), is_mock_callback_alive_(true) {}
+
+    R Run() {
+      base::AutoLock auto_lock(lock_);
+      EXPECT_TRUE(is_mock_callback_alive_)
+          << "MockCallback should be alive when Callback::Run happens.";
+      return is_mock_callback_alive_ ? mock_callback_->Run()
+                                     : testing::DefaultValue<R>::Get();
+    }
+
+    void MarkMockCallbackAsDead() {
+      base::AutoLock auto_lock(lock_);
+      is_mock_callback_alive_ = false;
+      mock_callback_ = nullptr;
+    }
+
+   private:
+    // Ref-counting helpers.
+    friend class base::RefCountedThreadSafe<MockCallbackTracker>;
+    ~MockCallbackTracker() {}
+
+    MockCallback<R()>* mock_callback_;
+    base::Lock lock_;
+    bool is_mock_callback_alive_;
+  };
+
+  scoped_refptr<MockCallbackTracker> mock_callback_tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockCallback);
+};
+
+template <typename R, typename A1>
+class MockCallback<R(A1)> {
+ public:
+  MOCK_CONST_METHOD1_T(Run, R(A1));
+
+  MockCallback() : mock_callback_tracker_(new MockCallbackTracker(this)) {}
+
+  ~MockCallback() { mock_callback_tracker_->MarkMockCallbackAsDead(); }
+
   // Caller of GetCallback has to guarantee that the returned callback
   // will not be run after |this| is destroyed.
-  base::Callback<R()> GetCallback() {
-    return base::Bind(&MockCallback<R()>::Run, base::Unretained(this));
+  base::Callback<R(A1)> GetCallback() const {
+    return base::Bind(&MockCallbackTracker::Run, mock_callback_tracker_);
+  }
+
+  // Returns |true| if callbacks returned by |GetCallback| have not been
+  // destroyed yet.
+  bool HasRemainingCallbacks() const {
+    bool is_tracker_field_the_only_ref = mock_callback_tracker_->HasOneRef();
+    return !is_tracker_field_the_only_ref;
   }
 
  private:
+  class MockCallbackTracker
+      : public base::RefCountedThreadSafe<MockCallbackTracker> {
+   public:
+    MockCallbackTracker(MockCallback* mock_callback)
+        : mock_callback_(mock_callback), is_mock_callback_alive_(true) {}
+
+    R Run(A1 a1) {
+      base::AutoLock auto_lock(lock_);
+      EXPECT_TRUE(is_mock_callback_alive_)
+          << "MockCallback should be alive when Callback::Run happens.";
+      return is_mock_callback_alive_ ? mock_callback_->Run(a1)
+                                     : testing::DefaultValue<R>::Get();
+    }
+
+    void MarkMockCallbackAsDead() {
+      base::AutoLock auto_lock(lock_);
+      is_mock_callback_alive_ = false;
+      mock_callback_ = nullptr;
+    }
+
+   private:
+    // Ref-counting helpers.
+    friend class base::RefCountedThreadSafe<MockCallbackTracker>;
+    ~MockCallbackTracker() {}
+
+    MockCallback<R(A1)>* mock_callback_;
+    base::Lock lock_;
+    bool is_mock_callback_alive_;
+  };
+
+  scoped_refptr<MockCallbackTracker> mock_callback_tracker_;
+
   DISALLOW_COPY_AND_ASSIGN(MockCallback);
 };