Track active references in ShillClientHelper

chromeos/dbus/shill_client_helper.cc

Index: chromeos/dbus/shill_client_helper.cc
diff --git a/chromeos/dbus/shill_client_helper.cc b/chromeos/dbus/shill_client_helper.cc
index 87b526cc40906d0d2dad7fbd22881dea0cf1ba26..1108e11dd41c3c274d3a69e857fdfe92e6e8fdcd 100644
--- a/chromeos/dbus/shill_client_helper.cc
+++ b/chromeos/dbus/shill_client_helper.cc
@@ -18,10 +18,25 @@ namespace {
 const char kInvalidResponseErrorName[] = "";  // No error name.
 const char kInvalidResponseErrorMessage[] = "Invalid response.";
 
+class HelperRelease {

[hashimoto, 2013/09/17 03:28:53]

nit: "Helper" here sounds ambiguous, does it mean "this class is a helper to
release some class", or "this class is to release ShillClientHelper"? (maybe
both)
How about renaming this to ReferenceKeeper or ReleaseCaller or something?

[stevenjb, 2013/09/17 21:03:17]

-> ShillClientHelperAutoRelease

+ public:
+  HelperRelease(base::WeakPtr<ShillClientHelper> helper)
+      : helper_(helper) {
+  }
+  ~HelperRelease() {
+    if (helper_)
+      helper_->Release();
+  }
+ private:
+  base::WeakPtr<ShillClientHelper> helper_;
+};
+
 void OnBooleanMethodWithErrorCallback(
+    base::WeakPtr<ShillClientHelper> helper,
     const ShillClientHelper::BooleanCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
+  HelperRelease release(helper);

[hashimoto, 2013/09/17 03:28:53]

I'm a bit worried about manually adding HelperRelease and AddRef for each
methods because it's quite easy to forget to add one or both of this pair in
future.

How about doing something like:
 1. Call AddRef from the ctor of HelperRelease.
 2. Create an object which deletes HelperRelease in its dtor. (you can use
WeakPtr<HelperRelease> to avoid deleting the same HelperRelease twice)
 3. Change the argument of OnBooleanMethodWithErrorCallback (and its family
members) from base::WeakPtr<ShillClientHelper> to scoped_ptr<The new class
mentioned in #2>. (or use base::Owned instead of scoped_ptr)

This way, what you should do for each method is just adding one scoped_ptr
argument for the callback function.

[stevenjb, 2013/09/17 21:03:17]

I initially considered that approach but decided against it because I thought it
would add more complexity then it would be worth. That said, I made the decision
before adding HelperRelease, but maybe if I create a ShillClientHelperWrapper
object to these instead, that can handle the AddRef and Release calls.
 

   if (!response) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
@@ -32,13 +47,14 @@ void OnBooleanMethodWithErrorCallback(
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
-  callback.Run(result);
 }
 
 void OnStringMethodWithErrorCallback(
+    base::WeakPtr<ShillClientHelper> helper,
     const ShillClientHelper::StringCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   if (!response) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
@@ -53,8 +69,10 @@ void OnStringMethodWithErrorCallback(
 }
 
 // Handles responses for methods without results.
-void OnVoidMethod(const VoidDBusMethodCallback& callback,
+void OnVoidMethod(base::WeakPtr<ShillClientHelper> helper,
+                  const VoidDBusMethodCallback& callback,
                   dbus::Response* response) {
+  HelperRelease release(helper);
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE);
     return;
@@ -64,8 +82,10 @@ void OnVoidMethod(const VoidDBusMethodCallback& callback,
 
 // Handles responses for methods with ObjectPath results.
 void OnObjectPathMethod(
+    base::WeakPtr<ShillClientHelper> helper,
     const ObjectPathDBusMethodCallback& callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE, dbus::ObjectPath());
     return;
@@ -81,9 +101,11 @@ void OnObjectPathMethod(
 
 // Handles responses for methods with ObjectPath results and no status.
 void OnObjectPathMethodWithoutStatus(
+    base::WeakPtr<ShillClientHelper> helper,
     const ObjectPathCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   if (!response) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
@@ -99,8 +121,10 @@ void OnObjectPathMethodWithoutStatus(
 
 // Handles responses for methods with DictionaryValue results.
 void OnDictionaryValueMethod(
+    base::WeakPtr<ShillClientHelper> helper,
     const ShillClientHelper::DictionaryValueCallback& callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   if (!response) {
     base::DictionaryValue result;
     callback.Run(DBUS_METHOD_CALL_FAILURE, result);
@@ -119,17 +143,21 @@ void OnDictionaryValueMethod(
 
 // Handles responses for methods without results.
 void OnVoidMethodWithErrorCallback(
+    base::WeakPtr<ShillClientHelper> helper,
     const base::Closure& callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   callback.Run();
 }
 
 // Handles responses for methods with DictionaryValue results.
 // Used by CallDictionaryValueMethodWithErrorCallback().
 void OnDictionaryValueMethodWithErrorCallback(
+    base::WeakPtr<ShillClientHelper> helper,
     const ShillClientHelper::DictionaryValueCallbackWithoutStatus& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   dbus::MessageReader reader(response);
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   base::DictionaryValue* result = NULL;
@@ -142,9 +170,11 @@ void OnDictionaryValueMethodWithErrorCallback(
 
 // Handles responses for methods with ListValue results.
 void OnListValueMethodWithErrorCallback(
+    base::WeakPtr<ShillClientHelper> helper,
     const ShillClientHelper::ListValueCallback& callback,
     const ShillClientHelper::ErrorCallback& error_callback,
     dbus::Response* response) {
+  HelperRelease release(helper);
   dbus::MessageReader reader(response);
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   base::ListValue* result = NULL;
@@ -156,8 +186,10 @@ void OnListValueMethodWithErrorCallback(
 }
 
 // Handles running appropriate error callbacks.
-void OnError(const ShillClientHelper::ErrorCallback& error_callback,
+void OnError(base::WeakPtr<ShillClientHelper> helper,
+             const ShillClientHelper::ErrorCallback& error_callback,
              dbus::ErrorResponse* response) {
+  HelperRelease release(helper);
   std::string error_name;
   std::string error_message;
   if (response) {
@@ -171,9 +203,10 @@ void OnError(const ShillClientHelper::ErrorCallback& error_callback,
 
 }  // namespace
 
-ShillClientHelper::ShillClientHelper(dbus::Bus* bus,
-                                     dbus::ObjectProxy* proxy)
+ShillClientHelper::ShillClientHelper(dbus::ObjectProxy* proxy)
     : proxy_(proxy),
+      owner_(NULL),
+      active_refs_(0),
       weak_ptr_factory_(this) {
 }
 
@@ -182,8 +215,24 @@ ShillClientHelper::~ShillClientHelper() {
       << "ShillClientHelper destroyed with active observers";
 }
 
+void ShillClientHelper::SetOwner(Owner* owner) {
+  CHECK(!owner_);
+  owner_ = owner;
+}
+
+void ShillClientHelper::AddRef() {
+  ++active_refs_;
+}
+
+void ShillClientHelper::Release() {
+  --active_refs_;
+  if (active_refs_ == 0 && owner_)
+    owner_->NotifyReleased(this);  // May delete this
+}
+
 void ShillClientHelper::AddPropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
+  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]);
@@ -196,6 +245,7 @@ void ShillClientHelper::AddPropertyChangedObserver(
 void ShillClientHelper::RemovePropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
   observer_list_.RemoveObserver(observer);
+  Release();
 }
 
 void ShillClientHelper::MonitorPropertyChanged(
@@ -225,8 +275,10 @@ void ShillClientHelper::CallVoidMethod(
     dbus::MethodCall* method_call,
     const VoidDBusMethodCallback& callback) {
   DCHECK(!callback.is_null());
+  AddRef();
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&OnVoidMethod,
+                                weak_ptr_factory_.GetWeakPtr(),
                                 callback));
 }
 
@@ -234,8 +286,10 @@ void ShillClientHelper::CallObjectPathMethod(
     dbus::MethodCall* method_call,
     const ObjectPathDBusMethodCallback& callback) {
   DCHECK(!callback.is_null());
+  AddRef();
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&OnObjectPathMethod,
+                                weak_ptr_factory_.GetWeakPtr(),
                                 callback));
 }
 
@@ -245,13 +299,16 @@ void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
     const ErrorCallback& error_callback) {
   DCHECK(!callback.is_null());
   DCHECK(!error_callback.is_null());
+  AddRef();
   proxy_->CallMethodWithErrorCallback(
       method_call,
       dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnObjectPathMethodWithoutStatus,
+                 weak_ptr_factory_.GetWeakPtr(),
                  callback,
                  error_callback),
       base::Bind(&OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
@@ -259,8 +316,10 @@ void ShillClientHelper::CallDictionaryValueMethod(
     dbus::MethodCall* method_call,
     const DictionaryValueCallback& callback) {
   DCHECK(!callback.is_null());
+  AddRef();
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&OnDictionaryValueMethod,
+                                weak_ptr_factory_.GetWeakPtr(),
                                 callback));
 }
 
@@ -270,11 +329,14 @@ void ShillClientHelper::CallVoidMethodWithErrorCallback(
     const ErrorCallback& error_callback) {
   DCHECK(!callback.is_null());
   DCHECK(!error_callback.is_null());
+  AddRef();
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnVoidMethodWithErrorCallback,
+                 weak_ptr_factory_.GetWeakPtr(),
                  callback),
       base::Bind(&OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
@@ -284,12 +346,15 @@ void ShillClientHelper::CallBooleanMethodWithErrorCallback(
     const ErrorCallback& error_callback) {
   DCHECK(!callback.is_null());
   DCHECK(!error_callback.is_null());
+  AddRef();
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnBooleanMethodWithErrorCallback,
+                 weak_ptr_factory_.GetWeakPtr(),
                  callback,
                  error_callback),
       base::Bind(&OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
@@ -299,12 +364,15 @@ void ShillClientHelper::CallStringMethodWithErrorCallback(
     const ErrorCallback& error_callback) {
   DCHECK(!callback.is_null());
   DCHECK(!error_callback.is_null());
+  AddRef();
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&OnStringMethodWithErrorCallback,
+                 weak_ptr_factory_.GetWeakPtr(),
                  callback,
                  error_callback),
       base::Bind(&OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
@@ -314,13 +382,16 @@ void ShillClientHelper::CallDictionaryValueMethodWithErrorCallback(
     const ErrorCallback& error_callback) {
   DCHECK(!callback.is_null());
   DCHECK(!error_callback.is_null());
+  AddRef();
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(
           &OnDictionaryValueMethodWithErrorCallback,
+          weak_ptr_factory_.GetWeakPtr(),
           callback,
           error_callback),
       base::Bind(&OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
@@ -330,13 +401,16 @@ void ShillClientHelper::CallListValueMethodWithErrorCallback(
     const ErrorCallback& error_callback) {
   DCHECK(!callback.is_null());
   DCHECK(!error_callback.is_null());
+  AddRef();
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(
           &OnListValueMethodWithErrorCallback,
+          weak_ptr_factory_.GetWeakPtr(),
           callback,
           error_callback),
       base::Bind(&OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }