Mojo: Allow InterfacePtr<> to be deleted during a client method call.

BUG=383917

Review URL: https://codereview.chromium.org/332943004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@277251 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 109 insertions, 8 deletions

diff --git a/mojo/public/cpp/bindings/lib/connector.cc b/mojo/public/cpp/bindings/lib/connector.cc
index 8b97ff7..8054830 100644
--- a/mojo/public/cpp/bindings/lib/connector.cc
+++ b/mojo/public/cpp/bindings/lib/connector.cc
@@ -23,13 +23,17 @@ Connector::Connector(ScopedMessagePipeHandle message_pipe,
       async_wait_id_(0),
       error_(false),
       drop_writes_(false),
-      enforce_errors_from_incoming_receiver_(true) {
+      enforce_errors_from_incoming_receiver_(true),
+      destroyed_flag_(NULL) {
   // Even though we don't have an incoming receiver, we still want to monitor
   // the message pipe to know if is closed or encounters an error.
   WaitToReadMore();
 }
 
 Connector::~Connector() {
+  if (destroyed_flag_)
+    *destroyed_flag_ = true;
+
   if (async_wait_id_)
     waiter_->CancelWait(async_wait_id_);
 }
@@ -97,7 +101,9 @@ void Connector::OnHandleReady(MojoResult result) {
   async_wait_id_ = 0;
 
   if (result == MOJO_RESULT_OK) {
-    ReadMore();
+    // Return immediately if |this| was destroyed. Do not touch any members!
+    if (!ReadMore())
+      return;
   } else {
     error_ = true;
   }
@@ -114,11 +120,26 @@ void Connector::WaitToReadMore() {
                                       this);
 }
 
-void Connector::ReadMore() {
+bool Connector::ReadMore() {
   while (true) {
     bool receiver_result = false;
+
+    // Detect if |this| was destroyed during message dispatch. Allow for the
+    // possibility of re-entering ReadMore() through message dispatch.
+    bool was_destroyed_during_dispatch = false;
+    bool* previous_destroyed_flag = destroyed_flag_;
+    destroyed_flag_ = &was_destroyed_during_dispatch;
+
     MojoResult rv = ReadAndDispatchMessage(
         message_pipe_.get(), incoming_receiver_, &receiver_result);
+
+    if (was_destroyed_during_dispatch) {
+      if (previous_destroyed_flag)
+        *previous_destroyed_flag = true;  // Propagate flag.
+      return false;
+    }
+    destroyed_flag_ = previous_destroyed_flag;
+
     if (rv == MOJO_RESULT_SHOULD_WAIT) {
       WaitToReadMore();
       break;
@@ -129,6 +150,7 @@ void Connector::ReadMore() {
       break;
     }
   }
+  return true;
 }
 
 }  // namespace internal
diff --git a/mojo/public/cpp/bindings/lib/connector.h b/mojo/public/cpp/bindings/lib/connector.h
index ef503d4..d4b26f4 100644
--- a/mojo/public/cpp/bindings/lib/connector.h
+++ b/mojo/public/cpp/bindings/lib/connector.h
@@ -69,7 +69,9 @@ class Connector : public MessageReceiver {
   void OnHandleReady(MojoResult result);
 
   void WaitToReadMore();
-  void ReadMore();
+
+  // Returns false if |this| was destroyed during message dispatch.
+  MOJO_WARN_UNUSED_RESULT bool ReadMore();
 
   ErrorHandler* error_handler_;
   const MojoAsyncWaiter* waiter_;
@@ -82,6 +84,11 @@ class Connector : public MessageReceiver {
   bool drop_writes_;
   bool enforce_errors_from_incoming_receiver_;
 
+  // If non-null, this will be set to true when the Connector is destroyed.  We
+  // use this flag to allow for the Connector to be destroyed as a side-effect
+  // of dispatching an incoming message.
+  bool* destroyed_flag_;
+
   MOJO_DISALLOW_COPY_AND_ASSIGN(Connector);
 };
 
diff --git a/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc b/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc
index 518afd4..c4f7c51 100644
--- a/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc
+++ b/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc
@@ -110,6 +110,45 @@ class MathCalculatorUIImpl : public math::CalculatorUI {
   double output_;
 };
 
+class SelfDestructingMathCalculatorUIImpl : public math::CalculatorUI {
+ public:
+  explicit SelfDestructingMathCalculatorUIImpl(math::CalculatorPtr calculator)
+      : calculator_(calculator.Pass()),
+        nesting_level_(0) {
+    ++num_instances_;
+    calculator_.set_client(this);
+  }
+
+  void BeginTest(bool nested) {
+    nesting_level_ = nested ? 2 : 1;
+    calculator_->Add(1.0);
+  }
+
+  static int num_instances() { return num_instances_; }
+
+ private:
+  virtual ~SelfDestructingMathCalculatorUIImpl() {
+    --num_instances_;
+  }
+
+  virtual void Output(double value) MOJO_OVERRIDE {
+    if (--nesting_level_ > 0) {
+      // Add some more and wait for re-entrant call to Output!
+      calculator_->Add(1.0);
+      RunLoop::current()->RunUntilIdle();
+    } else {
+      delete this;
+    }
+  }
+
+  math::CalculatorPtr calculator_;
+  int nesting_level_;
+  static int num_instances_;
+};
+
+// static
+int SelfDestructingMathCalculatorUIImpl::num_instances_ = 0;
+
 class InterfacePtrTest : public testing::Test {
  public:
   virtual ~InterfacePtrTest() {
@@ -245,6 +284,36 @@ TEST_F(InterfacePtrTest, NoClientAttribute) {
   port.Bind(pipe.handle0.Pass());
 }
 
+TEST_F(InterfacePtrTest, DestroyInterfacePtrOnClientMethod) {
+  math::CalculatorPtr proxy;
+  BindToProxy(new MathCalculatorImpl(), &proxy);
+
+  EXPECT_EQ(0, SelfDestructingMathCalculatorUIImpl::num_instances());
+
+  SelfDestructingMathCalculatorUIImpl* impl =
+      new SelfDestructingMathCalculatorUIImpl(proxy.Pass());
+  impl->BeginTest(false);
+
+  PumpMessages();
+
+  EXPECT_EQ(0, SelfDestructingMathCalculatorUIImpl::num_instances());
+}
+
+TEST_F(InterfacePtrTest, NestedDestroyInterfacePtrOnClientMethod) {
+  math::CalculatorPtr proxy;
+  BindToProxy(new MathCalculatorImpl(), &proxy);
+
+  EXPECT_EQ(0, SelfDestructingMathCalculatorUIImpl::num_instances());
+
+  SelfDestructingMathCalculatorUIImpl* impl =
+      new SelfDestructingMathCalculatorUIImpl(proxy.Pass());
+  impl->BeginTest(true);
+
+  PumpMessages();
+
+  EXPECT_EQ(0, SelfDestructingMathCalculatorUIImpl::num_instances());
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace mojo
diff --git a/mojo/public/cpp/utility/lib/run_loop.cc b/mojo/public/cpp/utility/lib/run_loop.cc
index 2bf6a07..d900d0b 100644
--- a/mojo/public/cpp/utility/lib/run_loop.cc
+++ b/mojo/public/cpp/utility/lib/run_loop.cc
@@ -92,8 +92,6 @@ bool RunLoop::HasHandler(const Handle& handle) const {
 
 void RunLoop::Run() {
   assert(current() == this);
-  // We don't currently support nesting.
-  assert(!run_state_);
   RunState* old_state = run_state_;
   RunState run_state;
   run_state_ = &run_state;
@@ -104,8 +102,6 @@ void RunLoop::Run() {
 
 void RunLoop::RunUntilIdle() {
   assert(current() == this);
-  // We don't currently support nesting.
-  assert(!run_state_);
   RunState* old_state = run_state_;
   RunState run_state;
   run_state_ = &run_state;
@@ -157,6 +153,11 @@ bool RunLoop::NotifyDeadlineExceeded() {
 
   // Make a copy in case someone tries to add/remove new handlers as part of
   // notifying.
+  //
+  // TODO(darin): This does not protect against removal of handlers! After a
+  // call to OnHandleError, |cloned_handlers| could contain an invalid pointer
+  // to a RunLoopHandler! See http://crbug.com/384578.
+  //
   const HandleToHandlerData cloned_handlers(handler_data_);
   const MojoTimeTicks now(GetTimeTicksNow());
   for (HandleToHandlerData::const_iterator i = cloned_handlers.begin();
diff --git a/mojo/public/cpp/utility/tests/run_loop_unittest.cc b/mojo/public/cpp/utility/tests/run_loop_unittest.cc
index b594e6c..ff4b2d1 100644
--- a/mojo/public/cpp/utility/tests/run_loop_unittest.cc
+++ b/mojo/public/cpp/utility/tests/run_loop_unittest.cc
@@ -190,5 +190,7 @@ TEST_F(RunLoopTest, Current) {
   EXPECT_TRUE(RunLoop::current() == NULL);
 }
 
+// TODO(darin): Add tests for nested calls to RunLoop::Run(). See crbug/384633.
+
 }  // namespace
 }  // namespace mojo