assistant: fix shutdown crash

Fix shutdown crash in default_url_request_context_getter. Base class
url_request_context_getter::OnDestruct() already posted the destruction
to network_task_runner. This makes the thread join in
default_url_request_context_getter crash because it is joining on the
same thread.

This cl removes the posting and joining logic in
default_url_reqeust_context_getter since it is duplicate with the base
class. We cannot create and hold the thread object either because that
will implicitly join when destrcted, causing the same error. We use the
global IO task runner instead.

Bug: b:120096335
Test: locally build and run. Does not crash during shutdown.
Change-Id: I80672c99eeab52198158bbb0e6471ccca1dcc21f
Reviewed-on: https://chromium-review.googlesource.com/c/1354034Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612097}

chrome/browser/profiles/profile_impl.cc

@@ -1213,7 +1213,9 @@ std::unique_ptr<service_manager::Service> ProfileImpl::HandleServiceRequest(
 #if BUILDFLAG(ENABLE_CROS_ASSISTANT)
   if (service_name == chromeos::assistant::mojom::kServiceName) {
     return std::make_unique<chromeos::assistant::Service>(
-        std::move(request), content::GetNetworkConnectionTracker());
+        std::move(request), content::GetNetworkConnectionTracker(),
+        base::CreateSingleThreadTaskRunnerWithTraits(
+            {content::BrowserThread::IO}));
   }
 #endif
 #endif

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -107,7 +107,7 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
     network::NetworkConnectionTracker* network_connection_tracker)
     : action_module_(std::make_unique<action::CrosActionModule>(this)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      chromium_api_delegate_(),
+      chromium_api_delegate_(service->io_task_runner()),
       assistant_settings_manager_(
           std::make_unique<AssistantSettingsManagerImpl>(this)),
       display_connection_(std::make_unique<CrosDisplayConnection>(this)),

chromeos/services/assistant/chromium_api_delegate.cc

@@ -4,15 +4,17 @@
 
 #include "chromeos/services/assistant/chromium_api_delegate.h"
 
+#include "base/single_thread_task_runner.h"
 #include "chromeos/services/assistant/default_url_request_context_getter.h"
 
 namespace chromeos {
 namespace assistant {
 
-ChromiumApiDelegate::ChromiumApiDelegate()
+ChromiumApiDelegate::ChromiumApiDelegate(
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
     : http_connection_factory_(
           base::MakeRefCounted<DefaultURLRequestContextGetter>(
-              "chromium_http_connection")) {}
+              io_task_runner)) {}
 
 ChromiumApiDelegate::~ChromiumApiDelegate() = default;
 

chromeos/services/assistant/chromium_api_delegate.h

@@ -19,7 +19,8 @@ class ChromiumHttpConnectionFactory;
 
 class ChromiumApiDelegate : public assistant_client::FuchsiaApiDelegate {
  public:
-  ChromiumApiDelegate();
+  ChromiumApiDelegate(
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
   ~ChromiumApiDelegate() override;
   // assistant_client::FuchsiaApiDelegate overrides:
   assistant_client::HttpConnectionFactory* GetHttpConnectionFactory() override;

chromeos/services/assistant/default_url_request_context_getter.cc

@@ -37,29 +37,13 @@ std::unique_ptr<::net::ProxyConfigServiceFixed> GetProxyConfigurationFromParams(
 namespace chromeos {
 namespace assistant {
 
-DefaultURLRequestContextGetter::DefaultURLRequestContextGetter(
-    const std::string& network_thread_name)
-    : thread_(new base::Thread(network_thread_name)) {
-  thread_->StartWithOptions(
-      base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
-  network_task_runner_ = thread_->task_runner();
-  DCHECK(network_task_runner_);
-}
-
 DefaultURLRequestContextGetter::DefaultURLRequestContextGetter(
     scoped_refptr<base::SingleThreadTaskRunner> network_task_runner)
-    : network_task_runner_(network_task_runner) {
+    : network_task_runner_(std::move(network_task_runner)) {
   DCHECK(network_task_runner_);
 }
 
-DefaultURLRequestContextGetter::~DefaultURLRequestContextGetter() {
-  if (request_context_) {
-    // The context should be destroyed on the network thread.
-    network_task_runner_->DeleteSoon(FROM_HERE, request_context_.release());
-  }
-  if (thread_)
-    thread_->Stop();
-}
+DefaultURLRequestContextGetter::~DefaultURLRequestContextGetter() = default;
 
 void DefaultURLRequestContextGetter::CreateContext() {
   // Context must be created on network thread since its internal objects

chromeos/services/assistant/default_url_request_context_getter.h

@@ -13,10 +13,6 @@
 #include "base/memory/ref_counted.h"
 #include "net/url_request/url_request_context_getter.h"
 
-namespace base {
-class Thread;
-}  // namespace base
-
 namespace chromeos {
 namespace assistant {
 
@@ -54,8 +50,6 @@ class DefaultURLRequestContextGetter : public ::net::URLRequestContextGetter {
   void SetProxyConfigurationInternal(const std::string& proxy_server,
                                      const std::string& bypass_list);
 
-  // |thread_| is non-null if created by this class.
-  std::unique_ptr<base::Thread> thread_;
   scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
   std::unique_ptr<::net::URLRequestContext> request_context_;
 

chromeos/services/assistant/service.cc

@@ -53,7 +53,8 @@ constexpr base::TimeDelta kMaxTokenRefreshDelay =
 }  // namespace
 
 Service::Service(service_manager::mojom::ServiceRequest request,
-                 network::NetworkConnectionTracker* network_connection_tracker)
+                 network::NetworkConnectionTracker* network_connection_tracker,
+                 scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
     : service_binding_(this, std::move(request)),
       platform_binding_(this),
       session_observer_binding_(this),
@@ -61,6 +62,7 @@ Service::Service(service_manager::mojom::ServiceRequest request,
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       power_manager_observer_(this),
       network_connection_tracker_(network_connection_tracker),
+      io_task_runner_(std::move(io_task_runner)),
       weak_ptr_factory_(this) {
   registry_.AddInterface<mojom::AssistantPlatform>(base::BindRepeating(
       &Service::BindAssistantPlatformConnection, base::Unretained(this)));

chromeos/services/assistant/service.h

@@ -17,6 +17,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"
+#include "base/single_thread_task_runner.h"
 #include "base/time/time.h"
 #include "chromeos/dbus/power_manager_client.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
@@ -54,7 +55,8 @@ class Service : public service_manager::Service,
                 public ash::DefaultVoiceInteractionObserver {
  public:
   Service(service_manager::mojom::ServiceRequest request,
-          network::NetworkConnectionTracker* network_connection_tracker);
+          network::NetworkConnectionTracker* network_connection_tracker,
+          scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
   ~Service() override;
 
   mojom::Client* client() { return client_.get(); }
@@ -71,6 +73,9 @@ class Service : public service_manager::Service,
   }
 
   ash::AssistantStateBase* assistant_state() { return &assistant_state_; }
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() {
+    return io_task_runner_;
+  }
 
   void RequestAccessToken();
 
@@ -168,6 +173,7 @@ class Service : public service_manager::Service,
   ash::AssistantStateProxy assistant_state_;
 
   network::NetworkConnectionTracker* network_connection_tracker_;
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
 
   base::WeakPtrFactory<Service> weak_ptr_factory_;
 

chromeos/services/assistant/service_unittest.cc

@@ -17,7 +17,6 @@
 #include "chromeos/dbus/fake_power_manager_client.h"
 #include "chromeos/services/assistant/fake_assistant_manager_service_impl.h"
 #include "chromeos/services/assistant/public/mojom/constants.mojom.h"
-#include "chromeos/services/assistant/service.h"
 #include "services/identity/public/mojom/identity_manager.mojom.h"
 #include "services/service_manager/public/cpp/service_binding.h"
 #include "services/service_manager/public/cpp/test/test_connector_factory.h"
@@ -168,7 +167,7 @@ class AssistantServiceTest : public testing::Test {
 
     service_ = std::make_unique<Service>(
         test_connector_factory_.RegisterInstance(mojom::kServiceName),
-        nullptr /* network_connection_tracker */);
+        nullptr /* network_connection_tracker */, nullptr /* io_task_runner */);
 
     mock_task_runner_ = base::MakeRefCounted<base::TestMockTimeTaskRunner>(
         base::Time::Now(), base::TimeTicks::Now());