Remove ServiceWorkerContextWrapper::RunOrPostTaskOnCoreThread()

The method has been introduced for ServiceWorkerOnUI.
The feature has already fully launched, so let's remove it.

Unfortunately, I found a few methods still need to be called from the IO
thread. Instead of changing the callsites, this CL simply keeps those
methods callbable from any threads. Updated the comment along with that.

Bug: 1138155
Change-Id: If919c1f87fc2d6d5ce5e74f1254eb0f41676acc0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599573
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842478}

chrome/browser/extensions/service_worker_apitest.cc

@@ -233,17 +233,7 @@ class ServiceWorkerTest : public ExtensionApiTest {
         content::BrowserContext::GetDefaultStoragePartition(
             browser()->profile())
             ->GetServiceWorkerContext();
-    base::RunLoop run_loop;
-    size_t ref_count = 0;
-    auto set_ref_count = [](size_t* ref_count, base::RunLoop* run_loop,
-                            size_t external_request_count) {
-      *ref_count = external_request_count;
-      run_loop->Quit();
-    };
-    sw_context->CountExternalRequestsForTest(
-        origin, base::BindOnce(set_ref_count, &ref_count, &run_loop));
-    run_loop.Run();
-    return ref_count;
+    return sw_context->CountExternalRequestsForTest(origin);
   }
 
  private:

components/performance_manager/service_worker_context_adapter.cc

@@ -131,10 +131,10 @@ ServiceWorkerContextAdapter::FinishedExternalRequest(
   return content::ServiceWorkerExternalRequestResult::kOk;
 }
 
-void ServiceWorkerContextAdapter::CountExternalRequestsForTest(
-    const url::Origin& origin,
-    CountExternalRequestsCallback callback) {
+size_t ServiceWorkerContextAdapter::CountExternalRequestsForTest(
+    const url::Origin& origin) {
   NOTIMPLEMENTED();
+  return 0u;
 }
 
 bool ServiceWorkerContextAdapter::MaybeHasRegistrationForOrigin(

components/performance_manager/service_worker_context_adapter.h

@@ -58,9 +58,7 @@ class ServiceWorkerContextAdapter
   content::ServiceWorkerExternalRequestResult FinishedExternalRequest(
       int64_t service_worker_version_id,
       const std::string& request_uuid) override;
-  void CountExternalRequestsForTest(
-      const url::Origin& origin,
-      CountExternalRequestsCallback callback) override;
+  size_t CountExternalRequestsForTest(const url::Origin& origin) override;
   bool MaybeHasRegistrationForOrigin(const url::Origin& origin) override;
   void GetInstalledRegistrationOrigins(
       base::Optional<std::string> host_filter,

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -43,12 +43,11 @@ EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(
           base::MakeRefCounted<URLLoaderFactoryGetter>()) {
   scoped_refptr<base::SequencedTaskRunner> database_task_runner =
       base::ThreadTaskRunnerHandle::Get();
-  wrapper_->InitOnCoreThread(
-      user_data_directory, std::move(database_task_runner),
+  wrapper_->InitInternal(
+      user_data_directory,
       /*quota_manager_proxy=*/nullptr, special_storage_policy, nullptr,
-      url_loader_factory_getter_.get(),
-      wrapper_->CreateNonNetworkPendingURLLoaderFactoryBundleForUpdateCheck(
-          browser_context_.get()));
+      url_loader_factory_getter_.get(), std::move(database_task_runner),
+      browser_context_.get());
   wrapper_->process_manager()->SetProcessIdForTest(mock_render_process_id());
   wrapper_->process_manager()->SetNewProcessIdForTest(new_render_process_id());
 

content/browser/service_worker/service_worker_browsertest.cc

@@ -351,16 +351,15 @@ class ServiceWorkerBrowserTest : public ContentBrowserTest {
         shell()->web_contents()->GetBrowserContext());
     wrapper_ = static_cast<ServiceWorkerContextWrapper*>(
         partition->GetServiceWorkerContext());
-
-    RunOnCoreThread(
-        base::BindOnce(&self::SetUpOnCoreThread, base::Unretained(this)));
   }
 
   void TearDownOnMainThread() override {
-    base::RunLoop loop;
-    RunOnCoreThread(base::BindOnce(&self::TearDownOnCoreThread,
-                                   base::Unretained(this), loop.QuitClosure()));
-    loop.Run();
+    // Flush remote storage control so that all pending callbacks are executed.
+    wrapper()
+        ->context()
+        ->registry()
+        ->GetRemoteStorageControl()
+        .FlushForTesting();
     content::RunAllTasksUntilIdle();
     wrapper_ = nullptr;
   }
@@ -379,18 +378,6 @@ class ServiceWorkerBrowserTest : public ContentBrowserTest {
         1);
   }
 
-  virtual void SetUpOnCoreThread() {}
-
-  virtual void TearDownOnCoreThread(base::OnceClosure callback) {
-    // Flush remote storage control so that all pending callbacks are executed.
-    wrapper()
-        ->context()
-        ->registry()
-        ->GetRemoteStorageControl()
-        .FlushForTesting();
-    std::move(callback).Run();
-  }
-
   ServiceWorkerContextWrapper* wrapper() { return wrapper_.get(); }
   ServiceWorkerContext* public_context() { return wrapper(); }
 

content/browser/service_worker/service_worker_register_job.cc

@@ -336,39 +336,6 @@ bool ServiceWorkerRegisterJob::IsUpdateCheckNeeded() const {
   return !skip_script_comparison_;
 }
 
-void ServiceWorkerRegisterJob::TriggerUpdateCheck(
-    scoped_refptr<network::SharedURLLoaderFactory> loader_factory) {
-  DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
-
-  if (!loader_factory) {
-    // We can't continue with update checking appropriately without
-    // |loader_factory|. Null |loader_factory| means that the storage partition
-    // was not available probably because it's shutting down.
-    // This terminates the current job (|this|).
-    Complete(blink::ServiceWorkerStatusCode::kErrorAbort,
-             ServiceWorkerConsts::kShutdownErrorMessage);
-    return;
-  }
-
-  ServiceWorkerVersion* version_to_update = registration()->GetNewestVersion();
-  base::TimeDelta time_since_last_check =
-      base::Time::Now() - registration()->last_update_check();
-  std::vector<storage::mojom::ServiceWorkerResourceRecordPtr> resources;
-  version_to_update->script_cache_map()->GetResources(&resources);
-  int64_t script_resource_id =
-      version_to_update->script_cache_map()->LookupResourceId(script_url_);
-  DCHECK_NE(script_resource_id, blink::mojom::kInvalidServiceWorkerResourceId);
-
-  update_checker_ = std::make_unique<ServiceWorkerUpdateChecker>(
-      std::move(resources), script_url_, script_resource_id, version_to_update,
-      std::move(loader_factory), force_bypass_cache_,
-      registration()->update_via_cache(), time_since_last_check, context_,
-      outside_fetch_client_settings_object_.Clone());
-  update_checker_->Start(
-      base::BindOnce(&ServiceWorkerRegisterJob::OnUpdateCheckFinished,
-                     weak_factory_.GetWeakPtr()));
-}
-
 void ServiceWorkerRegisterJob::OnUpdateCheckFinished(
     ServiceWorkerSingleScriptUpdateChecker::Result result,
     std::unique_ptr<ServiceWorkerSingleScriptUpdateChecker::FailureInfo>
@@ -518,10 +485,35 @@ void ServiceWorkerRegisterJob::UpdateAndContinue() {
     return;
   }
 
-  // This will start the update check after loader factory is retrieved.
-  context_->wrapper()->GetLoaderFactoryForUpdateCheck(
-      scope_, base::BindOnce(&ServiceWorkerRegisterJob::TriggerUpdateCheck,
-                             weak_factory_.GetWeakPtr()));
+  scoped_refptr<network::SharedURLLoaderFactory> loader_factory =
+      context_->wrapper()->GetLoaderFactoryForUpdateCheck(scope_);
+  if (!loader_factory) {
+    // We can't continue with update checking appropriately without
+    // |loader_factory|. Null |loader_factory| means that the storage partition
+    // was not available probably because it's shutting down.
+    // This terminates the current job (|this|).
+    Complete(blink::ServiceWorkerStatusCode::kErrorAbort,
+             ServiceWorkerConsts::kShutdownErrorMessage);
+    return;
+  }
+
+  ServiceWorkerVersion* version_to_update = registration()->GetNewestVersion();
+  base::TimeDelta time_since_last_check =
+      base::Time::Now() - registration()->last_update_check();
+  std::vector<storage::mojom::ServiceWorkerResourceRecordPtr> resources;
+  version_to_update->script_cache_map()->GetResources(&resources);
+  int64_t script_resource_id =
+      version_to_update->script_cache_map()->LookupResourceId(script_url_);
+  DCHECK_NE(script_resource_id, blink::mojom::kInvalidServiceWorkerResourceId);
+
+  update_checker_ = std::make_unique<ServiceWorkerUpdateChecker>(
+      std::move(resources), script_url_, script_resource_id, version_to_update,
+      std::move(loader_factory), force_bypass_cache_,
+      registration()->update_via_cache(), time_since_last_check, context_,
+      outside_fetch_client_settings_object_.Clone());
+  update_checker_->Start(
+      base::BindOnce(&ServiceWorkerRegisterJob::OnUpdateCheckFinished,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void ServiceWorkerRegisterJob::OnStartWorkerFinished(

content/browser/service_worker/service_worker_register_job.h

@@ -115,8 +115,6 @@ class ServiceWorkerRegisterJob : public ServiceWorkerRegisterJobBase {
       scoped_refptr<ServiceWorkerRegistration> registration);
 
   bool IsUpdateCheckNeeded() const;
-  void TriggerUpdateCheck(
-      scoped_refptr<network::SharedURLLoaderFactory> loader_factory);
 
   // Refer ServiceWorkerUpdateChecker::UpdateStatusCallback for the meaning of
   // the parameters.

content/public/browser/service_worker_context.h

@@ -64,6 +64,12 @@ enum class StartServiceWorkerForNavigationHintResult {
 // See service_worker_context_wrapper.cc for the implementation
 // of ServiceWorkerContext and ServiceWorkerContextWrapper (the
 // primary implementation of this abstract class).
+//
+// All methods are basically expected to be called on the UI thread.
+// Currently, only two methods are allowed to be called on any threads but it's
+// discouraged.
+// TODO(https://crbug.com/1161153): Disallow methods to be called on any
+// threads.
 class CONTENT_EXPORT ServiceWorkerContext {
  public:
   using ResultCallback = base::OnceCallback<void(bool success)>;
@@ -81,9 +87,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
       base::OnceCallback<void(OfflineCapability capability,
                               int64_t registration_id)>;
 
-  using CountExternalRequestsCallback =
-      base::OnceCallback<void(size_t external_request_count)>;
-
   using StartServiceWorkerForNavigationHintCallback = base::OnceCallback<void(
       StartServiceWorkerForNavigationHintResult result)>;
 
@@ -93,6 +96,8 @@ class CONTENT_EXPORT ServiceWorkerContext {
   using StartWorkerFailureCallback =
       base::OnceCallback<void(blink::ServiceWorkerStatusCode status_code)>;
 
+  // Returns BrowserThread::UI always.
+  // TODO(https://crbug.com/1138155): Remove this.
   static content::BrowserThread::ID GetCoreThreadId();
 
   // Returns true if |url| is within the service worker |scope|.
@@ -105,7 +110,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
                       ServiceWorkerContext* service_worker_context,
                       base::OnceClosure task);
 
-  // Observer methods are always dispatched on the UI thread.
   virtual void AddObserver(ServiceWorkerContextObserver* observer) = 0;
   virtual void RemoveObserver(ServiceWorkerContextObserver* observer) = 0;
 
@@ -118,9 +122,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
   //  * Fetching |script_url| fails.
   //  * |script_url| fails to parse or its top-level execution fails.
   //  * Something unexpected goes wrong, like a renderer crash or a full disk.
-  //
-  // This function can be called from any thread, but the callback will always
-  // be called on the UI thread.
   virtual void RegisterServiceWorker(
       const GURL& script_url,
       const blink::mojom::ServiceWorkerRegistrationOptions& options,
@@ -133,9 +134,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
   // Unregistration can fail if:
   //  * No registration exists for |scope|.
   //  * Something unexpected goes wrong, like a renderer crash.
-  //
-  // This function can be called from any thread, but the callback will always
-  // be called on the UI thread.
   virtual void UnregisterServiceWorker(const GURL& scope,
                                        ResultCallback callback) = 0;
 
@@ -147,8 +145,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
   // calls FinishedExternalRequest(). This ensures that content/ does not
   // shut the worker down while embedder is expecting the worker to be kept
   // alive.
-  //
-  // Must be called from the core thread.
   virtual ServiceWorkerExternalRequestResult StartingExternalRequest(
       int64_t service_worker_version_id,
       const std::string& request_uuid) = 0;
@@ -157,18 +153,14 @@ class CONTENT_EXPORT ServiceWorkerContext {
       const std::string& request_uuid) = 0;
 
   // Returns the pending external request count for the worker with the
-  // specified |origin| via |callback|. Must be called from the UI thread. The
-  // callback is called on the UI thread.
-  virtual void CountExternalRequestsForTest(
-      const url::Origin& origin,
-      CountExternalRequestsCallback callback) = 0;
+  // specified |origin|.
+  virtual size_t CountExternalRequestsForTest(const url::Origin& origin) = 0;
 
   // Whether |origin| has any registrations. Uninstalling and uninstalled
   // registrations do not cause this to return true, that is, only registrations
   // with status ServiceWorkerRegistration::Status::kIntact are considered, such
   // as even if the corresponding live registrations may still exist. Also,
   // returns true if it doesn't know (registrations are not yet initialized).
-  // Must be called on the UI thread.
   virtual bool MaybeHasRegistrationForOrigin(const url::Origin& origin) = 0;
 
   // Returns a set of origins which have at least one stored registration.
@@ -181,14 +173,13 @@ class CONTENT_EXPORT ServiceWorkerContext {
       base::Optional<std::string> host_filter,
       GetInstalledRegistrationOriginsCallback callback) = 0;
 
-  // May be called from any thread, and the callback is called on that thread.
   virtual void GetAllOriginsInfo(GetUsageInfoCallback callback) = 0;
 
-  // This function can be called from any thread, and the callback is called
-  // on that thread.  Deletes all registrations in the origin and clears all
-  // service workers belonging to the registrations. All clients controlled by
-  // those service workers will lose their controllers immediately after this
-  // operation.
+  // Deletes all registrations in the origin and clears all service workers
+  // belonging to the registrations. All clients controlled by those service
+  // workers will lose their controllers immediately after this operation.
+  // This function can be called from any thread, and the callback is called on
+  // that thread.
   virtual void DeleteForOrigin(const url::Origin& origin_url,
                                ResultCallback callback) = 0;
 
@@ -203,9 +194,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
   // Service Worker registration matching |url|. In case the service
   // worker is being installed as of calling this method, it will wait for the
   // installation to finish before coming back with the result.
-  //
-  // This function can be called from any thread, but the callback will always
-  // be called on the UI thread.
   virtual void CheckHasServiceWorker(
       const GURL& url,
       CheckHasServiceWorkerCallback callback) = 0;
@@ -214,9 +202,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
   // event. Returns OfflineCapability::kSupported and the registration id if
   // the response's status code is 200.
   //
-  // This function can be called from any thread, but the callback will always
-  // be called on the UI thread.
-  //
   // TODO(hayato): Re-visit to integrate this function with
   // |ServiceWorkerContext::CheckHasServiceWorker|.
   virtual void CheckOfflineCapability(
@@ -226,9 +211,6 @@ class CONTENT_EXPORT ServiceWorkerContext {
   // Stops all running service workers and unregisters all service worker
   // registrations. This method is used in web tests to make sure that the
   // existing service worker will not affect the succeeding tests.
-  //
-  // This function can be called from any thread, but the callback will always
-  // be called on the UI thread.
   virtual void ClearAllServiceWorkersForTest(base::OnceClosure callback) = 0;
 
   // Starts the active worker of the registration for the given |scope|. If
@@ -237,9 +219,8 @@ class CONTENT_EXPORT ServiceWorkerContext {
   // successful, otherwise |failure_callback| is passed information about the
   // error.
   //
-  // Must be called on the core thread, and the callback is called on that
-  // thread. There is no guarantee about whether the callback is called
-  // synchronously or asynchronously.
+  // There is no guarantee about whether the callback is called synchronously or
+  // asynchronously.
   virtual void StartWorkerForScope(
       const GURL& scope,
       StartWorkerCallback info_callback,
@@ -257,26 +238,18 @@ class CONTENT_EXPORT ServiceWorkerContext {
       ResultCallback result_callback) = 0;
 
   // Starts the service worker for |document_url|. Called when a navigation to
-  // that URL is predicted to occur soon. Must be called from the UI thread. The
-  // |callback| will always be called on the UI thread.
+  // that URL is predicted to occur soon.
   virtual void StartServiceWorkerForNavigationHint(
       const GURL& document_url,
       StartServiceWorkerForNavigationHintCallback callback) = 0;
 
   // Stops all running workers on the given |origin|.
-  //
-  // This function can be called from any thread.
   virtual void StopAllServiceWorkersForOrigin(const url::Origin& origin) = 0;
 
   // Stops all running service workers.
-  //
-  // This function can be called from any thread.
-  // |callback| is called on the caller's thread.
   virtual void StopAllServiceWorkers(base::OnceClosure callback) = 0;
 
   // Gets info about all running workers.
-  //
-  // Must be called on the UI thread. The callback is called on the UI thread.
   virtual const base::flat_map<int64_t /* version_id */,
                                ServiceWorkerRunningInfo>&
   GetRunningServiceWorkerInfos() = 0;

content/public/test/fake_service_worker_context.cc

@@ -50,10 +50,10 @@ FakeServiceWorkerContext::FinishedExternalRequest(
   NOTREACHED();
   return ServiceWorkerExternalRequestResult::kWorkerNotFound;
 }
-void FakeServiceWorkerContext::CountExternalRequestsForTest(
-    const url::Origin& origin,
-    CountExternalRequestsCallback callback) {
+size_t FakeServiceWorkerContext::CountExternalRequestsForTest(
+    const url::Origin& origin) {
   NOTREACHED();
+  return 0u;
 }
 bool FakeServiceWorkerContext::MaybeHasRegistrationForOrigin(
     const url::Origin& origin) {

content/public/test/fake_service_worker_context.h

@@ -45,9 +45,7 @@ class FakeServiceWorkerContext : public ServiceWorkerContext {
   ServiceWorkerExternalRequestResult FinishedExternalRequest(
       int64_t service_worker_version_id,
       const std::string& request_uuid) override;
-  void CountExternalRequestsForTest(
-      const url::Origin& origin,
-      CountExternalRequestsCallback callback) override;
+  size_t CountExternalRequestsForTest(const url::Origin& origin) override;
   bool MaybeHasRegistrationForOrigin(const url::Origin& origin) override;
   void GetInstalledRegistrationOrigins(
       base::Optional<std::string> host_filter,