From fdf70d37fa79abdd556a7cef6f999032cc81d341 Mon Sep 17 00:00:00 2001
From: "ager@chromium.org"
 <ager@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Fri, 21 Aug 2009 08:52:24 +0000
Subject: [PATCH] Land change to bail out from post garbage collection
 processing if another post gc processing was trigger because of weak
 callbacks.

Review URL: http://codereview.chromium.org/174141


git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@2737 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---
 src/global-handles.cc   | 40 ++++++++++++++++++++++++++--------------
 src/heap.cc             |  5 ++++-
 src/heap.h              | 20 ++++++++++++++++++++
 test/cctest/test-api.cc | 23 +++++++++++++++++++++++
 4 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/src/global-handles.cc b/src/global-handles.cc
index f868974..e51c4aa 100644
--- a/src/global-handles.cc
+++ b/src/global-handles.cc
@@ -144,8 +144,8 @@ class GlobalHandles::Node : public Malloced {
   // Returns the callback for this weak handle.
   WeakReferenceCallback callback() { return callback_; }
 
-  void PostGarbageCollectionProcessing() {
-    if (state_ != Node::PENDING) return;
+  bool PostGarbageCollectionProcessing() {
+    if (state_ != Node::PENDING) return false;
     LOG(HandleEvent("GlobalHandle::Processing", handle().location()));
     void* par = parameter();
     state_ = NEAR_DEATH;
@@ -153,18 +153,19 @@ class GlobalHandles::Node : public Malloced {
     // The callback function is resolved as late as possible to preserve old
     // behavior.
     WeakReferenceCallback func = callback();
-    if (func != NULL) {
-      v8::Persistent<v8::Object> object = ToApi<v8::Object>(handle());
-      {
-        // Forbid reuse of destroyed nodes as they might be already deallocated.
-        // It's fine though to reuse nodes that were destroyed in weak callback
-        // as those cannot be deallocated until we are back from the callback.
-        set_first_free(NULL);
-        // Leaving V8.
-        VMState state(EXTERNAL);
-        func(object, par);
-      }
+    if (func == NULL) return false;
+
+    v8::Persistent<v8::Object> object = ToApi<v8::Object>(handle());
+    {
+      // Forbid reuse of destroyed nodes as they might be already deallocated.
+      // It's fine though to reuse nodes that were destroyed in weak callback
+      // as those cannot be deallocated until we are back from the callback.
+      set_first_free(NULL);
+      // Leaving V8.
+      VMState state(EXTERNAL);
+      func(object, par);
     }
+    return true;
   }
 
   // Place the handle address first to avoid offset computation.
@@ -275,15 +276,26 @@ void GlobalHandles::IdentifyWeakHandles(WeakSlotCallback f) {
 }
 
 
+int post_gc_processing_count = 0;
+
 void GlobalHandles::PostGarbageCollectionProcessing() {
   // Process weak global handle callbacks. This must be done after the
   // GC is completely done, because the callbacks may invoke arbitrary
   // API functions.
   // At the same time deallocate all DESTROYED nodes
   ASSERT(Heap::gc_state() == Heap::NOT_IN_GC);
+  const int initial_post_gc_processing_count = ++post_gc_processing_count;
   Node** p = &head_;
   while (*p != NULL) {
-    (*p)->PostGarbageCollectionProcessing();
+    if ((*p)->PostGarbageCollectionProcessing()) {
+      if (initial_post_gc_processing_count != post_gc_processing_count) {
+        // Weak callback triggered another GC and another round of
+        // PostGarbageCollection processing.  The current node might
+        // have been deleted in that round, so we need to bail out (or
+        // restart the processing).
+        break;
+      }
+    }
     if ((*p)->state_ == Node::DESTROYED) {
       // Delete the link.
       Node* node = *p;
diff --git a/src/heap.cc b/src/heap.cc
index e778c96..ad25f93 100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -487,7 +487,10 @@ void Heap::PerformGarbageCollection(AllocationSpace space,
 
 void Heap::PostGarbageCollectionProcessing() {
   // Process weak handles post gc.
-  GlobalHandles::PostGarbageCollectionProcessing();
+  {
+    DisableAssertNoAllocation allow_allocation;
+    GlobalHandles::PostGarbageCollectionProcessing();
+  }
   // Update flat string readers.
   FlatStringReader::PostGarbageCollectionProcessing();
 }
diff --git a/src/heap.h b/src/heap.h
index a9d44c6..ac6f5be 100644
--- a/src/heap.h
+++ b/src/heap.h
@@ -1388,6 +1388,20 @@ class AssertNoAllocation {
   bool old_state_;
 };
 
+class DisableAssertNoAllocation {
+ public:
+  DisableAssertNoAllocation() {
+    old_state_ = Heap::allow_allocation(true);
+  }
+
+  ~DisableAssertNoAllocation() {
+    Heap::allow_allocation(old_state_);
+  }
+
+ private:
+  bool old_state_;
+};
+
 #else  // ndef DEBUG
 
 class AssertNoAllocation {
@@ -1396,6 +1410,12 @@ class AssertNoAllocation {
   ~AssertNoAllocation() { }
 };
 
+class DisableAssertNoAllocation {
+ public:
+  DisableAssertNoAllocation() { }
+  ~DisableAssertNoAllocation() { }
+};
+
 #endif
 
 #ifdef ENABLE_LOGGING_AND_PROFILING
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 9b69f6c..7b7e1a3 100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -6246,6 +6246,29 @@ THREADED_TEST(NewPersistentHandleFromWeakCallback) {
 }
 
 
+v8::Persistent<v8::Object> to_be_disposed;
+
+void DisposeAndForceGcCallback(v8::Persistent<v8::Value> handle, void*) {
+  to_be_disposed.Dispose();
+  i::Heap::CollectAllGarbage();
+}
+
+
+THREADED_TEST(DoNotUseDeletedNodesInSecondLevelGc) {
+  LocalContext context;
+
+  v8::Persistent<v8::Object> handle1, handle2;
+  {
+    v8::HandleScope scope;
+    handle1 = v8::Persistent<v8::Object>::New(v8::Object::New());
+    handle2 = v8::Persistent<v8::Object>::New(v8::Object::New());
+  }
+  handle1.MakeWeak(NULL, DisposeAndForceGcCallback);
+  to_be_disposed = handle2;
+  i::Heap::CollectAllGarbage();
+}
+
+
 THREADED_TEST(CheckForCrossContextObjectLiterals) {
   v8::V8::Initialize();
 
-- 
2.7.4