Do not finish GlobalHandles::PostGarbageCollectionProcessing if weak callback...

src/global-handles.cc

Index: src/global-handles.cc
===================================================================
--- src/global-handles.cc	(revision 2724)
+++ src/global-handles.cc	(working copy)
@@ -144,8 +144,8 @@
   // 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 @@
     // 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,25 @@
 }
 
 
+int time_stamp = 0;

[Mads Ager (chromium), 2009/08/21 05:58:16]

You could expose Heap::mc_count() instead of introducing a new variable for
this.

[Mads Ager (chromium), 2009/08/21 06:09:28]

Talking to Kasper, maybe it would be better to use Heap::gc_count() instead so
that the fix does not assume that global handle processing only occurs when
using the MarkCompact collector.

+
 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 local_time_stamp = ++time_stamp;
   Node** p = &head_;
   while (*p != NULL) {
-    (*p)->PostGarbageCollectionProcessing();
+    if ((*p)->PostGarbageCollectionProcessing()) {
+      if (local_time_stamp != time_stamp) {
+        // Weak callback triggered another GC, the only safe thing

[Mads Ager (chromium), 2009/08/21 05:58:16]

Could you describe in the comment why it is unsafe to delete the DESTROYED
nodes: When we return to the other instance of PostGarbageCollectionProcessing
the p in the code below could have been deleted if it was Disposed in the
callback.

+        // we can do is either to restart PostGarbageCollectionProcessing
+        // or abort it, let's abort as it's simpler :)

[Mads Ager (chromium), 2009/08/21 05:58:16]

Terminate comment with '.' and remove the smiley.  :)

+        break;
+      }
+    }
     if ((*p)->state_ == Node::DESTROYED) {
       // Delete the link.
       Node* node = *p;