Move {Did,Will}Evaluate*Script() call out of Script::RunScriptOnWorker()

This CL moves
WorkerReportingProxy::{Will,Did}Evaluate{Classic,Module}Script()
calls from Script::RunScriptOnWorker()
to its call site in WorkerGlobalScope,
to ensure they are only called for worker top-level scripts.

While currently Script::RunScriptOnWorker() is only called
for worker top-level scripts, RunScriptOnWorker() will be called for
other scripts in subsequent CLs for crbug/1111134.

This CL also merges DidEvaluate{Classic,Module}Script()
into DidEvaluateTopLevelScript().

This CL doesn't change the behavior,
except for the trace in ServiceWorkerGlobalScopeProxy.
The trace is renamed and now added in module script cases.

Bug: 1111134
Change-Id: I5a05d56577f7f87bd62aff71ee63e57f1957255b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2327354
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794519}

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

@@ -125,14 +125,7 @@ void WebSharedWorkerImpl::DidFailToFetchModuleScript() {
   // DidTerminateWorkerThread() will be called asynchronously.
 }
 
-void WebSharedWorkerImpl::DidEvaluateClassicScript(bool success) {
-  DCHECK(IsMainThread());
-  DCHECK(!running_);
-  running_ = true;
-  DispatchPendingConnections();
-}
-
-void WebSharedWorkerImpl::DidEvaluateModuleScript(bool success) {
+void WebSharedWorkerImpl::DidEvaluateTopLevelScript(bool success) {
   DCHECK(IsMainThread());
   DCHECK(!running_);
   running_ = true;

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

@@ -81,8 +81,7 @@ class CORE_EXPORT WebSharedWorkerImpl final : public WebSharedWorker {
   void CountFeature(WebFeature);
   void DidFailToFetchClassicScript();
   void DidFailToFetchModuleScript();
-  void DidEvaluateClassicScript(bool success);
-  void DidEvaluateModuleScript(bool success);
+  void DidEvaluateTopLevelScript(bool success);
   void DidCloseWorkerGlobalScope();
   // This synchronously destroys |this|.
   void DidTerminateWorkerThread();

third_party/blink/renderer/core/script/classic_script.cc

@@ -53,21 +53,22 @@ v8::Local<v8::Value> ClassicScript::RunScriptInIsolatedWorldAndReturnValue(
       world_id, GetScriptSourceCode(), BaseURL(), sanitize_script_errors_);
 }
 
-void ClassicScript::RunScriptOnWorker(WorkerGlobalScope& worker_global_scope) {
+bool ClassicScript::RunScriptOnWorker(WorkerGlobalScope& worker_global_scope) {
   DCHECK(worker_global_scope.IsContextThread());
 
-  WorkerReportingProxy& worker_reporting_proxy =
-      worker_global_scope.ReportingProxy();
-
-  worker_reporting_proxy.WillEvaluateClassicScript(
-      GetScriptSourceCode().Source().length(),
-      GetScriptSourceCode().CacheHandler()
-          ? GetScriptSourceCode().CacheHandler()->GetCodeCacheSize()
-          : 0);
   bool success = worker_global_scope.ScriptController()->Evaluate(
       GetScriptSourceCode(), sanitize_script_errors_, nullptr /* error_event */,
       worker_global_scope.GetV8CacheOptions());
-  worker_reporting_proxy.DidEvaluateClassicScript(success);
+  return success;
+}
+
+std::pair<size_t, size_t> ClassicScript::GetClassicScriptSizes() const {
+  size_t cached_metadata_size =
+      GetScriptSourceCode().CacheHandler()
+          ? GetScriptSourceCode().CacheHandler()->GetCodeCacheSize()
+          : 0;
+  return std::pair<size_t, size_t>(GetScriptSourceCode().Source().length(),
+                                   cached_metadata_size);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/script/classic_script.h

@@ -50,7 +50,7 @@ class CORE_EXPORT ClassicScript final : public Script {
   // a tentative interface. When crbug/1111134 is done, this should be gone.
   void RunScript(LocalFrame*) override;
   void RunScript(LocalFrame*, ScriptController::ExecuteScriptPolicy);
-  void RunScriptOnWorker(WorkerGlobalScope&) override;
+  bool RunScriptOnWorker(WorkerGlobalScope&) override;
 
   // Unlike RunScript() and RunScriptOnWorker(), callers of the following
   // methods must enter a v8::HandleScope before calling.
@@ -65,6 +65,9 @@ class CORE_EXPORT ClassicScript final : public Script {
   mojom::ScriptType GetScriptType() const override {
     return mojom::ScriptType::kClassic;
   }
+
+  std::pair<size_t, size_t> GetClassicScriptSizes() const override;
+
   const ScriptSourceCode script_source_code_;
   const SanitizeScriptErrors sanitize_script_errors_;
 };

third_party/blink/renderer/core/script/module_script.cc

@@ -111,22 +111,23 @@ void ModuleScript::RunScript(LocalFrame* frame) {
                                   Modulator::CaptureEvalErrorFlag::kReport);
 }
 
-void ModuleScript::RunScriptOnWorker(WorkerGlobalScope& worker_global_scope) {
+bool ModuleScript::RunScriptOnWorker(WorkerGlobalScope& worker_global_scope) {
   // We need a HandleScope for the ModuleEvaluationResult that is created
   // in ::ExecuteModule(...).
   ScriptState::Scope scope(SettingsObject()->GetScriptState());
   DCHECK(worker_global_scope.IsContextThread());
 
-  WorkerReportingProxy& worker_reporting_proxy =
-      worker_global_scope.ReportingProxy();
-
-  worker_reporting_proxy.WillEvaluateModuleScript();
   // This |error| is always null because the second argument is |kReport|.
   // TODO(nhiroki): Catch an error when an evaluation error happens.
   // (https://crbug.com/680046)
   ModuleEvaluationResult result = SettingsObject()->ExecuteModule(
       this, Modulator::CaptureEvalErrorFlag::kReport);
-  worker_reporting_proxy.DidEvaluateModuleScript(result.IsSuccess());
+  return result.IsSuccess();
+}
+
+std::pair<size_t, size_t> ModuleScript::GetClassicScriptSizes() const {
+  NOTREACHED();
+  return std::pair<size_t, size_t>(0, 0);
 }
 
 std::ostream& operator<<(std::ostream& stream,

third_party/blink/renderer/core/script/module_script.h

@@ -68,7 +68,9 @@ class CORE_EXPORT ModuleScript : public Script {
     return mojom::ScriptType::kModule;
   }
   void RunScript(LocalFrame*) override;
-  void RunScriptOnWorker(WorkerGlobalScope&) override;
+  bool RunScriptOnWorker(WorkerGlobalScope&) override;
+
+  std::pair<size_t, size_t> GetClassicScriptSizes() const override;
 
   friend class ModuleTreeLinkerTestModulator;
 

third_party/blink/renderer/core/script/script.h

@@ -34,12 +34,20 @@ class CORE_EXPORT Script : public GarbageCollected<Script> {
   // https://html.spec.whatwg.org/C/#run-a-module-script,
   // depending on the script type,
   // on Window or on WorkerGlobalScope, respectively.
+  // RunScriptOnWorker returns true if evaluated successfully.
   virtual void RunScript(LocalFrame*) = 0;
-  virtual void RunScriptOnWorker(WorkerGlobalScope&) = 0;
+  virtual bool RunScriptOnWorker(WorkerGlobalScope&) = 0;
 
   const ScriptFetchOptions& FetchOptions() const { return fetch_options_; }
   const KURL& BaseURL() const { return base_url_; }
 
+  // Returns a pair of (script's size, cached metadata's size) only for classic
+  // scripts. This is used only for metrics via
+  // ServiceWorkerGlobalScopeProxy::WillEvaluateClassicScript().
+  // TODO(asamidoi, hiroshige): Remove this once the metrics are no longer
+  // referenced.
+  virtual std::pair<size_t, size_t> GetClassicScriptSizes() const = 0;
+
  protected:
   explicit Script(const ScriptFetchOptions& fetch_options, const KURL& base_url)
       : fetch_options_(fetch_options), base_url_(base_url) {}

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

@@ -109,15 +109,7 @@ void DedicatedWorkerObjectProxy::DidFailToFetchModuleScript() {
                           messaging_proxy_weak_ptr_));
 }
 
-void DedicatedWorkerObjectProxy::DidEvaluateClassicScript(bool success) {
-  PostCrossThreadTask(
-      *GetParentExecutionContextTaskRunners()->Get(TaskType::kInternalDefault),
-      FROM_HERE,
-      CrossThreadBindOnce(&DedicatedWorkerMessagingProxy::DidEvaluateScript,
-                          messaging_proxy_weak_ptr_, success));
-}
-
-void DedicatedWorkerObjectProxy::DidEvaluateModuleScript(bool success) {
+void DedicatedWorkerObjectProxy::DidEvaluateTopLevelScript(bool success) {
   PostCrossThreadTask(
       *GetParentExecutionContextTaskRunners()->Get(TaskType::kInternalDefault),
       FROM_HERE,

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

@@ -70,8 +70,7 @@ class CORE_EXPORT DedicatedWorkerObjectProxy : public ThreadedObjectProxyBase {
                        int exception_id) override;
   void DidFailToFetchClassicScript() final;
   void DidFailToFetchModuleScript() final;
-  void DidEvaluateClassicScript(bool success) override;
-  void DidEvaluateModuleScript(bool success) override;
+  void DidEvaluateTopLevelScript(bool success) override;
 
   const DedicatedWorkerToken& token() const { return token_; }
 

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

@@ -73,21 +73,12 @@ void SharedWorkerReportingProxy::DidFailToFetchModuleScript() {
                           CrossThreadUnretained(worker_)));
 }
 
-void SharedWorkerReportingProxy::DidEvaluateClassicScript(bool success) {
+void SharedWorkerReportingProxy::DidEvaluateTopLevelScript(bool success) {
   DCHECK(!IsMainThread());
   PostCrossThreadTask(
       *parent_execution_context_task_runners_->Get(TaskType::kInternalDefault),
       FROM_HERE,
-      CrossThreadBindOnce(&WebSharedWorkerImpl::DidEvaluateClassicScript,
-                          CrossThreadUnretained(worker_), success));
-}
-
-void SharedWorkerReportingProxy::DidEvaluateModuleScript(bool success) {
-  DCHECK(!IsMainThread());
-  PostCrossThreadTask(
-      *parent_execution_context_task_runners_->Get(TaskType::kInternalDefault),
-      FROM_HERE,
-      CrossThreadBindOnce(&WebSharedWorkerImpl::DidEvaluateModuleScript,
+      CrossThreadBindOnce(&WebSharedWorkerImpl::DidEvaluateTopLevelScript,
                           CrossThreadUnretained(worker_), success));
 }
 

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

@@ -36,8 +36,7 @@ class SharedWorkerReportingProxy final
                             SourceLocation*) override;
   void DidFailToFetchClassicScript() override;
   void DidFailToFetchModuleScript() override;
-  void DidEvaluateClassicScript(bool success) override;
-  void DidEvaluateModuleScript(bool success) override;
+  void DidEvaluateTopLevelScript(bool success) override;
   void DidCloseWorkerGlobalScope() override;
   void WillDestroyWorkerGlobalScope() override {}
   void DidTerminateWorkerThread() override;

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

@@ -443,9 +443,22 @@ void WorkerGlobalScope::RunWorkerScript() {
   if (debugger && stack_id_)
     debugger->ExternalAsyncTaskStarted(*stack_id_);
 
+  switch (worker_script_->GetScriptType()) {
+    case mojom::blink::ScriptType::kClassic: {
+      auto sizes = worker_script_->GetClassicScriptSizes();
+      ReportingProxy().WillEvaluateClassicScript(sizes.first, sizes.second);
+      break;
+    }
+    case mojom::blink::ScriptType::kModule:
+      ReportingProxy().WillEvaluateModuleScript();
+      break;
+  }
+
   // Step 24. If script is a classic script, then run the classic script script.
   // Otherwise, it is a module script; run the module script script. [spec text]
-  std::move(worker_script_)->RunScriptOnWorker(*this);
+  bool is_success = std::move(worker_script_)->RunScriptOnWorker(*this);
+
+  ReportingProxy().DidEvaluateTopLevelScript(is_success);
 
   if (debugger && stack_id_)
     debugger->ExternalAsyncTaskFinished(*stack_id_);

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

@@ -97,13 +97,9 @@ class CORE_EXPORT WorkerReportingProxy {
   // Invoked when the worker's main module script is about to be evaluated.
   virtual void WillEvaluateModuleScript() {}
 
-  // Invoked when the main classic script is evaluated. |success| is true if the
+  // Invoked when the worker main script is evaluated. |success| is true if the
   // evaluation completed with no uncaught exception.
-  virtual void DidEvaluateClassicScript(bool success) {}
-
-  // Invoked when the worker's main module script is evaluated. |success| is
-  // true if the evaluation completed with no uncaught exception.
-  virtual void DidEvaluateModuleScript(bool success) {}
+  virtual void DidEvaluateTopLevelScript(bool success) {}
 
   // Invoked when close() is invoked on the worker context.
   virtual void DidCloseWorkerGlobalScope() {}

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

@@ -103,7 +103,7 @@ void CreateNestedWorkerThenTerminateParent(
               WillEvaluateClassicScriptMock(_, _))
       .Times(1);
   EXPECT_CALL(*nested_worker_helper->reporting_proxy,
-              DidEvaluateClassicScript(true))
+              DidEvaluateTopLevelScript(true))
       .Times(1);
   EXPECT_CALL(*nested_worker_helper->reporting_proxy,
               WillDestroyWorkerGlobalScope())
@@ -191,7 +191,7 @@ class WorkerThreadTest : public testing::Test {
     EXPECT_CALL(*reporting_proxy_, DidCreateWorkerGlobalScope(_)).Times(1);
     EXPECT_CALL(*reporting_proxy_, WillEvaluateClassicScriptMock(_, _))
         .Times(1);
-    EXPECT_CALL(*reporting_proxy_, DidEvaluateClassicScript(true)).Times(1);
+    EXPECT_CALL(*reporting_proxy_, DidEvaluateTopLevelScript(true)).Times(1);
     EXPECT_CALL(*reporting_proxy_, WillDestroyWorkerGlobalScope()).Times(1);
     EXPECT_CALL(*reporting_proxy_, DidTerminateWorkerThread()).Times(1);
   }
@@ -200,7 +200,7 @@ class WorkerThreadTest : public testing::Test {
     EXPECT_CALL(*reporting_proxy_, DidCreateWorkerGlobalScope(_)).Times(1);
     EXPECT_CALL(*reporting_proxy_, WillEvaluateClassicScriptMock(_, _))
         .Times(AtMost(1));
-    EXPECT_CALL(*reporting_proxy_, DidEvaluateClassicScript(_))
+    EXPECT_CALL(*reporting_proxy_, DidEvaluateTopLevelScript(_))
         .Times(AtMost(1));
     EXPECT_CALL(*reporting_proxy_, WillDestroyWorkerGlobalScope())
         .Times(AtMost(1));
@@ -211,7 +211,7 @@ class WorkerThreadTest : public testing::Test {
     EXPECT_CALL(*reporting_proxy_, DidCreateWorkerGlobalScope(_)).Times(1);
     EXPECT_CALL(*reporting_proxy_, WillEvaluateClassicScriptMock(_, _))
         .Times(1);
-    EXPECT_CALL(*reporting_proxy_, DidEvaluateClassicScript(false)).Times(1);
+    EXPECT_CALL(*reporting_proxy_, DidEvaluateTopLevelScript(false)).Times(1);
     EXPECT_CALL(*reporting_proxy_, WillDestroyWorkerGlobalScope()).Times(1);
     EXPECT_CALL(*reporting_proxy_, DidTerminateWorkerThread()).Times(1);
   }

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

@@ -182,7 +182,7 @@ class MockWorkerReportingProxy final : public WorkerReportingProxy {
   MOCK_METHOD1(DidCreateWorkerGlobalScope, void(WorkerOrWorkletGlobalScope*));
   MOCK_METHOD2(WillEvaluateClassicScriptMock,
                void(size_t scriptSize, size_t cachedMetadataSize));
-  MOCK_METHOD1(DidEvaluateClassicScript, void(bool success));
+  MOCK_METHOD1(DidEvaluateTopLevelScript, void(bool success));
   MOCK_METHOD0(DidCloseWorkerGlobalScope, void());
   MOCK_METHOD0(WillDestroyWorkerGlobalScope, void());
   MOCK_METHOD0(DidTerminateWorkerThread, void());

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

@@ -78,7 +78,7 @@ void WorkletModuleTreeClient::NotifyModuleTreeLoadFinished(
   auto* global_scope =
       To<WorkletGlobalScope>(ExecutionContext::From(script_state_));
 
-  global_scope->ReportingProxy().DidEvaluateModuleScript(result.IsSuccess());
+  global_scope->ReportingProxy().DidEvaluateTopLevelScript(result.IsSuccess());
 
   // Step 6: "Queue a task on outsideSettings's responsible event loop to run
   // these steps:"

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

@@ -175,7 +175,7 @@ void ServiceWorkerGlobalScopeProxy::WillEvaluateClassicScript(
     size_t cached_metadata_size) {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(
-      "ServiceWorker", "ServiceWorkerGlobalScopeProxy::EvaluateClassicScript",
+      "ServiceWorker", "ServiceWorkerGlobalScopeProxy::EvaluateTopLevelScript",
       TRACE_ID_LOCAL(this));
   // TODO(asamidoi): Remove CountWorkerScript which is called for recording
   // metrics if the metrics are no longer referenced, and then merge
@@ -197,27 +197,24 @@ void ServiceWorkerGlobalScopeProxy::WillEvaluateImportedClassicScript(
 
 void ServiceWorkerGlobalScopeProxy::WillEvaluateModuleScript() {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
+  TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(
+      "ServiceWorker", "ServiceWorkerGlobalScopeProxy::EvaluateTopLevelScript",
+      TRACE_ID_LOCAL(this));
   ScriptState::Scope scope(
       WorkerGlobalScope()->ScriptController()->GetScriptState());
   Client().WillEvaluateScript(
       WorkerGlobalScope()->ScriptController()->GetContext());
 }
 
-void ServiceWorkerGlobalScopeProxy::DidEvaluateClassicScript(bool success) {
+void ServiceWorkerGlobalScopeProxy::DidEvaluateTopLevelScript(bool success) {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
   WorkerGlobalScope()->DidEvaluateScript();
   Client().DidEvaluateScript(success);
   TRACE_EVENT_NESTABLE_ASYNC_END1(
-      "ServiceWorker", "ServiceWorkerGlobalScopeProxy::EvaluateClassicScript",
+      "ServiceWorker", "ServiceWorkerGlobalScopeProxy::EvaluateTopLevelScript",
       TRACE_ID_LOCAL(this), "success", success);
 }
 
-void ServiceWorkerGlobalScopeProxy::DidEvaluateModuleScript(bool success) {
-  DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
-  WorkerGlobalScope()->DidEvaluateScript();
-  Client().DidEvaluateScript(success);
-}
-
 void ServiceWorkerGlobalScopeProxy::DidCloseWorkerGlobalScope() {
   DCHECK_CALLED_ON_VALID_THREAD(worker_thread_checker_);
   // close() is not web-exposed for ServiceWorker. This is called when

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

@@ -121,8 +121,7 @@ class ServiceWorkerGlobalScopeProxy final : public WebServiceWorkerContextProxy,
   void WillEvaluateImportedClassicScript(size_t script_size,
                                          size_t cached_metadata_size) override;
   void WillEvaluateModuleScript() override;
-  void DidEvaluateClassicScript(bool success) override;
-  void DidEvaluateModuleScript(bool success) override;
+  void DidEvaluateTopLevelScript(bool success) override;
   void DidCloseWorkerGlobalScope() override;
   void WillDestroyWorkerGlobalScope() override;
   void DidTerminateWorkerThread() override;

third_party/blink/renderer/modules/webaudio/audio_worklet_object_proxy.cc

@@ -28,7 +28,7 @@ void AudioWorkletObjectProxy::DidCreateWorkerGlobalScope(
   global_scope_->SetSampleRate(context_sample_rate_);
 }
 
-void AudioWorkletObjectProxy::DidEvaluateModuleScript(bool success) {
+void AudioWorkletObjectProxy::DidEvaluateTopLevelScript(bool success) {
   DCHECK(global_scope_);
 
   if (!success || global_scope_->NumberOfRegisteredDefinitions() == 0)

third_party/blink/renderer/modules/webaudio/audio_worklet_object_proxy.h

@@ -21,7 +21,7 @@ class AudioWorkletObjectProxy final
 
   // Implements WorkerReportingProxy.
   void DidCreateWorkerGlobalScope(WorkerOrWorkletGlobalScope*) override;
-  void DidEvaluateModuleScript(bool success) override;
+  void DidEvaluateTopLevelScript(bool success) override;
   void WillDestroyWorkerGlobalScope() override;
 
  private: