Mojo C++ bindings: remove the lock in MultiplexRouter if it only serves a single interface.

BUG=594244

Review-Url: https://codereview.chromium.org/2345013002
Cr-Commit-Position: refs/heads/master@{#419036}

mojo/public/cpp/bindings/BUILD.gn

@@ -55,6 +55,7 @@ static_library("bindings") {
     "lib/interface_ptr_state.h",
     "lib/map_data_internal.h",
     "lib/map_serialization.h",
+    "lib/may_auto_lock.h",
     "lib/message.cc",
     "lib/message_buffer.cc",
     "lib/message_buffer.h",

mojo/public/cpp/bindings/associated_interface_ptr.h

@@ -231,8 +231,9 @@ template <typename Interface>
 void GetDummyProxyForTesting(AssociatedInterfacePtr<Interface>* proxy) {
   MessagePipe pipe;
   scoped_refptr<internal::MultiplexRouter> router =
-      new internal::MultiplexRouter(false, std::move(pipe.handle0),
-                                    base::ThreadTaskRunnerHandle::Get());
+      new internal::MultiplexRouter(std::move(pipe.handle0),
+                                    internal::MultiplexRouter::MULTI_INTERFACE,
+                                    false, base::ThreadTaskRunnerHandle::Get());
   std::unique_ptr<AssociatedGroup> group = router->CreateAssociatedGroup();
   GetProxy(proxy, group.get());
 }

mojo/public/cpp/bindings/lib/binding_state.cc

@@ -146,7 +146,8 @@ void MultiplexedBindingState::BindInternal(
     uint32_t interface_version) {
   DCHECK(!router_);
 
-  router_ = new internal::MultiplexRouter(false, std::move(handle), runner);
+  router_ = new MultiplexRouter(
+      std::move(handle), MultiplexRouter::MULTI_INTERFACE, false, runner);
   router_->SetMasterInterfaceName(interface_name);
 
   endpoint_client_.reset(new InterfaceEndpointClient(

mojo/public/cpp/bindings/lib/connector.cc

@@ -12,37 +12,11 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/synchronization/lock.h"
+#include "mojo/public/cpp/bindings/lib/may_auto_lock.h"
 #include "mojo/public/cpp/bindings/sync_handle_watcher.h"
 
 namespace mojo {
 
-namespace {
-
-// Similar to base::AutoLock, except that it does nothing if |lock| passed into
-// the constructor is null.
-class MayAutoLock {
- public:
-  explicit MayAutoLock(base::Lock* lock) : lock_(lock) {
-    if (lock_)
-      lock_->Acquire();
-  }
-
-  ~MayAutoLock() {
-    if (lock_) {
-      lock_->AssertAcquired();
-      lock_->Release();
-    }
-  }
-
- private:
-  base::Lock* lock_;
-  DISALLOW_COPY_AND_ASSIGN(MayAutoLock);
-};
-
-}  // namespace
-
-// ----------------------------------------------------------------------------
-
 Connector::Connector(ScopedMessagePipeHandle message_pipe,
                      ConnectorConfig config,
                      scoped_refptr<base::SingleThreadTaskRunner> runner)
@@ -73,7 +47,7 @@ void Connector::CloseMessagePipe() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   CancelWait();
-  MayAutoLock locker(lock_.get());
+  internal::MayAutoLock locker(lock_.get());
   message_pipe_.reset();
 
   base::AutoLock lock(connected_lock_);
@@ -84,7 +58,7 @@ ScopedMessagePipeHandle Connector::PassMessagePipe() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   CancelWait();
-  MayAutoLock locker(lock_.get());
+  internal::MayAutoLock locker(lock_.get());
   ScopedMessagePipeHandle message_pipe = std::move(message_pipe_);
 
   base::AutoLock lock(connected_lock_);
@@ -149,7 +123,7 @@ bool Connector::Accept(Message* message) {
   if (error_)
     return false;
 
-  MayAutoLock locker(lock_.get());
+  internal::MayAutoLock locker(lock_.get());
 
   if (!message_pipe_.is_valid() || drop_writes_)
     return true;
@@ -331,7 +305,7 @@ void Connector::HandleError(bool force_pipe_reset, bool force_async_handler) {
 
   if (force_pipe_reset) {
     CancelWait();
-    MayAutoLock locker(lock_.get());
+    internal::MayAutoLock locker(lock_.get());
     message_pipe_.reset();
     MessagePipe dummy_pipe;
     message_pipe_ = std::move(dummy_pipe.handle0);

mojo/public/cpp/bindings/lib/interface_ptr_state.h

@@ -345,7 +345,8 @@ class InterfacePtrState<Interface, true> {
     if (!handle_.is_valid())
       return;
 
-    router_ = new MultiplexRouter(true, std::move(handle_), runner_);
+    router_ = new MultiplexRouter(
+        std::move(handle_), MultiplexRouter::MULTI_INTERFACE, true, runner_);
     router_->SetMasterInterfaceName(Interface::Name_);
     endpoint_client_.reset(new InterfaceEndpointClient(
         router_->CreateLocalEndpointHandle(kMasterInterfaceId), nullptr,

mojo/public/cpp/bindings/lib/may_auto_lock.h

+// Copyright 2016 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 "base/macros.h"
+#include "base/synchronization/lock.h"
+
+namespace mojo {
+namespace internal {
+
+// Similar to base::AutoLock, except that it does nothing if |lock| passed into
+// the constructor is null.
+class MayAutoLock {
+ public:
+  explicit MayAutoLock(base::Lock* lock) : lock_(lock) {
+    if (lock_)
+      lock_->Acquire();
+  }
+
+  ~MayAutoLock() {
+    if (lock_) {
+      lock_->AssertAcquired();
+      lock_->Release();
+    }
+  }
+
+ private:
+  base::Lock* lock_;
+  DISALLOW_COPY_AND_ASSIGN(MayAutoLock);
+};
+
+// Similar to base::AutoUnlock, except that it does nothing if |lock| passed
+// into the constructor is null.
+class MayAutoUnlock {
+ public:
+  explicit MayAutoUnlock(base::Lock* lock) : lock_(lock) {
+    if (lock_) {
+      lock_->AssertAcquired();
+      lock_->Release();
+    }
+  }
+
+  ~MayAutoUnlock() {
+    if (lock_)
+      lock_->Acquire();
+  }
+
+ private:
+  base::Lock* lock_;
+  DISALLOW_COPY_AND_ASSIGN(MayAutoUnlock);
+};
+
+}  // namespace internal
+}  // namespace mojo

mojo/public/cpp/bindings/lib/multiplex_router.h

@@ -56,10 +56,22 @@ class MultiplexRouter
       public AssociatedGroupController,
       public PipeControlMessageHandlerDelegate {
  public:
+  enum Config {
+    // There is only the master interface running on this router. Please note
+    // that because of interface versioning, the other side of the message pipe
+    // may use a newer master interface definition which passes associated
+    // interfaces. In that case, this router may still receive pipe control
+    // messages or messages targetting associated interfaces.
+    SINGLE_INTERFACE,
+    // There may be associated interfaces running on this router.
+    MULTI_INTERFACE
+  };
+
   // If |set_interface_id_namespace_bit| is true, the interface IDs generated by
   // this router will have the highest bit set.
-  MultiplexRouter(bool set_interface_id_namespace_bit,
-                  ScopedMessagePipeHandle message_pipe,
+  MultiplexRouter(ScopedMessagePipeHandle message_pipe,
+                  Config config,
+                  bool set_interface_id_namespace_bit,
                   scoped_refptr<base::SingleThreadTaskRunner> runner);
 
   // Sets the master interface name for this router. Only used when reporting
@@ -205,6 +217,8 @@ class MultiplexRouter
 
   InterfaceEndpoint* FindOrInsertEndpoint(InterfaceId id, bool* inserted);
 
+  void AssertLockAcquired();
+
   // Whether to set the namespace bit when generating interface IDs. Please see
   // comments of kInterfaceIdNamespaceMask.
   const bool set_interface_id_namespace_bit_;
@@ -220,7 +234,8 @@ class MultiplexRouter
   base::ThreadChecker thread_checker_;
 
   // Protects the following members.
-  mutable base::Lock lock_;
+  // Sets to nullptr in Config::SINGLE_INTERFACE mode.
+  std::unique_ptr<base::Lock> lock_;
   PipeControlMessageHandler control_message_handler_;
   PipeControlMessageProxy control_message_proxy_;
 

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

@@ -113,9 +113,11 @@ class AssociatedInterfaceTest : public testing::Test {
   void CreateRouterPair(scoped_refptr<MultiplexRouter>* router0,
                         scoped_refptr<MultiplexRouter>* router1) {
     MessagePipe pipe;
-    *router0 = new MultiplexRouter(true, std::move(pipe.handle0),
+    *router0 = new MultiplexRouter(std::move(pipe.handle0),
+                                   MultiplexRouter::MULTI_INTERFACE, true,
                                    base::ThreadTaskRunnerHandle::Get());
-    *router1 = new MultiplexRouter(false, std::move(pipe.handle1),
+    *router1 = new MultiplexRouter(std::move(pipe.handle1),
+                                   MultiplexRouter::MULTI_INTERFACE, false,
                                    base::ThreadTaskRunnerHandle::Get());
   }
 

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

@@ -217,11 +217,13 @@ TEST_F(MojoBindingsPerftest, RouterPingPong) {
 TEST_F(MojoBindingsPerftest, MultiplexRouterPingPong) {
   MessagePipe pipe;
   scoped_refptr<internal::MultiplexRouter> router0(
-      new internal::MultiplexRouter(true, std::move(pipe.handle0),
-                                    base::ThreadTaskRunnerHandle::Get()));
+      new internal::MultiplexRouter(std::move(pipe.handle0),
+                                    internal::MultiplexRouter::SINGLE_INTERFACE,
+                                    true, base::ThreadTaskRunnerHandle::Get()));
   scoped_refptr<internal::MultiplexRouter> router1(
-      new internal::MultiplexRouter(false, std::move(pipe.handle1),
-                                    base::ThreadTaskRunnerHandle::Get()));
+      new internal::MultiplexRouter(
+          std::move(pipe.handle1), internal::MultiplexRouter::SINGLE_INTERFACE,
+          false, base::ThreadTaskRunnerHandle::Get()));
 
   PingPongPaddle paddle0(nullptr);
   PingPongPaddle paddle1(nullptr);
@@ -302,7 +304,8 @@ TEST_F(MojoBindingsPerftest, RouterDispatchCost) {
 TEST_F(MojoBindingsPerftest, MultiplexRouterDispatchCost) {
   MessagePipe pipe;
   scoped_refptr<internal::MultiplexRouter> router(new internal::MultiplexRouter(
-      true, std::move(pipe.handle0), base::ThreadTaskRunnerHandle::Get()));
+      std::move(pipe.handle0), internal::MultiplexRouter::SINGLE_INTERFACE,
+      true, base::ThreadTaskRunnerHandle::Get()));
   CounterReceiver receiver;
   InterfaceEndpointClient client(
       router->CreateLocalEndpointHandle(kMasterInterfaceId), &receiver, nullptr,

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

@@ -31,9 +31,11 @@ class MultiplexRouterTest : public testing::Test {
 
   void SetUp() override {
     MessagePipe pipe;
-    router0_ = new MultiplexRouter(true, std::move(pipe.handle0),
+    router0_ = new MultiplexRouter(std::move(pipe.handle0),
+                                   MultiplexRouter::MULTI_INTERFACE, false,
                                    base::ThreadTaskRunnerHandle::Get());
-    router1_ = new MultiplexRouter(true, std::move(pipe.handle1),
+    router1_ = new MultiplexRouter(std::move(pipe.handle1),
+                                   MultiplexRouter::MULTI_INTERFACE, true,
                                    base::ThreadTaskRunnerHandle::Get());
     router0_->CreateEndpointHandlePair(&endpoint0_, &endpoint1_);
     endpoint1_ =