diff --git a/include/v8-util.h b/include/v8-util.h index 21990a8fd9..b01d527754 100644 --- a/include/v8-util.h +++ b/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 p; + p.val_ = FromVal(Traits::Remove(&impl_, key)); + p.Reset(); + } private: PersistentValueMapBase(PersistentValueMapBase&); @@ -476,7 +480,6 @@ class GlobalValueMap : public PersistentValueMapBase { K key = Traits::KeyFromWeakCallbackInfo(data); persistentValueMap->RemoveWeak(key); Traits::DisposeWeak(data.GetIsolate(), data, key); - Traits::DisposeCallbackData(data.GetParameter()); } } }; diff --git a/include/v8.h b/include/v8.h index 1b1ab19c74..41240bd837 100644 --- a/include/v8.h +++ b/include/v8.h @@ -479,11 +479,13 @@ class WeakCallbackInfo { public: typedef void (*Callback)(const WeakCallbackInfo& 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]; }; diff --git a/src/debug.cc b/src/debug.cc index 32cbf6f8e6..2ef40c4edb 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -593,9 +593,10 @@ void ScriptCache::HandleWeakScript( void Debug::HandlePhantomDebugInfo( - const v8::PhantomCallbackData& data) { - Debug* debug = reinterpret_cast(data.GetIsolate())->debug(); + const v8::WeakCallbackInfo& data) { DebugInfoListNode* node = data.GetParameter(); + node->ClearInfo(); + Debug* debug = reinterpret_cast(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::cast(global_handles->Create(debug_info)); - typedef PhantomCallbackData::Callback Callback; + debug_info_ = + Handle::cast(global_handles->Create(debug_info)).location(); + typedef WeakCallbackInfo::Callback Callback; GlobalHandles::MakeWeak( - reinterpret_cast(debug_info_.location()), this, + reinterpret_cast(debug_info_), this, reinterpret_cast(Debug::HandlePhantomDebugInfo), v8::WeakCallbackType::kParameter); } -DebugInfoListNode::~DebugInfoListNode() { - GlobalHandles::Destroy(reinterpret_cast(debug_info_.location())); +void DebugInfoListNode::ClearInfo() { + if (debug_info_ == nullptr) return; + GlobalHandles::Destroy(reinterpret_cast(debug_info_)); + debug_info_ = nullptr; } diff --git a/src/debug.h b/src/debug.h index 73b8b00ec8..3a9f82388c 100644 --- a/src/debug.h +++ b/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 debug_info() { return debug_info_; } + Handle debug_info() { return Handle(debug_info_); } + + void ClearInfo(); private: // Global (weak) handle to the debug info object. - Handle debug_info_; + DebugInfo** debug_info_; // Next pointer for linked list. DebugInfoListNode* next_; diff --git a/src/global-handles.cc b/src/global-handles.cc index 0554c53bb5..25c6b4efda 100644 --- a/src/global-handles.cc +++ b/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(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); + // Zap with something dangerous. + *location() = reinterpret_cast(0x6057ca11); + typedef v8::WeakCallbackInfo Data; - - Data data(api_isolate, parameter(), internal_field0, internal_field1); - Data::Callback callback = - reinterpret_cast(weak_callback_); - + auto callback = reinterpret_cast(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(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()) { diff --git a/src/global-handles.h b/src/global-handles.h index d1d5d1ebe6..cb5619ffbe 100644 --- a/src/global-handles.h +++ b/src/global-handles.h @@ -349,17 +349,25 @@ class GlobalHandles { class GlobalHandles::PendingPhantomCallback { public: typedef v8::WeakCallbackInfo 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]; }; diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 5edbda777c..62cdda429c 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -3099,6 +3099,123 @@ THREADED_TEST(Global) { } +namespace { + +class TwoPassCallbackData; +void FirstPassCallback(const v8::WeakCallbackInfo& data); +void SecondPassCallback(const v8::WeakCallbackInfo& 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 buffer(40); + i::SNPrintF(buffer, "%p", static_cast(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 cell_; + int* instance_counter_; +}; + + +void SecondPassCallback(const v8::WeakCallbackInfo& 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& 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(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(kLength), instance_counter); + CcTest::heap()->CollectAllGarbage(i::Heap::kAbortIncrementalMarkingMask); + CHECK_EQ(0, instance_counter); +} + + template class WeakStdMapTraits : public v8::StdMapTraits { public: @@ -3242,9 +3359,11 @@ class PhantomStdMapTraits : public v8::StdMapTraits { v8::Isolate* isolate, const v8::WeakCallbackInfo& 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 handle; + v8::Global handle; }; static void SetFlag(const v8::WeakCallbackInfo& data) { data.GetParameter()->flag = true; + data.GetParameter()->handle.Reset(); }