Revert 227100 "Track active references in ShillClientHelper (Tak..."

> Track active references in ShillClientHelper (Take 2)
> To prevent Shill Service DBus ObjectProxy instances from accumulating,
> remove them when the service becomes inactive.
> 
> Original CL: https://codereview.chromium.org/23658053/
> 
> BUG=223483
> TBR=hashimoto@chromium.org
> 
> Review URL: https://codereview.chromium.org/24558004

Failure:
http://build.chromium.org/p/chromium.memory/builders/Chromium%20OS%20%28amd64%29%20ASAN/builds/6931

TBR=stevenjb@chromium.org

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

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

7 files changed, 29 insertions, 140 deletions

diff --git a/chromeos/dbus/shill_client_helper.cc b/chromeos/dbus/shill_client_helper.cc
index c927180..b430609 100644
--- a/chromeos/dbus/shill_client_helper.cc
+++ b/chromeos/dbus/shill_client_helper.cc
@@ -5,7 +5,6 @@
 #include "chromeos/dbus/shill_client_helper.h"
 
 #include "base/bind.h"
-#include "base/callback_helpers.h"
 #include "base/values.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
@@ -14,36 +13,12 @@
 
 namespace chromeos {
 
-// Class to hold onto a reference to a ShillClientHelper. This calss
-// is owned by callbacks and released once the callback completes.
-// Note: Only success callbacks hold the reference. If an error callback is
-// invoked instead, the success callback will still be destroyed and the
-// RefHolder with it, once the callback chain completes.
-class ShillClientHelper::RefHolder {
- public:
-  explicit RefHolder(base::WeakPtr<ShillClientHelper> helper)
-      : helper_(helper) {
-    helper_->AddRef();
-  }
-  ~RefHolder() {
-    if (helper_)
-      helper_->Release();
-  }
-
- private:
-  base::WeakPtr<ShillClientHelper> helper_;
-};
-
 namespace {
 
 const char kInvalidResponseErrorName[] = "";  // No error name.
 const char kInvalidResponseErrorMessage[] = "Invalid response.";
 
-// Note: here and below, |ref_holder| is unused in the function body. It only
-// exists so that it will be destroyed (and the reference released) with the
-// Callback object once completed.
 void OnBooleanMethodWithErrorCallback(
-    ShillClientHelper::RefHolder* ref_holder,
     const ShillClientHelper::BooleanCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
@@ -61,7 +36,6 @@ void OnBooleanMethodWithErrorCallback(
 }
 
 void OnStringMethodWithErrorCallback(
-    ShillClientHelper::RefHolder* ref_holder,
     const ShillClientHelper::StringCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
@@ -79,8 +53,7 @@ void OnStringMethodWithErrorCallback(
 }
 
 // Handles responses for methods without results.
-void OnVoidMethod(ShillClientHelper::RefHolder* ref_holder,
-                  const VoidDBusMethodCallback& callback,
+void OnVoidMethod(const VoidDBusMethodCallback& callback,
                   dbus::Response* response) {
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE);
@@ -91,7 +64,6 @@ void OnVoidMethod(ShillClientHelper::RefHolder* ref_holder,
 
 // Handles responses for methods with ObjectPath results.
 void OnObjectPathMethod(
-    ShillClientHelper::RefHolder* ref_holder,
     const ObjectPathDBusMethodCallback& callback,
     dbus::Response* response) {
   if (!response) {
@@ -109,7 +81,6 @@ void OnObjectPathMethod(
 
 // Handles responses for methods with ObjectPath results and no status.
 void OnObjectPathMethodWithoutStatus(
-    ShillClientHelper::RefHolder* ref_holder,
     const ObjectPathCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
@@ -128,7 +99,6 @@ void OnObjectPathMethodWithoutStatus(
 
 // Handles responses for methods with DictionaryValue results.
 void OnDictionaryValueMethod(
-    ShillClientHelper::RefHolder* ref_holder,
     const ShillClientHelper::DictionaryValueCallback& callback,
     dbus::Response* response) {
   if (!response) {
@@ -149,7 +119,6 @@ void OnDictionaryValueMethod(
 
 // Handles responses for methods without results.
 void OnVoidMethodWithErrorCallback(
-    ShillClientHelper::RefHolder* ref_holder,
     const base::Closure& callback,
     dbus::Response* response) {
   callback.Run();
@@ -158,7 +127,6 @@ void OnVoidMethodWithErrorCallback(
 // Handles responses for methods with DictionaryValue results.
 // Used by CallDictionaryValueMethodWithErrorCallback().
 void OnDictionaryValueMethodWithErrorCallback(
-    ShillClientHelper::RefHolder* ref_holder,
     const ShillClientHelper::DictionaryValueCallbackWithoutStatus& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
@@ -174,7 +142,6 @@ void OnDictionaryValueMethodWithErrorCallback(
 
 // Handles responses for methods with ListValue results.
 void OnListValueMethodWithErrorCallback(
-    ShillClientHelper::RefHolder* ref_holder,
     const ShillClientHelper::ListValueCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
@@ -204,9 +171,9 @@ void OnError(const ShillClientHelper::ErrorCallback& error_callback,
 
 }  // namespace
 
-ShillClientHelper::ShillClientHelper(dbus::ObjectProxy* proxy)
+ShillClientHelper::ShillClientHelper(dbus::Bus* bus,
+                                     dbus::ObjectProxy* proxy)
     : proxy_(proxy),
-      active_refs_(0),
       weak_ptr_factory_(this) {
 }
 
@@ -215,16 +182,8 @@ ShillClientHelper::~ShillClientHelper() {
       << "ShillClientHelper destroyed with active observers";
 }
 
-void ShillClientHelper::SetReleasedCallback(ReleasedCallback callback) {
-  CHECK(released_callback_.is_null());
-  released_callback_ = callback;
-}
-
 void ShillClientHelper::AddPropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
-  if (observer_list_.HasObserver(observer))
-    return;
-  AddRef();
   // Excecute all the pending MonitorPropertyChanged calls.
   for (size_t i = 0; i < interfaces_to_be_monitored_.size(); ++i) {
     MonitorPropertyChangedInternal(interfaces_to_be_monitored_[i]);
@@ -236,10 +195,7 @@ void ShillClientHelper::AddPropertyChangedObserver(
 
 void ShillClientHelper::RemovePropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
-  if (!observer_list_.HasObserver(observer))
-    return;
   observer_list_.RemoveObserver(observer);
-  Release();
 }
 
 void ShillClientHelper::MonitorPropertyChanged(
@@ -269,22 +225,18 @@ void ShillClientHelper::CallVoidMethod(
     dbus::MethodCall* method_call,
     const VoidDBusMethodCallback& callback) {
   DCHECK(!callback.is_null());
-  proxy_->CallMethod(
-      method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-      base::Bind(&OnVoidMethod,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
-                 callback));
+  proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                     base::Bind(&OnVoidMethod,
+                                callback));
 }
 
 void ShillClientHelper::CallObjectPathMethod(
     dbus::MethodCall* method_call,
     const ObjectPathDBusMethodCallback& callback) {
   DCHECK(!callback.is_null());
-  proxy_->CallMethod(
-      method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-      base::Bind(&OnObjectPathMethod,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
-                 callback));
+  proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                     base::Bind(&OnObjectPathMethod,
+                                callback));
 }
 
 void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
@@ -297,7 +249,6 @@ void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
       method_call,
       dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnObjectPathMethodWithoutStatus,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
                  callback,
                  error_callback),
       base::Bind(&OnError,
@@ -308,11 +259,9 @@ void ShillClientHelper::CallDictionaryValueMethod(
     dbus::MethodCall* method_call,
     const DictionaryValueCallback& callback) {
   DCHECK(!callback.is_null());
-  proxy_->CallMethod(
-      method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-      base::Bind(&OnDictionaryValueMethod,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
-                 callback));
+  proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                     base::Bind(&OnDictionaryValueMethod,
+                                callback));
 }
 
 void ShillClientHelper::CallVoidMethodWithErrorCallback(
@@ -324,7 +273,6 @@ void ShillClientHelper::CallVoidMethodWithErrorCallback(
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnVoidMethodWithErrorCallback,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
                  callback),
       base::Bind(&OnError,
                  error_callback));
@@ -339,7 +287,6 @@ void ShillClientHelper::CallBooleanMethodWithErrorCallback(
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnBooleanMethodWithErrorCallback,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
                  callback,
                  error_callback),
       base::Bind(&OnError,
@@ -355,7 +302,6 @@ void ShillClientHelper::CallStringMethodWithErrorCallback(
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnStringMethodWithErrorCallback,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
                  callback,
                  error_callback),
       base::Bind(&OnError,
@@ -370,10 +316,10 @@ void ShillClientHelper::CallDictionaryValueMethodWithErrorCallback(
   DCHECK(!error_callback.is_null());
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-      base::Bind(&OnDictionaryValueMethodWithErrorCallback,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
-                 callback,
-                 error_callback),
+      base::Bind(
+          &OnDictionaryValueMethodWithErrorCallback,
+          callback,
+          error_callback),
       base::Bind(&OnError,
                  error_callback));
 }
@@ -386,10 +332,10 @@ void ShillClientHelper::CallListValueMethodWithErrorCallback(
   DCHECK(!error_callback.is_null());
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-      base::Bind(&OnListValueMethodWithErrorCallback,
-                 base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
-                 callback,
-                 error_callback),
+      base::Bind(
+          &OnListValueMethodWithErrorCallback,
+          callback,
+          error_callback),
       base::Bind(&OnError,
                  error_callback));
 }
@@ -474,16 +420,6 @@ void ShillClientHelper::AppendServicePropertiesDictionary(
   writer->CloseContainer(&array_writer);
 }
 
-void ShillClientHelper::AddRef() {
-  ++active_refs_;
-}
-
-void ShillClientHelper::Release() {
-  --active_refs_;
-  if (active_refs_ == 0 && !released_callback_.is_null())
-    base::ResetAndReturn(&released_callback_).Run(this);  // May delete this
-}
-
 void ShillClientHelper::OnSignalConnected(const std::string& interface,
                                           const std::string& signal,
                                           bool success) {
@@ -507,4 +443,5 @@ void ShillClientHelper::OnPropertyChanged(dbus::Signal* signal) {
                     OnPropertyChanged(name, *value));
 }
 
+
 }  // namespace chromeos
diff --git a/chromeos/dbus/shill_client_helper.h b/chromeos/dbus/shill_client_helper.h
index a52fc22..12cd994 100644
--- a/chromeos/dbus/shill_client_helper.h
+++ b/chromeos/dbus/shill_client_helper.h
@@ -41,8 +41,6 @@ namespace chromeos {
 // A class to help implement Shill clients.
 class ShillClientHelper {
  public:
-  class RefHolder;
-
   // A callback to handle PropertyChanged signals.
   typedef base::Callback<void(const std::string& name,
                               const base::Value& value)> PropertyChangedHandler;
@@ -70,17 +68,11 @@ class ShillClientHelper {
   // A callback that handles responses for methods with boolean results.
   typedef base::Callback<void(bool result)> BooleanCallback;
 
-  // Callback used to notify owner when this can be safely released.
-  typedef base::Callback<void(ShillClientHelper* helper)> ReleasedCallback;
 
-  explicit ShillClientHelper(dbus::ObjectProxy* proxy);
+  ShillClientHelper(dbus::Bus* bus, dbus::ObjectProxy* proxy);
 
   virtual ~ShillClientHelper();
 
-  // Sets |released_callback_|. This is optional and should only be called at
-  // most once.
-  void SetReleasedCallback(ReleasedCallback callback);
-
   // Adds an |observer| of the PropertyChanged signal.
   void AddPropertyChangedObserver(ShillPropertyChangedObserver* observer);
 
@@ -139,8 +131,6 @@ class ShillClientHelper {
       const ListValueCallback& callback,
       const ErrorCallback& error_callback);
 
-  const dbus::ObjectProxy* object_proxy() const { return proxy_; }
-
   // Appends the value (basic types and string-to-string dictionary) to the
   // writer as a variant.
   static void AppendValueDataAsVariant(dbus::MessageWriter* writer,
@@ -151,13 +141,6 @@ class ShillClientHelper {
       dbus::MessageWriter* writer,
       const base::DictionaryValue& dictionary);
 
- protected:
-  // Reference / Ownership management. If the number of active refs (observers
-  // + in-progress method calls) becomes 0, |released_callback_| (if set) will
-  // be called.
-  void AddRef();
-  void Release();
-
  private:
   // Starts monitoring PropertyChanged signal.
   void MonitorPropertyChangedInternal(const std::string& interface_name);
@@ -171,8 +154,6 @@ class ShillClientHelper {
   void OnPropertyChanged(dbus::Signal* signal);
 
   dbus::ObjectProxy* proxy_;
-  ReleasedCallback released_callback_;
-  int active_refs_;
   PropertyChangedHandler property_changed_handler_;
   ObserverList<ShillPropertyChangedObserver, true /* check_empty */>
       observer_list_;
diff --git a/chromeos/dbus/shill_device_client.cc b/chromeos/dbus/shill_device_client.cc
index 5aa4332..931e71a 100644
--- a/chromeos/dbus/shill_device_client.cc
+++ b/chromeos/dbus/shill_device_client.cc
@@ -215,7 +215,7 @@ class ShillDeviceClientImpl : public ShillDeviceClient {
     // There is no helper for the profile, create it.
     dbus::ObjectProxy* object_proxy =
         bus_->GetObjectProxy(shill::kFlimflamServiceName, device_path);
-    ShillClientHelper* helper = new ShillClientHelper(object_proxy);
+    ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy);
     CHECK(helper) << "Unable to create Shill client helper.";
     helper->MonitorPropertyChanged(shill::kFlimflamDeviceInterface);
     helpers_.insert(HelperMap::value_type(device_path.value(), helper));
diff --git a/chromeos/dbus/shill_ipconfig_client.cc b/chromeos/dbus/shill_ipconfig_client.cc
index 46aeb71..4483078 100644
--- a/chromeos/dbus/shill_ipconfig_client.cc
+++ b/chromeos/dbus/shill_ipconfig_client.cc
@@ -70,7 +70,7 @@ class ShillIPConfigClientImpl : public ShillIPConfigClient {
     // There is no helper for the profile, create it.
     dbus::ObjectProxy* object_proxy =
         bus_->GetObjectProxy(shill::kFlimflamServiceName, ipconfig_path);
-    ShillClientHelper* helper = new ShillClientHelper(object_proxy);
+    ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy);
     helper->MonitorPropertyChanged(shill::kFlimflamIPConfigInterface);
     helpers_.insert(HelperMap::value_type(ipconfig_path.value(), helper));
     return helper;
diff --git a/chromeos/dbus/shill_manager_client.cc b/chromeos/dbus/shill_manager_client.cc
index cae9b68..f3cd1bb 100644
--- a/chromeos/dbus/shill_manager_client.cc
+++ b/chromeos/dbus/shill_manager_client.cc
@@ -219,7 +219,7 @@ class ShillManagerClientImpl : public ShillManagerClient {
   virtual void Init(dbus::Bus* bus) OVERRIDE {
     proxy_ = bus->GetObjectProxy(shill::kFlimflamServiceName,
                                  dbus::ObjectPath(shill::kFlimflamServicePath));
-    helper_.reset(new ShillClientHelper(proxy_));
+    helper_.reset(new ShillClientHelper(bus, proxy_));
     helper_->MonitorPropertyChanged(shill::kFlimflamManagerInterface);
   }
 
diff --git a/chromeos/dbus/shill_profile_client.cc b/chromeos/dbus/shill_profile_client.cc
index 0e63354..e0d8a71 100644
--- a/chromeos/dbus/shill_profile_client.cc
+++ b/chromeos/dbus/shill_profile_client.cc
@@ -86,7 +86,7 @@ ShillClientHelper* ShillProfileClientImpl::GetHelper(
   // There is no helper for the profile, create it.
   dbus::ObjectProxy* object_proxy =
       bus_->GetObjectProxy(shill::kFlimflamServiceName, profile_path);
-  ShillClientHelper* helper = new ShillClientHelper(object_proxy);
+  ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy);
   helper->MonitorPropertyChanged(shill::kFlimflamProfileInterface);
   helpers_.insert(HelperMap::value_type(profile_path.value(), helper));
   return helper;
diff --git a/chromeos/dbus/shill_service_client.cc b/chromeos/dbus/shill_service_client.cc
index faaf75a..40143f6 100644
--- a/chromeos/dbus/shill_service_client.cc
+++ b/chromeos/dbus/shill_service_client.cc
@@ -5,13 +5,11 @@
 #include "chromeos/dbus/shill_service_client.h"
 
 #include "base/bind.h"
-#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/values.h"
 #include "chromeos/dbus/shill_property_changed_observer.h"
 #include "chromeos/dbus/shill_service_client_stub.h"
-#include "chromeos/network/network_event_log.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
@@ -56,18 +54,7 @@ class ShillServiceClientImpl : public ShillServiceClient {
  public:
   explicit ShillServiceClientImpl()
       : bus_(NULL),
-        weak_ptr_factory_(this) {
-  }
-
-  virtual ~ShillServiceClientImpl() {
-    for (HelperMap::iterator iter = helpers_.begin();
-         iter != helpers_.end(); ++iter) {
-      ShillClientHelper* helper = iter->second;
-      bus_->RemoveObjectProxy(shill::kFlimflamServiceName,
-                              helper->object_proxy()->object_path(),
-                              base::Bind(&base::DoNothing));
-      delete helper;
-    }
+        helpers_deleter_(&helpers_) {
   }
 
   virtual void AddPropertyChangedObserver(
@@ -235,33 +222,17 @@ class ShillServiceClientImpl : public ShillServiceClient {
       return it->second;
 
     // There is no helper for the profile, create it.
-    NET_LOG_DEBUG("AddShillClientHelper", service_path.value());
     dbus::ObjectProxy* object_proxy =
         bus_->GetObjectProxy(shill::kFlimflamServiceName, service_path);
-    ShillClientHelper* helper = new ShillClientHelper(object_proxy);
-    helper->SetReleasedCallback(
-        base::Bind(&ShillServiceClientImpl::NotifyReleased,
-                   weak_ptr_factory_.GetWeakPtr()));
+    ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy);
     helper->MonitorPropertyChanged(shill::kFlimflamServiceInterface);
     helpers_.insert(HelperMap::value_type(service_path.value(), helper));
     return helper;
   }
 
-  void NotifyReleased(ShillClientHelper* helper) {
-    // New Shill Service DBus objects are created relatively frequently, so
-    // remove them when they become inactive (no observers and no active method
-    // calls).
-    const dbus::ObjectPath& object_path = helper->object_proxy()->object_path();
-    NET_LOG_DEBUG("RemoveShillClientHelper", object_path.value());
-    bus_->RemoveObjectProxy(shill::kFlimflamServiceName,
-                            object_path, base::Bind(&base::DoNothing));
-    helpers_.erase(object_path.value());
-    delete helper;
-  }
-
   dbus::Bus* bus_;
   HelperMap helpers_;
-  base::WeakPtrFactory<ShillServiceClientImpl> weak_ptr_factory_;
+  STLValueDeleter<HelperMap> helpers_deleter_;
 
   DISALLOW_COPY_AND_ASSIGN(ShillServiceClientImpl);
 };