From 19853e39a6dbf6ce1a1d8fb8015754832d4d409e Mon Sep 17 00:00:00 2001 From: "stevenjb@chromium.org" Date: Fri, 4 Oct 2013 21:50:47 +0000 Subject: Track active references in ShillClientHelper (Take 2) 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 --- chromeos/dbus/shill_client_helper.cc | 105 ++++++++++++++++++++++++++------- chromeos/dbus/shill_client_helper.h | 21 ++++++- chromeos/dbus/shill_device_client.cc | 2 +- chromeos/dbus/shill_ipconfig_client.cc | 2 +- chromeos/dbus/shill_manager_client.cc | 2 +- chromeos/dbus/shill_profile_client.cc | 2 +- chromeos/dbus/shill_service_client.cc | 35 ++++++++++- 7 files changed, 140 insertions(+), 29 deletions(-) (limited to 'chromeos/dbus') diff --git a/chromeos/dbus/shill_client_helper.cc b/chromeos/dbus/shill_client_helper.cc index b430609..c927180 100644 --- a/chromeos/dbus/shill_client_helper.cc +++ b/chromeos/dbus/shill_client_helper.cc @@ -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" @@ -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 helper) + : helper_(helper) { + helper_->AddRef(); + } + ~RefHolder() { + if (helper_) + helper_->Release(); + } + + private: + base::WeakPtr 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) { @@ -36,6 +61,7 @@ void OnBooleanMethodWithErrorCallback( } void OnStringMethodWithErrorCallback( + ShillClientHelper::RefHolder* ref_holder, const ShillClientHelper::StringCallback& callback, const ShillClientHelper::ErrorCallback& error_callback, dbus::Response* response) { @@ -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); @@ -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) { @@ -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) { @@ -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) { @@ -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(); @@ -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) { @@ -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) { @@ -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) { } @@ -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]); @@ -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( @@ -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( @@ -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, @@ -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( @@ -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)); @@ -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, @@ -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, @@ -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)); } @@ -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)); } @@ -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) { @@ -443,5 +507,4 @@ void ShillClientHelper::OnPropertyChanged(dbus::Signal* signal) { OnPropertyChanged(name, *value)); } - } // namespace chromeos diff --git a/chromeos/dbus/shill_client_helper.h b/chromeos/dbus/shill_client_helper.h index 12cd994..a52fc22 100644 --- a/chromeos/dbus/shill_client_helper.h +++ b/chromeos/dbus/shill_client_helper.h @@ -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 PropertyChangedHandler; @@ -68,11 +70,17 @@ class ShillClientHelper { // A callback that handles responses for methods with boolean results. typedef base::Callback BooleanCallback; + // Callback used to notify owner when this can be safely released. + typedef base::Callback 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); @@ -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, @@ -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); @@ -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 observer_list_; diff --git a/chromeos/dbus/shill_device_client.cc b/chromeos/dbus/shill_device_client.cc index 931e71a..5aa4332 100644 --- a/chromeos/dbus/shill_device_client.cc +++ b/chromeos/dbus/shill_device_client.cc @@ -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)); diff --git a/chromeos/dbus/shill_ipconfig_client.cc b/chromeos/dbus/shill_ipconfig_client.cc index 4483078..46aeb71 100644 --- a/chromeos/dbus/shill_ipconfig_client.cc +++ b/chromeos/dbus/shill_ipconfig_client.cc @@ -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; diff --git a/chromeos/dbus/shill_manager_client.cc b/chromeos/dbus/shill_manager_client.cc index f3cd1bb..cae9b68 100644 --- a/chromeos/dbus/shill_manager_client.cc +++ b/chromeos/dbus/shill_manager_client.cc @@ -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); } diff --git a/chromeos/dbus/shill_profile_client.cc b/chromeos/dbus/shill_profile_client.cc index e0d8a71..0e63354 100644 --- a/chromeos/dbus/shill_profile_client.cc +++ b/chromeos/dbus/shill_profile_client.cc @@ -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; diff --git a/chromeos/dbus/shill_service_client.cc b/chromeos/dbus/shill_service_client.cc index 40143f6..faaf75a 100644 --- a/chromeos/dbus/shill_service_client.cc +++ b/chromeos/dbus/shill_service_client.cc @@ -5,11 +5,13 @@ #include "chromeos/dbus/shill_service_client.h" #include "base/bind.h" +#include "base/memory/weak_ptr.h" #include "base/message_loop/message_loop.h" #include "base/stl_util.h" #include "base/values.h" #include "chromeos/dbus/shill_property_changed_observer.h" #include "chromeos/dbus/shill_service_client_stub.h" +#include "chromeos/network/network_event_log.h" #include "dbus/bus.h" #include "dbus/message.h" #include "dbus/object_proxy.h" @@ -54,7 +56,18 @@ class ShillServiceClientImpl : public ShillServiceClient { public: explicit ShillServiceClientImpl() : bus_(NULL), - helpers_deleter_(&helpers_) { + weak_ptr_factory_(this) { + } + + virtual ~ShillServiceClientImpl() { + for (HelperMap::iterator iter = helpers_.begin(); + iter != helpers_.end(); ++iter) { + ShillClientHelper* helper = iter->second; + bus_->RemoveObjectProxy(shill::kFlimflamServiceName, + helper->object_proxy()->object_path(), + base::Bind(&base::DoNothing)); + delete helper; + } } virtual void AddPropertyChangedObserver( @@ -222,17 +235,33 @@ class ShillServiceClientImpl : public ShillServiceClient { return it->second; // There is no helper for the profile, create it. + NET_LOG_DEBUG("AddShillClientHelper", service_path.value()); dbus::ObjectProxy* object_proxy = bus_->GetObjectProxy(shill::kFlimflamServiceName, service_path); - ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy); + ShillClientHelper* helper = new ShillClientHelper(object_proxy); + helper->SetReleasedCallback( + base::Bind(&ShillServiceClientImpl::NotifyReleased, + weak_ptr_factory_.GetWeakPtr())); helper->MonitorPropertyChanged(shill::kFlimflamServiceInterface); helpers_.insert(HelperMap::value_type(service_path.value(), helper)); return helper; } + void NotifyReleased(ShillClientHelper* helper) { + // New Shill Service DBus objects are created relatively frequently, so + // remove them when they become inactive (no observers and no active method + // calls). + const dbus::ObjectPath& object_path = helper->object_proxy()->object_path(); + NET_LOG_DEBUG("RemoveShillClientHelper", object_path.value()); + bus_->RemoveObjectProxy(shill::kFlimflamServiceName, + object_path, base::Bind(&base::DoNothing)); + helpers_.erase(object_path.value()); + delete helper; + } + dbus::Bus* bus_; HelperMap helpers_; - STLValueDeleter helpers_deleter_; + base::WeakPtrFactory weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(ShillServiceClientImpl); }; -- cgit v1.1