two pass phantom collection

R=jochen@chromium.org, erikcorry@chromium.org

BUG=

Review URL: https://codereview.chromium.org/998253006

Cr-Commit-Position: refs/heads/master@{#27475}

include/v8-util.h

@@ -322,7 +322,11 @@ class PersistentValueMapBase {
     return p.Pass();
   }
 
-  void RemoveWeak(const K& key) { Traits::Remove(&impl_, key); }
+  void RemoveWeak(const K& key) {
+    Global<V> p;
+    p.val_ = FromVal(Traits::Remove(&impl_, key));
+    p.Reset();
+  }
 
  private:
   PersistentValueMapBase(PersistentValueMapBase&);
@@ -476,7 +480,6 @@ class GlobalValueMap : public PersistentValueMapBase<K, V, Traits> {
       K key = Traits::KeyFromWeakCallbackInfo(data);
       persistentValueMap->RemoveWeak(key);
       Traits::DisposeWeak(data.GetIsolate(), data, key);
-      Traits::DisposeCallbackData(data.GetParameter());
     }
   }
 };

include/v8.h

@@ -479,11 +479,13 @@ class WeakCallbackInfo {
  public:
   typedef void (*Callback)(const WeakCallbackInfo<T>& data);
 
-  WeakCallbackInfo(Isolate* isolate, T* parameter, void* internal_field1,
-                   void* internal_field2)
-      : isolate_(isolate), parameter_(parameter) {
-    internal_fields_[0] = internal_field1;
-    internal_fields_[1] = internal_field2;
+  WeakCallbackInfo(Isolate* isolate, T* parameter,
+                   void* internal_fields[kInternalFieldsInWeakCallback],
+                   Callback* callback)
+      : isolate_(isolate), parameter_(parameter), callback_(callback) {
+    for (int i = 0; i < kInternalFieldsInWeakCallback; ++i) {
+      internal_fields_[i] = internal_fields[i];
+    }
   }
 
   V8_INLINE Isolate* GetIsolate() const { return isolate_; }
@@ -499,10 +501,21 @@ class WeakCallbackInfo {
     return internal_fields_[1];
   }
 
+  bool IsFirstPass() const { return callback_ != nullptr; }
+
+  // When first called, the embedder MUST Reset() the Global which triggered the
+  // callback. The Global itself is unusable for anything else. No v8 other api
+  // calls may be called in the first callback. Should additional work be
+  // required, the embedder must set a second pass callback, which will be
+  // called after all the initial callbacks are processed.
+  // Calling SetSecondPassCallback on the second pass will immediately crash.
+  void SetSecondPassCallback(Callback callback) const { *callback_ = callback; }
+
  private:
   Isolate* isolate_;
   T* parameter_;
-  void* internal_fields_[2];
+  Callback* callback_;
+  void* internal_fields_[kInternalFieldsInWeakCallback];
 };
 
 

src/debug.cc

@@ -593,9 +593,10 @@ void ScriptCache::HandleWeakScript(
 
 
 void Debug::HandlePhantomDebugInfo(
-    const v8::PhantomCallbackData<DebugInfoListNode>& data) {
-  Debug* debug = reinterpret_cast<Isolate*>(data.GetIsolate())->debug();
+    const v8::WeakCallbackInfo<DebugInfoListNode>& data) {
   DebugInfoListNode* node = data.GetParameter();
+  node->ClearInfo();
+  Debug* debug = reinterpret_cast<Isolate*>(data.GetIsolate())->debug();
   debug->RemoveDebugInfo(node);
 #ifdef DEBUG
   for (DebugInfoListNode* n = debug->debug_info_list_;
@@ -610,17 +611,20 @@ void Debug::HandlePhantomDebugInfo(
 DebugInfoListNode::DebugInfoListNode(DebugInfo* debug_info): next_(NULL) {
   // Globalize the request debug info object and make it weak.
   GlobalHandles* global_handles = debug_info->GetIsolate()->global_handles();
-  debug_info_ = Handle<DebugInfo>::cast(global_handles->Create(debug_info));
-  typedef PhantomCallbackData<void>::Callback Callback;
+  debug_info_ =
+      Handle<DebugInfo>::cast(global_handles->Create(debug_info)).location();
+  typedef WeakCallbackInfo<void>::Callback Callback;
   GlobalHandles::MakeWeak(
-      reinterpret_cast<Object**>(debug_info_.location()), this,
+      reinterpret_cast<Object**>(debug_info_), this,
       reinterpret_cast<Callback>(Debug::HandlePhantomDebugInfo),
       v8::WeakCallbackType::kParameter);
 }
 
 
-DebugInfoListNode::~DebugInfoListNode() {
-  GlobalHandles::Destroy(reinterpret_cast<Object**>(debug_info_.location()));
+void DebugInfoListNode::ClearInfo() {
+  if (debug_info_ == nullptr) return;
+  GlobalHandles::Destroy(reinterpret_cast<Object**>(debug_info_));
+  debug_info_ = nullptr;
 }
 
 

src/debug.h

@@ -261,15 +261,17 @@ class ScriptCache : private HashMap {
 class DebugInfoListNode {
  public:
   explicit DebugInfoListNode(DebugInfo* debug_info);
-  virtual ~DebugInfoListNode();
+  virtual ~DebugInfoListNode() { ClearInfo(); }
 
   DebugInfoListNode* next() { return next_; }
   void set_next(DebugInfoListNode* next) { next_ = next; }
-  Handle<DebugInfo> debug_info() { return debug_info_; }
+  Handle<DebugInfo> debug_info() { return Handle<DebugInfo>(debug_info_); }
+
+  void ClearInfo();
 
  private:
   // Global (weak) handle to the debug info object.
-  Handle<DebugInfo> debug_info_;
+  DebugInfo** debug_info_;
 
   // Next pointer for linked list.
   DebugInfoListNode* next_;

src/global-handles.cc

@@ -264,38 +264,28 @@ class GlobalHandles::Node {
     if (weak_callback_ != NULL) {
       if (weakness_type() == NORMAL_WEAK) return;
 
-      v8::Isolate* api_isolate = reinterpret_cast<v8::Isolate*>(isolate);
-
       DCHECK(weakness_type() == PHANTOM_WEAK ||
              weakness_type() == PHANTOM_WEAK_2_INTERNAL_FIELDS);
 
-      Object* internal_field0 = nullptr;
-      Object* internal_field1 = nullptr;
-      if (weakness_type() != PHANTOM_WEAK) {
-        if (object()->IsJSObject()) {
-          JSObject* jsobject = JSObject::cast(object());
-          int field_count = jsobject->GetInternalFieldCount();
-          if (field_count > 0) {
-            internal_field0 = jsobject->GetInternalField(0);
-            if (!internal_field0->IsSmi()) internal_field0 = nullptr;
-          }
-          if (field_count > 1) {
-            internal_field1 = jsobject->GetInternalField(1);
-            if (!internal_field1->IsSmi()) internal_field1 = nullptr;
-          }
+      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 harmless value.
-      *location() = Smi::FromInt(0);
-      typedef v8::WeakCallbackInfo<void> Data;
-
-      Data data(api_isolate, parameter(), internal_field0, internal_field1);
-      Data::Callback callback =
-          reinterpret_cast<Data::Callback>(weak_callback_);
+      // 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, data, callback));
+          PendingPhantomCallback(this, callback, parameter(), internal_fields));
       DCHECK(IsInUse());
       set_state(NEAR_DEATH);
     }
@@ -838,17 +828,50 @@ void GlobalHandles::UpdateListOfNewSpaceNodes() {
 
 int GlobalHandles::DispatchPendingPhantomCallbacks() {
   int freed_nodes = 0;
+  {
+    // The initial pass callbacks must simply clear the nodes.
+    for (auto i = pending_phantom_callbacks_.begin();
+         i != pending_phantom_callbacks_.end(); ++i) {
+      auto callback = i;
+      // Skip callbacks that have already been processed once.
+      if (callback->node() == nullptr) continue;
+      callback->Invoke(isolate());
+      freed_nodes++;
+    }
+  }
+  // The second pass empties the list.
   while (pending_phantom_callbacks_.length() != 0) {
-    PendingPhantomCallback callback = pending_phantom_callbacks_.RemoveLast();
-    DCHECK(callback.node()->IsInUse());
-    callback.invoke();
-    DCHECK(!callback.node()->IsInUse());
-    freed_nodes++;
+    auto callback = pending_phantom_callbacks_.RemoveLast();
+    DCHECK(callback.node() == nullptr);
+    // No second pass callback required.
+    if (callback.callback() == nullptr) continue;
+    // Fire second pass callback.
+    callback.Invoke(isolate());
   }
   return freed_nodes;
 }
 
 
+void GlobalHandles::PendingPhantomCallback::Invoke(Isolate* isolate) {
+  Data::Callback* callback_addr = nullptr;
+  if (node_ != nullptr) {
+    // Initialize for first pass callback.
+    DCHECK(node_->state() == Node::NEAR_DEATH);
+    callback_addr = &callback_;
+  }
+  Data data(reinterpret_cast<v8::Isolate*>(isolate), parameter_,
+            internal_fields_, callback_addr);
+  Data::Callback callback = callback_;
+  callback_ = nullptr;
+  callback(data);
+  if (node_ != nullptr) {
+    // Transition to second pass state.
+    DCHECK(node_->state() == Node::FREE);
+    node_ = nullptr;
+  }
+}
+
+
 int GlobalHandles::PostGarbageCollectionProcessing(GarbageCollector collector) {
   // Process weak global handle callbacks. This must be done after the
   // GC is completely done, because the callbacks may invoke arbitrary
@@ -879,14 +902,6 @@ int GlobalHandles::PostGarbageCollectionProcessing(GarbageCollector collector) {
 }
 
 
-void GlobalHandles::PendingPhantomCallback::invoke() {
-  if (node_->state() == Node::FREE) return;
-  DCHECK(node_->state() == Node::NEAR_DEATH);
-  callback_(data_);
-  if (node_->state() != Node::FREE) node_->Release();
-}
-
-
 void GlobalHandles::IterateStrongRoots(ObjectVisitor* v) {
   for (NodeIterator it(this); !it.done(); it.Advance()) {
     if (it.node()->IsStrongRetainer()) {

src/global-handles.h

@@ -349,17 +349,25 @@ class GlobalHandles {
 class GlobalHandles::PendingPhantomCallback {
  public:
   typedef v8::WeakCallbackInfo<void> Data;
-  PendingPhantomCallback(Node* node, Data data, Data::Callback callback)
-      : node_(node), data_(data), callback_(callback) {}
+  PendingPhantomCallback(
+      Node* node, Data::Callback callback, void* parameter,
+      void* internal_fields[v8::kInternalFieldsInWeakCallback])
+      : node_(node), callback_(callback), parameter_(parameter) {
+    for (int i = 0; i < v8::kInternalFieldsInWeakCallback; ++i) {
+      internal_fields_[i] = internal_fields[i];
+    }
+  }
 
-  void invoke();
+  void Invoke(Isolate* isolate);
 
   Node* node() { return node_; }
+  Data::Callback callback() { return callback_; }
 
  private:
   Node* node_;
-  Data data_;
   Data::Callback callback_;
+  void* parameter_;
+  void* internal_fields_[v8::kInternalFieldsInWeakCallback];
 };
 
 

test/cctest/test-api.cc

@@ -3099,6 +3099,123 @@ THREADED_TEST(Global) {
 }
 
 
+namespace {
+
+class TwoPassCallbackData;
+void FirstPassCallback(const v8::WeakCallbackInfo<TwoPassCallbackData>& data);
+void SecondPassCallback(const v8::WeakCallbackInfo<TwoPassCallbackData>& data);
+
+
+class TwoPassCallbackData {
+ public:
+  TwoPassCallbackData(v8::Isolate* isolate, int* instance_counter)
+      : first_pass_called_(false),
+        second_pass_called_(false),
+        trigger_gc_(false),
+        instance_counter_(instance_counter) {
+    HandleScope scope(isolate);
+    i::ScopedVector<char> buffer(40);
+    i::SNPrintF(buffer, "%p", static_cast<void*>(this));
+    auto string =
+        v8::String::NewFromUtf8(isolate, buffer.start(),
+                                v8::NewStringType::kNormal).ToLocalChecked();
+    cell_.Reset(isolate, string);
+    (*instance_counter_)++;
+  }
+
+  ~TwoPassCallbackData() {
+    CHECK(first_pass_called_);
+    CHECK(second_pass_called_);
+    CHECK(cell_.IsEmpty());
+    (*instance_counter_)--;
+  }
+
+  void FirstPass() {
+    CHECK(!first_pass_called_);
+    CHECK(!second_pass_called_);
+    CHECK(!cell_.IsEmpty());
+    cell_.Reset();
+    first_pass_called_ = true;
+  }
+
+  void SecondPass() {
+    CHECK(first_pass_called_);
+    CHECK(!second_pass_called_);
+    CHECK(cell_.IsEmpty());
+    second_pass_called_ = true;
+    delete this;
+  }
+
+  void SetWeak() {
+    cell_.SetWeak(this, FirstPassCallback, v8::WeakCallbackType::kParameter);
+  }
+
+  void MarkTriggerGc() { trigger_gc_ = true; }
+  bool trigger_gc() { return trigger_gc_; }
+
+  int* instance_counter() { return instance_counter_; }
+
+ private:
+  bool first_pass_called_;
+  bool second_pass_called_;
+  bool trigger_gc_;
+  v8::Global<v8::String> cell_;
+  int* instance_counter_;
+};
+
+
+void SecondPassCallback(const v8::WeakCallbackInfo<TwoPassCallbackData>& data) {
+  ApiTestFuzzer::Fuzz();
+  bool trigger_gc = data.GetParameter()->trigger_gc();
+  int* instance_counter = data.GetParameter()->instance_counter();
+  data.GetParameter()->SecondPass();
+  if (!trigger_gc) return;
+  auto data_2 = new TwoPassCallbackData(data.GetIsolate(), instance_counter);
+  data_2->SetWeak();
+  CcTest::heap()->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
+}
+
+
+void FirstPassCallback(const v8::WeakCallbackInfo<TwoPassCallbackData>& data) {
+  data.GetParameter()->FirstPass();
+  data.SetSecondPassCallback(SecondPassCallback);
+}
+
+}  // namespace
+
+
+TEST(TwoPassPhantomCallbacks) {
+  auto isolate = CcTest::isolate();
+  const size_t kLength = 20;
+  int instance_counter = 0;
+  for (size_t i = 0; i < kLength; ++i) {
+    auto data = new TwoPassCallbackData(isolate, &instance_counter);
+    data->SetWeak();
+  }
+  CHECK_EQ(static_cast<int>(kLength), instance_counter);
+  CcTest::heap()->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
+  CHECK_EQ(0, instance_counter);
+}
+
+
+TEST(TwoPassPhantomCallbacksNestedGc) {
+  auto isolate = CcTest::isolate();
+  const size_t kLength = 20;
+  TwoPassCallbackData* array[kLength];
+  int instance_counter = 0;
+  for (size_t i = 0; i < kLength; ++i) {
+    array[i] = new TwoPassCallbackData(isolate, &instance_counter);
+    array[i]->SetWeak();
+  }
+  array[5]->MarkTriggerGc();
+  array[10]->MarkTriggerGc();
+  array[15]->MarkTriggerGc();
+  CHECK_EQ(static_cast<int>(kLength), instance_counter);
+  CcTest::heap()->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask);
+  CHECK_EQ(0, instance_counter);
+}
+
+
 template <typename K, typename V>
 class WeakStdMapTraits : public v8::StdMapTraits<K, V> {
  public:
@@ -3242,9 +3359,11 @@ class PhantomStdMapTraits : public v8::StdMapTraits<K, V> {
       v8::Isolate* isolate,
       const v8::WeakCallbackInfo<WeakCallbackDataType>& info, K key) {
     CHECK_EQ(IntKeyToVoidPointer(key), info.GetInternalField(0));
+    DisposeCallbackData(info.GetParameter());
   }
 };
-}
+
+}  // namespace
 
 
 TEST(GlobalValueMap) {
@@ -6381,12 +6500,13 @@ THREADED_TEST(ErrorWithMissingScriptInfo) {
 
 struct FlagAndPersistent {
   bool flag;
-  v8::Persistent<v8::Object> handle;
+  v8::Global<v8::Object> handle;
 };
 
 
 static void SetFlag(const v8::WeakCallbackInfo<FlagAndPersistent>& data) {
   data.GetParameter()->flag = true;
+  data.GetParameter()->handle.Reset();
 }