Bug 29932 – [CHROMIUM] Memory leak in V8EventListenerList

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Stephen White]

When running test_shell_tests under purify, it appears that V8EventListenerList does not always clean up its Vector<V8EventListener*>*.

See http://crbug.com/23396.

[Stephen White]

Created attachment 40388 [details]
Proposed fix for leak

I'm not 100% sure of this fix, and can't check it until the Purify run, so comments from someone more familiar with this code would be appreciated.

[Adam Barth]

Comment on attachment 40388 [details]
Proposed fix for leak

I'm not sure this is right.  From the code, it looks like these vectors get deleted when they're empty.  If they're still around, then they must not be empty.  Are we leaking the objects contained in the vector?  Those objects seem to be refcounted, but I don't see that we're grabbing a reference to them....

In any case, I'm probably not the right person to review this change.  We need an expert on this code.

In general, I'd like to see this code become easier to understand.  Ideally we'd use some of the OwnPtr / RefPtr self-documenting classes to explain the expected ownership pattern.  A comment explaining the ownership pattern would be useful.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 40388 [details])
> I'm not sure this is right.  From the code, it looks like these vectors get
> deleted when they're empty.  If they're still around, then they must not be
> empty.  Are we leaking the objects contained in the vector?  Those objects seem
> to be refcounted, but I don't see that we're grabbing a reference to them....

That's my impression -- the V8EventListenerList owns the vector, but not the V8EventListeners contained in it.  The V8EventListeners seem to be owned by
the JS that created them (through refcounts), so the listeners may be longer-lived than the list holding them.

I did end up running these tests under Purify on my machine, before and after 
this change, and it does fix the leak, and doesn't introduce any other memory 
errors.

Another possible solution would be to remove() all the V8EventListeners 
remaining in the hash map, and allow it to delete the vector, which might be 
a bit cleaner.

> In any case, I'm probably not the right person to review this change.  We need
> an expert on this code.

Ok, I'll reassign.

> In general, I'd like to see this code become easier to understand.  Ideally
> we'd use some of the OwnPtr / RefPtr self-documenting classes to explain the
> expected ownership pattern.  A comment explaining the ownership pattern would
> be useful.

The ownership semantics weren't clear to me either, and using smart pointers
would definitely help.  However, I don't know the wtf classes well enough to 
implement this, e.g., is OwnPtr is HashMap-safe (the auto_ptr problem).

[Dimitri Glazkov (Google)]

Vitaly, does your patch make this obsolete?

[Eric Seidel (no email)]

Vitaly?  Can we cancel this review and close this bug, or should it still be open?

[Stephen White]

(In reply to comment #5)

This patch is obsolete, and the memory leak is fixed (no longer shows up on purify runs).  I'll close.

[Eric Seidel (no email)]

Comment on attachment 40388 [details]
Proposed fix for leak

Our "needs review" query is not smart enough to exclude resolved bugs.   Clearing flag.

[Vitaly Repeshko]

(In reply to comment #5)
> Vitaly?  Can we cancel this review and close this bug, or should it still be
> open?

Sorry, I'm on vacation now and not super-responsive. Yes, I think this was already fixed by https://bugs.webkit.org/show_bug.cgi?id=29825