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

@@ -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<CefV8ContextState> context_state,
-                     CefTrackNode* object)
-      : isolate_(isolate),
-        context_state_(context_state),
-        object_(object) {
-    DCHECK(isolate_);
-    DCHECK(object_);
-
-    isolate_->AdjustAmountOfExternalAllocatedMemory(
-        static_cast<int>(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<int>(sizeof(CefV8MakeWeakParam)));
-  }
-
- private:
-  v8::Isolate* isolate_;
-  scoped_refptr<CefV8ContextState> context_state_;
-  CefTrackNode* object_;
-};
-
-// Callback for weak persistent reference destruction.
-void TrackDestructor(
-    const v8::WeakCallbackData<v8::Value, CefV8MakeWeakParam>& data) {
-  if (data.GetParameter())
-    delete data.GetParameter();
-}
-
-
 // Convert a CefString to a V8::String.
 v8::Handle<v8::String> 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<v8::Context> 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:
+  DCHECK(BelongsToCurrentThread());
-  // A. The handle has been passed into a V8 function or used as a return value
+
-  //    from a V8 callback, and
+  if (tracker_) {
-  // B. The associated context, if any, is still valid.
+    if (is_set_weak_) {
-  if (should_persist_ && (!context_state_.get() || context_state_->IsValid())) {
+      if (context_state_.get()) {
-    handle_.SetWeak(
+        // If the associated context is still valid then delete |tracker_|.
-        (tracker_ ?
+        // Otherwise, |tracker_| will already have been deleted.
-            new CefV8MakeWeakParam(isolate(), context_state_, tracker_) : NULL),
+        if (context_state_->IsValid())
-        TrackDestructor);
+          context_state_->DeleteTrackObject(tracker_);
-  } else if (tracker_) {
+      } else {
+        GetIsolateManager()->DeleteGlobalTrackObject(tracker_);
+      }
+
+      isolate_->AdjustAmountOfExternalAllocatedMemory(
+          -static_cast<int>(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<int>(sizeof(Handle)));
+
+    // The added reference will be released in Destructor.
+    AddRef();
+    handle_.SetWeak(this, Destructor);
+  }
+}
+
+// static
+void CefV8ValueImpl::Handle::Destructor(
+    const v8::WeakCallbackData<v8::Value, Handle>& 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<v8::Value> value) {

libcef/renderer/v8_impl.h

@@ -281,26 +281,19 @@ class CefV8ValueImpl : public CefV8Value {
     Handle(v8::Isolate* isolate,
            v8::Handle<v8::Context> context,
            handleType v,
-           CefTrackNode* tracker)
+           CefTrackNode* tracker);
-        : CefV8HandleBase(isolate, context),
-          handle_(isolate, v),
-          tracker_(tracker),
-          should_persist_(false) {
-    }
     virtual ~Handle();
 
-    handleType GetNewV8Handle(bool should_persist) {
+    handleType GetNewV8Handle(bool should_persist);
-      DCHECK(IsValid());
-      if (should_persist && !should_persist_)
-        should_persist_ = true;
-      return handleType::New(isolate(), handle_);
-    }
 
-    persistentType& GetPersistentV8Handle() {
+    persistentType& GetPersistentV8Handle();
-      return handle_;
+
-    }
+    void SetWeakIfNecessary();
 
    private:
+    // Callback for weak persistent reference destruction.
+    static void Destructor(const v8::WeakCallbackData<v8::Value, Handle>& 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);
   };