Remove D-Bus dependency from Managerbrillo-m8-releasebrillo-m8-dev

Here are the main changes:
1. Cleanup Manager::CreateService and Manager::RemoveService API
   to remove D-Bus dependency.
1. Use refptr for Service since it will be maintained by both
   Manager and its adaptor.
2. Move the monitoring of the service creator/owner to the adaptor,
   since the DBusServiceWatcher is D-Bus specific.

Bug: 24194427
TEST=Start AP service on Brillo board, stop weaved, verify service
     is destroyed and removed from apmanager.
TEST=Verify device setup works on Brillo board.
TEST=Verify apmanager does not crash on restart.
TEST=Run unittests on both Brillo and Chrome OS.

Change-Id: I33fd4eec2c1adf12830484ca087bd9dd56767288

8 files changed, 128 insertions, 134 deletions

diff --git a/dbus/dbus_control.cc b/dbus/dbus_control.cc
index d682fb8..c29421b 100644
--- a/dbus/dbus_control.cc
+++ b/dbus/dbus_control.cc
@@ -60,8 +60,6 @@ void DBusControl::Init() {
   // Create and register Manager.
   manager_.reset(new Manager(this));
   manager_->RegisterAsync(
-      object_manager_.get(),
-      bus_,
       sequencer->GetHandler("Manager.RegisterAsync() failed.", true));
 
   // Take over the service ownership once the objects registration is completed.
diff --git a/dbus/manager_dbus_adaptor.cc b/dbus/manager_dbus_adaptor.cc
index a205cea..bfc4e6d 100644
--- a/dbus/manager_dbus_adaptor.cc
+++ b/dbus/manager_dbus_adaptor.cc
@@ -36,6 +36,7 @@ ManagerDBusAdaptor::ManagerDBusAdaptor(
     Manager* manager)
     : adaptor_(this),
       dbus_object_(object_manager, bus, ManagerAdaptor::GetObjectPath()),
+      bus_(bus),
       manager_(manager) {}
 
 ManagerDBusAdaptor::~ManagerDBusAdaptor() {}
@@ -46,18 +47,67 @@ void ManagerDBusAdaptor::RegisterAsync(
   dbus_object_.RegisterAsync(completion_callback);
 }
 
-bool ManagerDBusAdaptor::CreateService(brillo::ErrorPtr* error,
+bool ManagerDBusAdaptor::CreateService(brillo::ErrorPtr* dbus_error,
                                        dbus::Message* message,
                                        dbus::ObjectPath* out_service) {
-  // TODO(zqiu): to be implemented.
+  auto service = manager_->CreateService();
+  if (!service) {
+    brillo::Error::AddTo(dbus_error,
+                         FROM_HERE,
+                         brillo::errors::dbus::kDomain,
+                         kErrorInternalError,
+                         "Failed to create new service");
+    return false;
+  }
+
+  *out_service = service->adaptor()->GetRpcObjectIdentifier();
+
+  // Setup monitoring for the service's remote owner.
+  service_owner_watchers_[*out_service] =
+      ServiceOwnerWatcherContext(
+          service,
+          std::unique_ptr<DBusServiceWatcher>(
+              new DBusServiceWatcher(
+                  bus_,
+                  message->GetSender(),
+                  base::Bind(&ManagerDBusAdaptor::OnServiceOwnerVanished,
+                             base::Unretained(this),
+                             *out_service))));
   return true;
 }
 
-bool ManagerDBusAdaptor::RemoveService(brillo::ErrorPtr* error,
+bool ManagerDBusAdaptor::RemoveService(brillo::ErrorPtr* dbus_error,
                                        dbus::Message* message,
                                        const dbus::ObjectPath& in_service) {
-  // TODO(zqiu): to be implemented.
-  return true;
+  auto watcher_context = service_owner_watchers_.find(in_service);
+  if (watcher_context == service_owner_watchers_.end()) {
+    brillo::Error::AddToPrintf(
+        dbus_error,
+        FROM_HERE,
+        brillo::errors::dbus::kDomain,
+        kErrorInvalidArguments,
+        "Service %s not found",
+        in_service.value().c_str());
+    return false;
+  }
+
+  Error error;
+  manager_->RemoveService(watcher_context->second.service, &error);
+  service_owner_watchers_.erase(watcher_context);
+  return !error.ToDBusError(dbus_error);
+}
+
+void ManagerDBusAdaptor::OnServiceOwnerVanished(
+    const dbus::ObjectPath& service_path) {
+  LOG(INFO) << "Owner for service " << service_path.value() << " vanished";
+  // Remove service watcher.
+  auto watcher_context = service_owner_watchers_.find(service_path);
+  CHECK(watcher_context != service_owner_watchers_.end())
+      << "Owner vanished without watcher setup.";
+
+  // Tell Manager to remove this service.
+  manager_->RemoveService(watcher_context->second.service, nullptr);
+  service_owner_watchers_.erase(watcher_context);
 }
 
 }  // namespace apmanager
diff --git a/dbus/manager_dbus_adaptor.h b/dbus/manager_dbus_adaptor.h
index 1a2f4cd..a3c05ee 100644
--- a/dbus/manager_dbus_adaptor.h
+++ b/dbus/manager_dbus_adaptor.h
@@ -17,7 +17,10 @@
 #ifndef APMANAGER_DBUS_MANAGER_DBUS_ADAPTOR_H_
 #define APMANAGER_DBUS_MANAGER_DBUS_ADAPTOR_H_
 
+#include <map>
+
 #include <base/macros.h>
+#include <brillo/dbus/dbus_service_watcher.h>
 #include <dbus_bindings/org.chromium.apmanager.Manager.h>
 
 #include "apmanager/manager_adaptor_interface.h"
@@ -25,6 +28,7 @@
 namespace apmanager {
 
 class Manager;
+class Service;
 
 class ManagerDBusAdaptor : public org::chromium::apmanager::ManagerInterface,
                            public ManagerAdaptorInterface {
@@ -35,10 +39,10 @@ class ManagerDBusAdaptor : public org::chromium::apmanager::ManagerInterface,
   ~ManagerDBusAdaptor() override;
 
   // Implementation of org::chromium::apmanager::ManagerInterface.
-  bool CreateService(brillo::ErrorPtr* error,
+  bool CreateService(brillo::ErrorPtr* dbus_error,
                      dbus::Message* message,
                      dbus::ObjectPath* out_service) override;
-  bool RemoveService(brillo::ErrorPtr* error,
+  bool RemoveService(brillo::ErrorPtr* dbus_error,
                      dbus::Message* message,
                      const dbus::ObjectPath& in_service) override;
 
@@ -47,9 +51,28 @@ class ManagerDBusAdaptor : public org::chromium::apmanager::ManagerInterface,
       const base::Callback<void(bool)>& completion_callback) override;
 
  private:
+  using DBusServiceWatcher = brillo::dbus_utils::DBusServiceWatcher;
+  // Context for service owner watcher.
+  struct ServiceOwnerWatcherContext {
+    ServiceOwnerWatcherContext() {}
+    ServiceOwnerWatcherContext(const scoped_refptr<Service>& in_service,
+                               std::unique_ptr<DBusServiceWatcher> in_watcher)
+        : service(in_service),
+          watcher(std::move(in_watcher)) {}
+    scoped_refptr<Service> service;
+    std::unique_ptr<DBusServiceWatcher> watcher;
+  };
+
+  // Invoked when the owner of a Service vanished.
+  void OnServiceOwnerVanished(const dbus::ObjectPath& service_path);
+
   org::chromium::apmanager::ManagerAdaptor adaptor_;
   brillo::dbus_utils::DBusObject dbus_object_;
+  scoped_refptr<dbus::Bus> bus_;
   Manager* manager_;
+  // Map of service path to owner monitor context.
+  std::map<dbus::ObjectPath, ServiceOwnerWatcherContext>
+      service_owner_watchers_;
 
   DISALLOW_COPY_AND_ASSIGN(ManagerDBusAdaptor);
 };
diff --git a/manager.cc b/manager.cc
index c5b1cb4..88c54ef 100644
--- a/manager.cc
+++ b/manager.cc
@@ -18,51 +18,29 @@
 
 #include <base/bind.h>
 
-#if !defined(__ANDROID__)
-#include <chromeos/dbus/service_constants.h>
-#else
-#include "dbus/apmanager/dbus-constants.h"
-#endif  // __ANDROID__
-
-using brillo::dbus_utils::AsyncEventSequencer;
-using brillo::dbus_utils::ExportedObjectManager;
-using brillo::dbus_utils::DBusMethodResponse;
+#include "apmanager/control_interface.h"
+#include "apmanager/manager.h"
+
 using std::string;
 
 namespace apmanager {
 
 Manager::Manager(ControlInterface* control_interface)
-    : org::chromium::apmanager::ManagerAdaptor(this),
-      control_interface_(control_interface),
+    : control_interface_(control_interface),
       service_identifier_(0),
-      device_info_(this) {}
+      device_info_(this),
+      adaptor_(control_interface->CreateManagerAdaptor(this)) {}
 
-Manager::~Manager() {
-  // Terminate all services before cleanup other resources.
-  for (auto& service : services_) {
-    service.reset();
-  }
-}
+Manager::~Manager() {}
 
 void Manager::RegisterAsync(
-    ExportedObjectManager* object_manager,
-    const scoped_refptr<dbus::Bus>& bus,
     const base::Callback<void(bool)>& completion_callback) {
-  CHECK(!dbus_object_) << "Already registered";
-  dbus_object_.reset(
-      new brillo::dbus_utils::DBusObject(
-          object_manager,
-          bus,
-          org::chromium::apmanager::ManagerAdaptor::GetObjectPath()));
-  RegisterWithDBusObject(dbus_object_.get());
-  dbus_object_->RegisterAsync(completion_callback);
-  bus_ = bus;
-
-  shill_manager_.Init(control_interface_);
-  firewall_manager_.Init(control_interface_);
+  adaptor_->RegisterAsync(completion_callback);
 }
 
 void Manager::Start() {
+  shill_manager_.Init(control_interface_);
+  firewall_manager_.Init(control_interface_);
   device_info_.Start();
 }
 
@@ -70,51 +48,26 @@ void Manager::Stop() {
   device_info_.Stop();
 }
 
-bool Manager::CreateService(brillo::ErrorPtr* error,
-                            dbus::Message* message,
-                            dbus::ObjectPath* out_service) {
+scoped_refptr<Service> Manager::CreateService() {
   LOG(INFO) << "Manager::CreateService";
-  std::unique_ptr<Service> service(new Service(this, service_identifier_));
-  *out_service = service->adaptor()->GetRpcObjectIdentifier();
-  services_.push_back(std::move(service));
-
-  base::Closure on_connection_vanish = base::Bind(
-      &Manager::OnAPServiceOwnerDisappeared,
-      base::Unretained(this),
-      service_identifier_);
-  service_watchers_[service_identifier_].reset(
-      new DBusServiceWatcher{bus_, message->GetSender(), on_connection_vanish});
-  service_identifier_++;
-
-  return true;
+  scoped_refptr<Service> service = new Service(this, service_identifier_++);
+  services_.push_back(service);
+  return service;
 }
 
-bool Manager::RemoveService(brillo::ErrorPtr* error,
-                            dbus::Message* message,
-                            const dbus::ObjectPath& in_service) {
+bool Manager::RemoveService(const scoped_refptr<Service>& service,
+                            Error* error) {
   for (auto it = services_.begin(); it != services_.end(); ++it) {
-    if ((*it)->adaptor()->GetRpcObjectIdentifier() == in_service) {
-      // Verify the owner.
-      auto watcher = service_watchers_.find((*it)->identifier());
-      CHECK(watcher != service_watchers_.end())
-          << "DBus watcher not created for service: " << (*it)->identifier();
-      if (watcher->second->connection_name() != message->GetSender()) {
-        brillo::Error::AddToPrintf(
-            error, FROM_HERE, brillo::errors::dbus::kDomain, kManagerError,
-            "Service %d is owned by another local process.",
-            (*it)->identifier());
-        return false;
-      }
-      service_watchers_.erase(watcher);
-
+    if (*it == service) {
       services_.erase(it);
       return true;
     }
   }
 
-  brillo::Error::AddTo(
-      error, FROM_HERE, brillo::errors::dbus::kDomain, kManagerError,
-      "Service does not exist");
+  Error::PopulateAndLog(error,
+                        Error::kInvalidArguments,
+                        "Service does not exist",
+                        FROM_HERE);
   return false;
 }
 
@@ -138,7 +91,7 @@ scoped_refptr<Device> Manager::GetDeviceFromInterfaceName(
   return nullptr;
 }
 
-void Manager::RegisterDevice(scoped_refptr<Device> device) {
+void Manager::RegisterDevice(const scoped_refptr<Device>& device) {
   LOG(INFO) << "Manager::RegisterDevice: registering device "
             << device->GetDeviceName();
   devices_.push_back(device);
@@ -171,23 +124,4 @@ void Manager::ReleaseDHCPPortAccess(const string& interface) {
   firewall_manager_.ReleaseDHCPPortAccess(interface);
 }
 
-void Manager::OnAPServiceOwnerDisappeared(int service_identifier) {
-  LOG(INFO) << "Owner for service " << service_identifier << " disappeared";
-  // Remove service watcher.
-  auto watcher = service_watchers_.find(service_identifier);
-  CHECK(watcher != service_watchers_.end())
-      << "Owner disappeared without watcher setup";
-  service_watchers_.erase(watcher);
-
-  // Remove the service.
-  for (auto it = services_.begin(); it != services_.end(); ++it) {
-    if ((*it)->identifier() == service_identifier) {
-      services_.erase(it);
-      return;
-    }
-  }
-  LOG(INFO) << "Owner for service " << service_identifier
-            << " disappeared before it is registered";
-}
-
 }  // namespace apmanager
diff --git a/manager.h b/manager.h
index 06a9c3b..578a1db 100644
--- a/manager.h
+++ b/manager.h
@@ -17,52 +17,43 @@
 #ifndef APMANAGER_MANAGER_H_
 #define APMANAGER_MANAGER_H_
 
-#include <map>
 #include <string>
 #include <vector>
 
 #include <base/macros.h>
-#include <brillo/dbus/dbus_service_watcher.h>
 
 #include "apmanager/device_info.h"
 #include "apmanager/firewall_manager.h"
+#include "apmanager/manager_adaptor_interface.h"
 #include "apmanager/service.h"
 #include "apmanager/shill_manager.h"
-#include "dbus_bindings/org.chromium.apmanager.Manager.h"
 
 namespace apmanager {
 
 class ControlInterface;
 
-class Manager : public org::chromium::apmanager::ManagerAdaptor,
-                public org::chromium::apmanager::ManagerInterface {
+class Manager {
  public:
-  template<typename T>
-  using DBusMethodResponse = brillo::dbus_utils::DBusMethodResponse<T>;
-
   explicit Manager(ControlInterface* control_interface);
   virtual ~Manager();
 
-  // Implementation of ManagerInterface.
-  // Handles calls to org.chromium.apmanager.Manager.CreateService().
-  virtual bool CreateService(brillo::ErrorPtr* error,
-                             dbus::Message* message,
-                             dbus::ObjectPath* out_service);
-  // Handles calls to org.chromium.apmanager.Manager.RemoveService().
-  virtual bool RemoveService(brillo::ErrorPtr* error,
-                             dbus::Message* message,
-                             const dbus::ObjectPath& in_service);
-
-  // Register DBus object.
-  void RegisterAsync(
-      brillo::dbus_utils::ExportedObjectManager* object_manager,
-      const scoped_refptr<dbus::Bus>& bus,
-      const base::Callback<void(bool)>& completion_callback);
+  // Register this object to the RPC interface asynchronously.
+  void RegisterAsync(const base::Callback<void(bool)>& completion_callback);
+
+  // Create and return a new Service instance. The newly created instance
+  // will be added to the service list, it will only get deleted via
+  // RemoveService.
+  scoped_refptr<Service> CreateService();
+
+  // Remove |service| from the service list. Return true if service is found
+  // and deleted from the list, false otherwise. |error| will be populated
+  // on failure.
+  bool RemoveService(const scoped_refptr<Service>& service, Error* error);
 
   virtual void Start();
   virtual void Stop();
 
-  virtual void RegisterDevice(scoped_refptr<Device> device);
+  virtual void RegisterDevice(const scoped_refptr<Device>& device);
 
   // Return an unuse device with AP interface mode support.
   virtual scoped_refptr<Device> GetAvailableDevice();
@@ -94,18 +85,9 @@ class Manager : public org::chromium::apmanager::ManagerAdaptor,
  private:
   friend class ManagerTest;
 
-  // This is invoked when the owner of an AP service disappeared.
-  void OnAPServiceOwnerDisappeared(int service_identifier);
-
   ControlInterface* control_interface_;
   int service_identifier_;
-  std::unique_ptr<brillo::dbus_utils::DBusObject> dbus_object_;
-  scoped_refptr<dbus::Bus> bus_;
-  std::vector<std::unique_ptr<Service>> services_;
   std::vector<scoped_refptr<Device>> devices_;
-  // DBus service watchers for the owner of AP services.
-  using DBusServiceWatcher = brillo::dbus_utils::DBusServiceWatcher;
-  std::map<int, std::unique_ptr<DBusServiceWatcher>> service_watchers_;
   DeviceInfo device_info_;
 
   // Manager for communicating with shill (connection manager).
@@ -113,6 +95,12 @@ class Manager : public org::chromium::apmanager::ManagerAdaptor,
   // Manager for communicating with remote firewall service.
   FirewallManager firewall_manager_;
 
+  // Put the service list after ShillManager and FirewallManager, since both
+  // are needed for tearing down an active/running Service.
+  std::vector<scoped_refptr<Service>> services_;
+
+  std::unique_ptr<ManagerAdaptorInterface> adaptor_;
+
   DISALLOW_COPY_AND_ASSIGN(Manager);
 };
 
diff --git a/mock_manager.h b/mock_manager.h
index d67d87e..f8b54b7 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -33,7 +33,7 @@ class MockManager : public Manager {
 
   MOCK_METHOD0(Start, void());
   MOCK_METHOD0(Stop, void());
-  MOCK_METHOD1(RegisterDevice, void(scoped_refptr<Device> device));
+  MOCK_METHOD1(RegisterDevice, void(const scoped_refptr<Device>& device));
   MOCK_METHOD0(GetAvailableDevice, scoped_refptr<Device>());
   MOCK_METHOD1(GetDeviceFromInterfaceName,
                scoped_refptr<Device>(const std::string& interface_name));
diff --git a/service.h b/service.h
index 80dccea..e1b6a05 100644
--- a/service.h
+++ b/service.h
@@ -21,6 +21,7 @@
 
 #include <base/callback.h>
 #include <base/macros.h>
+#include <base/memory/ref_counted.h>
 #include <base/memory/weak_ptr.h>
 #include <brillo/process.h>
 
@@ -40,7 +41,7 @@ class Manager;
 class EventDispatcher;
 #endif  // __BRILLO__
 
-class Service {
+class Service : public base::RefCounted<Service> {
  public:
   Service(Manager* manager, int service_identifier);
   virtual ~Service();
diff --git a/service_unittest.cc b/service_unittest.cc
index a23e06c..34d8721 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -76,7 +76,7 @@ class ServiceTest : public testing::Test {
         .WillByDefault(ReturnNew<FakeConfigAdaptor>());
     // Defer creation of Service object to allow ControlInterface to
     // setup expectations for generating fake adaptors.
-    service_.reset(new Service(&manager_, kServiceIdentifier));
+    service_ = new Service(&manager_, kServiceIdentifier);
   }
   virtual void SetUp() {
     service_->dhcp_server_factory_ = &dhcp_server_factory_;
@@ -116,7 +116,7 @@ class ServiceTest : public testing::Test {
   MockFileWriter file_writer_;
   MockProcessFactory process_factory_;
   MockHostapdMonitor* hostapd_monitor_;
-  std::unique_ptr<Service> service_;
+  scoped_refptr<Service> service_;
 };
 
 TEST_F(ServiceTest, StartWhenServiceAlreadyRunning) {