Bug 22710 - Memory leak due to circular reference Document->DOMTimer->ScheduledAction->[JS objects]->Document
Summary: Memory leak due to circular reference Document->DOMTimer->ScheduledAction->[J...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-06 04:54 PST by Dmitry Titov
Modified: 2008-12-07 18:05 PST (History)
1 user (show)

See Also:


Attachments
proposed fix (1.47 KB, patch)
2008-12-06 05:04 PST, Dmitry Titov
ap: review+
Details | Formatted Diff | Diff
repro file (53 bytes, text/html)
2008-12-06 05:20 PST, Dmitry Titov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2008-12-06 04:54:36 PST
DOMTimer::stop() will stop the timer but not release the ScheduledAction which holds to a JSFunction which can hold onto a bunch of JS wrappers They can keep a reference back to Document that owns the DOMTimer. Hence, refcount on a Document never goes to 0.
Fix is to delete the ScheduledAction in DOMTimer::stop().
Comment 1 Dmitry Titov 2008-12-06 05:04:29 PST
Created attachment 25813 [details]
proposed fix
Comment 2 Dmitry Titov 2008-12-06 05:20:28 PST
Created attachment 25814 [details]
repro file

repro file by ap@webkit.org.
Set a breakpoint at DOMTimer::stop and ~DOMTimer. Load the file and then close the window.
You should see stop() called, then ~DOMTimer() called when Document is destroyed.
Before the fix applied, you see only stop(). After the patch with fix applied, both are invoked.
Comment 3 Alexey Proskuryakov 2008-12-06 07:48:19 PST
Comment on attachment 25813 [details]
proposed fix

r=me

I don't think the added null check in DOMTimer destructor is useful, but it isn't harmful either.
Comment 4 Alexey Proskuryakov 2008-12-06 08:04:41 PST
Committed revision 39066.

Comment 5 Darin Adler 2008-12-07 15:15:07 PST
Can we make a test case for this?
Comment 6 Dmitry Titov 2008-12-07 15:52:38 PST
(In reply to comment #5)
> Can we make a test case for this?

I think it's doable by adding a method to LayoutTestController (getJSObjectCount() which will do GCController::collect first). This way, it's possible to navigate say iframe to a test page and back and then see the number of objects alive.
But perhaps there is a simpler way that I don't see?
Comment 7 Dmitry Titov 2008-12-07 18:05:59 PST
Of course there is :-) Patch with test is here: bug 22730