Fix DCHECK in finalizer scavenging logic

The DCHECK was overly restrictive when checking that a weak handle is
not a finalizer when hitting it through a regular scavenge processing
path.

Only happened with finalizers to unmodified API wrappers or regular
objects that were also marked as independent.

Bug: v8:8586
Change-Id: I2c2a5b21f6e8a5ddc6671f762b508ba083c04335
Reviewed-on: https://chromium-review.googlesource.com/c/1387485Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58414}

src/global-handles.cc

@@ -713,8 +713,8 @@ void GlobalHandles::IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles(
     DCHECK(node->is_in_new_space_list());
     if ((node->is_independent() || !node->is_active()) &&
         node->IsWeakRetainer() && (node->state() != Node::PENDING)) {
-      DCHECK(node->IsPhantomResetHandle() || node->IsPhantomCallback());
       if (should_reset_handle(isolate_->heap(), node->location())) {
+        DCHECK(node->IsPhantomResetHandle() || node->IsPhantomCallback());
         if (node->IsPhantomResetHandle()) {
           node->MarkPending();
           node->ResetPhantomHandle();

test/cctest/test-global-handles.cc

@@ -37,6 +37,10 @@ namespace internal {
 
 namespace {
 
+void InvokeScavenge() { CcTest::CollectGarbage(i::NEW_SPACE); }
+
+void InvokeMarkSweep() { CcTest::CollectAllGarbage(); }
+
 void SimpleCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
   info.GetReturnValue().Set(v8_num(0));
 }
@@ -272,14 +276,14 @@ TEST(WeakHandleToUnmodifiedJSObjectSurvivesScavenge) {
   CcTest::InitializeVM();
   WeakHandleTest(
       CcTest::isolate(), &ConstructJSObject, [](FlagAndPersistent* fp) {},
-      []() { CcTest::CollectGarbage(i::NEW_SPACE); }, SurvivalMode::kSurvives);
+      []() { InvokeScavenge(); }, SurvivalMode::kSurvives);
 }
 
 TEST(WeakHandleToUnmodifiedJSObjectDiesOnMarkCompact) {
   CcTest::InitializeVM();
   WeakHandleTest(
       CcTest::isolate(), &ConstructJSObject, [](FlagAndPersistent* fp) {},
-      []() { CcTest::CollectGarbage(i::OLD_SPACE); }, SurvivalMode::kDies);
+      []() { InvokeMarkSweep(); }, SurvivalMode::kDies);
 }
 
 TEST(WeakHandleToUnmodifiedJSObjectSurvivesMarkCompactWhenInHandle) {
@@ -291,14 +295,14 @@ TEST(WeakHandleToUnmodifiedJSObjectSurvivesMarkCompactWhenInHandle) {
             v8::Local<v8::Object>::New(CcTest::isolate(), fp->handle);
         USE(handle);
       },
-      []() { CcTest::CollectGarbage(i::OLD_SPACE); }, SurvivalMode::kSurvives);
+      []() { InvokeMarkSweep(); }, SurvivalMode::kSurvives);
 }
 
 TEST(WeakHandleToUnmodifiedJSApiObjectDiesOnScavenge) {
   CcTest::InitializeVM();
   WeakHandleTest(
       CcTest::isolate(), &ConstructJSApiObject, [](FlagAndPersistent* fp) {},
-      []() { CcTest::CollectGarbage(i::NEW_SPACE); }, SurvivalMode::kDies);
+      []() { InvokeScavenge(); }, SurvivalMode::kDies);
 }
 
 TEST(WeakHandleToUnmodifiedJSApiObjectSurvivesScavengeWhenInHandle) {
@@ -310,14 +314,14 @@ TEST(WeakHandleToUnmodifiedJSApiObjectSurvivesScavengeWhenInHandle) {
             v8::Local<v8::Object>::New(CcTest::isolate(), fp->handle);
         USE(handle);
       },
-      []() { CcTest::CollectGarbage(i::NEW_SPACE); }, SurvivalMode::kSurvives);
+      []() { InvokeScavenge(); }, SurvivalMode::kSurvives);
 }
 
 TEST(WeakHandleToUnmodifiedJSApiObjectDiesOnMarkCompact) {
   CcTest::InitializeVM();
   WeakHandleTest(
       CcTest::isolate(), &ConstructJSApiObject, [](FlagAndPersistent* fp) {},
-      []() { CcTest::CollectGarbage(i::OLD_SPACE); }, SurvivalMode::kDies);
+      []() { InvokeMarkSweep(); }, SurvivalMode::kDies);
 }
 
 TEST(WeakHandleToUnmodifiedJSApiObjectSurvivesMarkCompactWhenInHandle) {
@@ -329,23 +333,23 @@ TEST(WeakHandleToUnmodifiedJSApiObjectSurvivesMarkCompactWhenInHandle) {
             v8::Local<v8::Object>::New(CcTest::isolate(), fp->handle);
         USE(handle);
       },
-      []() { CcTest::CollectGarbage(i::OLD_SPACE); }, SurvivalMode::kSurvives);
+      []() { InvokeMarkSweep(); }, SurvivalMode::kSurvives);
 }
 
 TEST(WeakHandleToActiveUnmodifiedJSApiObjectSurvivesScavenge) {
   CcTest::InitializeVM();
-  WeakHandleTest(CcTest::isolate(), &ConstructJSApiObject,
-                 [](FlagAndPersistent* fp) { fp->handle.MarkActive(); },
-                 []() { CcTest::CollectGarbage(i::NEW_SPACE); },
-                 SurvivalMode::kSurvives);
+  WeakHandleTest(
+      CcTest::isolate(), &ConstructJSApiObject,
+      [](FlagAndPersistent* fp) { fp->handle.MarkActive(); },
+      []() { InvokeScavenge(); }, SurvivalMode::kSurvives);
 }
 
 TEST(WeakHandleToActiveUnmodifiedJSApiObjectDiesOnMarkCompact) {
   CcTest::InitializeVM();
-  WeakHandleTest(CcTest::isolate(), &ConstructJSApiObject,
-                 [](FlagAndPersistent* fp) { fp->handle.MarkActive(); },
-                 []() { CcTest::CollectGarbage(i::OLD_SPACE); },
-                 SurvivalMode::kDies);
+  WeakHandleTest(
+      CcTest::isolate(), &ConstructJSApiObject,
+      [](FlagAndPersistent* fp) { fp->handle.MarkActive(); },
+      []() { InvokeMarkSweep(); }, SurvivalMode::kDies);
 }
 
 TEST(WeakHandleToActiveUnmodifiedJSApiObjectSurvivesMarkCompactWhenInHandle) {
@@ -358,7 +362,29 @@ TEST(WeakHandleToActiveUnmodifiedJSApiObjectSurvivesMarkCompactWhenInHandle) {
             v8::Local<v8::Object>::New(CcTest::isolate(), fp->handle);
         USE(handle);
       },
-      []() { CcTest::CollectGarbage(i::OLD_SPACE); }, SurvivalMode::kSurvives);
+      []() { InvokeMarkSweep(); }, SurvivalMode::kSurvives);
+}
+
+TEST(FinalizerOnUnmodifiedJSApiObjectDoesNotCrash) {
+  // See crbug.com/v8/8586.
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Context> context = v8::Context::New(isolate);
+  v8::Context::Scope context_scope(context);
+
+  FlagAndPersistent fp;
+  // Could use a regular object and MarkIndependent too.
+  ConstructJSApiObject(isolate, context, &fp);
+  fp.handle.SetWeak(&fp, &ResetHandleAndSetFlag,
+                    v8::WeakCallbackType::kFinalizer);
+  fp.flag = false;
+  {
+    v8::HandleScope scope(isolate);
+    v8::Local<v8::Object> tmp = v8::Local<v8::Object>::New(isolate, fp.handle);
+    USE(tmp);
+    InvokeScavenge();
+  }
 }
 
 namespace {
@@ -391,12 +417,12 @@ TEST(FinalizerResurrectsAndKeepsPhantomAliveOnMarkCompact) {
   v8::Global<v8::Object> g1, g2;
   ConstructFinalizerPointingPhantomHandle(CcTest::isolate(), &g1, &g2,
                                           ResurrectingFinalizer);
-  CcTest::CollectGarbage(i::OLD_SPACE);
+  InvokeMarkSweep();
   // Both, g1 and g2, should stay alive as the finalizer resurrects the root
   // object that transitively keeps the other one alive.
   CHECK(!g1.IsEmpty());
   CHECK(!g2.IsEmpty());
-  CcTest::CollectGarbage(i::OLD_SPACE);
+  InvokeMarkSweep();
   // The finalizer handle is now strong, so it should keep the objects alive.
   CHECK(!g1.IsEmpty());
   CHECK(!g2.IsEmpty());
@@ -407,12 +433,12 @@ TEST(FinalizerDiesAndKeepsPhantomAliveOnMarkCompact) {
   v8::Global<v8::Object> g1, g2;
   ConstructFinalizerPointingPhantomHandle(CcTest::isolate(), &g1, &g2,
                                           ResettingFinalizer);
-  CcTest::CollectGarbage(i::OLD_SPACE);
+  InvokeMarkSweep();
   // Finalizer (g1) dies but the phantom handle (g2) is kept alive for one
   // more round as the underlying object only dies on the next GC.
   CHECK(g1.IsEmpty());
   CHECK(!g2.IsEmpty());
-  CcTest::CollectGarbage(i::OLD_SPACE);
+  InvokeMarkSweep();
   // Phantom handle dies after one more round.
   CHECK(g1.IsEmpty());
   CHECK(g2.IsEmpty());
@@ -420,10 +446,6 @@ TEST(FinalizerDiesAndKeepsPhantomAliveOnMarkCompact) {
 
 namespace {
 
-void InvokeScavenge() { CcTest::CollectGarbage(i::NEW_SPACE); }
-
-void InvokeMarkSweep() { CcTest::CollectAllGarbage(); }
-
 void ForceScavenge2(const v8::WeakCallbackInfo<FlagAndPersistent>& data) {
   data.GetParameter()->flag = true;
   InvokeScavenge();
@@ -505,8 +527,8 @@ TEST(SecondPassPhantomCallbacks) {
   fp.flag = false;
   fp.handle.SetWeak(&fp, FirstPassCallback, v8::WeakCallbackType::kParameter);
   CHECK(!fp.flag);
-  CcTest::CollectGarbage(i::OLD_SPACE);
-  CcTest::CollectGarbage(i::OLD_SPACE);
+  InvokeMarkSweep();
+  InvokeMarkSweep();
   CHECK(fp.flag);
 }