Eliminate deferred destruction of AssociatedGroupController

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

 // 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/lib/multiplex_router.h"
 
 #include <stdint.h>
 
 #include <utility>
 
@@ skipping 267 matching lines @@
   Type type;
 
  private:
   explicit Task(Type in_type) : type(in_type) {}
 };
 
 MultiplexRouter::MultiplexRouter(
     bool set_interface_id_namesapce_bit,
     ScopedMessagePipeHandle message_pipe,
     scoped_refptr<base::SingleThreadTaskRunner> runner)
-    : AssociatedGroupController(base::ThreadTaskRunnerHandle::Get()),
-      set_interface_id_namespace_bit_(set_interface_id_namesapce_bit),
+    : set_interface_id_namespace_bit_(set_interface_id_namesapce_bit),
+      task_runner_(runner),
       header_validator_(this),
       connector_(std::move(message_pipe),
                  Connector::MULTI_THREADED_SEND,
                  std::move(runner)),
       control_message_handler_(this),
       control_message_proxy_(&connector_),
       next_interface_id_value_(1),
       posted_to_process_tasks_(false),
       encountered_error_(false),
       paused_(false),
       testing_mode_(false) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
   // Always participate in sync handle watching, because even if it doesn't
   // expect sync requests during sync handle watching, it may still need to
   // dispatch messages to associated endpoints on a different thread.
   connector_.AllowWokenUpBySyncWatchOnSameThread();
   connector_.set_incoming_receiver(&header_validator_);
   connector_.set_connection_error_handler(
-      base::Bind(&MultiplexRouter::OnPipeConnectionError,
-                 base::Unretained(this)));

[yzshen1, 2016/07/26 21:21:15]

It seems safe to use base::Unretained() here. The master interface endpoint will
hold a ref to the multiplex router and it will call CloseMessagePipe or
PassMessagePipe before it releases that ref.

If we let the connector holds a ref it is easier to leak. (I think it already
leaks if we call PassMessagePipe() because it doesn't reset the connection error
handler.)

So maybe it makes sense to keep base::Unretained() and add comments to
MultiplexRouter header file saying that CloseMessagePipe or PassMessageMessage
must be called on the master thread before destruction?

[Ken Rockot(use gerrit already), 2016/07/26 22:07:00]

Done

+      base::Bind(&MultiplexRouter::OnPipeConnectionError, this));
 }
 
 MultiplexRouter::~MultiplexRouter() {
   base::AutoLock locker(lock_);
 
   sync_message_tasks_.clear();
   tasks_.clear();
 
   for (auto iter = endpoints_.begin(); iter != endpoints_.end();) {
     InterfaceEndpoint* endpoint = iter->second.get();
@@ skipping 264 matching lines @@
 
   control_message_proxy_.NotifyPeerEndpointClosed(id);
 
   return true;
 }
 
 void MultiplexRouter::OnPipeConnectionError() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   scoped_refptr<MultiplexRouter> protector(this);
+
+  // We explcitly clear the error handler since it has a ref to |this|.
+  connector_.set_connection_error_handler(base::Closure());
+
   base::AutoLock locker(lock_);
 
   encountered_error_ = true;
 
   for (auto iter = endpoints_.begin(); iter != endpoints_.end();) {
     InterfaceEndpoint* endpoint = iter->second.get();
     // Increment the iterator before calling UpdateEndpointStateMayRemove()
     // because it may remove the corresponding value from the map.
     ++iter;
 
@@ skipping 273 matching lines @@
       *inserted = true;
   } else {
     endpoint = iter->second.get();
   }
 
   return endpoint;
 }
 
 }  // namespace internal
 }  // namespace mojo