From beb198fca9d3a97b6f24eec655c6ebde5dfb01d5 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Thu, 29 May 2014 21:25:19 +0000 Subject: [PATCH] Change CefV8ValueImpl::Handle lifespan so that persistent V8 handles can be reset after execution of the weak persistent destructor. The previous implementation reset persistent V8 handles immediately after calling SetWeak which caused a memory leak due to the weak persistent destructor never being executed (issue #1278). git-svn-id: https://chromiumembedded.googlecode.com/svn/trunk@1712 5089003a-bbd8-11dd-ad1f-f1f9622dbc98 --- libcef/renderer/v8_impl.cc | 157 +++++++++++++++++++++---------------- libcef/renderer/v8_impl.h | 26 +++--- 2 files changed, 99 insertions(+), 84 deletions(-) diff --git a/libcef/renderer/v8_impl.cc b/libcef/renderer/v8_impl.cc index 414634a55..7e15697c0 100644 --- a/libcef/renderer/v8_impl.cc +++ b/libcef/renderer/v8_impl.cc @@ -358,63 +358,6 @@ class V8TrackString : public CefTrackNode { }; -// Manages the life span of a CefTrackNode associated with a persistent Object -// or Function. -class CefV8MakeWeakParam { - public: - CefV8MakeWeakParam(v8::Isolate* isolate, - scoped_refptr context_state, - CefTrackNode* object) - : isolate_(isolate), - context_state_(context_state), - object_(object) { - DCHECK(isolate_); - DCHECK(object_); - - isolate_->AdjustAmountOfExternalAllocatedMemory( - static_cast(sizeof(CefV8MakeWeakParam))); - - if (context_state_.get()) { - // |object_| will be deleted when: - // A. The associated context is released, or - // B. TrackDestructor is called for the weak handle. - DCHECK(context_state_->IsValid()); - context_state_->AddTrackObject(object_); - } else { - // |object_| will be deleted when: - // A. The process shuts down, or - // B. TrackDestructor is called for the weak handle. - GetIsolateManager()->AddGlobalTrackObject(object_); - } - } - ~CefV8MakeWeakParam() { - if (context_state_.get()) { - // If the associated context is still valid then delete |object_|. - // Otherwise, |object_| will already have been deleted. - if (context_state_->IsValid()) - context_state_->DeleteTrackObject(object_); - } else { - GetIsolateManager()->DeleteGlobalTrackObject(object_); - } - - isolate_->AdjustAmountOfExternalAllocatedMemory( - -static_cast(sizeof(CefV8MakeWeakParam))); - } - - private: - v8::Isolate* isolate_; - scoped_refptr context_state_; - CefTrackNode* object_; -}; - -// Callback for weak persistent reference destruction. -void TrackDestructor( - const v8::WeakCallbackData& data) { - if (data.GetParameter()) - delete data.GetParameter(); -} - - // Convert a CefString to a V8::String. v8::Handle GetV8String(v8::Isolate* isolate, const CefString& str) { @@ -1035,24 +978,98 @@ blink::WebFrame* CefV8ContextImpl::GetWebFrame() { // CefV8ValueImpl::Handle +CefV8ValueImpl::Handle::Handle(v8::Isolate* isolate, + v8::Handle context, + handleType v, + CefTrackNode* tracker) + : CefV8HandleBase(isolate, context), + handle_(isolate, v), + tracker_(tracker), + should_persist_(false), + is_set_weak_(false) { +} + CefV8ValueImpl::Handle::~Handle() { - // Persist the handle (call MakeWeak) if: - // A. The handle has been passed into a V8 function or used as a return value - // from a V8 callback, and - // B. The associated context, if any, is still valid. - if (should_persist_ && (!context_state_.get() || context_state_->IsValid())) { - handle_.SetWeak( - (tracker_ ? - new CefV8MakeWeakParam(isolate(), context_state_, tracker_) : NULL), - TrackDestructor); - } else if (tracker_) { - delete tracker_; + DCHECK(BelongsToCurrentThread()); + + if (tracker_) { + if (is_set_weak_) { + if (context_state_.get()) { + // If the associated context is still valid then delete |tracker_|. + // Otherwise, |tracker_| will already have been deleted. + if (context_state_->IsValid()) + context_state_->DeleteTrackObject(tracker_); + } else { + GetIsolateManager()->DeleteGlobalTrackObject(tracker_); + } + + isolate_->AdjustAmountOfExternalAllocatedMemory( + -static_cast(sizeof(Handle))); + } else { + delete tracker_; + } } // Always call Reset() on a persistent handle to avoid the // CHECK(state() != NEAR_DEATH) in V8's PostGarbageCollectionProcessing. handle_.Reset(); - tracker_ = NULL; +} + +CefV8ValueImpl::Handle::handleType +CefV8ValueImpl::Handle::GetNewV8Handle(bool should_persist) { + DCHECK(IsValid()); + if (should_persist && !should_persist_) + should_persist_ = true; + return handleType::New(isolate(), handle_); +} + +CefV8ValueImpl::Handle::persistentType& +CefV8ValueImpl::Handle::GetPersistentV8Handle() { + return handle_; +} + +void CefV8ValueImpl::Handle::SetWeakIfNecessary() { + if (!BelongsToCurrentThread()) { + task_runner()->PostTask(FROM_HERE, + base::Bind(&CefV8ValueImpl::Handle::SetWeakIfNecessary, this)); + return; + } + + // Persist the handle (call SetWeak) if: + // A. The handle has been passed into a V8 function or used as a return value + // from a V8 callback, and + // B. The associated context, if any, is still valid. + if (should_persist_ && (!context_state_.get() || context_state_->IsValid())) { + is_set_weak_ = true; + + if (tracker_) { + if (context_state_.get()) { + // |tracker_| will be deleted when: + // A. The associated context is released, or + // B. Destructor is called for the weak handle. + DCHECK(context_state_->IsValid()); + context_state_->AddTrackObject(tracker_); + } else { + // |tracker_| will be deleted when: + // A. The process shuts down, or + // B. Destructor is called for the weak handle. + GetIsolateManager()->AddGlobalTrackObject(tracker_); + } + } + + isolate_->AdjustAmountOfExternalAllocatedMemory( + static_cast(sizeof(Handle))); + + // The added reference will be released in Destructor. + AddRef(); + handle_.SetWeak(this, Destructor); + } +} + +// static +void CefV8ValueImpl::Handle::Destructor( + const v8::WeakCallbackData& data) { + data.GetParameter()->Release(); } @@ -1263,6 +1280,8 @@ CefV8ValueImpl::CefV8ValueImpl(v8::Isolate* isolate, CefV8ValueImpl::~CefV8ValueImpl() { if (type_ == TYPE_STRING) cef_string_clear(&string_value_); + if (handle_) + handle_->SetWeakIfNecessary(); } void CefV8ValueImpl::InitFromV8Value(v8::Handle value) { diff --git a/libcef/renderer/v8_impl.h b/libcef/renderer/v8_impl.h index c2178c3fa..09935a2db 100644 --- a/libcef/renderer/v8_impl.h +++ b/libcef/renderer/v8_impl.h @@ -281,26 +281,19 @@ class CefV8ValueImpl : public CefV8Value { Handle(v8::Isolate* isolate, v8::Handle context, handleType v, - CefTrackNode* tracker) - : CefV8HandleBase(isolate, context), - handle_(isolate, v), - tracker_(tracker), - should_persist_(false) { - } + CefTrackNode* tracker); virtual ~Handle(); - handleType GetNewV8Handle(bool should_persist) { - DCHECK(IsValid()); - if (should_persist && !should_persist_) - should_persist_ = true; - return handleType::New(isolate(), handle_); - } + handleType GetNewV8Handle(bool should_persist); - persistentType& GetPersistentV8Handle() { - return handle_; - } + persistentType& GetPersistentV8Handle(); + + void SetWeakIfNecessary(); private: + // Callback for weak persistent reference destruction. + static void Destructor(const v8::WeakCallbackData& data); + persistentType handle_; // For Object and Function types, we need to hold on to a reference to their @@ -310,6 +303,9 @@ class CefV8ValueImpl : public CefV8Value { // True if the handle needs to persist due to it being passed into V8. bool should_persist_; + // True if the handle has been set as weak. + bool is_set_weak_; + DISALLOW_COPY_AND_ASSIGN(Handle); };