Shill: ShillServiceClient destroys ShillClientHelpers when they are not need.

chromeos/dbus/shill_service_client.cc

Index: chromeos/dbus/shill_service_client.cc
diff --git a/chromeos/dbus/shill_service_client.cc b/chromeos/dbus/shill_service_client.cc
index 8e0ede00f565ea2ecb7d795702c48a3891816029..8d8aa491c44773e9de9ca0bce4a3132636d5e18d 100644
--- a/chromeos/dbus/shill_service_client.cc
+++ b/chromeos/dbus/shill_service_client.cc
@@ -40,12 +40,81 @@ void OnGetPropertiesError(
   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) {
   }
 
   /////////////////////////////////////
@@ -68,8 +137,15 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  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,
@@ -82,9 +158,14 @@ class ShillServiceClientImpl : public ShillServiceClient {
     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,
@@ -95,9 +176,14 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  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)));
   }
 
 
@@ -111,8 +197,12 @@ class ShillServiceClientImpl : public ShillServiceClient {
     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,
@@ -121,7 +211,13 @@ class ShillServiceClientImpl : public ShillServiceClient {
     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,
@@ -129,9 +225,14 @@ class ShillServiceClientImpl : public ShillServiceClient {
                           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,
@@ -139,9 +240,14 @@ class ShillServiceClientImpl : public ShillServiceClient {
                       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(
@@ -153,9 +259,14 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  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(
@@ -165,7 +276,9 @@ class ShillServiceClientImpl : public ShillServiceClient {
                                  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 {
@@ -190,10 +303,32 @@ class ShillServiceClientImpl : public ShillServiceClient {
     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);
 };