Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

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 c83bfe0b9d3530f1c9f859eeba37beb225e625d7..4b94e266c3546643fe12c8ac1fa74f1f7eae2960 100644
--- a/chromeos/dbus/shill_client_helper.cc
+++ b/chromeos/dbus/shill_client_helper.cc
@@ -24,6 +24,7 @@ ShillClientHelper::ShillClientHelper(dbus::Bus* bus,
                                      dbus::ObjectProxy* proxy)
     : blocking_method_caller_(bus, proxy),
       proxy_(proxy),
+      ongoing_calls(0),
       weak_ptr_factory_(this) {

[hashimoto, 2013/02/06 03:29:06]

nit: Could you add ALLOW_THIS_IN_INITIALIZER_LIST here while you are at this
code?

[deymo, 2013/02/06 05:44:45]

Done.

 }
 
@@ -33,6 +34,14 @@ ShillClientHelper::~ShillClientHelper() {
       << observer_list_.size();
 }
 
+bool ShillClientHelper::IsObserved() const {
+  return observer_list_.size() > 0;
+}
+
+bool ShillClientHelper::IsWaitingResponse() const {
+  return ongoing_calls > 0;

[hashimoto, 2013/02/06 03:29:06]

How about adding another base::WeakPtrFactory dedicated for method calls and
returning its HasWeakPtrs() here?
This way, you don't need to increment and decrement |ongoing_calls| manually.

[deymo, 2013/02/06 05:44:45]

I need this to return false in the case of the last return (to allow the
callback detect and delete the object).
I'm not a WeakPtr expert, but, I think that during the destructor of the
ScopedCallbackRunner the previous callback is not yet destroyed, so it will not
return 0... right?
It think that the counter is simple and straightforward.

On 2013/02/06 03:29:06, hashimoto wrote:

[hashimoto, 2013/02/07 05:20:31]

You're correct.
Counter is simple but it comes with no maintainability.
You can make a new hard-to-debug resource management bug by simply adding a new
method carelessly.
I'd like to avoid this if possible.

+}
+
 void ShillClientHelper::AddPropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
   observer_list_.AddObserver(observer);
@@ -58,6 +67,7 @@ void ShillClientHelper::MonitorPropertyChanged(
 void ShillClientHelper::CallVoidMethod(
     dbus::MethodCall* method_call,
     const VoidDBusMethodCallback& callback) {
+  ++ongoing_calls;
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&ShillClientHelper::OnVoidMethod,
                                 weak_ptr_factory_.GetWeakPtr(),
@@ -67,6 +77,7 @@ void ShillClientHelper::CallVoidMethod(
 void ShillClientHelper::CallObjectPathMethod(
     dbus::MethodCall* method_call,
     const ObjectPathDBusMethodCallback& callback) {
+  ++ongoing_calls;
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&ShillClientHelper::OnObjectPathMethod,
                                 weak_ptr_factory_.GetWeakPtr(),
@@ -77,6 +88,7 @@ void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const ObjectPathCallback& callback,
     const ErrorCallback& error_callback) {
+  ++ongoing_calls;
   proxy_->CallMethodWithErrorCallback(
       method_call,
       dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
@@ -92,6 +104,7 @@ void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
 void ShillClientHelper::CallDictionaryValueMethod(
     dbus::MethodCall* method_call,
     const DictionaryValueCallback& callback) {
+  ++ongoing_calls;
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&ShillClientHelper::OnDictionaryValueMethod,
                                 weak_ptr_factory_.GetWeakPtr(),
@@ -102,6 +115,7 @@ void ShillClientHelper::CallVoidMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
+  ++ongoing_calls;
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&ShillClientHelper::OnVoidMethodWithErrorCallback,
@@ -116,6 +130,7 @@ void ShillClientHelper::CallDictionaryValueMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const DictionaryValueCallbackWithoutStatus& callback,
     const ErrorCallback& error_callback) {
+  ++ongoing_calls;
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(
@@ -132,6 +147,7 @@ void ShillClientHelper::CallListValueMethodWithErrorCallback(
     dbus::MethodCall* method_call,
     const ListValueCallback& callback,
     const ErrorCallback& error_callback) {
+  ++ongoing_calls;
   proxy_->CallMethodWithErrorCallback(
       method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(
@@ -237,6 +253,7 @@ void ShillClientHelper::OnPropertyChanged(dbus::Signal* signal) {
 
 void ShillClientHelper::OnVoidMethod(const VoidDBusMethodCallback& callback,
                                      dbus::Response* response) {
+  --ongoing_calls;
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE);
     return;
@@ -247,6 +264,7 @@ void ShillClientHelper::OnVoidMethod(const VoidDBusMethodCallback& callback,
 void ShillClientHelper::OnObjectPathMethod(
     const ObjectPathDBusMethodCallback& callback,
     dbus::Response* response) {
+  --ongoing_calls;
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE, dbus::ObjectPath());
     return;
@@ -264,6 +282,7 @@ void ShillClientHelper::OnObjectPathMethodWithoutStatus(
     const ObjectPathCallback& callback,
     const ErrorCallback& error_callback,
     dbus::Response* response) {
+  --ongoing_calls;
   if (!response) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
@@ -280,6 +299,7 @@ void ShillClientHelper::OnObjectPathMethodWithoutStatus(
 void ShillClientHelper::OnDictionaryValueMethod(
     const DictionaryValueCallback& callback,
     dbus::Response* response) {
+  --ongoing_calls;
   if (!response) {
     base::DictionaryValue result;
     callback.Run(DBUS_METHOD_CALL_FAILURE, result);
@@ -299,6 +319,7 @@ void ShillClientHelper::OnDictionaryValueMethod(
 void ShillClientHelper::OnVoidMethodWithErrorCallback(
     const base::Closure& callback,
     dbus::Response* response) {
+  --ongoing_calls;
   callback.Run();
 }
 
@@ -306,6 +327,7 @@ void ShillClientHelper::OnDictionaryValueMethodWithErrorCallback(
     const DictionaryValueCallbackWithoutStatus& callback,
     const ErrorCallback& error_callback,
     dbus::Response* response) {
+  --ongoing_calls;
   dbus::MessageReader reader(response);
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   base::DictionaryValue* result = NULL;
@@ -320,6 +342,7 @@ void ShillClientHelper::OnListValueMethodWithErrorCallback(
     const ListValueCallback& callback,
     const ErrorCallback& error_callback,
     dbus::Response* response) {
+  --ongoing_calls;
   dbus::MessageReader reader(response);
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   base::ListValue* result = NULL;
@@ -332,6 +355,7 @@ void ShillClientHelper::OnListValueMethodWithErrorCallback(
 
 void ShillClientHelper::OnError(const ErrorCallback& error_callback,
                                 dbus::ErrorResponse* response) {
+  --ongoing_calls;
   std::string error_name;
   std::string error_message;
   if (response) {