Avoid touching dbus::ObjectManager ref count before it's fully cosntructed

dbus::ObjectManager's reference count is zero at the beginning of its
constructor, and used to be incremented at base::Bind there implicitly.
However, if PostTaskAndReplyWithResult there failed, the reference
is gone and the reference count gets to 0 again. And as the result,
`new ObjectManager` may return a stale pointer.

This CL adds a static constructor to ObjectManager, and moves the
ref count manipulation part out from the constructor.

Bug: 866456
Change-Id: I5dd8947a5087359005cbcb75e1bb2a0a42ebeda8
Reviewed-on: https://chromium-review.googlesource.com/1148037
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577465}

dbus/BUILD.gn

@@ -64,8 +64,6 @@ static_library("test_support") {
     "mock_bus.h",
     "mock_exported_object.cc",
     "mock_exported_object.h",
-    "mock_object_manager.cc",
-    "mock_object_manager.h",
     "mock_object_proxy.cc",
     "mock_object_proxy.h",
   ]

dbus/bus.cc

@@ -311,7 +311,7 @@ ObjectManager* Bus::GetObjectManager(const std::string& service_name,
   }
 
   scoped_refptr<ObjectManager> object_manager =
-      new ObjectManager(this, service_name, object_path);
+      ObjectManager::Create(this, service_name, object_path);
   object_manager_table_[key] = object_manager;
 
   return object_manager.get();

dbus/mock_object_manager.cc

-// Copyright (c) 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.
-
-#include "dbus/mock_object_manager.h"
-
-namespace dbus {
-
-MockObjectManager::MockObjectManager(Bus* bus,
-                                     const std::string& service_name,
-                                     const ObjectPath& object_path)
-    : ObjectManager(bus, service_name, object_path) {
-}
-
-MockObjectManager::~MockObjectManager() = default;
-
-}  // namespace dbus

dbus/mock_object_manager.h

-// Copyright (c) 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 DBUS_MOCK_OBJECT_MANAGER_H_
-#define DBUS_MOCK_OBJECT_MANAGER_H_
-
-#include <string>
-
-#include "dbus/message.h"
-#include "dbus/object_manager.h"
-#include "dbus/object_path.h"
-#include "dbus/object_proxy.h"
-#include "testing/gmock/include/gmock/gmock.h"
-
-namespace dbus {
-
-// Mock for ObjectManager.
-class MockObjectManager : public ObjectManager {
- public:
-  MockObjectManager(Bus* bus,
-                    const std::string& service_name,
-                    const ObjectPath& object_path);
-
-  MOCK_METHOD2(RegisterInterface, void(const std::string&,
-                                       Interface*));
-  MOCK_METHOD1(UnregisterInterface, void(const std::string&));
-  MOCK_METHOD0(GetObjects, std::vector<ObjectPath>());
-  MOCK_METHOD1(GetObjectsWithInterface,
-               std::vector<ObjectPath>(const std::string&));
-  MOCK_METHOD1(GetObjectProxy, ObjectProxy*(const ObjectPath&));
-  MOCK_METHOD2(GetProperties, PropertySet*(const ObjectPath&,
-                                           const std::string&));
-
- protected:
-  ~MockObjectManager() override;
-};
-
-}  // namespace dbus
-
-#endif  // DBUS_MOCK_OBJECT_MANAGER_H_

dbus/object_manager.cc

@@ -28,6 +28,25 @@ ObjectManager::Object::Object()
 
 ObjectManager::Object::~Object() = default;
 
+scoped_refptr<ObjectManager> ObjectManager::Create(
+    Bus* bus,
+    const std::string& service_name,
+    const ObjectPath& object_path) {
+  auto object_manager =
+      base::WrapRefCounted(new ObjectManager(bus, service_name, object_path));
+
+  // Set up a match rule and a filter function to handle PropertiesChanged
+  // signals from the service. This is important to avoid any race conditions
+  // that might cause us to miss PropertiesChanged signals once all objects are
+  // initialized via GetManagedObjects.
+  base::PostTaskAndReplyWithResult(
+      bus->GetDBusTaskRunner(), FROM_HERE,
+      base::BindOnce(&ObjectManager::SetupMatchRuleAndFilter, object_manager),
+      base::BindOnce(&ObjectManager::OnSetupMatchRuleAndFilterComplete,
+                     object_manager));
+  return object_manager;
+}
+
 ObjectManager::ObjectManager(Bus* bus,
                              const std::string& service_name,
                              const ObjectPath& object_path)
@@ -46,16 +65,6 @@ ObjectManager::ObjectManager(Bus* bus,
   object_proxy_->SetNameOwnerChangedCallback(
       base::Bind(&ObjectManager::NameOwnerChanged,
                  weak_ptr_factory_.GetWeakPtr()));
-
-  // Set up a match rule and a filter function to handle PropertiesChanged
-  // signals from the service. This is important to avoid any race conditions
-  // that might cause us to miss PropertiesChanged signals once all objects are
-  // initialized via GetManagedObjects.
-  base::PostTaskAndReplyWithResult(
-      bus_->GetDBusTaskRunner(),
-      FROM_HERE,
-      base::Bind(&ObjectManager::SetupMatchRuleAndFilter, this),
-      base::Bind(&ObjectManager::OnSetupMatchRuleAndFilterComplete, this));
 }
 
 ObjectManager::~ObjectManager() {

dbus/object_manager.h

@@ -136,9 +136,9 @@ class Signal;
 // ObjectManager implements both the D-Bus client components of the D-Bus
 // Object Manager interface, as internal methods, and a public API for
 // client classes to utilize.
-class CHROME_DBUS_EXPORT ObjectManager
+class CHROME_DBUS_EXPORT ObjectManager final
     : public base::RefCountedThreadSafe<ObjectManager> {
-public:
+ public:
   // ObjectManager::Interface must be implemented by any class wishing to have
   // its remote objects managed by an ObjectManager.
   class Interface {
@@ -184,43 +184,43 @@ public:
   };
 
   // Client code should use Bus::GetObjectManager() instead of this constructor.
-  ObjectManager(Bus* bus,
-                const std::string& service_name,
-                const ObjectPath& object_path);
+  static scoped_refptr<ObjectManager> Create(Bus* bus,
+                                             const std::string& service_name,
+                                             const ObjectPath& object_path);
 
   // Register a client implementation class |interface| for the given D-Bus
   // interface named in |interface_name|. That object's CreateProperties()
   // method will be used to create instances of dbus::PropertySet* when
   // required.
-  virtual void RegisterInterface(const std::string& interface_name,
-                                 Interface* interface);
+  void RegisterInterface(const std::string& interface_name,
+                         Interface* interface);
 
   // Unregister the implementation class for the D-Bus interface named in
   // |interface_name|, objects and properties of this interface will be
   // ignored.
-  virtual void UnregisterInterface(const std::string& interface_name);
+  void UnregisterInterface(const std::string& interface_name);
 
   // Returns a list of object paths, in an undefined order, of objects known
   // to this manager.
-  virtual std::vector<ObjectPath> GetObjects();
+  std::vector<ObjectPath> GetObjects();
 
   // Returns the list of object paths, in an undefined order, of objects
   // implementing the interface named in |interface_name| known to this manager.
-  virtual std::vector<ObjectPath> GetObjectsWithInterface(
+  std::vector<ObjectPath> GetObjectsWithInterface(
       const std::string& interface_name);
 
   // Returns a ObjectProxy pointer for the given |object_path|. Unlike
   // the equivalent method on Bus this will return NULL if the object
   // manager has not been informed of that object's existence.
-  virtual ObjectProxy* GetObjectProxy(const ObjectPath& object_path);
+  ObjectProxy* GetObjectProxy(const ObjectPath& object_path);
 
   // Returns a PropertySet* pointer for the given |object_path| and
   // |interface_name|, or NULL if the object manager has not been informed of
   // that object's existence or the interface's properties. The caller should
   // cast the returned pointer to the appropriate type, e.g.:
   //   static_cast<Properties*>(GetProperties(object_path, my_interface));
-  virtual PropertySet* GetProperties(const ObjectPath& object_path,
-                                     const std::string& interface_name);
+  PropertySet* GetProperties(const ObjectPath& object_path,
+                             const std::string& interface_name);
 
   // Instructs the object manager to refresh its list of managed objects;
   // automatically called by the D-Bus thread manager, there should never be
@@ -233,12 +233,14 @@ public:
   // BLOCKING CALL.
   void CleanUp();
 
- protected:
-  virtual ~ObjectManager();
-
  private:
   friend class base::RefCountedThreadSafe<ObjectManager>;
 
+  ObjectManager(Bus* bus,
+                const std::string& service_name,
+                const ObjectPath& object_path);
+  ~ObjectManager();
+
   // Called from the constructor to add a match rule for PropertiesChanged
   // signals on the D-Bus thread and set up a corresponding filter function.
   bool SetupMatchRuleAndFilter();