Fix UAF in ScriptPromiseProperty caused by reentrant code

v8::Promise::Resolve can run user code synchronously, which caused a UAF
in ScriptPromiseProperty. Fix it.

Bug: 1108518
Change-Id: Ia9baec6eef0887323cd88ceb1d3fa0c14fdb77ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2325499Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792661}

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

@@ -30,7 +30,6 @@ class ScriptPromiseProperty final
     : public GarbageCollected<
           ScriptPromiseProperty<ResolvedType, RejectedType>>,
       public ExecutionContextClient {
-
  public:
   enum State {
     kPending,
@@ -101,10 +100,11 @@ class ScriptPromiseProperty final
     }
     state_ = kResolved;
     resolved_ = value;
-    for (const Member<ScriptPromiseResolver>& resolver : resolvers_) {
+    HeapVector<Member<ScriptPromiseResolver>> resolvers;
+    resolvers.swap(resolvers_);
+    for (const Member<ScriptPromiseResolver>& resolver : resolvers) {
       resolver->Resolve(resolved_);
     }
-    resolvers_.clear();
   }
 
   void ResolveWithUndefined() {
@@ -115,10 +115,11 @@ class ScriptPromiseProperty final
     }
     state_ = kResolved;
     resolved_with_undefined_ = true;
-    for (const Member<ScriptPromiseResolver>& resolver : resolvers_) {
+    HeapVector<Member<ScriptPromiseResolver>> resolvers;
+    resolvers.swap(resolvers_);
+    for (const Member<ScriptPromiseResolver>& resolver : resolvers) {
       resolver->Resolve();
     }
-    resolvers_.clear();
   }
 
   template <typename PassRejectedType>
@@ -130,10 +131,11 @@ class ScriptPromiseProperty final
     }
     state_ = kRejected;
     rejected_ = value;
-    for (const Member<ScriptPromiseResolver>& resolver : resolvers_) {
+    HeapVector<Member<ScriptPromiseResolver>> resolvers;
+    resolvers.swap(resolvers_);
+    for (const Member<ScriptPromiseResolver>& resolver : resolvers) {
       resolver->Reject(rejected_);
     }
-    resolvers_.clear();
   }
 
   // Resets this property by unregistering the Promise property from the

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

@@ -96,6 +96,35 @@ class GarbageCollectedHolder final : public GarbageCollectedScriptWrappable {
   Member<Property> property_;
 };
 
+class ScriptPromisePropertyResetter : public ScriptFunction {
+ public:
+  using Property =
+      ScriptPromiseProperty<Member<GarbageCollectedScriptWrappable>,
+                            Member<GarbageCollectedScriptWrappable>>;
+  static v8::Local<v8::Function> CreateFunction(ScriptState* script_state,
+                                                Property* property) {
+    auto* self = MakeGarbageCollected<ScriptPromisePropertyResetter>(
+        script_state, property);
+    return self->BindToV8Function();
+  }
+
+  ScriptPromisePropertyResetter(ScriptState* script_state, Property* property)
+      : ScriptFunction(script_state), property_(property) {}
+
+  void Trace(Visitor* visitor) const override {
+    visitor->Trace(property_);
+    ScriptFunction::Trace(visitor);
+  }
+
+ private:
+  ScriptValue Call(ScriptValue arg) override {
+    property_->Reset();
+    return ScriptValue();
+  }
+
+  const Member<Property> property_;
+};
+
 class ScriptPromisePropertyTestBase {
  public:
   ScriptPromisePropertyTestBase()
@@ -512,6 +541,48 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, MarkAsHandled) {
   }
 }
 
+TEST_F(ScriptPromisePropertyGarbageCollectedTest, SyncResolve) {
+  // Call getters to create resolvers in the property.
+  GetProperty()->Promise(DOMWrapperWorld::MainWorld());
+  GetProperty()->Promise(OtherWorld());
+
+  auto* resolution =
+      MakeGarbageCollected<GarbageCollectedScriptWrappable>("hi");
+  v8::HandleScope handle_scope(GetIsolate());
+  v8::Local<v8::Object> main_v8_resolution;
+  v8::Local<v8::Object> other_v8_resolution;
+  {
+    ScriptState::Scope scope(MainScriptState());
+    main_v8_resolution = ToV8(resolution, MainScriptState()).As<v8::Object>();
+    v8::PropertyDescriptor descriptor(
+        ScriptPromisePropertyResetter::CreateFunction(MainScriptState(),
+                                                      GetProperty()),
+        v8::Undefined(GetIsolate()));
+    ASSERT_EQ(
+        v8::Just(true),
+        main_v8_resolution->DefineProperty(
+            MainScriptState()->GetContext(),
+            v8::String::NewFromUtf8Literal(GetIsolate(), "then"), descriptor));
+  }
+  {
+    ScriptState::Scope scope(OtherScriptState());
+    other_v8_resolution = ToV8(resolution, OtherScriptState()).As<v8::Object>();
+    v8::PropertyDescriptor descriptor(
+        ScriptPromisePropertyResetter::CreateFunction(OtherScriptState(),
+                                                      GetProperty()),
+        v8::Undefined(GetIsolate()));
+    ASSERT_EQ(
+        v8::Just(true),
+        other_v8_resolution->DefineProperty(
+            OtherScriptState()->GetContext(),
+            v8::String::NewFromUtf8Literal(GetIsolate(), "then"), descriptor));
+  }
+
+  // This shouldn't crash.
+  GetProperty()->Resolve(resolution);
+  EXPECT_EQ(GetProperty()->GetState(), Property::State::kPending);
+}
+
 TEST_F(ScriptPromisePropertyNonScriptWrappableResolutionTargetTest,
        ResolveWithUndefined) {
   Test(ToV8UndefinedGenerator(), "undefined", __FILE__, __LINE__);