Track active references in ShillClientHelper

chromeos/dbus/shill_client_helper.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/shill_client_helper.h"
 
 #include "base/bind.h"
 #include "base/values.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
 #include "dbus/values_util.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 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;
   }
   dbus::MessageReader reader(response);
   bool result;
   if (!reader.PopBool(&result)) {
     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;
   }
   dbus::MessageReader reader(response);
   std::string result;
   if (!reader.PopString(&result)) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
   callback.Run(result);
 }
 
 // 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;
   }
   callback.Run(DBUS_METHOD_CALL_SUCCESS);
 }
 
 // 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;
   }
   dbus::MessageReader reader(response);
   dbus::ObjectPath result;
   if (!reader.PopObjectPath(&result)) {
     callback.Run(DBUS_METHOD_CALL_FAILURE, dbus::ObjectPath());
     return;
   }
   callback.Run(DBUS_METHOD_CALL_SUCCESS, result);
 }
 
 // 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;
   }
   dbus::MessageReader reader(response);
   dbus::ObjectPath result;
   if (!reader.PopObjectPath(&result)) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
   callback.Run(result);
 }
 
 // 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);
     return;
   }
   dbus::MessageReader reader(response);
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   base::DictionaryValue* result = NULL;
   if (!value.get() || !value->GetAsDictionary(&result)) {
     base::DictionaryValue result;
     callback.Run(DBUS_METHOD_CALL_FAILURE, result);
     return;
   }
   callback.Run(DBUS_METHOD_CALL_SUCCESS, *result);
 }
 
 // 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;
   if (!value.get() || !value->GetAsDictionary(&result)) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
   callback.Run(*result);
 }
 
 // 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;
   if (!value.get() || !value->GetAsList(&result)) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
   callback.Run(*result);
 }
 
 // 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) {
     // Error message may contain the error message as string.
     dbus::MessageReader reader(response);
     error_name = response->GetErrorName();
     reader.PopString(&error_message);
   }
   error_callback.Run(error_name, error_message);
 }
 
 }  // 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) {
 }
 
 ShillClientHelper::~ShillClientHelper() {
   LOG_IF(ERROR, observer_list_.might_have_observers())
       << "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]);
   }
   interfaces_to_be_monitored_.clear();
 
   observer_list_.AddObserver(observer);
 }
 
 void ShillClientHelper::RemovePropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
   observer_list_.RemoveObserver(observer);
+  Release();
 }
 
 void ShillClientHelper::MonitorPropertyChanged(
     const std::string& interface_name) {
   if (observer_list_.might_have_observers()) {
     // Effectively monitor the PropertyChanged now.
     MonitorPropertyChangedInternal(interface_name);
   } else {
     // Delay the ConnectToSignal until an observer is added.
     interfaces_to_be_monitored_.push_back(interface_name);
   }
 }
 
 void ShillClientHelper::MonitorPropertyChangedInternal(
     const std::string& interface_name) {
   // We are not using dbus::PropertySet to monitor PropertyChanged signal
   // because the interface is not "org.freedesktop.DBus.Properties".
   proxy_->ConnectToSignal(interface_name,
                           flimflam::kMonitorPropertyChanged,
                           base::Bind(&ShillClientHelper::OnPropertyChanged,
                                      weak_ptr_factory_.GetWeakPtr()),
                           base::Bind(&ShillClientHelper::OnSignalConnected,
                                      weak_ptr_factory_.GetWeakPtr()));
 }
 
 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));
 }
 
 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));
 }
 
 void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const ObjectPathCallback& callback,
     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));
 }
 
 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));
 }
 
 void ShillClientHelper::CallVoidMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const base::Closure& callback,
     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));
 }
 
 void ShillClientHelper::CallBooleanMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const BooleanCallback& callback,
     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));
 }
 
 void ShillClientHelper::CallStringMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const StringCallback& callback,
     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));
 }
 
 void ShillClientHelper::CallDictionaryValueMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const DictionaryValueCallbackWithoutStatus& callback,
     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));
 }
 
 void ShillClientHelper::CallListValueMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const ListValueCallback& callback,
     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));
 }
 
 // static
 void ShillClientHelper::AppendValueDataAsVariant(dbus::MessageWriter* writer,
                                                  const base::Value& value) {
   // Support basic types and string-to-string dictionary.
   switch (value.GetType()) {
     case base::Value::TYPE_DICTIONARY: {
       const base::DictionaryValue* dictionary = NULL;
@@ skipping 88 matching lines @@
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   if (!value.get())
     return;
 
   FOR_EACH_OBSERVER(ShillPropertyChangedObserver, observer_list_,
                     OnPropertyChanged(name, *value));
 }
 
 
 }  // namespace chromeos