Revision: 2737 Author: [email protected] Date: Fri Aug 21 01:52:24 2009 Log: 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 http://code.google.com/p/v8/source/detail?r=2737 Modified: /branches/bleeding_edge/src/global-handles.cc /branches/bleeding_edge/src/heap.cc /branches/bleeding_edge/src/heap.h /branches/bleeding_edge/test/cctest/test-api.cc ======================================= --- /branches/bleeding_edge/src/global-handles.cc Wed Aug 19 13:32:51 2009 +++ /branches/bleeding_edge/src/global-handles.cc Fri Aug 21 01:52:24 2009 @@ -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,26 @@ } +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; ======================================= --- /branches/bleeding_edge/src/heap.cc Fri Aug 21 01:44:21 2009 +++ /branches/bleeding_edge/src/heap.cc Fri Aug 21 01:52:24 2009 @@ -487,7 +487,10 @@ void Heap::PostGarbageCollectionProcessing() { // Process weak handles post gc. - GlobalHandles::PostGarbageCollectionProcessing(); + { + DisableAssertNoAllocation allow_allocation; + GlobalHandles::PostGarbageCollectionProcessing(); + } // Update flat string readers. FlatStringReader::PostGarbageCollectionProcessing(); } ======================================= --- /branches/bleeding_edge/src/heap.h Wed Aug 19 17:07:19 2009 +++ /branches/bleeding_edge/src/heap.h Fri Aug 21 01:52:24 2009 @@ -1383,6 +1383,20 @@ ~AssertNoAllocation() { Heap::allow_allocation(old_state_); } + + private: + bool old_state_; +}; + +class DisableAssertNoAllocation { + public: + DisableAssertNoAllocation() { + old_state_ = Heap::allow_allocation(true); + } + + ~DisableAssertNoAllocation() { + Heap::allow_allocation(old_state_); + } private: bool old_state_; @@ -1396,6 +1410,12 @@ ~AssertNoAllocation() { } }; +class DisableAssertNoAllocation { + public: + DisableAssertNoAllocation() { } + ~DisableAssertNoAllocation() { } +}; + #endif #ifdef ENABLE_LOGGING_AND_PROFILING ======================================= --- /branches/bleeding_edge/test/cctest/test-api.cc Thu Aug 20 23:30:59 2009 +++ /branches/bleeding_edge/test/cctest/test-api.cc Fri Aug 21 01:52:24 2009 @@ -6244,6 +6244,29 @@ handle2.Dispose(); i::Heap::CollectAllGarbage(); } + + +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-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
