chromeos: Make CrosDBusService not be a singleton.

chromeos/dbus/services/cros_dbus_service.cc

 // Copyright (c) 2012 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 "chromeos/dbus/services/cros_dbus_service.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/sys_info.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "dbus/bus.h"
 #include "dbus/exported_object.h"
 #include "dbus/object_path.h"
-#include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
-namespace {
-
-CrosDBusService* g_cros_dbus_service = NULL;
-
-}  // namespace
-
 // The CrosDBusService implementation used in production, and unit tests.
 class CrosDBusServiceImpl : public CrosDBusService {
  public:
-  CrosDBusServiceImpl(dbus::Bus* bus, ServiceProviderList service_providers)
+  CrosDBusServiceImpl(dbus::Bus* bus,
+                      const std::string& service_name,
+                      const dbus::ObjectPath& object_path,
+                      ServiceProviderList service_providers)
       : service_started_(false),
         origin_thread_id_(base::PlatformThread::CurrentId()),
         bus_(bus),
+        service_name_(service_name),
+        object_path_(object_path),
         service_providers_(std::move(service_providers)) {}

[James Cook, 2017/02/15 01:53:34]

nit: Is there anything useful you can DCHECK here, like the service_name or
object_path not being empty? I like preconditions. :-)

[Daniel Erat, 2017/02/15 16:56:59]

Done.

 
-  ~CrosDBusServiceImpl() override {
-  }
+  ~CrosDBusServiceImpl() override = default;
 
   // Starts the D-Bus service.
   void Start() {
     // Make sure we're running on the origin thread (i.e. the UI thread in
     // production).
     DCHECK(OnOriginThread());
-
-    // Return if the service has been already started.
-    if (service_started_)
-      return;
+    DCHECK(!service_started_);
 
     // There are some situations, described in http://crbug.com/234382#c27,
     // where processes on Linux can wind up stuck in an uninterruptible state
-    // for tens of seconds. If this happens when Chrome is trying to exit,
-    // this unkillable process can wind up clinging to ownership of
-    // kLibCrosServiceName while the system is trying to restart the browser.
-    // This leads to a fatal situation if we don't allow the new browser
-    // instance to replace the old as the owner of kLibCrosServiceName as seen
-    // in http://crbug.com/234382. Hence, REQUIRE_PRIMARY_ALLOW_REPLACEMENT.
-    bus_->RequestOwnership(kLibCrosServiceName,
-                           dbus::Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT,
-                           base::Bind(&CrosDBusServiceImpl::OnOwnership,
-                                      base::Unretained(this)));
+    // for tens of seconds. If this happens when Chrome is trying to exit, this
+    // unkillable process can wind up clinging to ownership of |service_name_|
+    // while the system is trying to restart the browser. This leads to a fatal
+    // situation if we don't allow the new browser instance to replace the old
+    // as the owner of |service_name_| as seen in http://crbug.com/234382.
+    // Hence, REQUIRE_PRIMARY_ALLOW_REPLACEMENT.
+    bus_->RequestOwnership(
+        service_name_, dbus::Bus::REQUIRE_PRIMARY_ALLOW_REPLACEMENT,
+        base::Bind(&CrosDBusServiceImpl::OnOwnership, base::Unretained(this)));
 
-    exported_object_ = bus_->GetExportedObject(
-        dbus::ObjectPath(kLibCrosServicePath));
-
+    exported_object_ = bus_->GetExportedObject(object_path_);
     for (size_t i = 0; i < service_providers_.size(); ++i)
       service_providers_[i]->Start(exported_object_);
 
+    VLOG(1) << "CrosDBusServiceImpl started for " << service_name_;
     service_started_ = true;
-
-    VLOG(1) << "CrosDBusServiceImpl started.";
   }
 
  private:
   // Returns true if the current thread is on the origin thread.
   bool OnOriginThread() {
     return base::PlatformThread::CurrentId() == origin_thread_id_;
   }
 
   // Called when an ownership request is completed.
   void OnOwnership(const std::string& service_name,
                    bool success) {
     LOG_IF(FATAL, !success) << "Failed to own: " << service_name;
   }
 
   bool service_started_;
   base::PlatformThreadId origin_thread_id_;
   dbus::Bus* bus_;
+  std::string service_name_;
+  dbus::ObjectPath object_path_;
   scoped_refptr<dbus::ExportedObject> exported_object_;
 
   // Service providers that form CrosDBusService.
   ServiceProviderList service_providers_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrosDBusServiceImpl);
 };
 
 // The stub CrosDBusService implementation used on Linux desktop,
 // which does nothing as of now.
 class CrosDBusServiceStubImpl : public CrosDBusService {
  public:
-  CrosDBusServiceStubImpl() {
-  }
+  CrosDBusServiceStubImpl() {}

[hashimoto, 2017/02/15 08:44:06]

nit: =default here doesn't work?

[Daniel Erat, 2017/02/15 16:56:59]

hmm. i thought i'd seen compiler errors from one toolchain when i tried this,
but maybe it was just an issue due to not defining a c'tor for the base class
earlier. it appears to work now in both desktop and simple chrome builds.

+  ~CrosDBusServiceStubImpl() override = default;
 
-  ~CrosDBusServiceStubImpl() override {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CrosDBusServiceStubImpl);
 };
 
 // static
-void CrosDBusService::Initialize(ServiceProviderList service_providers) {
-  if (g_cros_dbus_service) {
-    LOG(WARNING) << "CrosDBusService was already initialized";
-    return;
-  }
+std::unique_ptr<CrosDBusService> CrosDBusService::Create(
+    const std::string& service_name,
+    const dbus::ObjectPath& object_path,
+    ServiceProviderList service_providers) {
   dbus::Bus* bus = DBusThreadManager::Get()->GetSystemBus();
-  if (base::SysInfo::IsRunningOnChromeOS() && bus) {
-    auto* service = new CrosDBusServiceImpl(bus, std::move(service_providers));
-    g_cros_dbus_service = service;
-    service->Start();
-  } else {
-    g_cros_dbus_service = new CrosDBusServiceStubImpl;
-  }
-  VLOG(1) << "CrosDBusService initialized";
+  if (!base::SysInfo::IsRunningOnChromeOS() || !bus)

[hashimoto, 2017/02/15 08:44:06]

How about using DBusThreadManager::IsUsingFakes instead?

[Daniel Erat, 2017/02/15 16:56:59]

i'll give it a try; hopefully it detects the same thing (i'm not sure how we
initialize all tests).

+    return std::unique_ptr<CrosDBusService>(new CrosDBusServiceStubImpl());

[hashimoto, 2017/02/15 08:44:06]

nit: base::MakeUnique?

[Daniel Erat, 2017/02/15 16:56:59]

ah, thanks. i wasn't sure about the rules around how a std::unique_ptr's
templated type would be upcast when returning it.

+
+  return CreateRealImpl(bus, service_name, object_path,
+                        std::move(service_providers));
 }
 
 // static
-void CrosDBusService::InitializeForTesting(
+std::unique_ptr<CrosDBusService> CrosDBusService::CreateRealImpl(

[satorux1, 2017/02/15 06:48:01]

nit: this function looks pretty short. maybe just inline the code in Create()
and remove this function?

[Daniel Erat, 2017/02/15 16:56:59]

i would, except it's also needed by a test (see comment in header)

     dbus::Bus* bus,
+    const std::string& service_name,
+    const dbus::ObjectPath& object_path,
     ServiceProviderList service_providers) {
-  if (g_cros_dbus_service) {
-    LOG(WARNING) << "CrosDBusService was already initialized";
-    return;
-  }
-  auto* service = new CrosDBusServiceImpl(bus, std::move(service_providers));
+  auto service = base::MakeUnique<CrosDBusServiceImpl>(
+      bus, service_name, object_path, std::move(service_providers));
   service->Start();
-  g_cros_dbus_service = service;
-  VLOG(1) << "CrosDBusService initialized";
+  return std::move(service);
 }
 
-// static
-void CrosDBusService::Shutdown() {
-  delete g_cros_dbus_service;
-  g_cros_dbus_service = NULL;
-  VLOG(1) << "CrosDBusService Shutdown completed";

[James Cook, 2017/02/15 01:53:34]

Was this VLOG not useful? It kinda feels like we either need VLOGs for both
startup and shutdown, or we don't need them at all.

[Daniel Erat, 2017/02/15 16:57:00]

uh... well, _i_ never used it. :-P i've deleted the other VLOG in
CrosDBusServiceImpl's c'tor to match.

-}
+CrosDBusService::~CrosDBusService() = default;
 
-CrosDBusService::~CrosDBusService() {
-}
+CrosDBusService::CrosDBusService() = default;
 
-CrosDBusService::ServiceProviderInterface::~ServiceProviderInterface() {
-}
+CrosDBusService::ServiceProviderInterface::~ServiceProviderInterface() =
+    default;
 
 }  // namespace chromeos