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. This allows it to
+// call a 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(
+    const base::Closure& closure,
+    const ShillClientHelper::DictionaryValueCallback& callback,
+    DBusMethodCallStatus call_status,
+    const DictionaryValue& argument) {
+  callback.Run(call_status, argument);
+  closure.Run();
+}
+
+ShillClientHelper::DictionaryValueCallback
+    AttachClosureToDictionaryValueCallback(
+    const ShillClientHelper::DictionaryValueCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunDictionaryValueCallback, closure, callback);
+}
+

[stevenjb, 2013/02/07 01:12:30]

So, I apologize for continuing to iterate on this, I know we want to get this in
for 26, but I still find this really confusing. Couldn't we do something like
this:

void ShillServiceClientImpl::SuccessDictionaryValueCallback(service_path,
callback, argument) {
  callback.Run(DBUS_METHOD_CALL_SUCCESS, argument);
  OnHelperStatusUpdate(service_path);
}

void ShillServiceClientImpl::ErrorDictionaryValueCallback(service_path,
callback, error_name, error_message) {
  OnGetPropertiesError(service_path, callback, error_name, error_message);
  OnHelperStatusUpdate(service_path);
}

// similar for ListValue and Void

Then, the code below would look something like:


  GetHelper(service_path)->CallDictionaryValueMethodWithErrorCallback(
    &method_call,
    base::Bind(&ShillServiceClientImpl::SuccessDictionaryValueCallback,
               weak_ptr_factory_.GetWeakPtr(), service_path, callback),
    base::Bind(&ShillServiceClientImpl::ErrorDictionaryValueCallback,
               weak_ptr_factory_.GetWeakPtr(), service_path, callback));

Then we wouldn't need all of these rather confusing AttachCloserTo* wrappers.

What do you think?

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

Wasn't the bug marked as P1/ReleaseBlocker?
I think the fix would be worth merging even after the M26 branch cut.
Or, simply keep helpers alive as we did, unless the size of |helpers_| exceeds a
certain threshold (64, 128, 256, or whatever appropriate)
When it exceeds the threshold, we can delete all unneeded helpers.
(Eventually, this looks like a LRU approach which we have abandoned because it
was too complicated. But the current change looks more complicated than it.)

+// * Wrapper for ListValueCallback
+void RunListValueCallback(
+    const base::Closure& closure,
+    const ShillClientHelper::ListValueCallback& callback,
+    const ListValue& argument) {
+  callback.Run(argument);
+  closure.Run();
+}
+
+ShillClientHelper::ListValueCallback AttachClosureToListValueCallback(
+    const ShillClientHelper::ListValueCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunListValueCallback, closure, callback);
+}
+
+// * Wrapper for ErrorCallback
+void RunErrorCallback(
+    const base::Closure& closure,
+    const ShillClientHelper::ErrorCallback& callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  callback.Run(error_name, error_message);
+  closure.Run();
+}
+
+ShillClientHelper::ErrorCallback AttachClosureToErrorCallback(
+    const ShillClientHelper::ErrorCallback& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunErrorCallback, closure, callback);
+}
+
+// * Wrapper for Closure
+void RunClosure(const base::Closure& closure,
+                const base::Closure& callback) {
+  callback.Run();
+  closure.Run();
+}
+
+base::Closure AttachClosureToClosure(
+    const base::Closure& callback,
+    const base::Closure& closure) {
+  return base::Bind(&RunClosure, closure, callback);
+}
+
 // The ShillServiceClient implementation.
 class ShillServiceClientImpl : public ShillServiceClient {
  public:
   explicit ShillServiceClientImpl(dbus::Bus* bus)
       : bus_(bus),
-        helpers_deleter_(&helpers_) {
+        helpers_deleter_(&helpers_),
+        ALLOW_THIS_IN_INITIALIZER_LIST(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);
+    OnHelperStatusUpdate(service_path);
   }
 
   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(
+            AttachClosureToDictionaryValueCallback(callback,
+                base::Bind(&ShillServiceClientImpl::OnHelperStatusUpdate,
+                    weak_ptr_factory_.GetWeakPtr(), service_path)),
+            DBUS_METHOD_CALL_SUCCESS),
+        AttachClosureToErrorCallback(
+            base::Bind(&OnGetPropertiesError, service_path, callback),
+            HelperStatusUpdateCallback(service_path)));
   }
 
   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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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,
+            HelperStatusUpdateCallback(service_path)),
+        AttachClosureToErrorCallback(error_callback,
+            HelperStatusUpdateCallback(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);
+    OnHelperStatusUpdate(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);
+    // TODO(deymo): Delay the MonitorPropertyChanged call until an observer is
+    // added to the helper. If no observer is add and every call finishes we
+    // delete the helper making this AddMatch and RemoveMatch blocking cycle is
+    // useless.

[stevenjb, 2013/02/07 01:12:30]

This doesn't seem too difficult, I think it's worth doing now; I'm a little
concerned about the overhead of adding/removing the Monitoring. It also seems
like code that should be in the Helper anyway; we just need to implement
StartMonitorPropertyChanged() and make it keep track of whether or not it's
already been started.

     helper->MonitorPropertyChanged(flimflam::kFlimflamServiceInterface);
     helpers_.insert(HelperMap::value_type(service_path.value(), helper));
     return helper;
   }
 
+  // Returns a Closure that calls OnHelperStatusUpdate with the given
+  // |service_path| argument.
+  base::Closure HelperStatusUpdateCallback(
+      const dbus::ObjectPath& service_path) {
+    return base::Bind(&ShillServiceClientImpl::OnHelperStatusUpdate,
+        weak_ptr_factory_.GetWeakPtr(), service_path);
+  }
+
+  // This function is 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.
+  void OnHelperStatusUpdate(const dbus::ObjectPath& service_path) {
+    HelperMap::iterator it = helpers_.find(service_path.value());
+    if (it == helpers_.end())
+      return;
+
+    ShillClientHelper* const helper = it->second;
+    if (!helper->IsObserved() && !helper->IsWaitingResponse()) {
+      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) {
+  ShillServiceClientStubImpl() :
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
     SetDefaultProperties();
   }
 
   virtual ~ShillServiceClientStubImpl() {
     STLDeleteContainerPairSecondPointers(
         observer_list_.begin(), observer_list_.end());
   }
 
   // ShillServiceClient overrides.
 
@@ skipping 315 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