Simplify initialization of AssistantManager

The code to initialize the |AssistantManager| was quite complex because
it had to handle the fact that |AssistantManager| was created on a
background thread, and that this background task could be triggered
multiple times before it ever finished.

To simplify this the |AssistantManager| (and its related classes) is now
owned by an |AssistantManagerController| that will:
   * Ensure the |done_callback| is invoked only once, even if multiple
     controllers are created (we achieve this by storing the controller
     in a unique_ptr and ensuring it only calls the callback if the
     controller was not deleted, meaning only the last controller will
     call the callback).
   * Not access the |assistant_manager_| field from the background
     thread, meaning it can always safely be accessed from the main
     thread.

This simplification will make it easier to move the Libassistant process
to a sandbox.

Bug: b/171748795
Test: chromeos_unittest --gtest_filter="AssistantManagerServiceTest.*"
Change-Id: Icfce1163d5253713094145b5d7e7c637057527c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2500452
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823195}

chromeos/services/assistant/BUILD.gn

@@ -63,6 +63,8 @@ component("lib") {
     sources += [
       "assistant_device_settings_delegate.cc",
       "assistant_device_settings_delegate.h",
+      "assistant_manager_controller.cc",
+      "assistant_manager_controller.h",
       "assistant_manager_service_delegate.h",
       "assistant_manager_service_delegate_impl.cc",
       "assistant_manager_service_delegate_impl.h",

chromeos/services/assistant/assistant_manager_controller.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/services/assistant/assistant_manager_controller.h"
+
+#include "base/feature_list.h"
+#include "chromeos/assistant/internal/cros_display_connection.h"
+#include "chromeos/assistant/internal/internal_util.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "chromeos/services/assistant/assistant_manager_service_delegate.h"
+#include "chromeos/services/assistant/assistant_manager_service_impl.h"
+#include "chromeos/services/assistant/public/cpp/features.h"
+#include "libassistant/shared/internal_api/assistant_manager_internal.h"
+
+namespace chromeos {
+namespace assistant {
+
+namespace {
+
+using DoneCallback =
+    base::OnceCallback<void(std::unique_ptr<CrosDisplayConnection>,
+                            std::unique_ptr<assistant_client::AssistantManager>,
+                            assistant_client::AssistantManagerInternal*)>;
+
+constexpr base::Feature kChromeOSAssistantDogfood{
+    "ChromeOSAssistantDogfood", base::FEATURE_DISABLED_BY_DEFAULT};
+
+constexpr char kServersideDogfoodExperimentId[] = "20347368";
+constexpr char kServersideOpenAppExperimentId[] = "39651593";
+constexpr char kServersideResponseProcessingV2ExperimentId[] = "1793869";
+
+struct AssistantObjects {
+  std::unique_ptr<CrosDisplayConnection> display_connection;
+  std::unique_ptr<assistant_client::AssistantManager> assistant_manager;
+  assistant_client::AssistantManagerInternal* assistant_manager_internal =
+      nullptr;
+};
+
+void FillServerExperimentIds(std::vector<std::string>* server_experiment_ids) {
+  if (base::FeatureList::IsEnabled(kChromeOSAssistantDogfood)) {
+    server_experiment_ids->emplace_back(kServersideDogfoodExperimentId);
+  }
+
+  if (base::FeatureList::IsEnabled(features::kAssistantAppSupport))
+    server_experiment_ids->emplace_back(kServersideOpenAppExperimentId);
+
+  if (features::IsResponseProcessingV2Enabled()) {
+    server_experiment_ids->emplace_back(
+        kServersideResponseProcessingV2ExperimentId);
+  }
+}
+
+void SetServerExperiments(
+    assistant_client::AssistantManagerInternal* assistant_manager_internal) {
+  std::vector<std::string> server_experiment_ids;
+  FillServerExperimentIds(&server_experiment_ids);
+
+  if (server_experiment_ids.size() > 0) {
+    assistant_manager_internal->AddExtraExperimentIds(server_experiment_ids);
+  }
+}
+
+void SetAuthTokens(assistant_client::AssistantManager* assistant_manager,
+                   const AssistantManagerController::AuthTokens& tokens) {
+  assistant_manager->SetAuthTokens(tokens);
+}
+
+void UpdateInternalOptions(
+    assistant_client::AssistantManagerInternal* assistant_manager_internal,
+    const std::string& locale,
+    bool spoken_feedback_enabled) {
+  // NOTE: this method is called on multiple threads, it needs to be
+  // thread-safe.
+  auto* internal_options =
+      assistant_manager_internal->CreateDefaultInternalOptions();
+  SetAssistantOptions(internal_options, locale, spoken_feedback_enabled);
+
+  internal_options->SetClientControlEnabled(
+      assistant::features::IsRoutinesEnabled());
+
+  if (!features::IsVoiceMatchDisabled())
+    internal_options->EnableRequireVoiceMatchVerification();
+
+  assistant_manager_internal->SetOptions(*internal_options, [](bool success) {
+    DVLOG(2) << "set options: " << success;
+  });
+}
+
+// Creates the Assistant on the current thread, and stores the resulting
+// objects in |result|.
+void CreateAssistantOnCurrentThread(
+    AssistantObjects* result,
+    AssistantManagerServiceDelegate* delegate,
+    assistant_client::PlatformApi* platform_api,
+    assistant_client::ActionModule* action_module,
+    assistant_client::FuchsiaApiDelegate* fuchsia_api_delegate,
+    AssistantManagerServiceImpl* service,
+    const std::string& libassistant_config,
+    const std::string& locale,
+    const std::string& locale_override,
+    bool spoken_feedback_enabled,
+    const AssistantManagerController::AuthTokens& auth_tokens) {
+  result->display_connection = std::make_unique<CrosDisplayConnection>(
+      service, /*feedback_ui_enabled=*/true,
+      assistant::features::IsMediaSessionIntegrationEnabled());
+
+  result->assistant_manager =
+      delegate->CreateAssistantManager(platform_api, libassistant_config);
+
+  result->assistant_manager_internal =
+      delegate->UnwrapAssistantManagerInternal(result->assistant_manager.get());
+
+  UpdateInternalOptions(result->assistant_manager_internal, locale,
+                        spoken_feedback_enabled);
+
+  result->assistant_manager_internal->SetDisplayConnection(
+      result->display_connection.get());
+  result->assistant_manager_internal->SetLocaleOverride(locale_override);
+  result->assistant_manager_internal->RegisterActionModule(action_module);
+  result->assistant_manager_internal->SetAssistantManagerDelegate(service);
+  result->assistant_manager_internal->GetFuchsiaApiHelperOrDie()
+      ->SetFuchsiaApiDelegate(fuchsia_api_delegate);
+
+  result->assistant_manager->AddConversationStateListener(service);
+  result->assistant_manager->AddDeviceStateListener(service);
+
+  SetServerExperiments(result->assistant_manager_internal);
+  SetAuthTokens(result->assistant_manager.get(), auth_tokens);
+
+  result->assistant_manager->Start();
+}
+
+// Creates the Assistant on the given (background) thread, and passes it to
+// the callback on the current thread.
+void CreateAssistantOnBackgroundThread(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    AssistantManagerServiceDelegate* delegate,
+    assistant_client::PlatformApi* platform_api,
+    assistant_client::ActionModule* action_module,
+    assistant_client::FuchsiaApiDelegate* fuchsia_api_delegate,
+    AssistantManagerServiceImpl* service,
+    const std::string& libassistant_config,
+    const std::string& locale,
+    const std::string& locale_override,
+    bool spoken_feedback_enabled,
+    const AssistantManagerController::AuthTokens& auth_tokens,
+    DoneCallback done_callback) {
+  auto result = std::make_unique<AssistantObjects>();
+  auto* result_pointer = result.get();
+  task_runner->PostTaskAndReply(
+      FROM_HERE,
+      BindOnce(CreateAssistantOnCurrentThread, result_pointer, delegate,
+               platform_api, action_module, fuchsia_api_delegate, service,
+               libassistant_config, locale, locale_override,
+               spoken_feedback_enabled, auth_tokens),
+      BindOnce(
+          [](std::unique_ptr<AssistantObjects> result, DoneCallback callback) {
+            std::move(callback).Run(std::move(result->display_connection),
+                                    std::move(result->assistant_manager),
+                                    result->assistant_manager_internal);
+          },
+          std::move(result), std::move(done_callback)));
+}
+
+}  // namespace
+
+AssistantManagerController::AssistantManagerController()
+    : weak_factory_(this) {}
+
+AssistantManagerController::~AssistantManagerController() = default;
+
+void AssistantManagerController::Initialize(
+    AssistantManagerServiceImpl* service,
+    scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
+    AssistantManagerServiceDelegate* delegate,
+    assistant_client::PlatformApi* platform_api,
+    assistant_client::ActionModule* action_module,
+    assistant_client::FuchsiaApiDelegate* fuchsia_api_delegate,
+    const std::string& libassistant_config,
+    const std::string& locale,
+    const std::string& locale_override,
+    bool spoken_feedback_enabled,
+    const AuthTokens& auth_tokens,
+    base::OnceClosure done_callback) {
+  CreateAssistantOnBackgroundThread(
+      background_task_runner, delegate, platform_api, action_module,
+      fuchsia_api_delegate, service, libassistant_config, locale,
+      locale_override, spoken_feedback_enabled, auth_tokens,
+      base::BindOnce(&AssistantManagerController::OnAssistantCreated,
+                     weak_factory_.GetWeakPtr(), std::move(done_callback)));
+}
+
+void AssistantManagerController::UpdateInternalOptions(
+    const std::string& locale,
+    bool spoken_feedback_enabled) {
+  chromeos::assistant::UpdateInternalOptions(assistant_manager_internal(),
+                                             locale, spoken_feedback_enabled);
+}
+
+void AssistantManagerController::SetAuthTokens(const AuthTokens& tokens) {
+  chromeos::assistant::SetAuthTokens(assistant_manager(), tokens);
+}
+
+bool AssistantManagerController::IsInitialized() const {
+  return (display_connection_ != nullptr) && (assistant_manager_ != nullptr) &&
+         (assistant_manager_internal_ != nullptr);
+}
+
+void AssistantManagerController::OnAssistantCreated(
+    base::OnceClosure done_callback,
+    std::unique_ptr<CrosDisplayConnection> display_connection,
+    std::unique_ptr<assistant_client::AssistantManager> assistant_manager,
+    assistant_client::AssistantManagerInternal* assistant_manager_internal) {
+  DCHECK(display_connection);
+  DCHECK(assistant_manager);
+  DCHECK(assistant_manager_internal);
+
+  display_connection_ = std::move(display_connection);
+  assistant_manager_ = std::move(assistant_manager);
+  assistant_manager_internal_ = assistant_manager_internal;
+
+  std::move(done_callback).Run();
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/assistant_manager_controller.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_CONTROLLER_H_
+#define CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_CONTROLLER_H_
+
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/check.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+
+namespace assistant_client {
+
+class PlatformApi;
+class ActionModule;
+class FuchsiaApiDelegate;
+class AssistantManager;
+class AssistantManagerInternal;
+
+}  // namespace assistant_client
+
+namespace chromeos {
+namespace assistant {
+
+class AssistantManagerServiceDelegate;
+class CrosDisplayConnection;
+class AssistantManagerServiceImpl;
+
+class AssistantManagerController {
+ public:
+  // Each authentication token exists of a [gaia_id, access_token] tuple.
+  using AuthTokens = std::vector<std::pair<std::string, std::string>>;
+
+  AssistantManagerController();
+
+  AssistantManagerController(AssistantManagerController&) = delete;
+  AssistantManagerController& operator=(AssistantManagerController&) = delete;
+  ~AssistantManagerController();
+
+  // Can not be invoked before initialization has finished.
+  // Both LibAssistant and Chrome threads may access |display_connection|.
+  // |display_connection| is thread safe.
+  CrosDisplayConnection* display_connection() {
+    DCHECK(IsInitialized());
+    return display_connection_.get();
+  }
+
+  // Can not be invoked before initialization has finished.
+  assistant_client::AssistantManager* assistant_manager() {
+    DCHECK(IsInitialized());
+    return assistant_manager_.get();
+  }
+
+  // Can not be invoked before initialization has finished.
+  assistant_client::AssistantManagerInternal* assistant_manager_internal() {
+    DCHECK(IsInitialized());
+    return assistant_manager_internal_;
+  }
+
+  // Initialize the |AssistantManager| by creating the objects (on a background
+  // task) and by calling their Start() methods. Will signal the objects exist
+  // and can be accessed by calling the |done_callback|.
+  //
+  // If the |AssistantManagerController| is destroyed before Initialize()
+  // finishes, the created objects will safely be destructed.
+  // However, if any of the passed in objects (|service|, |delegate|,
+  // |platform_api| and so on) are destroyed, the caller *must* destroy
+  // |background_task_runner| first or invalid memory might be accessed.
+  // Also, if a new instance of |AssistantManagerController| is immediately
+  // created and initialized before the background thread has had any chance to
+  // run, it is theoretically possible for 2 instances of |AssistantManager|
+  // to exist at the same time. However, this is prevented by the logic in
+  // |service.cc|.
+  void Initialize(
+      AssistantManagerServiceImpl* service,
+      scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
+      AssistantManagerServiceDelegate* delegate,
+      assistant_client::PlatformApi* platform_api,
+      assistant_client::ActionModule* action_module,
+      assistant_client::FuchsiaApiDelegate* fuchsia_api_delegate,
+      const std::string& libassistant_config,
+      const std::string& locale,
+      const std::string& locale_override,
+      bool spoken_feedback_enabled,
+      const AuthTokens& auth_tokens,
+      base::OnceClosure done_callback);
+
+  // Whether Initialize() has been called and has finished.
+  // Until this is true trying to access any of the getters will fail.
+  bool IsInitialized() const;
+
+  void UpdateInternalOptions(const std::string& locale,
+                             bool spoken_feedback_enabled);
+
+  // Passing in an empty vector will start Libassistant in signed-out mode.
+  void SetAuthTokens(const AuthTokens& tokens);
+
+ private:
+  void OnAssistantCreated(
+      base::OnceClosure done_callback,
+      std::unique_ptr<CrosDisplayConnection> display_connection,
+      std::unique_ptr<assistant_client::AssistantManager> assistant_manager,
+      assistant_client::AssistantManagerInternal* assistant_manager_internal);
+
+  // NOTE: |display_connection_| is used by |assistant_manager_| and must be
+  // declared before so it will be destructed after.
+  std::unique_ptr<CrosDisplayConnection> display_connection_;
+  std::unique_ptr<assistant_client::AssistantManager> assistant_manager_;
+  assistant_client::AssistantManagerInternal* assistant_manager_internal_ =
+      nullptr;
+
+  base::WeakPtrFactory<AssistantManagerController> weak_factory_;
+};
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_CONTROLLER_H_

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -58,10 +58,11 @@ namespace chromeos {
 namespace assistant {
 
 class AssistantMediaSession;
+class AssistantDeviceSettingsDelegate;
+class AssistantManagerController;
+class AssistantManagerServiceDelegate;
 class CrosPlatformApi;
 class ServiceContext;
-class AssistantManagerServiceDelegate;
-class AssistantDeviceSettingsDelegate;
 
 // Enumeration of Assistant query response type, also recorded in histograms.
 // These values are persisted to logs. Entries should not be renumbered and
@@ -211,12 +212,8 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   void OnAndroidAppListRefreshed(
       const std::vector<AndroidAppInfo>& apps_info) override;
 
-  assistant_client::AssistantManager* assistant_manager() {
-    return assistant_manager_.get();
-  }
-  assistant_client::AssistantManagerInternal* assistant_manager_internal() {
-    return assistant_manager_internal_;
-  }
+  assistant_client::AssistantManager* assistant_manager();
+  assistant_client::AssistantManagerInternal* assistant_manager_internal();
   CrosPlatformApi* platform_api() { return platform_api_.get(); }
 
   // assistant_client::MediaManager::Listener overrides:
@@ -246,9 +243,10 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   }
 
  private:
-  void StartAssistantInternal(const base::Optional<UserInfo>& user,
-                              const std::string& locale);
+  void InitAssistant(const base::Optional<UserInfo>& user,
+                     const std::string& locale);
   void PostInitAssistant();
+  bool IsInitialized() const;
 
   // Update device id, type and locale
   void UpdateDeviceSettings();
@@ -304,6 +302,9 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   DeviceActions* device_actions();
   scoped_refptr<base::SequencedTaskRunner> main_task_runner();
 
+  CrosDisplayConnection* display_connection();
+  AssistantManagerController* assistant_manager_controller();
+
   void SetStateAndInformObservers(State new_state);
 
   State state_ = State::STOPPED;
@@ -311,25 +312,10 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   std::unique_ptr<CrosPlatformApi> platform_api_;
   std::unique_ptr<action::CrosActionModule> action_module_;
   ChromiumApiDelegate chromium_api_delegate_;
-  // NOTE: |display_connection_| is used by |assistant_manager_| and must be
-  // declared before so it will be destructed after.
-  std::unique_ptr<CrosDisplayConnection> display_connection_;
-  // Similar to |new_asssistant_manager_|, created on |background_thread_| then
-  // posted to main thread to finish initialization then move to
-  // |display_connection_|.
-  std::unique_ptr<CrosDisplayConnection> new_display_connection_;
-  std::unique_ptr<assistant_client::AssistantManager> assistant_manager_;
   std::unique_ptr<AssistantSettingsImpl> assistant_settings_;
-  // |new_assistant_manager_| is created on |background_thread_| then posted to
-  // main thread to finish initialization then move to |assistant_manager_|.
-  std::unique_ptr<assistant_client::AssistantManager> new_assistant_manager_;
-  // Same ownership as |new_assistant_manager_|.
-  assistant_client::AssistantManagerInternal* new_assistant_manager_internal_ =
-      nullptr;
-  base::Lock new_assistant_manager_lock_;
-  // same ownership as |assistant_manager_|.
-  assistant_client::AssistantManagerInternal* assistant_manager_internal_ =
-      nullptr;
+
+  std::unique_ptr<AssistantManagerController> assistant_manager_controller_;
+
   base::ObserverList<AssistantInteractionSubscriber> interaction_subscribers_;
   mojo::Remote<media_session::mojom::MediaController> media_controller_;