Connect to NameOwnerChanged signal when setting callback

In ObjectManager, when ConnectToNameOwnerChangedSignal is called the first time,
we could have already missed the NameOwnerChanged signal if we get a value from
UpdateNameOwnerAndBlock. This means NameOwnerChanged callbacks will
never be called until that service restarts. In ObjectManager we run
into this problem:

1. ObjectManager::SetupMatchRuleAndFilter is called, calling
bus_->GetServiceOwnerAndBlock shows empty service name owner.

2. ObjectManager::OnSetupManagerRuleAndFilterComplete callback will call
object_proxy_->ConnectToSignal, which in turn calls
ConnectToNameOwnerChangedSignal the first time. At this point,
UpdateNameOwnerAndBlock calling bus_->GetServiceOwnerAndBlock returns the
current name owner of the service, this means the NameOwnerChanged signal
is already sent on system bus.

3. As a result, ObjectManager::NameOwnerChanged is never called while
the service is already online. This in turn causes GetManagedObject to
never be called, and the object manager interface never added.

See detailed sample logs in b/138416411.

This CL adds the following:

1. Make SetNameOwnerChangedCallback run
ConnectToNameOwnerChangedSignal when called. Since ObjectManager calls
SetNameOwnerChangedCallback before posting SetupMatchRuleAndFilter (in which
ObjectManager attempts to get the service name owner through a blocking call),
this removes the time gap described above that causes lost signal.

2. Make dbus thread the only writer to |service_name_owner_|, given that
connecting to the NameOwnerChanged signal right away in ObjectManager
ctor causes potential data race in writing to |service_name_owner_| in
both NameOwnerChanged (on origin thread) and SetupMatchRuleAndFilter (on
dbus thread).

BUG=b:138416411
TEST=Manually on device.


Change-Id: Ie95a5b7b303637acadebda151cc478e52b6a1af5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1733111
Commit-Queue: Bing Xue <bingxue@google.com>
Auto-Submit: Bing Xue <bingxue@google.com>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684392}

dbus/object_manager.cc

@@ -187,8 +187,12 @@ bool ObjectManager::SetupMatchRuleAndFilter() {
   if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
     return false;
 
-  service_name_owner_ =
-      bus_->GetServiceOwnerAndBlock(service_name_, Bus::SUPPRESS_ERRORS);
+  // Try to get |service_name_owner_| from dbus if we haven't received any
+  // NameOwnerChanged signals.
+  if (service_name_owner_.empty()) {
+    service_name_owner_ =
+        bus_->GetServiceOwnerAndBlock(service_name_, Bus::SUPPRESS_ERRORS);
+  }
 
   const std::string match_rule =
       base::StringPrintf(
@@ -224,6 +228,7 @@ void ObjectManager::OnSetupMatchRuleAndFilterComplete(bool success) {
   DCHECK(bus_);
   DCHECK(object_proxy_);
   DCHECK(setup_success_);
+  bus_->AssertOnOriginThread();
 
   // |object_proxy_| is no longer valid if the Bus was shut down before this
   // call. Don't initiate any other action from the origin thread.
@@ -505,9 +510,18 @@ void ObjectManager::RemoveInterface(const ObjectPath& object_path,
   }
 }
 
+void ObjectManager::UpdateServiceNameOwner(const std::string& new_owner) {
+  bus_->AssertOnDBusThread();
+  service_name_owner_ = new_owner;
+}
+
 void ObjectManager::NameOwnerChanged(const std::string& old_owner,
                                      const std::string& new_owner) {
-  service_name_owner_ = new_owner;
+  bus_->AssertOnOriginThread();
+
+  bus_->GetDBusTaskRunner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&ObjectManager::UpdateServiceNameOwner, this, new_owner));
 
   if (!old_owner.empty()) {
     ObjectMap::iterator iter = object_map_.begin();

dbus/object_manager.h

@@ -317,6 +317,11 @@ class CHROME_DBUS_EXPORT ObjectManager final
   void NameOwnerChanged(const std::string& old_owner,
                         const std::string& new_owner);
 
+  // Write |new_owner| to |service_name_owner_|. This method makes sure write
+  // happens on the DBus thread, which is the sole writer to
+  // |service_name_owner_|.
+  void UpdateServiceNameOwner(const std::string& new_owner);
+
   Bus* bus_;
   std::string service_name_;
   std::string service_name_owner_;

dbus/object_proxy.cc

@@ -274,6 +274,10 @@ void ObjectProxy::SetNameOwnerChangedCallback(
   bus_->AssertOnOriginThread();
 
   name_owner_changed_callback_ = callback;
+
+  bus_->GetDBusTaskRunner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&ObjectProxy::TryConnectToNameOwnerChangedSignal, this));
 }
 
 void ObjectProxy::WaitForServiceToBeAvailable(
@@ -458,6 +462,15 @@ bool ObjectProxy::ConnectToNameOwnerChangedSignal() {
   return success;
 }
 
+void ObjectProxy::TryConnectToNameOwnerChangedSignal() {
+  bus_->AssertOnDBusThread();
+
+  bool success = ConnectToNameOwnerChangedSignal();
+  LOG_IF(WARNING, !success)
+      << "Failed to connect to NameOwnerChanged signal for object: "
+      << object_path_.value();
+}
+
 bool ObjectProxy::ConnectToSignalInternal(const std::string& interface_name,
                                           const std::string& signal_name,
                                           SignalCallback signal_callback) {

dbus/object_proxy.h

@@ -267,6 +267,9 @@ class CHROME_DBUS_EXPORT ObjectProxy
   // Connects to NameOwnerChanged signal.
   bool ConnectToNameOwnerChangedSignal();
 
+  // Tries to connect to NameOwnerChanged signal, ignores any error.
+  void TryConnectToNameOwnerChangedSignal();
+
   // Helper function for ConnectToSignal().
   bool ConnectToSignalInternal(const std::string& interface_name,
                                const std::string& signal_name,