Bug 83057 - [v8] Fix memory leak in V8LazyEventListener
Summary: [v8] Fix memory leak in V8LazyEventListener
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-03 12:32 PDT by Erik Arvidsson
Modified: 2014-12-16 00:47 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.07 KB, patch)
2012-04-03 12:35 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-04-03 12:32:16 PDT
[v8] Fix memory leak in V8LazyEventListener
Comment 1 Erik Arvidsson 2012-04-03 12:35:03 PDT
Created attachment 135395 [details]
Patch
Comment 2 David Levin 2012-04-03 12:53:34 PDT
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.)
Comment 3 David Levin 2012-04-03 12:55:27 PDT
(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.
Comment 4 Ojan Vafai 2012-04-03 13:31:26 PDT
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.
Comment 5 Erik Arvidsson 2012-04-03 13:59:21 PDT
Committed r113086: <http://trac.webkit.org/changeset/113086>
Comment 6 Zoltan Arvai 2012-04-04 03:06:18 PDT
fast/dom/inline-event-attributes-release.html
layout test fails or flaky on bots:
- Qt Linux Release
- Lion debug
Comment 7 Csaba Osztrogonác 2012-04-04 03:17:52 PDT
(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?
Comment 8 Erik Arvidsson 2012-04-04 10:12:58 PDT
(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
Comment 9 Ojan Vafai 2012-04-04 10:25:19 PDT
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?
Comment 10 Geoffrey Garen 2012-04-04 10:37:51 PDT
> 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 :(.
Comment 11 Ojan Vafai 2012-04-04 10:58:46 PDT
(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.
Comment 12 Brian Burg 2014-12-16 00:47:50 PST
Closing some V8-related work items.