Introduce WorkerOrWorkletScriptController::EvaluateAndReturnValue()

To unify WorkerOrWorkletScriptController entry point, this CL

- Renames WorkerOrWorkletScriptController::Evaluate() to
  EvaluateAndReturnValue(),
- Makes it (and EvaluateInternal()) return v8::Local<v8::Value>,
- Moves ScriptState::Scope to callers of
  EvaluateAndReturnValue(), and
- Merges EvaluateAndReturnValueForTest() to
  EvaluateAndReturnValue().

This CL doesn't change the non-test behavior.

This CL changes EvaluateAndReturnValue() behavior in tests:
Previously, for
EvaluateAndReturnValueForTest(ScriptSourceCode("someJavaScriptValue")),
when `someJavaScriptValue` is evaluated to `undefined`,
EvaluateAndReturnValueForTest() returned an empty ScriptValue().
After this CL, EvaluateAndReturnValue() returns a
v8::Local<v8::Value> representing `undefined`, not
an empty v8::Local<v8::Value>.
However, this doesn't affect observable test behavior
(`undefined` is not expected anyway in existing tests).

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

third_party/blink/renderer/bindings/core/v8/scheduled_action.cc

@@ -182,7 +182,8 @@ void ScheduledAction::Execute(WorkerGlobalScope* worker) {
     // behavior, but this causes failures on
     // wpt/html/webappapis/scripting/processing-model-2/compile-error-cross-origin-setTimeout.html
     // and friends.
-    worker->ScriptController()->Evaluate(
+    ScriptState::Scope scope(worker->ScriptController()->GetScriptState());
+    worker->ScriptController()->EvaluateAndReturnValue(
         ScriptSourceCode(code_,
                          ScriptSourceLocationType::kEvalForScheduledAction),
         SanitizeScriptErrors::kDoNotSanitize);

third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc

@@ -342,7 +342,7 @@ void WorkerOrWorkletScriptController::DisableEvalInternal(
       V8String(isolate_, error_message));
 }
 
-ScriptValue WorkerOrWorkletScriptController::EvaluateInternal(
+v8::Local<v8::Value> WorkerOrWorkletScriptController::EvaluateInternal(
     const ScriptSourceCode& source_code,
     SanitizeScriptErrors sanitize_script_errors,
     V8CacheOptions v8_cache_options) {
@@ -353,8 +353,6 @@ ScriptValue WorkerOrWorkletScriptController::EvaluateInternal(
                inspector_evaluate_script_event::Data(
                    nullptr, source_code.Url(), source_code.StartPosition()));
 
-  ScriptState::Scope scope(script_state_);
-
   v8::TryCatch block(isolate_);
 
   v8::Local<v8::Script> compiled_script;
@@ -380,7 +378,7 @@ ScriptValue WorkerOrWorkletScriptController::EvaluateInternal(
 
   if (!block.CanContinue()) {
     ForbidExecution();
-    return ScriptValue();
+    return v8::Local<v8::Value>();
   }
 
   if (block.HasCaught()) {
@@ -397,33 +395,33 @@ ScriptValue WorkerOrWorkletScriptController::EvaluateInternal(
   }
 
   v8::Local<v8::Value> result;
-  if (!maybe_result.ToLocal(&result) || result->IsUndefined())
-    return ScriptValue();
+  if (!maybe_result.ToLocal(&result))
+    return v8::Local<v8::Value>();
 
-  return ScriptValue(script_state_->GetIsolate(), result);
+  return result;
 }
 
-bool WorkerOrWorkletScriptController::Evaluate(
+v8::Local<v8::Value> WorkerOrWorkletScriptController::EvaluateAndReturnValue(
     const ScriptSourceCode& source_code,
     SanitizeScriptErrors sanitize_script_errors,
     ErrorEvent** error_event,
     V8CacheOptions v8_cache_options) {
   if (IsExecutionForbidden())
-    return false;
+    return v8::Local<v8::Value>();
 
   ExecutionState state(this);
-  EvaluateInternal(source_code, sanitize_script_errors, v8_cache_options);
+  v8::Local<v8::Value> result =
+      EvaluateInternal(source_code, sanitize_script_errors, v8_cache_options);
   if (IsExecutionForbidden())
-    return false;
+    return v8::Local<v8::Value>();
 
-  ScriptState::Scope scope(script_state_);
   if (state.had_exception) {
     if (error_event) {
       if (state.error_event_from_imported_script_) {
         // Propagate inner error event outwards.
         *error_event = state.error_event_from_imported_script_;
         state.error_event_from_imported_script_ = nullptr;
-        return false;
+        return v8::Local<v8::Value>();
       }
       if (sanitize_script_errors == SanitizeScriptErrors::kSanitize) {
         *error_event = ErrorEvent::CreateSanitizedError(script_state_);
@@ -444,16 +442,9 @@ bool WorkerOrWorkletScriptController::Evaluate(
       }
       global_scope_->DispatchErrorEvent(event, sanitize_script_errors);
     }
-    return false;
+    return v8::Local<v8::Value>();
   }
-  return true;
-}
-
-ScriptValue WorkerOrWorkletScriptController::EvaluateAndReturnValueForTest(
-    const ScriptSourceCode& source_code) {
-  ExecutionState state(this);
-  return EvaluateInternal(source_code, SanitizeScriptErrors::kSanitize,
-                          kV8CacheOptionsDefault);
+  return result;
 }
 
 void WorkerOrWorkletScriptController::ForbidExecution() {

third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.h

@@ -58,11 +58,13 @@ class CORE_EXPORT WorkerOrWorkletScriptController final
 
   bool IsExecutionForbidden() const;
 
-  // Returns true if the evaluation completed with no uncaught exception.
-  bool Evaluate(const ScriptSourceCode&,
-                SanitizeScriptErrors sanitize_script_errors,
-                ErrorEvent** = nullptr,
-                V8CacheOptions = kV8CacheOptionsDefault);
+  // Returns a non-Empty value if the evaluation completed with no uncaught
+  // exception. Callers should enter ScriptState::Scope before calling this.
+  v8::Local<v8::Value> EvaluateAndReturnValue(
+      const ScriptSourceCode&,
+      SanitizeScriptErrors sanitize_script_errors,
+      ErrorEvent** = nullptr,
+      V8CacheOptions = kV8CacheOptionsDefault);
 
   // Prevents future JavaScript execution.
   void ForbidExecution();
@@ -103,17 +105,15 @@ class CORE_EXPORT WorkerOrWorkletScriptController final
     return script_state_ && !!script_state_->PerContextData();
   }
 
-  ScriptValue EvaluateAndReturnValueForTest(const ScriptSourceCode&);
-
  private:
   class ExecutionState;
 
   void DisableEvalInternal(const String& error_message);
 
   // Evaluate a script file in the current execution environment.
-  ScriptValue EvaluateInternal(const ScriptSourceCode&,
-                               SanitizeScriptErrors,
-                               V8CacheOptions);
+  v8::Local<v8::Value> EvaluateInternal(const ScriptSourceCode&,
+                                        SanitizeScriptErrors,
+                                        V8CacheOptions);
   void DisposeContextIfNeeded();
 
   Member<WorkerOrWorkletGlobalScope> global_scope_;

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

@@ -57,10 +57,12 @@ bool ClassicScript::RunScriptOnWorkerOrWorklet(
     WorkerOrWorkletGlobalScope& global_scope) {
   DCHECK(global_scope.IsContextThread());
 
-  bool success = global_scope.ScriptController()->Evaluate(
-      GetScriptSourceCode(), sanitize_script_errors_, nullptr /* error_event */,
-      global_scope.GetV8CacheOptions());
-  return success;
+  ScriptState::Scope scope(global_scope.ScriptController()->GetScriptState());
+  v8::Local<v8::Value> result =
+      global_scope.ScriptController()->EvaluateAndReturnValue(
+          GetScriptSourceCode(), sanitize_script_errors_,
+          nullptr /* error_event */, global_scope.GetV8CacheOptions());
+  return !result.IsEmpty();
 }
 
 std::pair<size_t, size_t> ClassicScript::GetClassicScriptSizes() const {

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

@@ -303,7 +303,8 @@ void WorkerGlobalScope::ImportScriptsInternal(const Vector<String>& urls,
                                                 std::move(cached_meta_data)));
     ReportingProxy().WillEvaluateImportedClassicScript(
         source_code.length(), handler ? handler->GetCodeCacheSize() : 0);
-    ScriptController()->Evaluate(
+    ScriptState::Scope scope(ScriptController()->GetScriptState());
+    ScriptController()->EvaluateAndReturnValue(
         ScriptSourceCode(source_code, ScriptSourceLocationType::kUnknown,
                          handler,
                          ScriptSourceCode::UsePostRedirectURL() ? response_url

third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc

@@ -182,18 +182,18 @@ class AnimationWorkletGlobalScopeTest : public PageTestBase {
         ClassicScript::CreateUnspecifiedScript(ScriptSourceCode(source_code))
             ->RunScriptOnWorkerOrWorklet(*global_scope));
 
-    ScriptValue constructed_before =
-        global_scope->ScriptController()->EvaluateAndReturnValueForTest(
-            ScriptSourceCode("Function('return this')().constructed"));
-    EXPECT_FALSE(
-        ToBoolean(isolate, constructed_before.V8Value(), ASSERT_NO_EXCEPTION))
+    v8::Local<v8::Value> constructed_before =
+        global_scope->ScriptController()->EvaluateAndReturnValue(
+            ScriptSourceCode("Function('return this')().constructed"),
+            SanitizeScriptErrors::kSanitize);
+    EXPECT_FALSE(ToBoolean(isolate, constructed_before, ASSERT_NO_EXCEPTION))
         << "constructor is not invoked";
 
-    ScriptValue animated_before =
-        global_scope->ScriptController()->EvaluateAndReturnValueForTest(
-            ScriptSourceCode("Function('return this')().animated"));
-    EXPECT_FALSE(
-        ToBoolean(isolate, animated_before.V8Value(), ASSERT_NO_EXCEPTION))
+    v8::Local<v8::Value> animated_before =
+        global_scope->ScriptController()->EvaluateAndReturnValue(
+            ScriptSourceCode("Function('return this')().animated"),
+            SanitizeScriptErrors::kSanitize);
+    EXPECT_FALSE(ToBoolean(isolate, animated_before, ASSERT_NO_EXCEPTION))
         << "animate function is invoked early";
 
     // Passing a new input state with a new animation id should cause the
@@ -209,18 +209,18 @@ class AnimationWorkletGlobalScopeTest : public PageTestBase {
         ProxyClientMutate(state, global_scope);
     EXPECT_EQ(output->animations.size(), 1ul);
 
-    ScriptValue constructed_after =
-        global_scope->ScriptController()->EvaluateAndReturnValueForTest(
-            ScriptSourceCode("Function('return this')().constructed"));
-    EXPECT_TRUE(
-        ToBoolean(isolate, constructed_after.V8Value(), ASSERT_NO_EXCEPTION))
+    v8::Local<v8::Value> constructed_after =
+        global_scope->ScriptController()->EvaluateAndReturnValue(
+            ScriptSourceCode("Function('return this')().constructed"),
+            SanitizeScriptErrors::kSanitize);
+    EXPECT_TRUE(ToBoolean(isolate, constructed_after, ASSERT_NO_EXCEPTION))
         << "constructor is not invoked";
 
-    ScriptValue animated_after =
-        global_scope->ScriptController()->EvaluateAndReturnValueForTest(
-            ScriptSourceCode("Function('return this')().animated"));
-    EXPECT_TRUE(
-        ToBoolean(isolate, animated_after.V8Value(), ASSERT_NO_EXCEPTION))
+    v8::Local<v8::Value> animated_after =
+        global_scope->ScriptController()->EvaluateAndReturnValue(
+            ScriptSourceCode("Function('return this')().animated"),
+            SanitizeScriptErrors::kSanitize);
+    EXPECT_TRUE(ToBoolean(isolate, animated_after, ASSERT_NO_EXCEPTION))
         << "animate function is not invoked";
 
     waitable_event->Signal();