Potential memory leak in V8 extension

[GoogleCodeExporter]

See this thread for details:
http://magpcss.org/ceforum/viewtopic.php?f=6&t=287

Original issue reported on code.google.com by magreenb...@gmail.com on 1 Sep 2011 at 2:18

[GoogleCodeExporter]

This is not potential memory leak - this is *REAL* memory leak, in each usage 
of CefV8ValueImpl. I.e. in each V8Handler/Accessor and this is not depend from 
Externsion or JSBinding.

Problem can be demonstrated very easy via devtools console:
for(var i=0; i<10000; i++) cef.test.Dump(1);

(Choose correspond number of iterations (much better is use 1000000 and 
echoVoid call, but it not present in cefclient) - number of memory grows full 
depend from count of CefV8ValueImpl object count.

This is not GC issue of V8, V8 works as intended (as developed).

You will see that memory will grows upon no memory available.

Problem happens 'cause CefV8ValueHandle use V8::Persistent handles. 
V8::Persistent handles in fact means that they must explicitely disposed. And 
in ~CefV8ValueHandle is only mark handle as weak - and callback NEVER called. 
Also, as i known is not a V8 issue. Weak handles completely have different 
behaviour for objects and functions. Functions always destroyed only with 
context when it created, but objects can be collected with GC. So, as i think - 
V8::Persistent must be disposed when CefV8HandleImpl are disposed.

I do not know how to fix this problem in correct way, i do not test this 
solution very good. I run CEF only in release mode, but if we force delete of 
handles, as i propose before:
v8_impl.cc(385):
CefV8ValueHandle::~CefV8ValueHandle()
{
  if(tracker_) {
    TrackAdd(tracker_);
    v8_handle_.MakeWeak(tracker_, TrackDestructor);
    // TODO: when AdjustAmountOfExternalAllocatedMemory(sizeof(CefV8ValueImpl)) must be called?
  }
  else {
    v8_handle_.Dispose();
    v8_handle_.Clear();
  }
  tracker_ = NULL;
}

then even this code successfully executed without memory leaks:
for(var i=0; i<1000000; i++) cef.test.Dump(1);

I don't know how this problem must be solved in correct way. My way looks as 
works, but i have some doubts.
Please, do not ignore this issue - this is critical and blocker.

Original comment by fdd...@gmail.com on 15 Sep 2011 at 8:09

[GoogleCodeExporter]

I re-investigate this problem again.
For first, i put OutputDebugString in ~CefV8ValueHandle and in TrackDestructor.
Run DebugView (from SysinternalsSuite) it is only to view debug output.

Okay. Let's go:
Run cefclient:
In debugview:
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] TrackDesctuctor
[4596] TrackDesctuctor

(Sorry, i'm typo in TrackDesctuctor message, it means TrackDestructor function 
in v8_impl.cc).

Run DevTools:
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] TrackDesctuctor

Write in console gc() this is force GC collection:
gc(); 
No debug output (some times it shows, but this is not interested).

Let's go to call V8Handler.
cef_test.Dump("hello!");
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()

gc();
[4596] TrackDesctuctor
[4596] TrackDesctuctor

!!!!!!!!!!!!!!!!! one persistent handle can't be freed.

cef_test.Dump("hello!", "hello2!", 123.456);
[4596] ~CefV8ValueHandle() // args.This()
[4596] ~CefV8ValueHandle() // hello!
[4596] ~CefV8ValueHandle() // hello2!
[4596] ~CefV8ValueHandle() // 123.456
[4596] ~CefV8ValueHandle() // return value

gc();
[4596] TrackDesctuctor // hello!
[4596] TrackDesctuctor // hello2!
[4596] TrackDesctuctor // 123.456
[4596] TrackDesctuctor // return value

cef_test = null;
gc();
[4596] TrackDesctuctor  // args.This();
[4596] TrackDesctuctor  // args.This();
[4596] TrackDesctuctor  // some else - i don't know

It is demonstrates, that This() never freed 'cause This() object always 
referenced from global (window) object.
Weak references are destroyed only when object becomes unreacheable (i.e. only 
weak references stays to object). In our case we have strong reference and many 
weak references. This is issue and it is actual CEF's problem.

Same effect causes with v8 extension, but actually when i drop cef.test object 
from window - gc doesn't collect anything anyway. I don't know why - may be 
'cause object references shares across two windows (main window & devtools).

//////////////////////////////////////////////////////////////////
Another situation:
cef_test.Dump(1,true,null,undefined);
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()
[4596] ~CefV8ValueHandle()

gc();
[4596] TrackDesctuctor  // return value (which is string).

I don't know why TrackDestructor never been called for primitive values, such 
as undefined, null, false, true, and integers. For strings, doubles, regexps, 
objects, arrays - it executed as intended.

///////////////////////////////////////////////////////////////////
Another problem it is when you close browser window and or cefclient - any of 
destructors doesn't called.
It happens cause V8 is not call at all. It partially CAN (i'm not sure) related 
to v8 issue http://code.google.com/p/v8/issues/detail?id=1428 (v8 sources 
contain two TearDown methods with TODO on this issue).
In any case any GC never do explicit guarantees that finalizers will be called 
(even in Java/.NET run-time doesn't guarantee this, but unlike of V8 GC - they 
always execute finalizers :) ).
As temp workaround we can force GC before window will be closed.
But better if 1428 issue will be resolved and then we will see which effect of 
it.

///////////////////////////////////////////////////////////////////
Now i doesn't have any proposals and ideas how solve this issue.
May be, in most cases we do not needed to use persistent handles at all.
Only top-level (user) code can do any decisions about life-time of persistent 
objects.
Finalizer (weak callback), can only helps dispose persistent object handles 
more early.
So may be we need eliminate persistence from CefV8ValueImpl and provide 
explicit methods to mark objects as persistent / weak.
Please, let me know if you have any ideas about this.

Original comment by fdd...@gmail.com on 16 Sep 2011 at 7:15

[GoogleCodeExporter]

Revision 291 includes the V8 memory leak fix described above and the following 
related improvements:

- Make V8/CEF string conversions faster.
- Add a V8 performance testing tool to cefclient.

Thanks to fddima for doing most of the work on this.

Original comment by magreenb...@gmail.com on 20 Sep 2011 at 8:45

• Changed state: Fixed