OMT script fetch: Set ID of AppCache after main script load

Before this CL, WebSharedWorkerImpl didn't call SelectAppCacheID()
after the main script fetch when off-the-main-thread script fetch
is enabled. This CL moves the callsite of SelectAppCacheID()
to DidFetchScript(). DidFetchScript() is called in both "on" and
"off" the main thread script fetch so it will get called in both
code paths.

For off-the-main-thread script fetch, the appcache ID is plumbed
from WorkerClassicScriptLoader in SharedWorkerGlobalScope.

Also there could be a potential race between IPC messages for
appcache selection and subresource requests if we evaluate the
top-level script immediately after script fetch. To avoid the race
this CL introduces a callback to SelectAppCacheID(). The callback
is invoked when the browser process sends
AppCacheFrontend::CacheSelected mojo IPC. SharedWorkerGlobalScope
evaluates the top-level script after the callback is invoked.

This fixes shared worker related tests in
wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html

Bug: 945673
Change-Id: I03c965bff166d6265e9a8479745fa7c1ae1a976c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614656
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662894}

content/renderer/appcache/web_application_cache_host_impl.cc

@@ -103,6 +103,8 @@ void WebApplicationCacheHostImpl::CacheSelected(
     blink::mojom::AppCacheInfoPtr info) {
   cache_info_ = *info;
   client_->DidChangeCacheAssociation();
+  if (select_cache_for_shared_worker_completion_callback_)
+    std::move(select_cache_for_shared_worker_completion_callback_).Run();
 }
 
 void WebApplicationCacheHostImpl::EventRaised(
@@ -345,7 +347,10 @@ void WebApplicationCacheHostImpl::GetResourceList(
 }
 
 void WebApplicationCacheHostImpl::SelectCacheForSharedWorker(
-    long long app_cache_id) {
+    long long app_cache_id,
+    base::OnceClosure completion_callback) {
+  select_cache_for_shared_worker_completion_callback_ =
+      std::move(completion_callback);
   backend_host_->SelectCacheForSharedWorker(app_cache_id);
 }
 

content/renderer/appcache/web_application_cache_host_impl.h

@@ -63,7 +63,8 @@ class WebApplicationCacheHostImpl : public blink::WebApplicationCacheHost,
   void GetAssociatedCacheInfo(CacheInfo* info) override;
   const base::UnguessableToken& GetHostID() const override;
 
-  void SelectCacheForSharedWorker(long long app_cache_id);
+  void SelectCacheForSharedWorker(long long app_cache_id,
+                                  base::OnceClosure completion_callback);
 
  private:
   enum IsNewMasterEntry { MAYBE_NEW_ENTRY, NEW_ENTRY, OLD_ENTRY };
@@ -81,6 +82,8 @@ class WebApplicationCacheHostImpl : public blink::WebApplicationCacheHost,
   blink::mojom::AppCacheInfo cache_info_;
   GURL original_main_resource_url_;  // Used to detect redirection.
   bool was_select_cache_called_;
+  // Invoked when CacheSelected() is called.
+  base::OnceClosure select_cache_for_shared_worker_completion_callback_;
 };
 
 }  // namespace content

content/renderer/worker/embedded_shared_worker_stub.cc

@@ -173,11 +173,16 @@ void EmbeddedSharedWorkerStub::WorkerContextDestroyed() {
   delete this;
 }
 
-void EmbeddedSharedWorkerStub::SelectAppCacheID(int64_t app_cache_id) {
+void EmbeddedSharedWorkerStub::SelectAppCacheID(
+    int64_t app_cache_id,
+    base::OnceClosure completion_callback) {
   if (app_cache_host_) {
     // app_cache_host_ could become stale as it's owned by blink's
     // DocumentLoader. This method is assumed to be called while it's valid.
-    app_cache_host_->SelectCacheForSharedWorker(app_cache_id);
+    app_cache_host_->SelectCacheForSharedWorker(app_cache_id,
+                                                std::move(completion_callback));
+  } else {
+    std::move(completion_callback).Run();
   }
 }
 

content/renderer/worker/embedded_shared_worker_stub.h

@@ -84,7 +84,7 @@ class EmbeddedSharedWorkerStub : public blink::WebSharedWorkerClient,
   void WorkerScriptLoaded() override;
   void WorkerScriptLoadFailed() override;
   void WorkerScriptEvaluated(bool success) override;
-  void SelectAppCacheID(int64_t) override;
+  void SelectAppCacheID(int64_t, base::OnceClosure) override;
   std::unique_ptr<blink::WebApplicationCacheHost> CreateApplicationCacheHost(
       blink::WebApplicationCacheHostClient*) override;
   std::unique_ptr<blink::WebServiceWorkerNetworkProvider>

third_party/blink/public/web/web_shared_worker_client.h

@@ -56,7 +56,8 @@ class WebSharedWorkerClient {
   virtual void WorkerScriptLoaded() = 0;
   virtual void WorkerScriptLoadFailed() = 0;
   virtual void WorkerScriptEvaluated(bool success) = 0;
-  virtual void SelectAppCacheID(int64_t) = 0;
+  virtual void SelectAppCacheID(int64_t app_cache_id,
+                                base::OnceClosure completion_callback) = 0;
 
   // Called on the main webkit thread in the worker process during
   // initialization.

third_party/blink/renderer/core/exported/web_shared_worker_impl.cc

@@ -180,8 +180,11 @@ void WebSharedWorkerImpl::CountFeature(WebFeature feature) {
   client_->CountFeature(feature);
 }
 
-void WebSharedWorkerImpl::DidFetchScript() {
+void WebSharedWorkerImpl::DidFetchScript(int64_t app_cache_id) {
   DCHECK(IsMainThread());
+  client_->SelectAppCacheID(app_cache_id,
+                            WTF::Bind(&WebSharedWorkerImpl::OnAppCacheSelected,
+                                      weak_ptr_factory_.GetWeakPtr()));
   client_->WorkerScriptLoaded();
 }
 
@@ -276,7 +279,6 @@ void WebSharedWorkerImpl::DidReceiveScriptLoaderResponse() {
   DCHECK(IsMainThread());
   probe::DidReceiveScriptResponse(shadow_page_->GetDocument(),
                                   main_script_loader_->Identifier());
-  client_->SelectAppCacheID(main_script_loader_->AppCacheID());
 }
 
 void WebSharedWorkerImpl::OnScriptLoaderFinished() {
@@ -294,7 +296,7 @@ void WebSharedWorkerImpl::OnScriptLoaderFinished() {
     // |this| is deleted at this point.
     return;
   }
-  DidFetchScript();
+  DidFetchScript(main_script_loader_->AppCacheID());
   probe::ScriptImported(shadow_page_->GetDocument(),
                         main_script_loader_->Identifier(),
                         main_script_loader_->SourceText());
@@ -310,6 +312,14 @@ void WebSharedWorkerImpl::OnScriptLoaderFinished() {
                 weak_ptr_factory_.GetWeakPtr()));
 }
 
+void WebSharedWorkerImpl::OnAppCacheSelected() {
+  DCHECK(IsMainThread());
+  if (features::IsOffMainThreadSharedWorkerScriptFetchEnabled()) {
+    DCHECK(GetWorkerThread());
+    GetWorkerThread()->OnAppCacheSelected();
+  }
+}
+
 void WebSharedWorkerImpl::ContinueStartWorkerContext() {
   DCHECK(IsMainThread());
   if (asked_to_terminate_)

third_party/blink/renderer/core/exported/web_shared_worker_impl.h

@@ -59,6 +59,7 @@ class SharedURLLoaderFactory;
 
 namespace blink {
 
+class SharedWorkerThread;
 class WebApplicationCacheHost;
 class WebApplicationCacheHostClient;
 class WebSharedWorkerClient;
@@ -109,20 +110,21 @@ class CORE_EXPORT WebSharedWorkerImpl final : public WebSharedWorker,
 
   // Callback methods for SharedWorkerReportingProxy.
   void CountFeature(WebFeature);
-  void DidFetchScript();
+  void DidFetchScript(int64_t app_cache_id);
   void DidFailToFetchClassicScript();
   void DidEvaluateClassicScript(bool success);
   void DidCloseWorkerGlobalScope();
   void DidTerminateWorkerThread();
 
  private:
-  WorkerThread* GetWorkerThread() { return worker_thread_.get(); }
+  SharedWorkerThread* GetWorkerThread() { return worker_thread_.get(); }
 
   // Shuts down the worker thread.
   void TerminateWorkerThread();
 
   void DidReceiveScriptLoaderResponse();
   void OnScriptLoaderFinished();
+  void OnAppCacheSelected();
   void ContinueStartWorkerContext();
   void StartWorkerThread(
       std::unique_ptr<GlobalScopeCreationParams>,
@@ -139,7 +141,7 @@ class CORE_EXPORT WebSharedWorkerImpl final : public WebSharedWorker,
   base::UnguessableToken devtools_worker_token_;
 
   Persistent<SharedWorkerReportingProxy> reporting_proxy_;
-  std::unique_ptr<WorkerThread> worker_thread_;
+  std::unique_ptr<SharedWorkerThread> worker_thread_;
   mojom::blink::WorkerContentSettingsProxyPtrInfo content_settings_info_;
 
   // |client_| owns |this|.

third_party/blink/renderer/core/workers/dedicated_worker_global_scope.cc

@@ -270,7 +270,7 @@ void DedicatedWorkerGlobalScope::DidFetchClassicScript(
     ReportingProxy().DidFailToFetchClassicScript();
     return;
   }
-  ReportingProxy().DidFetchScript();
+  ReportingProxy().DidFetchScript(classic_script_loader->AppCacheID());
   probe::ScriptImported(this, classic_script_loader->Identifier(),
                         classic_script_loader->SourceText());
 

third_party/blink/renderer/core/workers/shared_worker_global_scope.cc

@@ -91,8 +91,10 @@ SharedWorkerGlobalScope::SharedWorkerGlobalScope(
     SharedWorkerThread* thread,
     base::TimeTicks time_origin)
     : WorkerGlobalScope(std::move(creation_params), thread, time_origin) {
-  // TODO(bashi): Call this after appcache host is set.
-  ReadyToRunClassicScript();
+  // When off-the-main-thread script fetch is enabled, ReadyToRunClassicScript()
+  // will be called after an app cache is selected.
+  if (!features::IsOffMainThreadSharedWorkerScriptFetchEnabled())
+    ReadyToRunClassicScript();
 }
 
 SharedWorkerGlobalScope::~SharedWorkerGlobalScope() = default;
@@ -203,6 +205,12 @@ void SharedWorkerGlobalScope::Connect(MessagePortChannel channel) {
   DispatchEvent(*event);
 }
 
+void SharedWorkerGlobalScope::OnAppCacheSelected() {
+  DCHECK(IsContextThread());
+  DCHECK(features::IsOffMainThreadSharedWorkerScriptFetchEnabled());
+  ReadyToRunClassicScript();
+}
+
 void SharedWorkerGlobalScope::DidReceiveResponseForClassicScript(
     WorkerClassicScriptLoader* classic_script_loader) {
   DCHECK(IsContextThread());
@@ -225,7 +233,7 @@ void SharedWorkerGlobalScope::DidFetchClassicScript(
     ReportingProxy().DidFailToFetchClassicScript();
     return;
   }
-  ReportingProxy().DidFetchScript();
+  ReportingProxy().DidFetchScript(classic_script_loader->AppCacheID());
   probe::ScriptImported(this, classic_script_loader->Identifier(),
                         classic_script_loader->SourceText());
 

third_party/blink/renderer/core/workers/shared_worker_global_scope.h

@@ -90,6 +90,8 @@ class CORE_EXPORT SharedWorkerGlobalScope final : public WorkerGlobalScope {
 
   void Connect(MessagePortChannel channel);
 
+  void OnAppCacheSelected();
+
   void Trace(blink::Visitor*) override;
 
  private:

third_party/blink/renderer/core/workers/shared_worker_reporting_proxy.cc

@@ -63,7 +63,7 @@ void SharedWorkerReportingProxy::ReportConsoleMessage(
   // Not supported in SharedWorker.
 }
 
-void SharedWorkerReportingProxy::DidFetchScript() {
+void SharedWorkerReportingProxy::DidFetchScript(int64_t app_cache_id) {
   DCHECK(!IsMainThread());
   // TODO(nhiroki): Change the task type to kDOMManipulation here and elsewhere
   // in this file. See the HTML spec:
@@ -72,7 +72,7 @@ void SharedWorkerReportingProxy::DidFetchScript() {
       *parent_execution_context_task_runners_->Get(TaskType::kInternalDefault),
       FROM_HERE,
       CrossThreadBindOnce(&WebSharedWorkerImpl::DidFetchScript,
-                          CrossThreadUnretained(worker_)));
+                          CrossThreadUnretained(worker_), app_cache_id));
 }
 
 void SharedWorkerReportingProxy::DidFailToFetchClassicScript() {

third_party/blink/renderer/core/workers/shared_worker_reporting_proxy.h

@@ -35,7 +35,7 @@ class SharedWorkerReportingProxy final
                             mojom::ConsoleMessageLevel,
                             const String& message,
                             SourceLocation*) override;
-  void DidFetchScript() override;
+  void DidFetchScript(int64_t app_cache_id) override;
   void DidFailToFetchClassicScript() override;
   void DidFailToFetchModuleScript() override;
   void DidEvaluateClassicScript(bool success) override;

third_party/blink/renderer/core/workers/shared_worker_thread.cc

@@ -50,10 +50,23 @@ void SharedWorkerThread::ClearWorkerBackingThread() {
   worker_backing_thread_ = nullptr;
 }
 
+void SharedWorkerThread::OnAppCacheSelected() {
+  DCHECK(IsMainThread());
+  PostCrossThreadTask(
+      *GetTaskRunner(TaskType::kDOMManipulation), FROM_HERE,
+      CrossThreadBindOnce(&SharedWorkerThread::OnAppCacheSelectedOnWorkerThread,
+                          WTF::CrossThreadUnretained(this)));
+}
+
 WorkerOrWorkletGlobalScope* SharedWorkerThread::CreateWorkerGlobalScope(
     std::unique_ptr<GlobalScopeCreationParams> creation_params) {
   return SharedWorkerGlobalScope::Create(std::move(creation_params), this,
                                          time_origin_);
 }
 
+void SharedWorkerThread::OnAppCacheSelectedOnWorkerThread() {
+  DCHECK(IsCurrentThread());
+  To<SharedWorkerGlobalScope>(GlobalScope())->OnAppCacheSelected();
+}
+
 }  // namespace blink

third_party/blink/renderer/core/workers/shared_worker_thread.h

@@ -48,6 +48,8 @@ class CORE_EXPORT SharedWorkerThread : public WorkerThread {
   }
   void ClearWorkerBackingThread() override;
 
+  void OnAppCacheSelected();
+
  private:
   WorkerOrWorkletGlobalScope* CreateWorkerGlobalScope(
       std::unique_ptr<GlobalScopeCreationParams>) override;
@@ -56,6 +58,8 @@ class CORE_EXPORT SharedWorkerThread : public WorkerThread {
     return WebThreadType::kSharedWorkerThread;
   }
 
+  void OnAppCacheSelectedOnWorkerThread();
+
   std::unique_ptr<WorkerBackingThread> worker_backing_thread_;
 };
 

third_party/blink/renderer/core/workers/worker_module_tree_client.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/core/workers/worker_module_tree_client.h"
 
+#include "third_party/blink/public/mojom/appcache/appcache.mojom-blink.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_value.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/script/module_script.h"
@@ -32,7 +33,7 @@ void WorkerModuleTreeClient::NotifyModuleTreeLoadFinished(
     worker_reporting_proxy.DidFailToFetchModuleScript();
     return;
   }
-  worker_reporting_proxy.DidFetchScript();
+  worker_reporting_proxy.DidFetchScript(mojom::blink::kAppCacheNoCacheId);
 
   // Step 12: "Otherwise, continue the rest of these steps after the algorithm's
   // asynchronous completion, with script being the asynchronous completion

third_party/blink/renderer/core/workers/worker_reporting_proxy.h

@@ -86,7 +86,9 @@ class CORE_EXPORT WorkerReportingProxy {
   // Invoked on success to fetch the worker's main classic/module script from
   // network. This is not called when the script is loaded from
   // InstalledScriptsManager.
-  virtual void DidFetchScript() {}
+  // |app_cache_id| should be set correctly for shared workers.
+  // In non-shared-worker cases, |app_cache_id| is not used.
+  virtual void DidFetchScript(int64_t app_cache_id) {}
 
   // Invoked on failure to fetch the worker's classic script from network. This
   // is not called when the script is loaded from InstalledScriptsManager.

third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc

@@ -389,7 +389,8 @@ void ServiceWorkerGlobalScope::DidFetchClassicScript(
     ReportingProxy().DidFailToFetchClassicScript();
     return;
   }
-  ReportingProxy().DidFetchScript();
+  // The app cache ID is not used.
+  ReportingProxy().DidFetchScript(classic_script_loader->AppCacheID());
   probe::ScriptImported(this, classic_script_loader->Identifier(),
                         classic_script_loader->SourceText());
 

third_party/blink/renderer/modules/service_worker/service_worker_global_scope_proxy.cc

@@ -690,7 +690,7 @@ void ServiceWorkerGlobalScopeProxy::DidFailToLoadClassicScript() {
   Client().FailedToLoadClassicScript();
 }
 
-void ServiceWorkerGlobalScopeProxy::DidFetchScript() {
+void ServiceWorkerGlobalScopeProxy::DidFetchScript(int64_t /* app_cache_id */) {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
   Client().WorkerScriptLoadedOnWorkerThread();
 }

third_party/blink/renderer/modules/service_worker/service_worker_global_scope_proxy.h

@@ -167,7 +167,7 @@ class ServiceWorkerGlobalScopeProxy final
   void DidInitializeWorkerContext() override;
   void DidLoadClassicScript() override;
   void DidFailToLoadClassicScript() override;
-  void DidFetchScript() override;
+  void DidFetchScript(int64_t app_cache_id) override;
   void DidFailToFetchClassicScript() override;
   void DidFailToFetchModuleScript() override;
   void WillEvaluateClassicScript(size_t script_size,

third_party/blink/renderer/modules/service_worker/service_worker_module_tree_client.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/modules/service_worker/service_worker_module_tree_client.h"
 
+#include "third_party/blink/public/mojom/appcache/appcache.mojom-blink.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/script/module_script.h"
 #include "third_party/blink/renderer/core/workers/worker_global_scope.h"
@@ -38,7 +39,7 @@ void ServiceWorkerModuleTreeClient::NotifyModuleTreeLoadFinished(
     worker_global_scope->close();
     return;
   }
-  worker_reporting_proxy.DidFetchScript();
+  worker_reporting_proxy.DidFetchScript(mojom::blink::kAppCacheNoCacheId);
 
   // (In the update case) Step 9: "Else, continue the rest of these steps after
   // the algorithm's asynchronous completion, with script being the asynchronous

third_party/blink/web_tests/virtual/omt-worker-fetch/external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https-expected.txt

@@ -2,8 +2,8 @@ This is a testharness.js-based test.
 FAIL Dedicated worker of the cached script assert_equals: expected "Done: cached" but got "Error: Importing a non-cached script must fail."
 FAIL Dedicated worker of the fallbacked script assert_equals: expected "Done: fallbacked" but got "Error: Importing a non-cached script must fail."
 FAIL Dedicated worker of the not-in-cache script promise_test: Unhandled rejection with value: "The worker not in the AppCache must not be executed."
-FAIL Shared worker of the cached script assert_equals: expected "Done: cached" but got "Error: Importing a non-cached script must fail."
-FAIL Shared worker of the fallbacked script assert_equals: expected "Done: fallbacked" but got "Error: Importing a non-cached script must fail."
+PASS Shared worker of the cached script
+PASS Shared worker of the fallbacked script
 PASS Shared worker of the not-in-cache script
 Harness: the test ran to completion.