Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

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.";
 
 }  // namespace
 
 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.

 }
 
 ShillClientHelper::~ShillClientHelper() {
   LOG_IF(ERROR, observer_list_.size() != 0u)
       << "ShillClientHelper destroyed with active observers: "
       << 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);
 }
 
 void ShillClientHelper::RemovePropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 void ShillClientHelper::MonitorPropertyChanged(
     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) {
+  ++ongoing_calls;
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&ShillClientHelper::OnVoidMethod,
                                 weak_ptr_factory_.GetWeakPtr(),
                                 callback));
 }
 
 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(),
                                 callback));
 }
 
 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,
       base::Bind(&ShillClientHelper::OnObjectPathMethodWithoutStatus,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback,
                  error_callback),
       base::Bind(&ShillClientHelper::OnError,
                  weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
 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(),
                                 callback));
 }
 
 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,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback),
       base::Bind(&ShillClientHelper::OnError,
                  weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
 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(
           &ShillClientHelper::OnDictionaryValueMethodWithErrorCallback,
           weak_ptr_factory_.GetWeakPtr(),
           callback,
           error_callback),
       base::Bind(&ShillClientHelper::OnError,
                  weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
 }
 
 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(
           &ShillClientHelper::OnListValueMethodWithErrorCallback,
           weak_ptr_factory_.GetWeakPtr(),
           callback,
           error_callback),
       base::Bind(&ShillClientHelper::OnError,
                  weak_ptr_factory_.GetWeakPtr(),
                  error_callback));
@@ skipping 85 matching lines @@
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   if (!value.get())
     return;
 
   FOR_EACH_OBSERVER(ShillPropertyChangedObserver, observer_list_,
                     OnPropertyChanged(name, *value));
 }
 
 void ShillClientHelper::OnVoidMethod(const VoidDBusMethodCallback& callback,
                                      dbus::Response* response) {
+  --ongoing_calls;
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE);
     return;
   }
   callback.Run(DBUS_METHOD_CALL_SUCCESS);
 }
 
 void ShillClientHelper::OnObjectPathMethod(
     const ObjectPathDBusMethodCallback& callback,
     dbus::Response* response) {
+  --ongoing_calls;
   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);
 }
 
 void ShillClientHelper::OnObjectPathMethodWithoutStatus(
     const ObjectPathCallback& callback,
     const ErrorCallback& error_callback,
     dbus::Response* response) {
+  --ongoing_calls;
   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);
 }
 
 void ShillClientHelper::OnDictionaryValueMethod(
     const DictionaryValueCallback& callback,
     dbus::Response* response) {
+  --ongoing_calls;
   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);
 }
 
 void ShillClientHelper::OnVoidMethodWithErrorCallback(
     const base::Closure& callback,
     dbus::Response* response) {
+  --ongoing_calls;
   callback.Run();
 }
 
 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;
   if (!value.get() || !value->GetAsDictionary(&result)) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
   callback.Run(*result);
 }
 
 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;
   if (!value.get() || !value->GetAsList(&result)) {
     error_callback.Run(kInvalidResponseErrorName, kInvalidResponseErrorMessage);
     return;
   }
   callback.Run(*result);
 }
 
 void ShillClientHelper::OnError(const ErrorCallback& error_callback,
                                 dbus::ErrorResponse* response) {
+  --ongoing_calls;
   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 chromeos