Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

chromeos/dbus/shill_service_client.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_service_client.h"
 
 #include "base/bind.h"
 #include "base/chromeos/chromeos_version.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
@@ skipping 22 matching lines @@
   // "org.freedesktop.DBus.Error.UnknownMethod". crbug.com/130660
   if (error_name == DBUS_ERROR_UNKNOWN_METHOD)
     VLOG(1) << log_string;
   else
     LOG(ERROR) << log_string;
 
   base::DictionaryValue empty_dictionary;
   callback.Run(DBUS_METHOD_CALL_FAILURE, empty_dictionary);
 }
 
+// ShillClientHelper callback wrappers.
+// For each one of the different callback types in ShillClientHelper, we define
+// a wrapper that attachs a clousure to the given callback. It allows to call a

[stevenjb, 2013/02/06 01:05:53]

s/It allows to/This allows it to/

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

Done.

+// second callback (the clousure) after the first callback is called. This is
+// used to get a notification in the ShillServiceClient when a MethodCall
+// returns.
+
+// * Wrapper for DictionaryValueCallback.
+void RunDictionaryValueCallback(
+    base::ScopedClosureRunner* closure_runner,
+    const ShillClientHelper::DictionaryValueCallback& callback,
+    DBusMethodCallStatus call_status,
+    const DictionaryValue& argument) {
+  callback.Run(call_status, argument);
+}
+
+ShillClientHelper::DictionaryValueCallback
+    AttachClosureToDictionaryValueCallabck(

[stevenjb, 2013/02/06 01:05:53]

s/abck/back

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

=)

+    const ShillClientHelper::DictionaryValueCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunDictionaryValueCallback,
+      base::Owned(new base::ScopedClosureRunner(closure)), callback);
+}
+
+// * Wrapper for ListValueCallback
+void RunListValueCallback(
+    base::ScopedClosureRunner* closure_runner,
+    const ShillClientHelper::ListValueCallback& callback,
+    const ListValue& argument) {
+  callback.Run(argument);
+}
+
+ShillClientHelper::ListValueCallback AttachClosureToListValueCallback(
+    const ShillClientHelper::ListValueCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunListValueCallback,
+      base::Owned(new base::ScopedClosureRunner(closure)), callback);
+}
+
+// * Wrapper for ErrorCallback
+void RunErrorCallback(
+    base::ScopedClosureRunner* closure_runner,
+    const ShillClientHelper::ErrorCallback& callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  callback.Run(error_name, error_message);
+}
+
+ShillClientHelper::ErrorCallback AttachClosureToErrorCallback(
+    const ShillClientHelper::ErrorCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunErrorCallback,
+      base::Owned(new base::ScopedClosureRunner(closure)), callback);
+}
+
+// * Wrapper for Closure
+void RunClosure(base::ScopedClosureRunner* closure_runner,
+                const base::Closure& callback) {
+  callback.Run();
+}
+
+base::Closure AttachClosureToClosure(
+    const base::Closure& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunClosure,
+      base::Owned(new base::ScopedClosureRunner(closure)), callback);
+}
+
 // The ShillServiceClient implementation.
 class ShillServiceClientImpl : public ShillServiceClient {
  public:
   explicit ShillServiceClientImpl(dbus::Bus* bus)
       : bus_(bus),
-        helpers_deleter_(&helpers_) {
+        helpers_deleter_(&helpers_),
+        weak_ptr_factory_(this) {
   }
 
   /////////////////////////////////////
   // ShillServiceClient overrides.
   virtual void AddPropertyChangedObserver(
       const dbus::ObjectPath& service_path,
       ShillPropertyChangedObserver* observer) OVERRIDE {
     GetHelper(service_path)->AddPropertyChangedObserver(observer);
   }
 
   virtual void RemovePropertyChangedObserver(
       const dbus::ObjectPath& service_path,
       ShillPropertyChangedObserver* observer) OVERRIDE {
     GetHelper(service_path)->RemovePropertyChangedObserver(observer);
   }
 
   virtual void GetProperties(const dbus::ObjectPath& service_path,
                              const DictionaryValueCallback& callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kGetPropertiesFunction);
     GetHelper(service_path)->CallDictionaryValueMethodWithErrorCallback(
         &method_call,
-        base::Bind(callback, DBUS_METHOD_CALL_SUCCESS),
-        base::Bind(&OnGetPropertiesError, service_path, callback));
+        base::Bind(
+            AttachClosureToDictionaryValueCallabck(callback,
+                base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                    weak_ptr_factory_.GetWeakPtr(), service_path)),
+            DBUS_METHOD_CALL_SUCCESS),
+        AttachClosureToErrorCallback(
+            base::Bind(&OnGetPropertiesError, service_path, callback),
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));

[stevenjb, 2013/02/06 01:05:53]

Rather than passing HelperStatusUpdate in each of these, could we write
AttachClosureToDictionaryValueCallback, etc, to do that binding instead?

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

If we do that we still need to pass the service_path and the
ShillServiceClientImpl object reference to each one of those functions.

I would prefer something more like a
ShillServiceClientImpl::HelperStatusUpdatedCallback(service_path) that returns a
base::Closure.

[stevenjb, 2013/02/06 17:10:20]

That sounds like it would be fine. It's definitely optional, there's just a lot
of copy/paste code here. It might be unavoidable, but anything we can do to
simplify it (which may be a bit subjective) would help.

   }
 
   virtual void SetProperty(const dbus::ObjectPath& service_path,
                            const std::string& name,
                            const base::Value& value,
                            const base::Closure& callback,
                            const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kSetPropertyFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(name);
     ShillClientHelper::AppendValueDataAsVariant(&writer, value);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,

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

I think ScopedClosureRunner runs the closure on its destruction even when the
callback is not called.
Can we go without AttachClosureToErrorCallback?

BTW, could you make a change for shill_service_client_unittest.cc to test this
behavior?

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

Yes, the ScopedClosureRunner will run allways... but, in this case it is safe to
call HelperStatusUpdated several times.

Anyway, I think I can simplify this a little if instead of a ScopedClosureRunner
we just run both callbacks in the RunDictionaryValueCallback (and the other). So
the body of that function will be like:
RunDictionaryValueCallback(const ...& closure, const ...& callback,
more_arguments) {
  callback.Run(more_arguments);
  closure.Run();
}
Does it look better for you?

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

+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
   virtual void ClearProperty(const dbus::ObjectPath& service_path,
                              const std::string& name,
                              const base::Closure& callback,
                              const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kClearPropertyFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(name);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
 
   virtual void ClearProperties(const dbus::ObjectPath& service_path,
                                const std::vector<std::string>& names,
                                const ListValueCallback& callback,
                                const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  shill::kClearPropertiesFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendArrayOfStrings(names);
     GetHelper(service_path)->CallListValueMethodWithErrorCallback(
         &method_call,
-        callback,
-        error_callback);
+        AttachClosureToListValueCallback(callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
   virtual void Connect(const dbus::ObjectPath& service_path,
                        const base::Closure& callback,
                        const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kConnectFunction);
     GetHelper(service_path)->CallVoidMethodWithErrorCallback(
-        &method_call, callback, error_callback);
+        &method_call,
+        AttachClosureToClosure(callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
   virtual void Disconnect(const dbus::ObjectPath& service_path,
                           const base::Closure& callback,
                           const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kDisconnectFunction);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
   virtual void Remove(const dbus::ObjectPath& service_path,
                       const base::Closure& callback,
                       const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kRemoveServiceFunction);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
   virtual void ActivateCellularModem(
       const dbus::ObjectPath& service_path,
       const std::string& carrier,
       const base::Closure& callback,
       const ErrorCallback& error_callback) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kActivateCellularModemFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(carrier);
-    GetHelper(service_path)->CallVoidMethodWithErrorCallback(&method_call,
-                                                             callback,
-                                                             error_callback);
+    GetHelper(service_path)->CallVoidMethodWithErrorCallback(
+        &method_call,
+        AttachClosureToClosure(callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            base::Bind(&ShillServiceClientImpl::HelperStatusUpdated,
+                weak_ptr_factory_.GetWeakPtr(), service_path)));
   }
 
   virtual bool CallActivateCellularModemAndBlock(
       const dbus::ObjectPath& service_path,
       const std::string& carrier) OVERRIDE {
     dbus::MethodCall method_call(flimflam::kFlimflamServiceInterface,
                                  flimflam::kActivateCellularModemFunction);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(carrier);
-    return GetHelper(service_path)->CallVoidMethodAndBlock(&method_call);
+    bool result = GetHelper(service_path)->CallVoidMethodAndBlock(&method_call);
+    HelperStatusUpdated(service_path);
+    return result;
   }
 
   virtual ShillServiceClient::TestInterface* GetTestInterface() OVERRIDE {
     return NULL;
   }
 
  private:
   typedef std::map<std::string, ShillClientHelper*> HelperMap;
 
   // Returns the corresponding ShillClientHelper for the profile.
   ShillClientHelper* GetHelper(const dbus::ObjectPath& service_path) {
     HelperMap::iterator it = helpers_.find(service_path.value());
     if (it != helpers_.end())
       return it->second;
 
     // There is no helper for the profile, create it.
     dbus::ObjectProxy* object_proxy =
         bus_->GetObjectProxy(flimflam::kFlimflamServiceName, service_path);
     ShillClientHelper* helper = new ShillClientHelper(bus_, object_proxy);
     helper->MonitorPropertyChanged(flimflam::kFlimflamServiceInterface);
     helpers_.insert(HelperMap::value_type(service_path.value(), helper));
     return helper;
   }
 
+  // This function should be called every time the IsObserved or the
+  // IsWaitingResponse helper's properties may have changed. If both are
+  // false, the cached helper can be safely removed and this function removes
+  // it in such case.

[stevenjb, 2013/02/06 01:05:53]

nit: s/and this function removes it in such case// (implied)

+  void HelperStatusUpdated(const dbus::ObjectPath& service_path) {

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

nit: This method should better named MaybeRemoveHelper or something?
Our style guide requires method names to be verb.

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

I think the name should be more like "OnHelperStatusUpdate" because, indeed, is
a callback.

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

Callback methods are named "OnXXX" when it is hard to describe its behavior with
a short phrase.
On the other hand, this method's functionality can be described with one phrase,
"(maybe) delete a helper".
Giving a meaningful names for methods is a good practice, will help people
visiting this code in future a lot.

+    HelperMap::iterator it = helpers_.find(service_path.value());
+    if (it == helpers_.end())
+      return;
+
+    ShillClientHelper* const helper = it->second;
+    if (!helper->IsObserved() and !helper->IsWaitingResponse()) {

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

nit: s/and/&&/

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

Very expressive method names + too much time coding python --> "and"
=)

+      helpers_.erase(it);
+      delete helper;
+      bus_->RemoveObjectProxy(flimflam::kFlimflamServiceName, service_path,
+          base::Bind(&base::DoNothing));
+    }
+  }
+
   dbus::Bus* bus_;
   HelperMap helpers_;
   STLValueDeleter<HelperMap> helpers_deleter_;
 
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<ShillServiceClientImpl> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ShillServiceClientImpl);
 };
 
 // A stub implementation of ShillServiceClient.
 class ShillServiceClientStubImpl : public ShillServiceClient,
                                    public ShillServiceClient::TestInterface {
  public:
   ShillServiceClientStubImpl() : weak_ptr_factory_(this) {
     SetDefaultProperties();
   }
@@ skipping 323 matching lines @@
 ShillServiceClient* ShillServiceClient::Create(
     DBusClientImplementationType type,
     dbus::Bus* bus) {
   if (type == REAL_DBUS_CLIENT_IMPLEMENTATION)
     return new ShillServiceClientImpl(bus);
   DCHECK_EQ(STUB_DBUS_CLIENT_IMPLEMENTATION, type);
   return new ShillServiceClientStubImpl();
 }
 
 }  // namespace chromeos