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

src/global-handles.cc

 // Copyright 2009 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 126 matching lines @@
     parameter_or_next_free_.parameter = parameter;
   }
   void* parameter() {
     ASSERT(state_ != DESTROYED);
     return parameter_or_next_free_.parameter;
   }
 
   // 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;
     set_parameter(NULL);
     // 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.
   Object* object_;  // Storage for object pointer.
 
   // Transition diagram:
   // NORMAL <-> WEAK -> PENDING -> NEAR_DEATH -> { NORMAL, WEAK, DESTROYED }
   enum State {
     NORMAL,      // Normal global handle.
     WEAK,        // Flagged as weak but not yet finalized.
@@ skipping 90 matching lines @@
     if (current->state_ == Node::WEAK) {
       if (f(&current->object_)) {
         current->state_ = Node::PENDING;
         LOG(HandleEvent("GlobalHandle::Pending", current->handle().location()));
       }
     }
   }
 }
 
 
+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;
       *p = node->next();  // Update the link.
       delete node;
     } else {
       p = (*p)->next_addr();
     }
   }
   set_first_free(NULL);
@@ skipping 82 matching lines @@
 void GlobalHandles::RemoveObjectGroups() {
   List<ObjectGroup*>* object_groups = ObjectGroups();
   for (int i = 0; i< object_groups->length(); i++) {
     delete object_groups->at(i);
   }
   object_groups->Clear();
 }
 
 
 } }  // namespace v8::internal