Move background thread to |AssistantProxy|

Bug: b/171748795
Test: chromeos_unittest --gtest_filter="AssistantManagerServiceTest.*"
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: Iec0158636b0002594db7aba2c57e2238ea42e0b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518759
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825151}

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -172,14 +172,11 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
       assistant_proxy_(std::make_unique<AssistantProxy>()),
       context_(context),
       delegate_(std::move(delegate)),
-      background_thread_("background thread"),
       libassistant_config_(
           CreateLibAssistantConfig(s3_server_uri_override, device_id_override)),
       weak_factory_(this) {
-  background_thread_.Start();
-
   platform_api_ = delegate_->CreatePlatformApi(
-      media_session_.get(), background_thread_.task_runner());
+      media_session_.get(), background_thread().task_runner());
 
   settings_delegate_ =
       std::make_unique<AssistantDeviceSettingsDelegate>(context);
@@ -193,7 +190,11 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
 }
 
 AssistantManagerServiceImpl::~AssistantManagerServiceImpl() {
-  background_thread_.Stop();
+  // Destroy the Assistant Proxy first so the background thread is flushed
+  // before any of the other objects are destroyed. If we don't do this
+  // the background thread could for example access |platform_api_| after it
+  // is destroyed.
+  assistant_proxy_ = nullptr;
 }
 
 void AssistantManagerServiceImpl::Start(const base::Optional<UserInfo>& user,
@@ -275,7 +276,7 @@ void AssistantManagerServiceImpl::RegisterFallbackMediaHandler() {
 void AssistantManagerServiceImpl::WaitUntilStartIsFinishedForTesting() {
   // First we wait until the |AssistantManager| is created on the background
   // thread.
-  background_thread_.FlushForTesting();
+  background_thread().FlushForTesting();
   // Then we wait until |PostInitAssistant| finishes.
   // (which runs on the main thread).
   base::RunLoop().RunUntilIdle();
@@ -985,8 +986,8 @@ void AssistantManagerServiceImpl::InitAssistant(
   DCHECK(!IsServiceStarted());
 
   service_controller().Start(
-      background_thread_.task_runner(), delegate_.get(), platform_api(),
-      action_module_.get(), &chromium_api_delegate_,
+      delegate_.get(), platform_api(), action_module_.get(),
+      &chromium_api_delegate_,
       /*assistant_manager_delegate=*/this,
       /*conversation_state_listener=*/this,
       /*device_state_listener=*/this,
@@ -1399,6 +1400,10 @@ const ServiceController& AssistantManagerServiceImpl::service_controller()
   return assistant_proxy_->service_controller();
 }
 
+base::Thread& AssistantManagerServiceImpl::background_thread() {
+  return assistant_proxy_->background_thread();
+}
+
 void AssistantManagerServiceImpl::SetStateAndInformObservers(State new_state) {
   state_ = new_state;
 

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -307,6 +307,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   CrosDisplayConnection* display_connection();
   ServiceController& service_controller();
   const ServiceController& service_controller() const;
+  base::Thread& background_thread();
 
   void SetStateAndInformObservers(State new_state);
 
@@ -334,8 +335,6 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   base::Lock last_trigger_source_lock_;
   base::TimeTicks started_time_;
 
-  base::Thread background_thread_;
-
   int next_interaction_id_ = 1;
   std::map<std::string, std::unique_ptr<AssistantInteractionMetadata>>
       pending_interactions_;

chromeos/services/assistant/proxy/assistant_proxy.cc

@@ -10,8 +10,11 @@
 namespace chromeos {
 namespace assistant {
 
-AssistantProxy::AssistantProxy()
-    : service_controller_(std::make_unique<ServiceController>()) {}
+AssistantProxy::AssistantProxy() {
+  background_thread_.Start();
+  service_controller_ =
+      std::make_unique<ServiceController>(background_thread_.task_runner());
+}
 
 AssistantProxy::~AssistantProxy() = default;
 

chromeos/services/assistant/proxy/assistant_proxy.h

@@ -7,6 +7,8 @@
 
 #include <memory>
 
+#include "base/threading/thread.h"
+
 namespace chromeos {
 namespace assistant {
 
@@ -25,7 +27,13 @@ class AssistantProxy {
   // service.
   ServiceController& service_controller();
 
+  // The background thread is temporary exposed until the entire Libassistant
+  // API is hidden behind this proxy API.
+  base::Thread& background_thread() { return background_thread_; }
+
  private:
+  base::Thread background_thread_{"Assistant background thread"};
+
   std::unique_ptr<ServiceController> service_controller_;
 };
 

chromeos/services/assistant/proxy/service_controller.cc

@@ -175,12 +175,15 @@ void CreateAssistantOnBackgroundThread(
 
 }  // namespace
 
-ServiceController::ServiceController() : weak_factory_(this) {}
+ServiceController::ServiceController(
+    scoped_refptr<base::SingleThreadTaskRunner> background_task_runner)
+    : background_task_runner_(background_task_runner), weak_factory_(this) {
+  DCHECK(background_task_runner_);
+}
 
 ServiceController::~ServiceController() = default;
 
 void ServiceController::Start(
-    scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
     AssistantManagerServiceDelegate* delegate,
     assistant_client::PlatformApi* platform_api,
     assistant_client::ActionModule* action_module,
@@ -200,7 +203,7 @@ void ServiceController::Start(
   state_ = State::kStarting;
 
   CreateAssistantOnBackgroundThread(
-      background_task_runner, delegate, platform_api, action_module,
+      background_task_runner_, delegate, platform_api, action_module,
       fuchsia_api_delegate, assistant_manager_delegate,
       conversation_state_listener, device_state_listener, event_observer,
       libassistant_config, locale, locale_override, spoken_feedback_enabled,

chromeos/services/assistant/proxy/service_controller.h

@@ -40,7 +40,8 @@ class ServiceController {
   // Each authentication token exists of a [gaia_id, access_token] tuple.
   using AuthTokens = std::vector<std::pair<std::string, std::string>>;
 
-  ServiceController();
+  explicit ServiceController(
+      scoped_refptr<base::SingleThreadTaskRunner> background_task_runner);
 
   ServiceController(ServiceController&) = delete;
   ServiceController& operator=(ServiceController&) = delete;
@@ -72,16 +73,12 @@ class ServiceController {
   //
   // If the |ServiceController| is destroyed before Start()
   // finishes, the created objects will safely be destructed.
-  // However, if any of the passed in objects (|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 |ServiceController| is immediately
+  // However, if a new instance of |ServiceController| 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 Start(
-      scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
       AssistantManagerServiceDelegate* delegate,
       assistant_client::PlatformApi* platform_api,
       assistant_client::ActionModule* action_module,
@@ -129,6 +126,8 @@ class ServiceController {
   // Used internally for consistency checks.
   State state_ = State::kStopped;
 
+  scoped_refptr<base::SingleThreadTaskRunner> background_task_runner_;
+
   // 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_;