service worker: Merge DidInitializeWorkerContext() into WillEvaluateScript()

This CL:
* removes one not-well-documented method from WebServiceWorkerContextClient
* is a preparation for a potential fix of race condition in service worker
  extension

The context of the second bullet is a bit complicated. Service worker
background extension has following ordering assumption at initialization:
1. ExtensionMsg_Loaded is received on renderer
   extensions::Dispatcher::OnLoaded()
2. Installing extension API bindings
   extensions::Dispatcher::DidInitializeServiceWorkerContextOnWorkerThread()

However there is no such guarantees. The current implementation
happens to be working just because creating EmbeddedWorkerInstanceClient
is bound to RenderThreadImpl and StartWorker() mojo message is handled
after ExtensionMsg_Loaded.

To make sure the above ordering we need to wait for the loaded message
before installing extension API bindings. DidInitializeWorkerContext()
isn't a right place to install extension API bindings because it is called
via blink::WorkerThread::InitializeOnWorkerThread(), which doesn't wait
for anything after StartWorker() mojo message is received.

Fortunately we have blink::WorkerGlobalScope::ReadyToRunScript(). This
was introduced for service worker update to suspend service worker
execution after script fetch. We could probably use this to wait for
the extension loaded IPC message. ReadyToRunScript() calls
WillEvaluateScript() before executing the script so this would be a
better place to install extension API bindings.

Bug: 995134
Change-Id: I54d44391149225c5c52294bcca32a86705a9c0a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1775856
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692997}

chrome/renderer/chrome_content_renderer_client.cc

@@ -1507,17 +1507,16 @@ void ChromeContentRendererClient::
       metrics::CallStackProfileParams::SERVICE_WORKER_THREAD);
 }
 
-void ChromeContentRendererClient::
-    DidInitializeServiceWorkerContextOnWorkerThread(
-        blink::WebServiceWorkerContextProxy* context_proxy,
-        v8::Local<v8::Context> v8_context,
-        int64_t service_worker_version_id,
-        const GURL& service_worker_scope,
-        const GURL& script_url) {
+void ChromeContentRendererClient::WillEvaluateServiceWorkerOnWorkerThread(
+    blink::WebServiceWorkerContextProxy* context_proxy,
+    v8::Local<v8::Context> v8_context,
+    int64_t service_worker_version_id,
+    const GURL& service_worker_scope,
+    const GURL& script_url) {
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   ChromeExtensionsRendererClient::GetInstance()
       ->extension_dispatcher()
-      ->DidInitializeServiceWorkerContextOnWorkerThread(
+      ->WillEvaluateServiceWorkerOnWorkerThread(
           context_proxy, v8_context, service_worker_version_id,
           service_worker_scope, script_url);
 #endif

chrome/renderer/chrome_content_renderer_client.h

@@ -181,7 +181,7 @@ class ChromeContentRendererClient
   void RunScriptsAtDocumentIdle(content::RenderFrame* render_frame) override;
   void SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() override;
   void WillInitializeServiceWorkerContextOnWorkerThread() override;
-  void DidInitializeServiceWorkerContextOnWorkerThread(
+  void WillEvaluateServiceWorkerOnWorkerThread(
       blink::WebServiceWorkerContextProxy* context_proxy,
       v8::Local<v8::Context> v8_context,
       int64_t service_worker_version_id,

content/public/renderer/content_renderer_client.h

@@ -327,11 +327,11 @@ class CONTENT_EXPORT ContentRendererClient {
   // function is called from the worker thread.
   virtual void WillInitializeServiceWorkerContextOnWorkerThread() {}
 
-  // Notifies that a service worker context has been created. This function
-  // is called from the worker thread.
+  // Notifies that the main script of a service worker is about to evaluate.
+  // This function is called from the worker thread.
   // |context_proxy| is valid until
   // WillDestroyServiceWorkerContextOnWorkerThread() is called.
-  virtual void DidInitializeServiceWorkerContextOnWorkerThread(
+  virtual void WillEvaluateServiceWorkerOnWorkerThread(
       blink::WebServiceWorkerContextProxy* context_proxy,
       v8::Local<v8::Context> v8_context,
       int64_t service_worker_version_id,

content/renderer/service_worker/service_worker_context_client.cc

@@ -260,7 +260,8 @@ void ServiceWorkerContextClient::WorkerContextStarted(
   proxy_->BindControllerServiceWorker(controller_receiver_.PassPipe());
 }
 
-void ServiceWorkerContextClient::WillEvaluateScript() {
+void ServiceWorkerContextClient::WillEvaluateScript(
+    v8::Local<v8::Context> v8_context) {
   DCHECK(worker_task_runner_->RunsTasksInCurrentSequence());
   start_timing_->script_evaluation_start_time = base::TimeTicks::Now();
 
@@ -275,6 +276,11 @@ void ServiceWorkerContextClient::WillEvaluateScript() {
            start_timing_->script_evaluation_start_time);
 
   instance_host_->OnScriptEvaluationStart();
+
+  DCHECK(proxy_);
+  GetContentClient()->renderer()->WillEvaluateServiceWorkerOnWorkerThread(
+      proxy_, v8_context, service_worker_version_id_, service_worker_scope_,
+      script_url_);
 }
 
 void ServiceWorkerContextClient::DidEvaluateScript(bool success) {
@@ -310,17 +316,6 @@ void ServiceWorkerContextClient::WillInitializeWorkerContext() {
       ->WillInitializeServiceWorkerContextOnWorkerThread();
 }
 
-void ServiceWorkerContextClient::DidInitializeWorkerContext(
-    blink::WebServiceWorkerContextProxy* context_proxy,
-    v8::Local<v8::Context> v8_context) {
-  DCHECK(worker_task_runner_->RunsTasksInCurrentSequence());
-  GetContentClient()
-      ->renderer()
-      ->DidInitializeServiceWorkerContextOnWorkerThread(
-          context_proxy, v8_context, service_worker_version_id_,
-          service_worker_scope_, script_url_);
-}
-
 void ServiceWorkerContextClient::WillDestroyWorkerContext(
     v8::Local<v8::Context> context) {
   DCHECK(worker_task_runner_->RunsTasksInCurrentSequence());

content/renderer/service_worker/service_worker_context_client.h

@@ -128,12 +128,9 @@ class CONTENT_EXPORT ServiceWorkerContextClient
   void WorkerContextStarted(
       blink::WebServiceWorkerContextProxy* proxy,
       scoped_refptr<base::SequencedTaskRunner> worker_task_runner) override;
-  void WillEvaluateScript() override;
+  void WillEvaluateScript(v8::Local<v8::Context> v8_context) override;
   void DidEvaluateScript(bool success) override;
   void WillInitializeWorkerContext() override;
-  void DidInitializeWorkerContext(
-      blink::WebServiceWorkerContextProxy* context_proxy,
-      v8::Local<v8::Context> v8_context) override;
   void WillDestroyWorkerContext(v8::Local<v8::Context> context) override;
   void WorkerContextDestroyed() override;
   void CountFeature(blink::mojom::WebFeature feature) override;

extensions/renderer/dispatcher.cc

@@ -322,7 +322,7 @@ void Dispatcher::DidCreateScriptContext(
       break;
     case Feature::BLESSED_EXTENSION_CONTEXT:
       // For service workers this is handled in
-      // DidInitializeServiceWorkerContextOnWorkerThread().
+      // WillEvaluateServiceWorkerOnWorkerThread().
       DCHECK(!context->IsForServiceWorker());
       UMA_HISTOGRAM_TIMES("Extensions.DidCreateScriptContext_Blessed", elapsed);
       break;
@@ -353,7 +353,7 @@ void Dispatcher::DidCreateScriptContext(
   VLOG(1) << "Num tracked contexts: " << script_context_set_->size();
 }
 
-void Dispatcher::DidInitializeServiceWorkerContextOnWorkerThread(
+void Dispatcher::WillEvaluateServiceWorkerOnWorkerThread(
     blink::WebServiceWorkerContextProxy* context_proxy,
     v8::Local<v8::Context> v8_context,
     int64_t service_worker_version_id,

extensions/renderer/dispatcher.h

@@ -107,7 +107,7 @@ class Dispatcher : public content::RenderThreadObserver,
 
   // Runs on a different thread and should only use thread safe member
   // variables.
-  void DidInitializeServiceWorkerContextOnWorkerThread(
+  void WillEvaluateServiceWorkerOnWorkerThread(
       blink::WebServiceWorkerContextProxy* context_proxy,
       v8::Local<v8::Context> v8_context,
       int64_t service_worker_version_id,

extensions/renderer/resources/service_worker_bindings.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// This function is returned to DidInitializeServiceWorkerContextOnWorkerThread
+// This function is returned to WillEvaluateServiceWorkerOnWorkerThread
 // then executed, passing in dependencies as function arguments.
 //
 // |backgroundUrl| is the URL of the extension's background page.

third_party/blink/public/web/modules/service_worker/web_service_worker_context_client.h

@@ -129,7 +129,10 @@ class WebServiceWorkerContextClient {
   // This means all setup is finally complete: the script has been loaded, the
   // worker thread has started, the script has been passed to the worker thread,
   // and CSP and ReferrerPolicy information has been set on the worker thread.
-  virtual void WillEvaluateScript() {}
+  //
+  // |v8_context| is the V8 context of the worker and is used to support
+  // service workers in Chrome extensions.
+  virtual void WillEvaluateScript(v8::Local<v8::Context> v8_context) {}
 
   // Called when initial script evaluation finished for the main script.
   // |success| is true if the evaluation completed with no uncaught exception.
@@ -139,23 +142,6 @@ class WebServiceWorkerContextClient {
   // initial method call after creating the worker scheduler.
   virtual void WillInitializeWorkerContext() {}
 
-  // Called when the worker context is initialized. This is probably called
-  // after WorkerContextStarted(). (WorkerThread::InitializeOnWorkerThread()
-  // calls WorkerContextStarted() via
-  // WorkerReportingProxy::DidCreateWorkerGlobalScope(),
-  // and then initializes the worker context if "needed" and calls
-  // DidInitializeWorkerContext(), but it's not clear when the context would
-  // already be initialized.)
-  //
-  // |context_proxy| is valid until WillDestroyWorkerContext() is called.
-  //
-  // This function is used to support service workers in Chrome extensions.
-  //
-  // TODO(nhiroki): Can you clarify this code and comment?
-  virtual void DidInitializeWorkerContext(
-      WebServiceWorkerContextProxy* context_proxy,
-      v8::Local<v8::Context> v8_context) {}
-
   // WorkerGlobalScope is about to be destroyed. The client should clear
   // the WebServiceWorkerGlobalScopeProxy when this is called.
   virtual void WillDestroyWorkerContext(v8::Local<v8::Context> context) {}

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

@@ -164,10 +164,6 @@ void ServiceWorkerGlobalScopeProxy::DidCreateWorkerGlobalScope(
 
 void ServiceWorkerGlobalScopeProxy::DidInitializeWorkerContext() {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
-  ScriptState::Scope scope(
-      WorkerGlobalScope()->ScriptController()->GetScriptState());
-  Client().DidInitializeWorkerContext(
-      this, WorkerGlobalScope()->ScriptController()->GetContext());
   TRACE_EVENT_END0("ServiceWorker",
                    "ServiceWorkerGlobalScopeProxy::InitializeWorkerContext");
 }
@@ -211,7 +207,11 @@ void ServiceWorkerGlobalScopeProxy::WillEvaluateClassicScript(
   // metrics if the metrics are no longer referenced, and then merge
   // WillEvaluateClassicScript and WillEvaluateModuleScript for cleanup.
   worker_global_scope_->CountWorkerScript(script_size, cached_metadata_size);
-  Client().WillEvaluateScript();
+
+  ScriptState::Scope scope(
+      WorkerGlobalScope()->ScriptController()->GetScriptState());
+  Client().WillEvaluateScript(
+      WorkerGlobalScope()->ScriptController()->GetContext());
 }
 
 void ServiceWorkerGlobalScopeProxy::WillEvaluateImportedClassicScript(
@@ -223,7 +223,10 @@ void ServiceWorkerGlobalScopeProxy::WillEvaluateImportedClassicScript(
 
 void ServiceWorkerGlobalScopeProxy::WillEvaluateModuleScript() {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
-  Client().WillEvaluateScript();
+  ScriptState::Scope scope(
+      WorkerGlobalScope()->ScriptController()->GetScriptState());
+  Client().WillEvaluateScript(
+      WorkerGlobalScope()->ScriptController()->GetContext());
 }
 
 void ServiceWorkerGlobalScopeProxy::DidEvaluateClassicScript(bool success) {