Summary: | [v8] Fix memory leak in V8LazyEventListener | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||
Component: | New Bugs | Assignee: | Erik Arvidsson <arv> | ||||
Status: | RESOLVED INVALID | ||||||
Severity: | Normal | CC: | abarth, barraclough, fpizlo, ggaren, haraken, japhet, levin, ojan, ossy, webkit.review.bot, zarvai | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Erik Arvidsson
2012-04-03 12:32:16 PDT
Created attachment 135395 [details]
Patch
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.) (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 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. Committed r113086: <http://trac.webkit.org/changeset/113086> fast/dom/inline-event-attributes-release.html layout test fails or flaky on bots: - Qt Linux Release - Lion debug (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? (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 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? > 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 :(. (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. Closing some V8-related work items. |