Remove ScriptValue::GetContext()

You should be careful not to use a wrong context,
 which leads to a leak of information.

Currently, we can get a context from scattered ScriptValue
without consideration, but we would like to prevent it.

Instead of from ScriptValue, you can get a context via
- ScriptState::GetContext() using ScriptState passed from binding code
- ScriptState::GetContext() using ScriptState stored on a C++ object
- Isolate::GetCurrentContext()

Also, we add GetScriptState() in InternalResolver
to get a script_state from instead of from ScriptValue.
It is safe because InternalResolver is only used internally.

Bug: 998994
Change-Id: Iaa5a936635279566abe4801d4f15774eb628e83e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1772793Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Rika Fujimaki <rikaf@google.com>
Cr-Commit-Position: refs/heads/master@{#692001}

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

@@ -65,7 +65,7 @@ class PromiseAllHandler final
     }
   }
 
-  virtual void Trace(blink::Visitor* visitor) {}
+  virtual void Trace(blink::Visitor* visitor) { visitor->Trace(resolver_); }
 
  private:
   class AdapterFunction : public ScriptFunction {
@@ -133,8 +133,7 @@ class PromiseAllHandler final
     if (--number_of_pending_promises_ > 0)
       return;
 
-    v8::Local<v8::Value> values =
-        ToV8(values_, value.GetContext()->Global(), value.GetIsolate());
+    v8::Local<v8::Value> values = ToV8(values_, resolver_.GetScriptState());
     MarkPromiseSettled();
     resolver_.Resolve(values);
   }
@@ -166,7 +165,8 @@ class PromiseAllHandler final
 }  // namespace
 
 ScriptPromise::InternalResolver::InternalResolver(ScriptState* script_state)
-    : resolver_(script_state,
+    : script_state_(script_state),
+      resolver_(script_state,
                 v8::Promise::Resolver::New(script_state->GetContext())) {
   // |resolver| can be empty when the thread is being terminated. We ignore such
   // errors.
@@ -189,7 +189,7 @@ void ScriptPromise::InternalResolver::Resolve(v8::Local<v8::Value> value) {
     return;
   v8::Maybe<bool> result =
       resolver_.V8Value().As<v8::Promise::Resolver>()->Resolve(
-          resolver_.GetContext(), value);
+          script_state_->GetContext(), value);
   // |result| can be empty when the thread is being terminated. We ignore such
   // errors.
   ALLOW_UNUSED_LOCAL(result);
@@ -202,7 +202,7 @@ void ScriptPromise::InternalResolver::Reject(v8::Local<v8::Value> value) {
     return;
   v8::Maybe<bool> result =
       resolver_.V8Value().As<v8::Promise::Resolver>()->Reject(
-          resolver_.GetContext(), value);
+          script_state_->GetContext(), value);
   // |result| can be empty when the thread is being terminated. We ignore such
   // errors.
   ALLOW_UNUSED_LOCAL(result);

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

@@ -135,8 +135,11 @@ class CORE_EXPORT ScriptPromise final {
     void Resolve(v8::Local<v8::Value>);
     void Reject(v8::Local<v8::Value>);
     void Clear() { resolver_.Clear(); }
+    ScriptState* GetScriptState() const { return script_state_; }
+    void Trace(blink::Visitor* visitor) { visitor->Trace(script_state_); }
 
    private:
+    Member<ScriptState> script_state_;
     ScriptValue resolver_;
   };
 

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

@@ -120,6 +120,7 @@ void ScriptPromiseResolver::ResolveOrRejectDeferred() {
 
 void ScriptPromiseResolver::Trace(blink::Visitor* visitor) {
   visitor->Trace(script_state_);
+  visitor->Trace(resolver_);
   ContextLifecycleObserver::Trace(visitor);
 }
 

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

@@ -100,11 +100,6 @@ class CORE_EXPORT ScriptValue final {
                          : v8::Isolate::GetCurrent();
   }
 
-  v8::Local<v8::Context> GetContext() const {
-    DCHECK(script_state_);
-    return script_state_->GetContext();
-  }
-
   ScriptValue& operator=(const ScriptValue& value) {
     if (this != &value) {
       script_state_ = value.script_state_;

third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.cc

@@ -31,15 +31,16 @@ void AbortPaymentRespondWithObserver::OnResponseRejected(
 }
 
 void AbortPaymentRespondWithObserver::OnResponseFulfilled(
+    ScriptState* script_state,
     const ScriptValue& value,
     ExceptionState::ContextType context_type,
     const char* interface_name,
     const char* property_name) {
   DCHECK(GetExecutionContext());
-  ExceptionState exception_state(value.GetIsolate(), context_type,
+  ExceptionState exception_state(script_state->GetIsolate(), context_type,
                                  interface_name, property_name);
   bool response =
-      ToBoolean(value.GetIsolate(), value.V8Value(), exception_state);
+      ToBoolean(script_state->GetIsolate(), value.V8Value(), exception_state);
   if (exception_state.HadException()) {
     exception_state.ClearException();
     OnResponseRejected(blink::mojom::ServiceWorkerResponseError::kNoV8Instance);

third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.h

@@ -26,7 +26,8 @@ class MODULES_EXPORT AbortPaymentRespondWithObserver final
   ~AbortPaymentRespondWithObserver() override = default;
 
   void OnResponseRejected(mojom::ServiceWorkerResponseError) override;
-  void OnResponseFulfilled(const ScriptValue&,
+  void OnResponseFulfilled(ScriptState*,
+                           const ScriptValue&,
                            ExceptionState::ContextType,
                            const char* interface_name,
                            const char* property_name) override;

third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.cc

@@ -31,15 +31,16 @@ void CanMakePaymentRespondWithObserver::OnResponseRejected(
 }
 
 void CanMakePaymentRespondWithObserver::OnResponseFulfilled(
+    ScriptState* script_state,
     const ScriptValue& value,
     ExceptionState::ContextType context_type,
     const char* interface_name,
     const char* property_name) {
   DCHECK(GetExecutionContext());
-  ExceptionState exception_state(value.GetIsolate(), context_type,
+  ExceptionState exception_state(script_state->GetIsolate(), context_type,
                                  interface_name, property_name);
   bool response =
-      ToBoolean(value.GetIsolate(), value.V8Value(), exception_state);
+      ToBoolean(script_state->GetIsolate(), value.V8Value(), exception_state);
   if (exception_state.HadException()) {
     exception_state.ClearException();
     OnResponseRejected(blink::mojom::ServiceWorkerResponseError::kNoV8Instance);

third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.h

@@ -26,7 +26,8 @@ class MODULES_EXPORT CanMakePaymentRespondWithObserver final
   ~CanMakePaymentRespondWithObserver() override = default;
 
   void OnResponseRejected(mojom::ServiceWorkerResponseError) override;
-  void OnResponseFulfilled(const ScriptValue&,
+  void OnResponseFulfilled(ScriptState*,
+                           const ScriptValue&,
                            ExceptionState::ContextType,
                            const char* interface_name,
                            const char* property_name) override;

third_party/blink/renderer/modules/payments/payment_request.cc

@@ -393,13 +393,14 @@ void SetBasicCardMethodData(const ScriptValue& input,
                                       output->supported_types, exception_state);
 }
 
-void StringifyAndParseMethodSpecificData(ExecutionContext& execution_context,
+void StringifyAndParseMethodSpecificData(v8::Isolate* isolate,
                                          const String& supported_method,
                                          const ScriptValue& input,
                                          PaymentMethodDataPtr& output,
                                          ExceptionState& exception_state) {
   PaymentsValidators::ValidateAndStringifyObject(
-      "Payment method data", input, output->stringified_data, exception_state);
+      isolate, "Payment method data", input, output->stringified_data,
+      exception_state);
   if (exception_state.HadException())
     return;
 
@@ -459,8 +460,8 @@ void ValidateAndConvertPaymentDetailsModifiers(
 
     if (modifier->hasData() && !modifier->data().IsEmpty()) {
       StringifyAndParseMethodSpecificData(
-          execution_context, modifier->supportedMethod(), modifier->data(),
-          output.back()->method_data, exception_state);
+          execution_context.GetIsolate(), modifier->supportedMethod(),
+          modifier->data(), output.back()->method_data, exception_state);
     } else {
       output.back()->method_data->stringified_data = "";
     }
@@ -563,7 +564,9 @@ void ValidateAndConvertPaymentDetailsUpdate(const PaymentDetailsUpdate* input,
 
   if (input->hasPaymentMethodErrors()) {
     PaymentsValidators::ValidateAndStringifyObject(
-        "Payment method errors", input->paymentMethodErrors(),
+        execution_context.GetIsolate(), "Payment method errors",
+        input->paymentMethodErrors(),
+
         output->stringified_payment_method_errors, exception_state);
   }
 }
@@ -601,8 +604,9 @@ void ValidateAndConvertPaymentMethodData(
     if (payment_method_data->hasData() &&
         !payment_method_data->data().IsEmpty()) {
       StringifyAndParseMethodSpecificData(
-          execution_context, payment_method_data->supportedMethod(),
-          payment_method_data->data(), output.back(), exception_state);
+          execution_context.GetIsolate(),
+          payment_method_data->supportedMethod(), payment_method_data->data(),
+          output.back(), exception_state);
     } else {
       output.back()->stringified_data = "";
     }

third_party/blink/renderer/modules/payments/payment_request_event.cc

@@ -180,8 +180,8 @@ ScriptPromise PaymentRequestEvent::changePaymentMethod(
   auto method_data = payments::mojom::blink::PaymentHandlerMethodData::New();
   if (!method_details.IsEmpty()) {
     PaymentsValidators::ValidateAndStringifyObject(
-        "Method details", method_details, method_data->stringified_data,
-        exception_state);
+        script_state->GetIsolate(), "Method details", method_details,
+        method_data->stringified_data, exception_state);
     if (exception_state.HadException())
       return ScriptPromise();
   }

third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.cc

@@ -42,16 +42,17 @@ void PaymentRequestRespondWithObserver::OnResponseRejected(
 }
 
 void PaymentRequestRespondWithObserver::OnResponseFulfilled(
+    ScriptState* script_state,
     const ScriptValue& value,
     ExceptionState::ContextType context_type,
     const char* interface_name,
     const char* property_name) {
   DCHECK(GetExecutionContext());
-  ExceptionState exception_state(value.GetIsolate(), context_type,
+  ExceptionState exception_state(script_state->GetIsolate(), context_type,
                                  interface_name, property_name);
   PaymentHandlerResponse* response =
       NativeValueTraits<PaymentHandlerResponse>::NativeValue(
-          value.GetIsolate(), value.V8Value(), exception_state);
+          script_state->GetIsolate(), value.V8Value(), exception_state);
   if (exception_state.HadException()) {
     exception_state.ClearException();
     OnResponseRejected(mojom::ServiceWorkerResponseError::kNoV8Instance);
@@ -87,7 +88,7 @@ void PaymentRequestRespondWithObserver::OnResponseFulfilled(
   }
 
   v8::Local<v8::String> details_value;
-  if (!v8::JSON::Stringify(response->details().GetContext(),
+  if (!v8::JSON::Stringify(script_state->GetContext(),
                            response->details().V8Value().As<v8::Object>())
            .ToLocal(&details_value)) {
     GetExecutionContext()->AddConsoleMessage(ConsoleMessage::Create(

third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.h

@@ -32,7 +32,8 @@ class MODULES_EXPORT PaymentRequestRespondWithObserver final
                                                    WaitUntilObserver*);
 
   void OnResponseRejected(mojom::ServiceWorkerResponseError) override;
-  void OnResponseFulfilled(const ScriptValue&,
+  void OnResponseFulfilled(ScriptState*,
+                           const ScriptValue&,
                            ExceptionState::ContextType,
                            const char* interface_name,
                            const char* property_name) override;

third_party/blink/renderer/modules/payments/payments_validators.cc

@@ -176,13 +176,15 @@ bool PaymentsValidators::IsValidMethodFormat(const String& identifier) {
 }
 
 void PaymentsValidators::ValidateAndStringifyObject(
+    v8::Isolate* isolate,
     const String& input_name,
     const ScriptValue& input,
     String& output,
     ExceptionState& exception_state) {
   v8::Local<v8::String> value;
   if (input.IsEmpty() || !input.V8Value()->IsObject() ||
-      !v8::JSON::Stringify(input.GetContext(), input.V8Value().As<v8::Object>())
+      !v8::JSON::Stringify(isolate->GetCurrentContext(),
+                           input.V8Value().As<v8::Object>())
            .ToLocal(&value)) {
     exception_state.ThrowTypeError(input_name +
                                    " should be a JSON-serializable object");

third_party/blink/renderer/modules/payments/payments_validators.h

@@ -9,6 +9,7 @@
 #include "third_party/blink/renderer/modules/modules_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+#include "v8/include/v8.h"
 
 namespace blink {
 
@@ -71,7 +72,8 @@ class MODULES_EXPORT PaymentsValidators final {
   //
   // If the |input| is invalid, throws a TypeError through the |exception_state|
   // and uses the |input_name| to better describe what was being validated.
-  static void ValidateAndStringifyObject(const String& input_name,
+  static void ValidateAndStringifyObject(v8::Isolate* isolate,
+                                         const String& input_name,
                                          const ScriptValue& input,
                                          String& output,
                                          ExceptionState& exception_state);

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

@@ -222,17 +222,18 @@ void FetchRespondWithObserver::OnResponseRejected(
 }
 
 void FetchRespondWithObserver::OnResponseFulfilled(
+    ScriptState* script_state,
     const ScriptValue& value,
     ExceptionState::ContextType context_type,
     const char* interface_name,
     const char* property_name) {
   DCHECK(GetExecutionContext());
-  if (!V8Response::HasInstance(value.V8Value(), value.GetIsolate())) {
+  if (!V8Response::HasInstance(value.V8Value(), script_state->GetIsolate())) {
     OnResponseRejected(ServiceWorkerResponseError::kNoV8Instance);
     return;
   }
-  Response* response =
-      V8Response::ToImplWithTypeCheck(value.GetIsolate(), value.V8Value());
+  Response* response = V8Response::ToImplWithTypeCheck(
+      script_state->GetIsolate(), value.V8Value());
   // "If one of the following conditions is true, return a network error:
   //   - |response|'s type is |error|.
   //   - |request|'s mode is |same-origin| and |response|'s type is |cors|.
@@ -280,8 +281,8 @@ void FetchRespondWithObserver::OnResponseFulfilled(
     return;
   }
 
-  ExceptionState exception_state(value.GetScriptState()->GetIsolate(),
-                                 context_type, interface_name, property_name);
+  ExceptionState exception_state(script_state->GetIsolate(), context_type,
+                                 interface_name, property_name);
   if (response->IsBodyLocked(exception_state) == Body::BodyLocked::kLocked) {
     DCHECK(!exception_state.HadException());
     OnResponseRejected(ServiceWorkerResponseError::kBodyLocked);

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

@@ -43,7 +43,8 @@ class MODULES_EXPORT FetchRespondWithObserver : public RespondWithObserver {
       WaitUntilObserver*);
 
   void OnResponseRejected(mojom::ServiceWorkerResponseError) override;
-  void OnResponseFulfilled(const ScriptValue&,
+  void OnResponseFulfilled(ScriptState*,
+                           const ScriptValue&,
                            ExceptionState::ContextType context_type,
                            const char* interface_name,
                            const char* property_name) override;

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

@@ -64,7 +64,8 @@ void RespondWithObserver::RespondWith(ScriptState* script_state,
   bool will_wait = observer_->WaitUntil(
       script_state, script_promise, exception_state,
       WTF::BindRepeating(&RespondWithObserver::ResponseWasFulfilled,
-                         WrapPersistent(this), exception_state.Context(),
+                         WrapPersistent(this), WrapPersistent(script_state),
+                         exception_state.Context(),
                          WTF::Unretained(exception_state.InterfaceName()),
                          WTF::Unretained(exception_state.PropertyName())),
       WTF::BindRepeating(&RespondWithObserver::ResponseWasRejected,
@@ -86,11 +87,13 @@ void RespondWithObserver::ResponseWasRejected(ServiceWorkerResponseError error,
 }
 
 void RespondWithObserver::ResponseWasFulfilled(
+    ScriptState* script_state,
     ExceptionState::ContextType context_type,
     const char* interface_name,
     const char* property_name,
     const ScriptValue& value) {
-  OnResponseFulfilled(value, context_type, interface_name, property_name);
+  OnResponseFulfilled(script_state, value, context_type, interface_name,
+                      property_name);
   state_ = kDone;
 }
 

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

@@ -45,7 +45,8 @@ class MODULES_EXPORT RespondWithObserver
   virtual void OnResponseRejected(mojom::ServiceWorkerResponseError) = 0;
 
   // Called when the respondWith() promise was fulfilled.
-  virtual void OnResponseFulfilled(const ScriptValue&,
+  virtual void OnResponseFulfilled(ScriptState*,
+                                   const ScriptValue&,
                                    ExceptionState::ContextType,
                                    const char* interface_name,
                                    const char* property_name) = 0;
@@ -65,7 +66,8 @@ class MODULES_EXPORT RespondWithObserver
 
   void ResponseWasRejected(mojom::ServiceWorkerResponseError,
                            const ScriptValue&);
-  void ResponseWasFulfilled(ExceptionState::ContextType,
+  void ResponseWasFulfilled(ScriptState* state,
+                            ExceptionState::ContextType,
                             const char* interface_name,
                             const char* property_name,
                             const ScriptValue&);

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

@@ -77,7 +77,7 @@ struct StubScriptFunction {
 class ScriptValueTest {
  public:
   virtual ~ScriptValueTest() = default;
-  virtual void operator()(ScriptValue) const = 0;
+  virtual void operator()(ScriptState*, ScriptValue) const = 0;
 };
 
 // Runs microtasks and expects |promise| to be rejected. Calls
@@ -92,7 +92,7 @@ void ExpectRejected(ScriptState* script_state,
   EXPECT_EQ(0ul, resolved.CallCount());
   EXPECT_EQ(1ul, rejected.CallCount());
   if (rejected.CallCount())
-    value_test(rejected.Arg());
+    value_test(script_state, rejected.Arg());
 }
 
 // DOM-related test support.
@@ -106,7 +106,7 @@ class ExpectDOMException : public ScriptValueTest {
 
   ~ExpectDOMException() override = default;
 
-  void operator()(ScriptValue value) const override {
+  void operator()(ScriptState* state, ScriptValue value) const override {
     DOMException* exception = V8DOMException::ToImplWithTypeCheck(
         value.GetIsolate(), value.V8Value());
     EXPECT_TRUE(exception) << "the value should be a DOMException";
@@ -129,9 +129,9 @@ class ExpectTypeError : public ScriptValueTest {
 
   ~ExpectTypeError() override = default;
 
-  void operator()(ScriptValue value) const override {
+  void operator()(ScriptState* state, ScriptValue value) const override {
     v8::Isolate* isolate = value.GetIsolate();
-    v8::Local<v8::Context> context = value.GetContext();
+    v8::Local<v8::Context> context = state->GetContext();
     v8::Local<v8::Object> error_object =
         value.V8Value()->ToObject(context).ToLocalChecked();
     v8::Local<v8::Value> name =