Fix a couple DCHECK errors

* LibAssistant will make a couple file IO during creation.  Moved the
  creation to background thread to avoid DCHECK.
* LibAssistant start will sync wait, this is not allowed in Chromium code
  and triggers a DCHECK error.  Now we created a new start API which takes
  a callback.  This API will be non-blocking and invokes callback when
  it's done.

Bug: 826536, 826462
Test: locally build and run, no DCHECK
Change-Id: If6384977838d3056a75df8becbba5257dba71555
Reviewed-on: https://chromium-review.googlesource.com/991458
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Muyuan Li <muyuanli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553331}

chromeos/services/assistant/assistant_manager_service.h

@@ -18,13 +18,24 @@ namespace assistant {
 // Interface class that defines all assistant functionalities.
 class AssistantManagerService : public mojom::Assistant {
  public:
+  enum State {
+    // Initial state, the service is created but not started yet.
+    STOPPED = 0,
+    // The service is started, it takes a little time to be fully running.
+    STARTED = 1,
+    // The service is fully running and ready to take requests.
+    RUNNING = 2
+  };
+
   ~AssistantManagerService() override = default;
 
-  // Start the assistant in the background with |token|.
-  virtual void Start(const std::string& access_token) = 0;
+  // Start the assistant in the background with |token|. When the service is
+  // fully started |callback| will be called on the thread where ctor was run.
+  virtual void Start(const std::string& access_token,
+                     base::OnceClosure callback) = 0;
 
-  // Returns whether assistant is running.
-  virtual bool IsRunning() const = 0;
+  // Returns the current state.
+  virtual State GetState() const = 0;
 
   // Set access token for assistant.
   virtual void SetAccessToken(const std::string& access_token) = 0;

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -26,39 +26,31 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
     mojom::AudioInputPtr audio_input)
     : platform_api_(kDefaultConfigStr, std::move(audio_input)),
       action_module_(std::make_unique<action::CrosActionModule>(this)),
-      assistant_manager_(
-          assistant_client::AssistantManager::Create(&platform_api_,
-                                                     kDefaultConfigStr)),
-      assistant_manager_internal_(
-          UnwrapAssistantManagerInternal(assistant_manager_.get())),
       display_connection_(std::make_unique<CrosDisplayConnection>(this)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      weak_factory_(this) {
-  assistant_manager_->AddConversationStateListener(this);
-}
+      weak_factory_(this) {}
 
 AssistantManagerServiceImpl::~AssistantManagerServiceImpl() {}
 
-void AssistantManagerServiceImpl::Start(const std::string& access_token) {
-  // Posting to background thread because GetARCVersion may be blocking
+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(&chromeos::version_loader::GetARCVersion),
+      base::BindOnce(&assistant_client::AssistantManager::Create,
+                     &platform_api_, kDefaultConfigStr),
       base::BindOnce(&AssistantManagerServiceImpl::StartAssistantInternal,
-                     base::Unretained(this), access_token));
+                     base::Unretained(this), std::move(callback), access_token,
+                     chromeos::version_loader::GetARCVersion()));
 
   // Set the flag to avoid starting the service multiple times.
-  running_ = true;
-
-  if (!assistant_settings_manager_) {
-    assistant_settings_manager_ =
-        std::make_unique<AssistantSettingsManagerImpl>(this);
-  }
+  state_ = State::STARTED;
 }
 
-bool AssistantManagerServiceImpl::IsRunning() const {
-  return running_;
-}
+AssistantManagerService::State AssistantManagerServiceImpl::GetState() const {
+  return state_;
+};
 
 void AssistantManagerServiceImpl::SetAccessToken(
     const std::string& access_token) {
@@ -179,8 +171,14 @@ void AssistantManagerServiceImpl::OnSpeechLevelUpdated(
 }
 
 void AssistantManagerServiceImpl::StartAssistantInternal(
+    base::OnceCallback<void()> callback,
     const std::string& access_token,
-    const std::string& arc_version) {
+    const std::string& arc_version,
+    assistant_client::AssistantManager* assistant_manager) {
+  assistant_manager_.reset(assistant_manager);
+  assistant_manager_internal_ =
+      UnwrapAssistantManagerInternal(assistant_manager_.get());
+
   auto* internal_options =
       assistant_manager_internal_->CreateDefaultInternalOptions();
   SetAssistantOptions(internal_options, BuildUserAgent(arc_version));
@@ -190,9 +188,23 @@ void AssistantManagerServiceImpl::StartAssistantInternal(
 
   assistant_manager_internal_->SetDisplayConnection(display_connection_.get());
   assistant_manager_internal_->RegisterActionModule(action_module_.get());
+  assistant_manager_->AddConversationStateListener(this);
+
+  if (!assistant_settings_manager_) {
+    assistant_settings_manager_ =
+        std::make_unique<AssistantSettingsManagerImpl>(this);
+  }
 
   SetAccessToken(access_token);
-  assistant_manager_->Start();
+
+  assistant_client::Callback0 assistant_callback([callback = std::move(
+                                                      callback)]() mutable {
+    std::move(callback).Run();
+  });
+
+  assistant_manager_->Start(std::move(assistant_callback));
+
+  state_ = State::RUNNING;
 }
 
 std::string AssistantManagerServiceImpl::BuildUserAgent(

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -43,8 +43,9 @@ class AssistantManagerServiceImpl
   ~AssistantManagerServiceImpl() override;
 
   // assistant::AssistantManagerService overrides
-  void Start(const std::string& access_token) override;
-  bool IsRunning() const override;
+  void Start(const std::string& access_token,
+             base::OnceClosure callback) override;
+  State GetState() const override;
   void SetAccessToken(const std::string& access_token) override;
   void EnableListening(bool enable) override;
   AssistantSettingsManager* GetAssistantSettingsManager() override;
@@ -77,8 +78,11 @@ class AssistantManagerServiceImpl
           recognition_result) override;
 
  private:
-  void StartAssistantInternal(const std::string& access_token,
-                              const std::string& arc_version);
+  void StartAssistantInternal(
+      base::OnceClosure callback,
+      const std::string& access_token,
+      const std::string& arc_version,
+      assistant_client::AssistantManager* assistant_manager);
   std::string BuildUserAgent(const std::string& arc_version) const;
 
   void HandleGetSettingsResponse(GetSettingsUiResponseCallback callback,
@@ -98,12 +102,12 @@ class AssistantManagerServiceImpl
           recognition_result);
   void OnSpeechLevelUpdatedOnMainThread(const float speech_level);
 
-  bool running_ = false;
+  State state_ = State::STOPPED;
   PlatformApiImpl platform_api_;
   std::unique_ptr<action::CrosActionModule> action_module_;
   std::unique_ptr<assistant_client::AssistantManager> assistant_manager_;
   std::unique_ptr<AssistantSettingsManagerImpl> assistant_settings_manager_;
-  assistant_client::AssistantManagerInternal* const assistant_manager_internal_;
+  assistant_client::AssistantManagerInternal* assistant_manager_internal_;
   std::unique_ptr<CrosDisplayConnection> display_connection_;
   mojo::InterfacePtrSet<mojom::AssistantEventSubscriber> subscribers_;
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;

chromeos/services/assistant/fake_assistant_manager_service_impl.cc

@@ -11,8 +11,12 @@ FakeAssistantManagerServiceImpl::FakeAssistantManagerServiceImpl() = default;
 
 FakeAssistantManagerServiceImpl::~FakeAssistantManagerServiceImpl() = default;
 
-void FakeAssistantManagerServiceImpl::Start(const std::string& access_token) {
-  running_ = true;
+void FakeAssistantManagerServiceImpl::Start(const std::string& access_token,
+                                            base::OnceClosure callback) {
+  state_ = State::RUNNING;
+
+  if (callback)
+    std::move(callback).Run();
 }
 
 void FakeAssistantManagerServiceImpl::SetAccessToken(
@@ -20,8 +24,9 @@ void FakeAssistantManagerServiceImpl::SetAccessToken(
 
 void FakeAssistantManagerServiceImpl::EnableListening(bool enable) {}
 
-bool FakeAssistantManagerServiceImpl::IsRunning() const {
-  return running_;
+AssistantManagerService::State FakeAssistantManagerServiceImpl::GetState()
+    const {
+  return state_;
 }
 
 AssistantSettingsManager*

chromeos/services/assistant/fake_assistant_manager_service_impl.h

@@ -23,10 +23,11 @@ class FakeAssistantManagerServiceImpl : public AssistantManagerService {
   ~FakeAssistantManagerServiceImpl() override;
 
   // assistant::AssistantManagerService overrides
-  void Start(const std::string& access_token) override;
+  void Start(const std::string& access_token,
+             base::OnceClosure callback) override;
   void SetAccessToken(const std::string& access_token) override;
   void EnableListening(bool enable) override;
-  bool IsRunning() const override;
+  State GetState() const override;
   AssistantSettingsManager* GetAssistantSettingsManager() override;
   void SendGetSettingsUiRequest(
       const std::string& selector,
@@ -38,7 +39,7 @@ class FakeAssistantManagerServiceImpl : public AssistantManagerService {
       mojom::AssistantEventSubscriberPtr subscriber) override;
 
  private:
-  bool running_ = false;
+  State state_ = State::STOPPED;
   DISALLOW_COPY_AND_ASSIGN(FakeAssistantManagerServiceImpl);
 };
 

chromeos/services/assistant/platform_api_impl.h

@@ -19,9 +19,9 @@
 // libassistant/contrib dependency.
 #include "libassistant/contrib/core/macros.h"
 #include "libassistant/contrib/platform/audio/output/audio_output_provider_impl.h"
-#include "libassistant/contrib/platform/auth/auth_provider_impl.h"
 #include "libassistant/contrib/platform/resources/resource_provider.h"
 #include "libassistant/shared/public/platform_api.h"
+#include "libassistant/shared/public/platform_auth.h"
 
 namespace chromeos {
 namespace assistant {

chromeos/services/assistant/service.cc

@@ -44,7 +44,9 @@ constexpr char kScopeAssistant[] =
 Service::Service()
     : platform_binding_(this),
       session_observer_binding_(this),
-      token_refresh_timer_(std::make_unique<base::OneShotTimer>()) {
+      token_refresh_timer_(std::make_unique<base::OneShotTimer>()),
+      main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_ptr_factory_(this) {
   registry_.AddInterface<mojom::AssistantPlatform>(base::BindRepeating(
       &Service::BindAssistantPlatformConnection, base::Unretained(this)));
 }
@@ -78,6 +80,8 @@ void Service::BindAssistantConnection(mojom::AssistantRequest request) {
   // Assistant interface is supposed to be used when UI is actually in
   // use, which should be way later than assistant is created.
   DCHECK(assistant_manager_service_);
+  DCHECK(assistant_manager_service_->GetState() ==
+         AssistantManagerService::State::RUNNING);
   bindings_.AddBinding(assistant_manager_service_.get(), std::move(request));
 }
 
@@ -122,18 +126,13 @@ void Service::Init(mojom::ClientPtr client, mojom::AudioInputPtr audio_input) {
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
   assistant_manager_service_ =
       std::make_unique<AssistantManagerServiceImpl>(std::move(audio_input));
-
-  // Bind to Assistant controller in ash.
-  ash::mojom::AshAssistantControllerPtr assistant_controller;
-  context()->connector()->BindInterface(ash::mojom::kServiceName,
-                                        &assistant_controller);
-  mojom::AssistantPtr ptr;
-  BindAssistantConnection(mojo::MakeRequest(&ptr));
-  assistant_controller->SetAssistant(std::move(ptr));
 #else
   assistant_manager_service_ =
       std::make_unique<FakeAssistantManagerServiceImpl>();
 #endif
+
+  // This will eventually trigger the actual start of assistant services because
+  // they all depend on it.
   RequestAccessToken();
 }
 
@@ -163,13 +162,19 @@ void Service::GetAccessTokenCallback(const base::Optional<std::string>& token,
   }
 
   DCHECK(assistant_manager_service_);
-  if (!assistant_manager_service_->IsRunning()) {
-    assistant_manager_service_->Start(token.value());
-    AddAshSessionObserver();
-    registry_.AddInterface<mojom::Assistant>(base::BindRepeating(
-        &Service::BindAssistantConnection, base::Unretained(this)));
-    client_->OnAssistantStatusChanged(true);
-    DVLOG(1) << "Assistant started";
+  if (assistant_manager_service_->GetState() ==
+      AssistantManagerService::State::STOPPED) {
+    assistant_manager_service_->Start(
+        token.value(),
+        base::BindOnce(
+            [](scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+               base::OnceCallback<void()> callback) {
+              task_runner->PostTask(FROM_HERE, std::move(callback));
+            },
+            main_thread_task_runner_,
+            base::BindOnce(&Service::FinalizeAssistantManangerService,
+                           weak_ptr_factory_.GetWeakPtr())));
+    DVLOG(1) << "Request Assistant start";
   } else {
     assistant_manager_service_->SetAccessToken(token.value());
   }
@@ -187,6 +192,25 @@ void Service::GetAccessTokenCallback(const base::Optional<std::string>& token,
                               this, &Service::RequestAccessToken);
 }
 
+void Service::FinalizeAssistantManangerService() {
+  DCHECK(assistant_manager_service_->GetState() ==
+         AssistantManagerService::State::RUNNING);
+
+  // Bind to Assistant controller in ash.
+  ash::mojom::AshAssistantControllerPtr assistant_controller;
+  context()->connector()->BindInterface(ash::mojom::kServiceName,
+                                        &assistant_controller);
+  mojom::AssistantPtr ptr;
+  BindAssistantConnection(mojo::MakeRequest(&ptr));
+  assistant_controller->SetAssistant(std::move(ptr));
+
+  AddAshSessionObserver();
+  registry_.AddInterface<mojom::Assistant>(base::BindRepeating(
+      &Service::BindAssistantConnection, base::Unretained(this)));
+  client_->OnAssistantStatusChanged(true);
+  DVLOG(1) << "Assistant is running";
+}
+
 void Service::AddAshSessionObserver() {
   ash::mojom::SessionControllerPtr session_controller;
   context()->connector()->BindInterface(ash::mojom::kServiceName,

chromeos/services/assistant/service.h

@@ -86,6 +86,8 @@ class Service : public service_manager::Service,
 
   void UpdateListeningState();
 
+  void FinalizeAssistantManangerService();
+
   service_manager::BinderRegistry registry_;
 
   mojo::BindingSet<mojom::Assistant> bindings_;
@@ -100,12 +102,15 @@ class Service : public service_manager::Service,
   std::unique_ptr<AssistantManagerService> assistant_manager_service_;
   AssistantSettingsManager* assistant_settings_manager_;
   std::unique_ptr<base::OneShotTimer> token_refresh_timer_;
+  scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
 
   // Whether the current user session is active.
   bool session_active_ = false;
   // Whether the lock screen is on.
   bool locked_ = false;
 
+  base::WeakPtrFactory<Service> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(Service);
 };