CPP bindings: DCHECK when a Callback is destructed without being invoked

mojo/public/cpp/bindings/tests/binding_callback_test.cc

Index: mojo/public/cpp/bindings/tests/binding_callback_test.cc
diff --git a/mojo/public/cpp/bindings/tests/binding_callback_test.cc b/mojo/public/cpp/bindings/tests/binding_callback_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1fc917cab9a61a161c25cd9e819181b39b597594
--- /dev/null
+++ b/mojo/public/cpp/bindings/tests/binding_callback_test.cc
@@ -0,0 +1,281 @@
+// Copyright 2015 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.
+#include "mojo/public/cpp/bindings/interface_ptr.h"

[jamesr, 2015/03/16 21:05:56]

nit: blank line after copyright header

[rudominer, 2015/03/16 22:57:19]

Done.

+#include "mojo/public/cpp/environment/environment.h"
+#include "mojo/public/cpp/test_support/test_support.h"
+#include "mojo/public/cpp/utility/run_loop.h"
+#include "mojo/public/interfaces/bindings/tests/binding_callback_test_interfaces.mojom.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+///////////////////////////////////////////////////////////////////////////////
+//
+// The tests in this file are designed to test the interaction between a
+// Callback and its associated Binding. If a Callback is deleted before
+// being used we DCHECK fail--unless the associated Binding has already
+// been closed or deleted. This contract must be explained to the Mojo
+// application developer. For example it is his responsibility to ensure that

[jamesr, 2015/03/16 21:05:57]

nit: avoid gendered pronouns like "his" in documentation. either go with
something neutral like "they" or rephrase to avoid needing the pronoun

[rudominer, 2015/03/16 22:57:19]

Done.

+// the Binding is destroyed before an unused Callback is destroyed.
+//
+///////////////////////////////////////////////////////////////////////////////
+
+namespace mojo {
+namespace test {
+namespace {
+
+// A Runnable object that saves the last value it sees via the
+// provided int32_t*. Used on the client side.
+class ValueSaver {
+ public:
+  explicit ValueSaver(int32_t* last_value_seen)
+      : last_value_seen_(last_value_seen) {}
+  void Run(int32_t x) const { *last_value_seen_ = x; }
+
+ private:
+  mutable int32_t* last_value_seen_;

[jamesr, 2015/03/16 21:05:57]

this can be const as the pointer value isn't changed and it is a pointer to a
non-const int32_t

[rudominer, 2015/03/16 22:57:19]

Done.

+};
+
+// An implementation of BindingCallbackTestInterface used on the server side.
+// All it does is save the values and Callbacks it sees.
+class BindingCallbackTestInterfaceImpl : public BindingCallbackTestInterface {
+ public:
+  BindingCallbackTestInterfaceImpl()
+      : last_server_value_seen(0),
+        callback_saved(new Callback<void(int32_t)>()) {}
+
+  ~BindingCallbackTestInterfaceImpl() override {
+    if (callback_saved) {
+      delete callback_saved;
+    }
+  }
+
+  // Run's the callback previously saved from the last invocation
+  // of |EchoInt()|.
+  bool RunCallback() {
+    if (callback_saved) {
+      callback_saved->Run(last_server_value_seen);
+      return true;
+    }
+    return false;
+  }
+
+  // Delete's the previously saved callback.
+  void DeleteCallback() {
+    delete callback_saved;
+    callback_saved = nullptr;
+  }
+
+  // Saves its two input values in member variables and does nothing else.
+  void EchoInt(int32_t x, const Callback<void(int32_t)>& callback) {
+    last_server_value_seen = x;
+    *callback_saved = callback;
+  }
+
+  int last_server_value_seen;

[jamesr, 2015/03/16 21:05:56]

s/int/int32_t/

if you want these to be private since this is a class add a private: declaration
and give these trailing underscores (and getters if needed)

[rudominer, 2015/03/16 22:57:19]

Done.

+  Callback<void(int32_t)>* callback_saved;
+};
+
+class BindingCallbackTest : public testing::Test {
+ public:
+  ~BindingCallbackTest() override {}
+
+ protected:
+  int last_client_callback_value_seen_;
+  ScopedMessagePipeHandle handle_client_;

[jamesr, 2015/03/16 21:05:56]

can these have more specific types (i.e. are these handles always used for the
same interface type)?

[rudominer, 2015/03/16 22:57:20]

Help me out here. I'm not familiar with any more specific types. What are you
referring to?

+  ScopedMessagePipeHandle handle_server_;
+  BindingCallbackTestInterfacePtr interface_ptr_;
+
+  void PumpMessages() { loop_.RunUntilIdle(); }
+  void SetUp() {
+    CreateMessagePipe(nullptr, &handle_client_, &handle_server_);
+    // Create the client InterfacePtr.
+    interface_ptr_ =
+        MakeProxy<BindingCallbackTestInterface>(handle_client_.Pass());
+  }
+
+ private:
+  Environment env_;
+  RunLoop loop_;
+};
+
+// Tests that the InterfacePtr and the Binding can communicate with each
+// other normally.
+TEST_F(BindingCallbackTest, Basic) {
+  // Create the ServerImpl and the Binding.
+  BindingCallbackTestInterfaceImpl server_impl;
+  Binding<BindingCallbackTestInterface> binding(
+      &server_impl,
+      MakeRequest<BindingCallbackTestInterface>(handle_server_.Pass()));
+
+  // Initialize the test values.
+  server_impl.last_server_value_seen = 0;
+  last_client_callback_value_seen_ = 0;
+
+  // Invoke the Echo method.
+  interface_ptr_->EchoInt(7, ValueSaver(&last_client_callback_value_seen_));
+  PumpMessages();
+
+  // Check that server saw the correct value, but the client has not yet.
+  EXPECT_EQ(7, server_impl.last_server_value_seen);
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Now run the Callback.
+  server_impl.RunCallback();
+  PumpMessages();
+
+  // Check that the client has now seen the correct value.
+  EXPECT_EQ(7, last_client_callback_value_seen_);
+
+  // Initialize the test values again.
+  server_impl.last_server_value_seen = 0;
+  last_client_callback_value_seen_ = 0;
+
+  // Invoke the Echo method again.
+  interface_ptr_->EchoInt(13, ValueSaver(&last_client_callback_value_seen_));
+  PumpMessages();
+
+  // Check that server saw the correct value, but the client has not yet.
+  EXPECT_EQ(13, server_impl.last_server_value_seen);
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Now run the Callback again.
+  server_impl.RunCallback();
+  PumpMessages();
+
+  // Check that the client has now seen the correct value again.
+  EXPECT_EQ(13, last_client_callback_value_seen_);
+}
+
+// Tests that running the Callback after the Binding has been deleted
+// results in a clean failure.
+TEST_F(BindingCallbackTest, DeleteBindingThenRunCallback) {
+  // Create the ServerImpl.
+  BindingCallbackTestInterfaceImpl server_impl;
+  {
+    // Create the binding in an inner scope so it can be deleted first.
+    Binding<BindingCallbackTestInterface> binding(
+        &server_impl,
+        MakeRequest<BindingCallbackTestInterface>(handle_server_.Pass()));
+
+    // Initialize the test values.
+    server_impl.last_server_value_seen = 0;
+    last_client_callback_value_seen_ = 0;
+
+    // Invoke the Echo method.
+    interface_ptr_->EchoInt(7, ValueSaver(&last_client_callback_value_seen_));
+    PumpMessages();
+  }
+  // The binding has now been destroyed and the pipe is closed.
+
+  // Check that server saw the correct value, but the client has not yet.
+  EXPECT_EQ(7, server_impl.last_server_value_seen);
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Now try to run the Callback. This should do nothing since the pipe
+  // is closed.
+  EXPECT_TRUE(server_impl.RunCallback());
+  PumpMessages();
+
+  // Check that the client has still not seen the correct value.
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Attempt to invoke the method again and confirm that an error was
+  // encountered.
+  interface_ptr_->EchoInt(13, ValueSaver(&last_client_callback_value_seen_));
+  PumpMessages();
+  EXPECT_TRUE(interface_ptr_.encountered_error());
+}
+
+// Tests that deleting a Callback without running it after the corresponding
+// binding has already been deleted does not result in a crash.
+TEST_F(BindingCallbackTest, DeleteBindingThenDeleteCallback) {
+  // Create the ServerImpl.
+  BindingCallbackTestInterfaceImpl server_impl;
+  {
+    // Create the binding in an inner scope so it can be deleted first.
+    Binding<BindingCallbackTestInterface> binding(
+        &server_impl,
+        MakeRequest<BindingCallbackTestInterface>(handle_server_.Pass()));
+
+    // Initialize the test values.
+    server_impl.last_server_value_seen = 0;
+    last_client_callback_value_seen_ = 0;
+
+    // Invoke the Echo method.
+    interface_ptr_->EchoInt(7, ValueSaver(&last_client_callback_value_seen_));
+    PumpMessages();
+  }
+  // The binding has now been destroyed and the pipe is closed.
+
+  // Check that server saw the correct value, but the client has not yet.
+  EXPECT_EQ(7, server_impl.last_server_value_seen);
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Delete the callback without running it. This should not
+  // cause a problem because the insfrastructure can detect that the
+  // binding has already been destroyed and the pipe is closed.
+  server_impl.DeleteCallback();
+}
+
+// Tests that closing a Binding allows us to delete a callback
+// without running it without encountering a crash.
+TEST_F(BindingCallbackTest, CloseBindingBeforeDeletingCallback) {
+  // Create the ServerImpl and the Binding.
+  BindingCallbackTestInterfaceImpl server_impl;
+  Binding<BindingCallbackTestInterface> binding(
+      &server_impl,
+      MakeRequest<BindingCallbackTestInterface>(handle_server_.Pass()));
+
+  // Initialize the test values.
+  server_impl.last_server_value_seen = 0;
+  last_client_callback_value_seen_ = 0;
+
+  // Invoke the Echo method.
+  interface_ptr_->EchoInt(7, ValueSaver(&last_client_callback_value_seen_));
+  PumpMessages();
+
+  // Check that server saw the correct value, but the client has not yet.
+  EXPECT_EQ(7, server_impl.last_server_value_seen);
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Now close the Binding.
+  binding.Close();
+
+  // Delete the callback without running it. This should not
+  // cause a crash because the insfrastructure can detect that the
+  // binding has already been closed.
+  server_impl.DeleteCallback();
+
+  // Check that the client has still not seen the correct value.
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+}
+
+// Tests that deleting a Callback without using it before the
+// Binding has been destroyed or closed results in a DCHECK.
+TEST_F(BindingCallbackTest, DeleteCallbackBeforeBindingDeathTest) {
+  // Create the ServerImpl and the Binding.
+  BindingCallbackTestInterfaceImpl server_impl;
+  Binding<BindingCallbackTestInterface> binding(
+      &server_impl,
+      MakeRequest<BindingCallbackTestInterface>(handle_server_.Pass()));
+
+  // Initialize the test values.
+  server_impl.last_server_value_seen = 0;
+  last_client_callback_value_seen_ = 0;
+
+  // Invoke the Echo method.
+  interface_ptr_->EchoInt(7, ValueSaver(&last_client_callback_value_seen_));
+  PumpMessages();
+
+  // Check that server saw the correct value, but the client has not yet.
+  EXPECT_EQ(7, server_impl.last_server_value_seen);
+  EXPECT_EQ(0, last_client_callback_value_seen_);
+
+  // Delete the callback without running it. This should cause a crash
+  // due to a DCHECK.
+  EXPECT_DEATH(server_impl.DeleteCallback(), "Check failed: was_run_");
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace mojo