Bug 27163 - Fix memory leak in V8 bindings
Summary: Fix memory leak in V8 bindings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-10 16:33 PDT by Mads Ager
Modified: 2009-07-13 02:33 PDT (History)
4 users (show)

See Also:


Attachments
patch to avoid leaking contexts (1.61 KB, patch)
2009-07-10 16:34 PDT, Mads Ager
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mads Ager 2009-07-10 16:33:12 PDT
Reinitializing the context when clearing the V8 proxy for navigation makes us hold on to a context object per frame that we should not hold on to.  Reinitialization is not necessary.

When updating the document wrapper cache, we check that the global object handle is not empty, we should be checking the context.
Comment 1 Mads Ager 2009-07-10 16:34:15 PDT
Created attachment 32591 [details]
patch to avoid leaking contexts
Comment 2 Adam Barth 2009-07-10 23:30:41 PDT
Comment on attachment 32591 [details]
patch to avoid leaking contexts

These seem like two independent changes, but that's ok.  I don't understand why calling initContextIfNeeded() in clearForNavigation() leads to a leak, but it makes sense that this isn't needed because all the other entry points are super excited about making sure the context is initialized before they use it.

I'm r+ing this, but it would be helpful for Dimitri to give his opinion as well.
Comment 3 Dimitri Glazkov (Google) 2009-07-11 08:28:16 PDT
I am pretty sure this will work, but please make sure this doesn't break any layout tests.
Comment 4 Mads Ager 2009-07-11 10:23:20 PDT
I ran all layout tests in both release and debug mode before uploading the patch and saw no new failures with this change.

Adam, Dimitri, could one of you land this patch for me?
Comment 5 Adam Barth 2009-07-11 10:49:13 PDT
(In reply to comment #4)
> Adam, Dimitri, could one of you land this patch for me?

Sure, but we should wait for the canary to compile before landing this so we can see if there are regressions.
Comment 6 David Levin 2009-07-13 02:18:50 PDT
A note for the future: Please make sure to put a link to the bug in the changelog.
Comment 7 David Levin 2009-07-13 02:33:11 PDT
Committed as http://trac.webkit.org/changeset/45797