Skip to content

Commit

Permalink
Track active references in ShillClientHelper (Take 2)
Browse files Browse the repository at this point in the history 
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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@227100 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
stevenjb@chromium.org committed Oct 4, 2013
1 parent 694498e commit 19853e3
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 29 deletions.
105 changes: 84 additions & 21 deletions chromeos/dbus/shill_client_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#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"
Expand All @@ -13,12 +14,36 @@

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) {
Expand All @@ -36,6 +61,7 @@ void OnBooleanMethodWithErrorCallback(
}

void OnStringMethodWithErrorCallback(
ShillClientHelper::RefHolder* ref_holder,
const ShillClientHelper::StringCallback& callback,
const ShillClientHelper::ErrorCallback& error_callback,
dbus::Response* response) {
Expand All @@ -53,7 +79,8 @@ void OnStringMethodWithErrorCallback(
}

// Handles responses for methods without results.
void OnVoidMethod(const VoidDBusMethodCallback& callback,
void OnVoidMethod(ShillClientHelper::RefHolder* ref_holder,
const VoidDBusMethodCallback& callback,
dbus::Response* response) {
if (!response) {
callback.Run(DBUS_METHOD_CALL_FAILURE);
Expand All @@ -64,6 +91,7 @@ void OnVoidMethod(const VoidDBusMethodCallback& callback,

// Handles responses for methods with ObjectPath results.
void OnObjectPathMethod(
ShillClientHelper::RefHolder* ref_holder,
const ObjectPathDBusMethodCallback& callback,
dbus::Response* response) {
if (!response) {
Expand All @@ -81,6 +109,7 @@ 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) {
Expand All @@ -99,6 +128,7 @@ void OnObjectPathMethodWithoutStatus(

// Handles responses for methods with DictionaryValue results.
void OnDictionaryValueMethod(
ShillClientHelper::RefHolder* ref_holder,
const ShillClientHelper::DictionaryValueCallback& callback,
dbus::Response* response) {
if (!response) {
Expand All @@ -119,6 +149,7 @@ void OnDictionaryValueMethod(

// Handles responses for methods without results.
void OnVoidMethodWithErrorCallback(
ShillClientHelper::RefHolder* ref_holder,
const base::Closure& callback,
dbus::Response* response) {
callback.Run();
Expand All @@ -127,6 +158,7 @@ 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) {
Expand All @@ -142,6 +174,7 @@ 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) {
Expand Down Expand Up @@ -171,9 +204,9 @@ void OnError(const ShillClientHelper::ErrorCallback& error_callback,

} // namespace

ShillClientHelper::ShillClientHelper(dbus::Bus* bus,
dbus::ObjectProxy* proxy)
ShillClientHelper::ShillClientHelper(dbus::ObjectProxy* proxy)
: proxy_(proxy),
active_refs_(0),
weak_ptr_factory_(this) {
}

Expand All @@ -182,8 +215,16 @@ 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]);
Expand All @@ -195,7 +236,10 @@ void ShillClientHelper::AddPropertyChangedObserver(

void ShillClientHelper::RemovePropertyChangedObserver(
ShillPropertyChangedObserver* observer) {
if (!observer_list_.HasObserver(observer))
return;
observer_list_.RemoveObserver(observer);
Release();
}

void ShillClientHelper::MonitorPropertyChanged(
Expand Down Expand Up @@ -225,18 +269,22 @@ 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,
callback));
proxy_->CallMethod(
method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&OnVoidMethod,
base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
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,
callback));
proxy_->CallMethod(
method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&OnObjectPathMethod,
base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
callback));
}

void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
Expand All @@ -249,6 +297,7 @@ 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,
Expand All @@ -259,9 +308,11 @@ 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,
callback));
proxy_->CallMethod(
method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&OnDictionaryValueMethod,
base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
callback));
}

void ShillClientHelper::CallVoidMethodWithErrorCallback(
Expand All @@ -273,6 +324,7 @@ 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));
Expand All @@ -287,6 +339,7 @@ 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,
Expand All @@ -302,6 +355,7 @@ 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,
Expand All @@ -316,10 +370,10 @@ void ShillClientHelper::CallDictionaryValueMethodWithErrorCallback(
DCHECK(!error_callback.is_null());
proxy_->CallMethodWithErrorCallback(
method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(
&OnDictionaryValueMethodWithErrorCallback,
callback,
error_callback),
base::Bind(&OnDictionaryValueMethodWithErrorCallback,
base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
callback,
error_callback),
base::Bind(&OnError,
error_callback));
}
Expand All @@ -332,10 +386,10 @@ void ShillClientHelper::CallListValueMethodWithErrorCallback(
DCHECK(!error_callback.is_null());
proxy_->CallMethodWithErrorCallback(
method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(
&OnListValueMethodWithErrorCallback,
callback,
error_callback),
base::Bind(&OnListValueMethodWithErrorCallback,
base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
callback,
error_callback),
base::Bind(&OnError,
error_callback));
}
Expand Down Expand Up @@ -420,6 +474,16 @@ 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) {
Expand All @@ -443,5 +507,4 @@ void ShillClientHelper::OnPropertyChanged(dbus::Signal* signal) {
OnPropertyChanged(name, *value));
}


} // namespace chromeos
21 changes: 20 additions & 1 deletion chromeos/dbus/shill_client_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ 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;
Expand Down Expand Up @@ -68,11 +70,17 @@ 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;

ShillClientHelper(dbus::Bus* bus, dbus::ObjectProxy* proxy);
explicit ShillClientHelper(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);

Expand Down Expand Up @@ -131,6 +139,8 @@ 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,
Expand All @@ -141,6 +151,13 @@ 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);
Expand All @@ -154,6 +171,8 @@ 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_;
Expand Down
2 changes: 1 addition & 1 deletion chromeos/dbus/shill_device_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(bus_, object_proxy);
ShillClientHelper* helper = new ShillClientHelper(object_proxy);
CHECK(helper) << "Unable to create Shill client helper.";
helper->MonitorPropertyChanged(shill::kFlimflamDeviceInterface);
helpers_.insert(HelperMap::value_type(device_path.value(), helper));
Expand Down
2 changes: 1 addition & 1 deletion chromeos/dbus/shill_ipconfig_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(bus_, object_proxy);
ShillClientHelper* helper = new ShillClientHelper(object_proxy);
helper->MonitorPropertyChanged(shill::kFlimflamIPConfigInterface);
helpers_.insert(HelperMap::value_type(ipconfig_path.value(), helper));
return helper;
Expand Down
2 changes: 1 addition & 1 deletion chromeos/dbus/shill_manager_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(bus, proxy_));
helper_.reset(new ShillClientHelper(proxy_));
helper_->MonitorPropertyChanged(shill::kFlimflamManagerInterface);
}

Expand Down
2 changes: 1 addition & 1 deletion chromeos/dbus/shill_profile_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(bus_, object_proxy);
ShillClientHelper* helper = new ShillClientHelper(object_proxy);
helper->MonitorPropertyChanged(shill::kFlimflamProfileInterface);
helpers_.insert(HelperMap::value_type(profile_path.value(), helper));
return helper;
Expand Down
Loading

0 comments on commit 19853e3

Please sign in to comment.