collect phantom handle data before it gets overwritten

src/global-handles.cc

Index: src/global-handles.cc
diff --git a/src/global-handles.cc b/src/global-handles.cc
index 25c6b4efda9dd9172f14df8d327828c417ab46ae..75a824deb073109f305028d0787819fc8d8686d5 100644
--- a/src/global-handles.cc
+++ b/src/global-handles.cc
@@ -169,12 +169,16 @@ class GlobalHandles::Node {
 
   bool IsInUse() const { return state() != FREE; }
 
-  bool IsRetainer() const { return state() != FREE; }
+  bool IsRetainer() const {
+    return state() != FREE &&
+           !(state() == NEAR_DEATH && weakness_type() != NORMAL_WEAK);

[Hannes Payer (out of office), 2015/04/13 11:50:45]

please simplify this statement

[dcarney, 2015/04/13 12:27:05]

this was the clearest version i could think of. what did you have in mind?

+  }
 
   bool IsStrongRetainer() const { return state() == NORMAL; }
 
   bool IsWeakRetainer() const {
-    return state() == WEAK || state() == PENDING || state() == NEAR_DEATH;
+    return state() == WEAK || state() == PENDING ||
+           (state() == NEAR_DEATH && weakness_type() == NORMAL_WEAK);
   }
 
   void MarkPending() {
@@ -260,35 +264,31 @@ class GlobalHandles::Node {
   void CollectPhantomCallbackData(
       Isolate* isolate,
       List<PendingPhantomCallback>* pending_phantom_callbacks) {
-    if (state() != PENDING) return;
-    if (weak_callback_ != NULL) {
-      if (weakness_type() == NORMAL_WEAK) return;
-
-      DCHECK(weakness_type() == PHANTOM_WEAK ||
-             weakness_type() == PHANTOM_WEAK_2_INTERNAL_FIELDS);
-
-      void* internal_fields[v8::kInternalFieldsInWeakCallback] = {nullptr,
-                                                                  nullptr};
-      if (weakness_type() != PHANTOM_WEAK && object()->IsJSObject()) {
-        auto jsobject = JSObject::cast(object());
-        int field_count = jsobject->GetInternalFieldCount();
-        for (int i = 0; i < v8::kInternalFieldsInWeakCallback; ++i) {
-          if (field_count == i) break;
-          auto field = jsobject->GetInternalField(i);
-          if (field->IsSmi()) internal_fields[i] = field;
-        }
+    DCHECK(weakness_type() == PHANTOM_WEAK ||
+           weakness_type() == PHANTOM_WEAK_2_INTERNAL_FIELDS);
+    DCHECK(state() == PENDING);
+
+    void* internal_fields[v8::kInternalFieldsInWeakCallback] = {nullptr,
+                                                                nullptr};
+    if (weakness_type() != PHANTOM_WEAK && object()->IsJSObject()) {
+      auto jsobject = JSObject::cast(object());
+      int field_count = jsobject->GetInternalFieldCount();
+      for (int i = 0; i < v8::kInternalFieldsInWeakCallback; ++i) {
+        if (field_count == i) break;
+        auto field = jsobject->GetInternalField(i);
+        if (field->IsSmi()) internal_fields[i] = field;
       }
+    }
 
-      // Zap with something dangerous.
-      *location() = reinterpret_cast<Object*>(0x6057ca11);
+    // Zap with something dangerous.
+    *location() = reinterpret_cast<Object*>(0x6057ca11);
 
-      typedef v8::WeakCallbackInfo<void> Data;
-      auto callback = reinterpret_cast<Data::Callback>(weak_callback_);
-      pending_phantom_callbacks->Add(
-          PendingPhantomCallback(this, callback, parameter(), internal_fields));
-      DCHECK(IsInUse());
-      set_state(NEAR_DEATH);
-    }
+    typedef v8::WeakCallbackInfo<void> Data;
+    auto callback = reinterpret_cast<Data::Callback>(weak_callback_);
+    pending_phantom_callbacks->Add(
+        PendingPhantomCallback(this, callback, parameter(), internal_fields));
+    DCHECK(IsInUse());
+    set_state(NEAR_DEATH);
   }
 
   bool PostGarbageCollectionProcessing(Isolate* isolate) {
@@ -562,23 +562,6 @@ void GlobalHandles::MakeWeak(Object** location, void* parameter,
 }
 
 
-void GlobalHandles::CollectAllPhantomCallbackData() {
-  for (NodeIterator it(this); !it.done(); it.Advance()) {
-    Node* node = it.node();
-    node->CollectPhantomCallbackData(isolate(), &pending_phantom_callbacks_);
-  }
-}
-
-
-void GlobalHandles::CollectYoungPhantomCallbackData() {
-  for (int i = 0; i < new_space_nodes_.length(); ++i) {
-    Node* node = new_space_nodes_[i];
-    DCHECK(node->is_in_new_space_list());
-    node->CollectPhantomCallbackData(isolate(), &pending_phantom_callbacks_);
-  }
-}
-
-
 void* GlobalHandles::ClearWeakness(Object** location) {
   return Node::FromLocation(location)->ClearWeakness();
 }
@@ -616,18 +599,12 @@ void GlobalHandles::IterateWeakRoots(ObjectVisitor* v) {
       // Weakness type can be normal or phantom, with or without internal
       // fields).  For normal weakness we mark through the handle so that the
       // object and things reachable from it are available to the callback.
-      //
-      // In the case of phantom with no internal fields, we can zap the object
-      // handle now and we won't need it, so we don't need to mark through it.
-      // In the internal fields case we will need the internal
-      // fields, so we can't zap the handle.
       if (node->state() == Node::PENDING) {
-        if (node->weakness_type() == PHANTOM_WEAK) {
-          *(node->location()) = Smi::FromInt(0);
-        } else if (node->weakness_type() == NORMAL_WEAK) {
+        if (node->weakness_type() == NORMAL_WEAK) {
           v->VisitPointer(node->location());
         } else {
-          DCHECK(node->weakness_type() == PHANTOM_WEAK_2_INTERNAL_FIELDS);
+          node->CollectPhantomCallbackData(isolate(),
+                                           &pending_phantom_callbacks_);
         }
       } else {
         // Node is not pending, so that means the object survived.  We still
@@ -680,19 +657,11 @@ void GlobalHandles::IterateNewSpaceWeakIndependentRoots(ObjectVisitor* v) {
     DCHECK(node->is_in_new_space_list());
     if ((node->is_independent() || node->is_partially_dependent()) &&
         node->IsWeakRetainer()) {
-      if (node->weakness_type() == PHANTOM_WEAK) {
-        *(node->location()) = Smi::FromInt(0);
-      } else if (node->weakness_type() == NORMAL_WEAK) {
+      if (node->weakness_type() == NORMAL_WEAK) {
         v->VisitPointer(node->location());
-      } else {
-        DCHECK(node->weakness_type() == PHANTOM_WEAK_2_INTERNAL_FIELDS);
-        // For this case we only need to trace if it's alive: The tracing of
-        // something that is already alive is just to get the pointer updated
-        // to the new location of the object).
-        DCHECK(node->state() != Node::NEAR_DEATH);
-        if (node->state() != Node::PENDING) {
-          v->VisitPointer(node->location());
-        }
+      } else if (node->state() == Node::PENDING) {
+        node->CollectPhantomCallbackData(isolate(),
+                                         &pending_phantom_callbacks_);
       }
     }
   }