1From c2e55024c6a03dcb099270718e70139542d8b0e4 Mon Sep 17 00:00:00 2001
2From: Bing Xue <[email protected]>
3Date: Thu, 1 Aug 2019 15:47:10 -0700
4Subject: [PATCH] Connect to NameOwnerChanged signal when setting callback
5
6In ObjectManager, when ConnectToNameOwnerChangedSignal is called the first time,
7we could have already missed the NameOwnerChanged signal if we get a value from
8UpdateNameOwnerAndBlock. This means NameOwnerChanged callbacks will
9never be called until that service restarts. In ObjectManager we run
10into this problem:
11
121. ObjectManager::SetupMatchRuleAndFilter is called, calling
13bus_->GetServiceOwnerAndBlock shows empty service name owner.
14
152. ObjectManager::OnSetupManagerRuleAndFilterComplete callback will call
16object_proxy_->ConnectToSignal, which in turn calls
17ConnectToNameOwnerChangedSignal the first time. At this point,
18UpdateNameOwnerAndBlock calling bus_->GetServiceOwnerAndBlock returns the
19current name owner of the service, this means the NameOwnerChanged signal
20is already sent on system bus.
21
223. As a result, ObjectManager::NameOwnerChanged is never called while
23the service is already online. This in turn causes GetManagedObject to
24never be called, and the object manager interface never added.
25
26See detailed sample logs in b/138416411.
27
28This CL adds the following:
29
301. Make SetNameOwnerChangedCallback run
31ConnectToNameOwnerChangedSignal when called. Since ObjectManager calls
32SetNameOwnerChangedCallback before posting SetupMatchRuleAndFilter (in which
33ObjectManager attempts to get the service name owner through a blocking call),
34this removes the time gap described above that causes lost signal.
35
362. Make dbus thread the only writer to |service_name_owner_|, given that
37connecting to the NameOwnerChanged signal right away in ObjectManager
38ctor causes potential data race in writing to |service_name_owner_| in
39both NameOwnerChanged (on origin thread) and SetupMatchRuleAndFilter (on
40dbus thread).
41
42BUG=b:138416411
43TEST=Manually on device.
44
45Change-Id: Ie95a5b7b303637acadebda151cc478e52b6a1af5
46---
47 dbus/object_manager.cc | 20 +++++++++++++++++---
48 dbus/object_manager.h  |  5 +++++
49 dbus/object_proxy.cc   | 13 +++++++++++++
50 dbus/object_proxy.h    |  3 +++
51 4 files changed, 38 insertions(+), 3 deletions(-)
52
53diff --git a/dbus/object_manager.cc b/dbus/object_manager.cc
54index 05d4b2ddeabd..44f120864310 100644
55--- a/dbus/object_manager.cc
56+++ b/dbus/object_manager.cc
57@@ -187,8 +187,12 @@ bool ObjectManager::SetupMatchRuleAndFilter() {
58   if (!bus_->Connect() || !bus_->SetUpAsyncOperations())
59     return false;
60
61-  service_name_owner_ =
62-      bus_->GetServiceOwnerAndBlock(service_name_, Bus::SUPPRESS_ERRORS);
63+  // Try to get |service_name_owner_| from dbus if we haven't received any
64+  // NameOwnerChanged signals.
65+  if (service_name_owner_.empty()) {
66+    service_name_owner_ =
67+        bus_->GetServiceOwnerAndBlock(service_name_, Bus::SUPPRESS_ERRORS);
68+  }
69
70   const std::string match_rule =
71       base::StringPrintf(
72@@ -224,6 +228,7 @@ void ObjectManager::OnSetupMatchRuleAndFilterComplete(bool success) {
73   DCHECK(bus_);
74   DCHECK(object_proxy_);
75   DCHECK(setup_success_);
76+  bus_->AssertOnOriginThread();
77
78   // |object_proxy_| is no longer valid if the Bus was shut down before this
79   // call. Don't initiate any other action from the origin thread.
80@@ -505,9 +510,18 @@ void ObjectManager::RemoveInterface(const ObjectPath& object_path,
81   }
82 }
83
84+void ObjectManager::UpdateServiceNameOwner(const std::string& new_owner) {
85+  bus_->AssertOnDBusThread();
86+  service_name_owner_ = new_owner;
87+}
88+
89 void ObjectManager::NameOwnerChanged(const std::string& old_owner,
90                                      const std::string& new_owner) {
91-  service_name_owner_ = new_owner;
92+  bus_->AssertOnOriginThread();
93+
94+  bus_->GetDBusTaskRunner()->PostTask(
95+      FROM_HERE,
96+      base::BindOnce(&ObjectManager::UpdateServiceNameOwner, this, new_owner));
97
98   if (!old_owner.empty()) {
99     ObjectMap::iterator iter = object_map_.begin();
100diff --git a/dbus/object_manager.h b/dbus/object_manager.h
101index 05388de8e6eb..4b5fb790412d 100644
102--- a/dbus/object_manager.h
103+++ b/dbus/object_manager.h
104@@ -317,6 +317,11 @@ class CHROME_DBUS_EXPORT ObjectManager final
105   void NameOwnerChanged(const std::string& old_owner,
106                         const std::string& new_owner);
107
108+  // Write |new_owner| to |service_name_owner_|. This method makes sure write
109+  // happens on the DBus thread, which is the sole writer to
110+  // |service_name_owner_|.
111+  void UpdateServiceNameOwner(const std::string& new_owner);
112+
113   Bus* bus_;
114   std::string service_name_;
115   std::string service_name_owner_;
116diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
117index 7adf8f179471..de5785e98307 100644
118--- a/dbus/object_proxy.cc
119+++ b/dbus/object_proxy.cc
120@@ -274,6 +274,10 @@ void ObjectProxy::SetNameOwnerChangedCallback(
121   bus_->AssertOnOriginThread();
122
123   name_owner_changed_callback_ = callback;
124+
125+  bus_->GetDBusTaskRunner()->PostTask(
126+      FROM_HERE,
127+      base::BindOnce(&ObjectProxy::TryConnectToNameOwnerChangedSignal, this));
128 }
129
130 void ObjectProxy::WaitForServiceToBeAvailable(
131@@ -458,6 +462,15 @@ bool ObjectProxy::ConnectToNameOwnerChangedSignal() {
132   return success;
133 }
134
135+void ObjectProxy::TryConnectToNameOwnerChangedSignal() {
136+  bus_->AssertOnDBusThread();
137+
138+  bool success = ConnectToNameOwnerChangedSignal();
139+  LOG_IF(WARNING, !success)
140+      << "Failed to connect to NameOwnerChanged signal for object: "
141+      << object_path_.value();
142+}
143+
144 bool ObjectProxy::ConnectToSignalInternal(const std::string& interface_name,
145                                           const std::string& signal_name,
146                                           SignalCallback signal_callback) {
147diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h
148index 22e44f1d64c0..b1bf622a12cb 100644
149--- a/dbus/object_proxy.h
150+++ b/dbus/object_proxy.h
151@@ -267,6 +267,9 @@ class CHROME_DBUS_EXPORT ObjectProxy
152   // Connects to NameOwnerChanged signal.
153   bool ConnectToNameOwnerChangedSignal();
154
155+  // Tries to connect to NameOwnerChanged signal, ignores any error.
156+  void TryConnectToNameOwnerChangedSignal();
157+
158   // Helper function for ConnectToSignal().
159   bool ConnectToSignalInternal(const std::string& interface_name,
160                                const std::string& signal_name,
161--
1622.22.0.770.g0f2c4a37fd-goog
163
164