dbus: Fix rlz calling dbus from a worker thread

- Fix RlzValueStoreChromeOS::SetRlzPingSent that calls
  DebugDaemonClient::SetRlzPingSent in a worker thread;
- Fix RlzLibTest failure because of missing TestTimeouts init;

Bug: 966178
Change-Id: I8c12425d2bc66f7f97fe40e81e9b02e65b894232
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1635429Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664735}

rlz/DEPS

@@ -6,6 +6,7 @@ include_rules = [
   "+build",
   "+chromeos/dbus",
   "+chromeos/system",
+  "+dbus",
   "+mojo/core/embedder",
   "+net",  # This is only used when force_rlz_use_chrome_net=1 is passed to gyp.
   "+third_party/zlib",

rlz/chromeos/lib/rlz_value_store_chromeos.cc

@@ -24,6 +24,7 @@
 #include "chromeos/dbus/debug_daemon_client.h"
 #include "chromeos/system/factory_ping_embargo_check.h"
 #include "chromeos/system/statistics_provider.h"
+#include "dbus/bus.h"
 #include "rlz/lib/financial_ping.h"
 #include "rlz/lib/lib_values.h"
 #include "rlz/lib/recursive_cross_process_lock_posix.h"
@@ -112,6 +113,48 @@ bool ConvertToDynamicRlz(const std::string& brand,
   return true;
 }
 
+// Forward declare so that it could be referred in SetRlzPingSent.
+void OnSetRlzPingSent(int retry_count, bool success);
+
+// Calls debug daemon client to set |should_send_rlz_ping| to 0 in RW_VPD.
+// Re-post the work on DBus's original thread if it is not called from there
+// because DBus code is not thread safe.
+void SetRlzPingSent(int retry_count) {
+  // GetSystemBus() could return null in tests.
+  base::TaskRunner* const origin_task_runner =
+      chromeos::DBusThreadManager::Get()->GetSystemBus()
+          ? chromeos::DBusThreadManager::Get()
+                ->GetSystemBus()
+                ->GetOriginTaskRunner()
+          : nullptr;
+  if (origin_task_runner && !origin_task_runner->RunsTasksInCurrentSequence()) {
+    origin_task_runner->PostTask(FROM_HERE,
+                                 base::BindOnce(&SetRlzPingSent, retry_count));
+    return;
+  }
+
+  chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->SetRlzPingSent(
+      base::BindOnce(&OnSetRlzPingSent, retry_count + 1));
+}
+
+// Callback invoked for DebugDaemonClient::SetRlzPingSent.
+void OnSetRlzPingSent(int retry_count, bool success) {
+  if (success) {
+    UMA_HISTOGRAM_BOOLEAN("Rlz.SetRlzPingSent", true);
+    return;
+  }
+
+  if (retry_count >= RlzValueStoreChromeOS::kMaxRetryCount) {
+    UMA_HISTOGRAM_BOOLEAN("Rlz.SetRlzPingSent", false);
+    LOG(ERROR) << "Setting " << chromeos::system::kShouldSendRlzPingKey
+               << " failed after " << RlzValueStoreChromeOS::kMaxRetryCount
+               << " attempts.";
+    return;
+  }
+
+  SetRlzPingSent(retry_count);
+}
+
 }  // namespace
 
 const int RlzValueStoreChromeOS::kMaxRetryCount = 3;
@@ -119,8 +162,7 @@ const int RlzValueStoreChromeOS::kMaxRetryCount = 3;
 RlzValueStoreChromeOS::RlzValueStoreChromeOS(const base::FilePath& store_path)
     : rlz_store_(new base::DictionaryValue),
       store_path_(store_path),
-      read_only_(true),
-      weak_ptr_factory_(this) {
+      read_only_(true) {
   ReadStore();
 }
 
@@ -274,10 +316,9 @@ bool RlzValueStoreChromeOS::AddStatefulEvent(Product product,
                                              const char* event_rlz) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (strcmp(event_rlz, "CAF") == 0) {
-    set_rlz_ping_sent_attempts_ = 0;
-    SetRlzPingSent();
-  }
+  if (strcmp(event_rlz, "CAF") == 0)
+    SetRlzPingSent(/*retry_count=*/0);
+
   return AddValueToList(GetKeyName(kStatefulEventKey, product),
                         std::make_unique<base::Value>(event_rlz));
 }
@@ -397,25 +438,6 @@ bool RlzValueStoreChromeOS::RemoveValueFromList(const std::string& list_name,
   return true;
 }
 
-void RlzValueStoreChromeOS::SetRlzPingSent() {
-  ++set_rlz_ping_sent_attempts_;
-  chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->SetRlzPingSent(
-      base::BindOnce(&RlzValueStoreChromeOS::OnSetRlzPingSent,
-                     weak_ptr_factory_.GetWeakPtr()));
-}
-
-void RlzValueStoreChromeOS::OnSetRlzPingSent(bool success) {
-  if (success) {
-    UMA_HISTOGRAM_BOOLEAN("Rlz.SetRlzPingSent", true);
-  } else if (set_rlz_ping_sent_attempts_ >= kMaxRetryCount) {
-    UMA_HISTOGRAM_BOOLEAN("Rlz.SetRlzPingSent", false);
-    LOG(ERROR) << "Setting " << chromeos::system::kShouldSendRlzPingKey
-               << " failed after " << kMaxRetryCount << " attempts.";
-  } else {
-    SetRlzPingSent();
-  }
-}
-
 namespace {
 
 // RlzValueStoreChromeOS keeps its data in memory and only writes it to disk

rlz/chromeos/lib/rlz_value_store_chromeos.h

@@ -12,7 +12,6 @@
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "rlz/lib/rlz_value_store.h"
 
@@ -78,13 +77,6 @@ class RlzValueStoreChromeOS : public RlzValueStore {
   bool RemoveValueFromList(const std::string& list_name,
                            const base::Value& value);
 
-  // Set |should_send_rlz_ping| to 0 in RW_VPD. This is a wrapper of
-  // |DebugDaemonClient::SetRlzPingSent|.
-  void SetRlzPingSent();
-
-  // Callback of |SetRlzPingSent|.
-  void OnSetRlzPingSent(bool success);
-
   // In-memory store with RLZ data.
   std::unique_ptr<base::DictionaryValue> rlz_store_;
 
@@ -92,13 +84,8 @@ class RlzValueStoreChromeOS : public RlzValueStore {
 
   bool read_only_;
 
-  // The number of attempts of |SetRlzPingSent| so far.
-  int set_rlz_ping_sent_attempts_;
-
   SEQUENCE_CHECKER(sequence_checker_);
 
-  base::WeakPtrFactory<RlzValueStoreChromeOS> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(RlzValueStoreChromeOS);
 };
 

rlz/test/rlz_unittest_main.cc

@@ -6,6 +6,7 @@
 
 #include "base/at_exit.h"
 #include "base/command_line.h"
+#include "base/test/test_timeouts.h"
 #include "build/build_config.h"
 #include "mojo/core/embedder/embedder.h"
 #include "rlz/lib/rlz_lib.h"
@@ -27,6 +28,9 @@ int main(int argc, char **argv) {
 
   mojo::core::Init();
 
+  // RlzLibTest uses base::test::ScopedTaskEnvironment that needs TestTimeouts.
+  TestTimeouts::Initialize();
+
   int ret = RUN_ALL_TESTS();
   if (ret == 0) {
     // Now re-run all the tests using a supplementary brand code.  This brand