Bug 83057 – [v8] Fix memory leak in V8LazyEventListener

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Erik Arvidsson]

[v8] Fix memory leak in V8LazyEventListener

[Erik Arvidsson]

Created attachment 135395 [details]
Patch

[David Levin]

Comment on attachment 135395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135395&action=review

> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:53
> +V8LazyEventListener::V8LazyEventListener(const AtomicString& functionName, const AtomicString& eventParameterName, const String& code, const String sourceURL, const TextPosition& position, Node* node, const WorldContextHandle& worldContext)

I think a PassRefPtr is appropriate here since the m_node is a RefPtr. (Of course if no one is calling using a PassRefPtr or release, then it doesn't really matter.)

[David Levin]

(In reply to comment #2)
> (From update of attachment 135395 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135395&action=review
> 
> > Source/WebCore/bindings/v8/V8LazyEventListener.cpp:53
> > +V8LazyEventListener::V8LazyEventListener(const AtomicString& functionName, const AtomicString& eventParameterName, const String& code, const String sourceURL, const TextPosition& position, Node* node, const WorldContextHandle& worldContext)
> 
> I think a PassRefPtr is appropriate here since the m_node is a RefPtr. (Of course if no one is calling using a PassRefPtr or release, then it doesn't really matter.)

Whoops, you're changing that. Sorry about that. Nevermind.

[Ojan Vafai]

Comment on attachment 135395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135395&action=review

> Source/WebCore/bindings/v8/V8LazyEventListener.cpp:217
> +    // Since we only parse once, there's no need to keep data used for parsing around anymore.
> +    m_functionName = String();
> +    m_code = String();
> +    m_eventParameterName = String();
> +    m_sourceURL = String();

Nice change. Deserves a mention in the changelog since it's not directly fixing the memory leak.

[Erik Arvidsson]

Committed r113086: <http://trac.webkit.org/changeset/113086>

[Zoltan Arvai]

fast/dom/inline-event-attributes-release.html
layout test fails or flaky on bots:
- Qt Linux Release
- Lion debug

[Csaba Osztrogonác]

(In reply to comment #6)
> fast/dom/inline-event-attributes-release.html
> layout test fails or flaky on bots:
> - Qt Linux Release
> - Lion debug

Reopen based on this comment. Have you got any idea why?

[Erik Arvidsson]

(In reply to comment #7)
> (In reply to comment #6)
> > fast/dom/inline-event-attributes-release.html
> > layout test fails or flaky on bots:
> > - Qt Linux Release
> > - Lion debug
> 
> Reopen based on this comment. Have you got any idea why?

I'm not sure but my gut feeling is that window.internals.numberOfLiveNodes() is not working as expected. I am the first one to use this method in a layout test so maybe it was never intended for this use case.

I'm fine removing this test even though I feel like having something like this is useful

[Ojan Vafai]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showExpectations=true&tests=fast%2Fdom%2Finline-event-attributes-release.html

Output on these is: FAIL afterCount - beforeCount should be 0. Was 1.

It looks to me like JSC might have an actual memory leak here, which is what this test is testing. Or maybe JSC's gc needs to run more than once to gc some objects?

Geoff, wdyt? Are there other JSC people we should CC who might have thoughts?

[Geoffrey Garen]

> Or maybe JSC's gc needs to run more than once to gc some objects?

No, that's not the case.

> Geoff, wdyt? Are there other JSC people we should CC who might have thoughts?

No immediate ideas without debugging it :(.

[Ojan Vafai]

(In reply to comment #10)
> > Geoff, wdyt? Are there other JSC people we should CC who might have thoughts?
> 
> No immediate ideas without debugging it :(.

Strangely, this test started passing for GTK after http://trac.webkit.org/changeset/113141/ and randomly passed on a couple runs on Qt Linux Release.

Seems likely the test is flaky with JSC, whic looks like a genuine bug to me (or a bug in numberOfLiveNodes). In either case, the test seems fine. We should skip it on the Apple ports for now unless there is someone in JSC land who wants to look into it.

[Brian Burg]

Closing some V8-related work items.