assistant: fix DCHECK due to thread synchronization primitive

Bug: b/109875600
Test: Debug build and run
Change-Id: I751611b7617e1683bd17307ad971f4b9ae1bb4a0
Reviewed-on: https://chromium-review.googlesource.com/1116289
Commit-Queue: Muyuan Li <muyuanli@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570869}

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -8,7 +8,6 @@
 
 #include "ash/public/interfaces/constants.mojom.h"
 #include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
@@ -45,7 +44,9 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
       display_connection_(std::make_unique<CrosDisplayConnection>(this)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       voice_interaction_observer_binding_(this),
+      background_thread_("background thread"),
       weak_factory_(this) {
+  background_thread_.Start();
   connector->BindInterface(ash::mojom::kServiceName,
                            &voice_interaction_controller_);
 
@@ -57,19 +58,19 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
 AssistantManagerServiceImpl::~AssistantManagerServiceImpl() {}
 
 void AssistantManagerServiceImpl::Start(const std::string& access_token,
-                                        base::OnceClosure callback) {
-  // LibAssistant creation will make file IO. Post the creation to
-  // background thread to avoid DCHECK.
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::BindOnce(&assistant_client::AssistantManager::Create,
-                     &platform_api_, CreateLibAssistantConfig()),
-      base::BindOnce(&AssistantManagerServiceImpl::StartAssistantInternal,
-                     base::Unretained(this), std::move(callback), access_token,
-                     chromeos::version_loader::GetARCVersion()));
-
+                                        base::OnceClosure post_init_callback) {
   // Set the flag to avoid starting the service multiple times.
   state_ = State::STARTED;
+
+  // LibAssistant creation will make file IO and sync wait. Post the creation to
+  // background thread to avoid DCHECK.
+  background_thread_.task_runner()->PostTaskAndReply(
+      FROM_HERE,
+      base::BindOnce(&AssistantManagerServiceImpl::StartAssistantInternal,
+                     base::Unretained(this), access_token,
+                     chromeos::version_loader::GetARCVersion()),
+      base::BindOnce(&AssistantManagerServiceImpl::PostInitAssistant,
+                     base::Unretained(this), std::move(post_init_callback)));
 }
 
 AssistantManagerService::State AssistantManagerServiceImpl::GetState() const {
@@ -99,6 +100,8 @@ AssistantManagerServiceImpl::GetAssistantSettingsManager() {
 void AssistantManagerServiceImpl::SendGetSettingsUiRequest(
     const std::string& selector,
     GetSettingsUiResponseCallback callback) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
   std::string serialized_proto = SerializeGetSettingsUiRequest(selector);
   assistant_manager_internal_->SendGetSettingsUiRequest(
       serialized_proto, std::string(), [
@@ -123,6 +126,8 @@ void AssistantManagerServiceImpl::SendGetSettingsUiRequest(
 void AssistantManagerServiceImpl::SendUpdateSettingsUiRequest(
     const std::string& update,
     UpdateSettingsUiResponseCallback callback) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
   std::string serialized_proto = SerializeUpdateSettingsUiRequest(update);
   assistant_manager_internal_->SendUpdateSettingsUiRequest(
       serialized_proto, std::string(), [
@@ -258,11 +263,12 @@ void AssistantManagerServiceImpl::OnVoiceInteractionSetupCompleted(
 }
 
 void AssistantManagerServiceImpl::StartAssistantInternal(
-    base::OnceCallback<void()> callback,
     const std::string& access_token,
-    const std::string& arc_version,
-    assistant_client::AssistantManager* assistant_manager) {
-  assistant_manager_.reset(assistant_manager);
+    const std::string& arc_version) {
+  DCHECK(background_thread_.task_runner()->BelongsToCurrentThread());
+
+  assistant_manager_.reset(assistant_client::AssistantManager::Create(
+      &platform_api_, CreateLibAssistantConfig()));
   assistant_manager_internal_ =
       UnwrapAssistantManagerInternal(assistant_manager_.get());
 
@@ -278,26 +284,23 @@ void AssistantManagerServiceImpl::StartAssistantInternal(
   assistant_manager_internal_->SetAssistantManagerDelegate(this);
   assistant_manager_->AddConversationStateListener(this);
 
-  if (!assistant_settings_manager_) {
-    assistant_settings_manager_ =
-        std::make_unique<AssistantSettingsManagerImpl>(this);
-  }
-
   SetAccessToken(access_token);
 
-  assistant_client::Callback0 assistant_callback([
-    callback = std::move(callback),
-    update_settings_callback =
-        base::BindOnce(&AssistantManagerServiceImpl::UpdateDeviceSettings,
-                       weak_factory_.GetWeakPtr())
-  ]() mutable {
-    std::move(callback).Run();
-    std::move(update_settings_callback).Run();
-  });
+  assistant_manager_->Start();
+}
 
-  assistant_manager_->Start(std::move(assistant_callback));
+void AssistantManagerServiceImpl::PostInitAssistant(
+    base::OnceClosure post_init_callback) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
 
   state_ = State::RUNNING;
+
+  if (!assistant_settings_manager_) {
+    assistant_settings_manager_ =
+        std::make_unique<AssistantSettingsManagerImpl>(this);
+  }
+  std::move(post_init_callback).Run();
+  UpdateDeviceSettings();
 }
 
 std::string AssistantManagerServiceImpl::BuildUserAgent(
@@ -324,6 +327,8 @@ std::string AssistantManagerServiceImpl::BuildUserAgent(
 }
 
 void AssistantManagerServiceImpl::UpdateDeviceSettings() {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
   const std::string device_id = assistant_manager_->GetDeviceId();
   if (device_id.empty())
     return;
@@ -342,16 +347,14 @@ void AssistantManagerServiceImpl::UpdateDeviceSettings() {
   SendUpdateSettingsUiRequest(update.SerializeAsString(), base::DoNothing());
 
   // Update device locale if voice interaction setup is completed.
-  main_thread_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &AssistantManagerServiceImpl::IsVoiceInteractionSetupCompleted,
-          weak_factory_.GetWeakPtr(),
+  AssistantManagerServiceImpl::IsVoiceInteractionSetupCompleted(
       base::BindOnce(&AssistantManagerServiceImpl::UpdateDeviceLocale,
-                         weak_factory_.GetWeakPtr())));
+                     weak_factory_.GetWeakPtr()));
 }
 
 void AssistantManagerServiceImpl::UpdateDeviceLocale(bool is_setup_completed) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+
   if (!is_setup_completed)
     return;
 

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -12,6 +12,7 @@
 // TODO(xiaohuic): replace with "base/macros.h" once we remove
 // libassistant/contrib dependency.
 #include "ash/public/interfaces/voice_interaction_controller.mojom.h"
+#include "base/threading/thread.h"
 #include "chromeos/assistant/internal/action/cros_action_module.h"
 #include "chromeos/assistant/internal/cros_display_connection.h"
 #include "chromeos/services/assistant/assistant_manager_service.h"
@@ -110,16 +111,16 @@ class AssistantManagerServiceImpl
       ash::mojom::AssistantAllowedState state) override {}
 
  private:
-  void StartAssistantInternal(
-      base::OnceClosure callback,
-      const std::string& access_token,
-      const std::string& arc_version,
-      assistant_client::AssistantManager* assistant_manager);
+  void StartAssistantInternal(const std::string& access_token,
+                              const std::string& arc_version);
+  void PostInitAssistant(base::OnceClosure post_init_callback);
+
   std::string BuildUserAgent(const std::string& arc_version) const;
 
   // Update device id, type, and call |UpdateDeviceLocale| when assistant
   // service starts.
   void UpdateDeviceSettings();
+
   // Update device locale if |is_setup_completed| is true;
   void UpdateDeviceLocale(bool is_setup_completed);
 
@@ -160,6 +161,9 @@ class AssistantManagerServiceImpl
   ash::mojom::VoiceInteractionControllerPtr voice_interaction_controller_;
   mojo::Binding<ash::mojom::VoiceInteractionObserver>
       voice_interaction_observer_binding_;
+
+  base::Thread background_thread_;
+
   base::WeakPtrFactory<AssistantManagerServiceImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AssistantManagerServiceImpl);