Simplify DeviceListener/HostController self-deletion mechanism in forwarder2.

Those objects which have similar lifetime characteristics can be deleted in two
different ways:
- Their internal thread requested self-deletion after an error happened.  In
  this case the owner (DeviceController/HostControllersManager) is notified
  on the construction thread through the provided ErrorCallback and deletes the
  given DeviceListener/HostController instance.
- Their owner (DeviceController/HostControllersManager) requested explicit
  deletion. In this case ~DeviceListener()/~HostController() cancels the
  blocking operation on its internal thread which is interpreted by this
  internal thread as an error which in turn triggers a self-deletion. This
  self-deletion *must not* happen since the object is already being deleted.
  WeakPtr is used for that purpose.

This complexity/subtleness which used to be duplicated is now encapsulated in
the new SelfDeleterHelper class used by both the DeviceController and the
HostControllersManager.

BUG=313809
R=digit@chromium.org

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

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

7 files changed, 200 insertions, 92 deletions

diff --git a/tools/android/forwarder2/device_controller.cc b/tools/android/forwarder2/device_controller.cc
index 23a55b1..37b754b 100644
--- a/tools/android/forwarder2/device_controller.cc
+++ b/tools/android/forwarder2/device_controller.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/logging.h"
@@ -92,8 +93,9 @@ void DeviceController::AcceptHostCommandInternal() {
       }
       scoped_ptr<DeviceListener> new_listener(
           DeviceListener::Create(
-              socket.Pass(), port, base::Bind(&DeviceController::DeleteListener,
-                                              weak_ptr_factory_.GetWeakPtr())));
+              socket.Pass(), port,
+              base::Bind(&DeviceController::DeleteListenerOnError,
+                         weak_ptr_factory_.GetWeakPtr())));
       if (!new_listener)
         return;
       new_listener->Start();
@@ -136,18 +138,20 @@ void DeviceController::AcceptHostCommandInternal() {
 }
 
 // static
-void DeviceController::DeleteListener(
+void DeviceController::DeleteListenerOnError(
       const base::WeakPtr<DeviceController>& device_controller_ptr,
-      int listener_port) {
+      scoped_ptr<DeviceListener> device_listener) {
   DeviceController* const controller = device_controller_ptr.get();
-  if (!controller)
+  if (!controller) {
+    // |device_listener| was already deleted by the controller that did have
+    // its ownership.
+    ignore_result(device_listener.release());
     return;
+  }
   DCHECK(controller->construction_task_runner_->RunsTasksOnCurrentThread());
-  const ListenersMap::iterator listener_it = controller->listeners_.find(
-      listener_port);
-  if (listener_it == controller->listeners_.end())
-    return;
-  DeleteRefCountedValueInMapFromIterator(listener_it, &controller->listeners_);
+  bool listener_did_exist = DeleteRefCountedValueInMap(
+      device_listener->listener_port(), &controller->listeners_);
+  DCHECK(listener_did_exist);
 }
 
 }  // namespace forwarder
diff --git a/tools/android/forwarder2/device_controller.h b/tools/android/forwarder2/device_controller.h
index 3daedb3..acce0e1 100644
--- a/tools/android/forwarder2/device_controller.h
+++ b/tools/android/forwarder2/device_controller.h
@@ -45,9 +45,9 @@ class DeviceController {
 
   // Note that this can end up being called after the DeviceController is
   // destroyed which is why a weak pointer is used.
-  static void DeleteListener(
+  static void DeleteListenerOnError(
       const base::WeakPtr<DeviceController>& device_controller_ptr,
-      int listener_port);
+      scoped_ptr<DeviceListener> device_listener);
 
   const scoped_ptr<Socket> host_socket_;
   // Used to notify the controller to exit.
diff --git a/tools/android/forwarder2/device_listener.cc b/tools/android/forwarder2/device_listener.cc
index 1819a8a..97131c8 100644
--- a/tools/android/forwarder2/device_listener.cc
+++ b/tools/android/forwarder2/device_listener.cc
@@ -21,7 +21,7 @@ namespace forwarder2 {
 scoped_ptr<DeviceListener> DeviceListener::Create(
     scoped_ptr<Socket> host_socket,
     int listener_port,
-    const DeleteCallback& delete_callback) {
+    const ErrorCallback& error_callback) {
   scoped_ptr<Socket> listener_socket(new Socket());
   scoped_ptr<DeviceListener> device_listener;
   if (!listener_socket->BindTcp("", listener_port)) {
@@ -37,7 +37,7 @@ scoped_ptr<DeviceListener> DeviceListener::Create(
   device_listener.reset(
       new DeviceListener(
           scoped_ptr<PipeNotifier>(new PipeNotifier()), listener_socket.Pass(),
-          host_socket.Pass(), listener_port, delete_callback));
+          host_socket.Pass(), listener_port, error_callback));
   return device_listener.Pass();
 }
 
@@ -62,12 +62,12 @@ DeviceListener::DeviceListener(scoped_ptr<PipeNotifier> pipe_notifier,
                                scoped_ptr<Socket> listener_socket,
                                scoped_ptr<Socket> host_socket,
                                int port,
-                               const DeleteCallback& delete_callback)
-    : exit_notifier_(pipe_notifier.Pass()),
+                               const ErrorCallback& error_callback)
+    : self_deleter_helper_(this, error_callback),
+      exit_notifier_(pipe_notifier.Pass()),
       listener_socket_(listener_socket.Pass()),
       host_socket_(host_socket.Pass()),
       listener_port_(port),
-      delete_callback_(delete_callback),
       deletion_task_runner_(base::MessageLoopProxy::current()),
       thread_("DeviceListener") {
   CHECK(host_socket_.get());
@@ -89,12 +89,12 @@ void DeviceListener::AcceptClientOnInternalThread() {
   if (!listener_socket_->Accept(device_data_socket_.get())) {
     if (listener_socket_->DidReceiveEvent()) {
       LOG(INFO) << "Received exit notification, stopped accepting clients.";
-      SelfDelete();
+      OnInternalThreadError();
       return;
     }
     LOG(WARNING) << "Could not Accept in ListenerSocket.";
     SendCommand(command::ACCEPT_ERROR, listener_port_, host_socket_.get());
-    SelfDelete();
+    OnInternalThreadError();
     return;
   }
   SendCommand(command::ACCEPT_SUCCESS, listener_port_, host_socket_.get());
@@ -106,7 +106,7 @@ void DeviceListener::AcceptClientOnInternalThread() {
     if (host_socket_->has_error()) {
       LOG(ERROR) << "Adb Control connection lost. "
                  << "Listener port: " << listener_port_;
-      SelfDelete();
+      OnInternalThreadError();
       return;
     }
     // It can continue if the host forwarder could not connect to the host
@@ -127,22 +127,8 @@ void DeviceListener::OnAdbDataSocketReceivedOnInternalThread(
   AcceptNextClientSoon();
 }
 
-void DeviceListener::SelfDelete() {
-  if (!deletion_task_runner_->RunsTasksOnCurrentThread()) {
-    deletion_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&DeviceListener::SelfDeleteOnDeletionTaskRunner,
-                   delete_callback_, listener_port_));
-    return;
-  }
-  SelfDeleteOnDeletionTaskRunner(delete_callback_, listener_port_);
-}
-
-// static
-void DeviceListener::SelfDeleteOnDeletionTaskRunner(
-    const DeleteCallback& delete_callback,
-    int listener_port) {
-  delete_callback.Run(listener_port);
+void DeviceListener::OnInternalThreadError() {
+  self_deleter_helper_.MaybeSelfDeleteSoon();
 }
 
 }  // namespace forwarder
diff --git a/tools/android/forwarder2/device_listener.h b/tools/android/forwarder2/device_listener.h
index 2a69823..85962ca 100644
--- a/tools/android/forwarder2/device_listener.h
+++ b/tools/android/forwarder2/device_listener.h
@@ -13,6 +13,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread.h"
 #include "tools/android/forwarder2/pipe_notifier.h"
+#include "tools/android/forwarder2/self_deleter_helper.h"
 #include "tools/android/forwarder2/socket.h"
 
 namespace base {
@@ -40,15 +41,14 @@ class Forwarder;
 // not to be running anymore once the object is deleted.
 class DeviceListener {
  public:
-  // Callback that is used for self-deletion as a way to let the device
+  // Callback that is used for self-deletion on error to let the device
   // controller perform some additional cleanup work (e.g. removing the device
   // listener instance from its internal map before deleting it).
-  typedef base::Callback<void (int /* listener port */)> DeleteCallback;
+  typedef base::Callback<void (scoped_ptr<DeviceListener>)> ErrorCallback;
 
-  static scoped_ptr<DeviceListener> Create(
-      scoped_ptr<Socket> host_socket,
-      int port,
-      const DeleteCallback& delete_callback);
+  static scoped_ptr<DeviceListener> Create(scoped_ptr<Socket> host_socket,
+                                           int port,
+                                           const ErrorCallback& error_callback);
 
   ~DeviceListener();
 
@@ -63,7 +63,7 @@ class DeviceListener {
                  scoped_ptr<Socket> listener_socket,
                  scoped_ptr<Socket> host_socket,
                  int port,
-                 const DeleteCallback& delete_callback);
+                 const ErrorCallback& error_callback);
 
   // Pushes an AcceptClientOnInternalThread() task to the internal thread's
   // message queue in order to wait for a new client soon.
@@ -74,14 +74,9 @@ class DeviceListener {
   void OnAdbDataSocketReceivedOnInternalThread(
       scoped_ptr<Socket> adb_data_socket);
 
-  void SelfDelete();
-
-  // Note that this can be called after the DeviceListener instance gets deleted
-  // which is why this method is static.
-  static void SelfDeleteOnDeletionTaskRunner(
-      const DeleteCallback& delete_callback,
-      int listener_port);
+  void OnInternalThreadError();
 
+  SelfDeleterHelper<DeviceListener> self_deleter_helper_;
   // Used for the listener thread to be notified on destruction. We have one
   // notifier per Listener thread since each Listener thread may be requested to
   // exit for different reasons independently from each other and independent
@@ -100,7 +95,6 @@ class DeviceListener {
   // the forwarder. Ownership transferred to the Forwarder.
   scoped_ptr<Socket> adb_data_socket_;
   const int listener_port_;
-  const DeleteCallback delete_callback_;
   // Task runner used for deletion set at construction time (i.e. the object is
   // deleted on the same thread it is created on).
   scoped_refptr<base::SingleThreadTaskRunner> deletion_task_runner_;
diff --git a/tools/android/forwarder2/host_controller.cc b/tools/android/forwarder2/host_controller.cc
index 9f0ed01..efc2887 100644
--- a/tools/android/forwarder2/host_controller.cc
+++ b/tools/android/forwarder2/host_controller.cc
@@ -22,7 +22,7 @@ scoped_ptr<HostController> HostController::Create(
     int host_port,
     int adb_port,
     int exit_notifier_fd,
-    const DeletionCallback& deletion_callback) {
+    const ErrorCallback& error_callback) {
   scoped_ptr<HostController> host_controller;
   scoped_ptr<PipeNotifier> delete_controller_notifier(new PipeNotifier());
   scoped_ptr<Socket> adb_control_socket(new Socket());
@@ -48,7 +48,7 @@ scoped_ptr<HostController> HostController::Create(
   host_controller.reset(
       new HostController(
           device_port_allocated, host_port, adb_port, exit_notifier_fd,
-          deletion_callback, adb_control_socket.Pass(),
+          error_callback, adb_control_socket.Pass(),
           delete_controller_notifier.Pass()));
   return host_controller.Pass();
 }
@@ -72,14 +72,14 @@ HostController::HostController(
     int host_port,
     int adb_port,
     int exit_notifier_fd,
-    const DeletionCallback& deletion_callback,
+    const ErrorCallback& error_callback,
     scoped_ptr<Socket> adb_control_socket,
     scoped_ptr<PipeNotifier> delete_controller_notifier)
-    : device_port_(device_port),
+    : self_deleter_helper_(this, error_callback),
+      device_port_(device_port),
       host_port_(host_port),
       adb_port_(adb_port),
       global_exit_notifier_fd_(exit_notifier_fd),
-      deletion_callback_(deletion_callback),
       adb_control_socket_(adb_control_socket.Pass()),
       delete_controller_notifier_(delete_controller_notifier.Pass()),
       deletion_task_runner_(base::MessageLoopProxy::current()),
@@ -97,7 +97,7 @@ void HostController::ReadCommandOnInternalThread() {
   if (!ReceivedCommand(command::ACCEPT_SUCCESS, adb_control_socket_.get())) {
     LOG(ERROR) << "Did not receive ACCEPT_SUCCESS for port: "
                << host_port_;
-    SelfDelete();
+    OnInternalThreadError();
     return;
   }
   // Try to connect to host server.
@@ -115,7 +115,7 @@ void HostController::ReadCommandOnInternalThread() {
       return;
     }
     LOG(ERROR) << "Will delete host controller: " << host_port_;
-    SelfDelete();
+    OnInternalThreadError();
     return;
   }
   LOG(INFO) << "Will send HOST_SERVER_SUCCESS: " << host_port_;
@@ -130,7 +130,7 @@ void HostController::StartForwarder(
   scoped_ptr<Socket> adb_data_socket(CreateSocket());
   if (!adb_data_socket->ConnectTcp("", adb_port_)) {
     LOG(ERROR) << "Could not connect AdbDataSocket on port: " << adb_port_;
-    SelfDelete();
+    OnInternalThreadError();
     return;
   }
   // Open the Adb data connection, and send a command with the
@@ -143,7 +143,7 @@ void HostController::StartForwarder(
   if (!ReceivedCommand(command::ADB_DATA_SOCKET_SUCCESS,
                        adb_control_socket_.get())) {
     LOG(ERROR) << "Device could not handle the new Adb Data Connection.";
-    SelfDelete();
+    OnInternalThreadError();
     return;
   }
   forwarder2::StartForwarder(
@@ -157,14 +157,12 @@ scoped_ptr<Socket> HostController::CreateSocket() {
   return socket.Pass();
 }
 
-void HostController::SelfDelete() {
-  scoped_ptr<HostController> self_deleter(this);
-  deletion_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&HostController::SelfDeleteOnDeletionTaskRunner,
-                 deletion_callback_, base::Passed(&self_deleter)));
-  // Tell the device to delete its corresponding controller instance before we
-  // self-delete.
+void HostController::OnInternalThreadError() {
+  UnmapPortOnDevice();
+  self_deleter_helper_.MaybeSelfDeleteSoon();
+}
+
+void HostController::UnmapPortOnDevice() {
   Socket socket;
   if (!socket.ConnectTcp("", adb_port_)) {
     LOG(ERROR) << "Could not connect to device on port " << adb_port_;
@@ -180,11 +178,4 @@ void HostController::SelfDelete() {
   }
 }
 
-// static
-void HostController::SelfDeleteOnDeletionTaskRunner(
-    const DeletionCallback& deletion_callback,
-    scoped_ptr<HostController> controller) {
-  deletion_callback.Run(controller.Pass());
-}
-
 }  // namespace forwarder2
diff --git a/tools/android/forwarder2/host_controller.h b/tools/android/forwarder2/host_controller.h
index aaedb94..1ab3c9b 100644
--- a/tools/android/forwarder2/host_controller.h
+++ b/tools/android/forwarder2/host_controller.h
@@ -13,6 +13,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread.h"
 #include "tools/android/forwarder2/pipe_notifier.h"
+#include "tools/android/forwarder2/self_deleter_helper.h"
 #include "tools/android/forwarder2/socket.h"
 
 namespace forwarder2 {
@@ -24,23 +25,22 @@ namespace forwarder2 {
 // - Its destructor was called by its owner (HostControllersManager).
 // - Its internal thread requested self-deletion after an error happened. In
 //   this case the owner (HostControllersManager) is notified on the
-//   construction thread through the provided DeletionCallback invoked with the
+//   construction thread through the provided ErrorCallback invoked with the
 //   HostController instance. When this callback is invoked, it's up to the
 //   owner to delete the instance.
 class HostController {
  public:
-  // Callback used for self-deletion that lets the client perform some cleanup
-  // work before deleting the HostController instance.
-  typedef base::Callback<void (scoped_ptr<HostController>)> DeletionCallback;
+  // Callback used for self-deletion when an error happens so that the client
+  // can perform some cleanup work before deleting the HostController instance.
+  typedef base::Callback<void (scoped_ptr<HostController>)> ErrorCallback;
 
   // If |device_port| is zero then a dynamic port is allocated (and retrievable
   // through device_port() below).
-  static scoped_ptr<HostController> Create(
-      int device_port,
-      int host_port,
-      int adb_port,
-      int exit_notifier_fd,
-      const DeletionCallback& deletion_callback);
+  static scoped_ptr<HostController> Create(int device_port,
+                                           int host_port,
+                                           int adb_port,
+                                           int exit_notifier_fd,
+                                           const ErrorCallback& error_callback);
 
   ~HostController();
 
@@ -56,7 +56,7 @@ class HostController {
                  int host_port,
                  int adb_port,
                  int exit_notifier_fd,
-                 const DeletionCallback& deletion_callback,
+                 const ErrorCallback& error_callback,
                  scoped_ptr<Socket> adb_control_socket,
                  scoped_ptr<PipeNotifier> delete_controller_notifier);
 
@@ -69,19 +69,17 @@ class HostController {
   // descriptors.
   scoped_ptr<Socket> CreateSocket();
 
-  void SelfDelete();
+  // Note that this gets also called when ~HostController() is invoked.
+  void OnInternalThreadError();
 
-  static void SelfDeleteOnDeletionTaskRunner(
-      const DeletionCallback& deletion_callback,
-      scoped_ptr<HostController> controller);
+  void UnmapPortOnDevice();
 
+  SelfDeleterHelper<HostController> self_deleter_helper_;
   const int device_port_;
   const int host_port_;
   const int adb_port_;
   // Used to notify the controller when the process is killed.
   const int global_exit_notifier_fd_;
-  // Used to let the client delete the instance in case an error happened.
-  const DeletionCallback deletion_callback_;
   scoped_ptr<Socket> adb_control_socket_;
   scoped_ptr<PipeNotifier> delete_controller_notifier_;
   // Used to cancel the pending blocking IO operations when the host controller
diff --git a/tools/android/forwarder2/self_deleter_helper.h b/tools/android/forwarder2/self_deleter_helper.h
new file mode 100644
index 0000000..ddf1463
--- /dev/null
+++ b/tools/android/forwarder2/self_deleter_helper.h
@@ -0,0 +1,135 @@
+// Copyright 2013 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.
+
+#ifndef TOOLS_ANDROID_FORWARDER2_SELF_DELETER_HELPER_H_
+#define TOOLS_ANDROID_FORWARDER2_SELF_DELETER_HELPER_H_
+
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop_proxy.h"
+
+namespace base {
+
+class SingleThreadTaskRunner;
+
+}  // namespace base
+
+namespace forwarder2 {
+
+// Helper template class to be used in the following case:
+//   * T is the type of an object that implements some work through an internal
+//     or worker thread.
+//   * T wants the internal thread to invoke deletion of its own instance, on
+//     the thread where the instance was created.
+//
+// To make this easier, do something like:
+//   1) Add a SelfDeleteHelper<T> member to your class T, and default-initialize
+//      it in its constructor.
+//   2) In the internal thread, to trigger self-deletion, call the
+//      MaybeDeleteSoon() method on this member.
+//
+// MaybeDeleteSoon() posts a task on the message loop where the T instance was
+// created to delete it. The task will be safely ignored if the instance is
+// otherwise deleted.
+//
+// Usage example:
+// class Object {
+//  public:
+//   typedef base::Callback<void (scoped_ptr<Object>)> ErrorCallback;
+//
+//   Object(const ErrorCallback& error_callback)
+//       : self_deleter_helper_(this, error_callback) {
+//   }
+//
+//   void StartWork() {
+//     // Post a callback to DoSomethingOnWorkerThread() below to another
+//     // thread.
+//   }
+//
+//   void DoSomethingOnWorkerThread() {
+//     ...
+//     if (error_happened)
+//       self_deleter_helper_.MaybeDeleteSoon();
+//   }
+//
+//  private:
+//   SelfDeleterHelper<MySelfDeletingClass> self_deleter_helper_;
+// };
+//
+// class ObjectOwner {
+//  public:
+//   ObjectOwner()
+//      : object_(new Object(base::Bind(&ObjectOwner::DeleteObjectOnError,
+//                                      base::Unretained(this))) {
+//      // To keep this example simple base::Unretained(this) is used above but
+//      // note that in a real world scenario the client would have to make sure
+//      // that the ObjectOwner instance is still alive when
+//      // DeleteObjectOnError() gets called below. This can be achieved by
+//      // using a WeakPtr<ObjectOwner> for instance.
+//   }
+//
+//   void StartWork() {
+//     object_->StartWork();
+//   }
+//
+//  private:
+//   void DeleteObjectOnError(scoped_ptr<Object> object) {
+//     DCHECK(thread_checker_.CalledOnValidThread());
+//     DCHECK_EQ(object_, object);
+//     // Do some extra work with |object| before it gets deleted...
+//     object_.reset();
+//     ignore_result(object.release());
+//   }
+//
+//   base::ThreadChecker thread_checker_;
+//   scoped_ptr<Object> object_;
+// };
+//
+template <typename T>
+class SelfDeleterHelper {
+ public:
+  typedef base::Callback<void (scoped_ptr<T>)> DeletionCallback;
+
+  SelfDeleterHelper(T* self_deleting_object,
+                    const DeletionCallback& deletion_callback)
+      : weak_ptr_factory_(this),
+        construction_runner_(base::MessageLoopProxy::current()),
+        self_deleting_object_(self_deleting_object),
+        deletion_callback_(deletion_callback) {
+  }
+
+  ~SelfDeleterHelper() {
+    DCHECK(construction_runner_->RunsTasksOnCurrentThread());
+  }
+
+  void MaybeSelfDeleteSoon() {
+    DCHECK(!construction_runner_->RunsTasksOnCurrentThread());
+    construction_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&SelfDeleterHelper::SelfDelete,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+ private:
+  void SelfDelete() {
+    DCHECK(construction_runner_->RunsTasksOnCurrentThread());
+    deletion_callback_.Run(make_scoped_ptr(self_deleting_object_));
+  }
+
+  base::WeakPtrFactory<SelfDeleterHelper<T> > weak_ptr_factory_;
+  const scoped_refptr<base::SingleThreadTaskRunner> construction_runner_;
+  T* const self_deleting_object_;
+  const DeletionCallback deletion_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SelfDeleterHelper);
+};
+
+}  // namespace forwarder2
+
+#endif  // TOOLS_ANDROID_FORWARDER2_SELF_DELETER_HELPER_H_