Bug 126610 – Memory leak from windowScriptObjectAvailable

• Component: WebKit API
• Status: RESOLVED WORKSFORME

[Alex Christensen]

I'm working on finding a memory leak in Source/WebKit/win.  We are getting a JSContextRef from the call to windowScriptObjectAvailable in WebFrameLoaderClient::dispatchDidClearWindowObjectInWorld.  The JSContextRef has an extra reference to it that is not being dereferenced when we call WebView::close, and it's causing our JSObjectFinalizeCallback not to be called.  I'm not sure where exactly to look, but it feels like a release of some sort should be added to WebView::close.

[Alex Christensen]

Inspired by https://bugs.webkit.org/attachment.cgi?id=93818&action=prettypatch I added vm.heap.collectAllGarbage() to JSGlobalContextRelease and it fixed my problem without appearing to break anything.  That's not optimal performance, and that's certainly not a good fix, but I think it's close.  Is there a reason we aren't garbage collecting at all in JSGlobalContextRelease at all anymore?

[Alex Christensen]

Created attachment 220862 [details]
Patch

[Alex Christensen]

This patch feels super hacky, and I'm sure there's a better way, and I'm sure there are reasons to not garbage collect when closing a WebView.  I'm expecting an r-, but hopefully some insight about how to do this better.

It would be really nice if there were a way to initiate garbage collection from a JSContextRef, so an API user can tell JavaScriptCore that now is a good time to garbage collect based on the state of my application that JavaScript doesn't know about.

[Geoffrey Garen]

Comment on attachment 220862 [details]
Patch

A better way to do this is to call GCController::garbageCollectSoon(). Can you try that?

GCController::garbageCollectSoon() does a better job of batching collection requests.

[Alex Christensen]

Adding a call to GCController::garbageCollectNow to WebView::close solves my problem, but GCController::garbageCollectSoon does not.  I'm not sure if every user of WebKit on Windows would want the lag of garbage collecting when closing a WebView.

[Alex Christensen]

Created attachment 221181 [details]
Patch

[Alex Christensen]

I found a solution using the JavaScript API that works for me and doesn't force other WebKit users to garbage collect when they don't want to.  I call JSReportExtraMemoryCost(ctx,1000000000); then JSGarbageCollect(ctx); and it garbage collects when I open the next WebKit widget.  This is also hacky, but I can refine it on my end without changing WebKit.

[Mark Hahnenberg]

Where is the extra reference to the JSContextRef coming from? Maybe the ref count is zero, but you're not sweeping anything so finalization never happens? Does the IncrementalSweeper work on Windows?

collectAllGarbage also forces eager sweeping, which is why it would cause finalization to happen.

[Alex Christensen]

(In reply to comment #7)
> I call JSReportExtraMemoryCost(ctx,1000000000); then JSGarbageCollect(ctx);
For the record, this is a terrible solution, and the garbage collector keeps the memory usage to a few megabytes when JSReportExtraMemoryCost is used correctly, which is reasonable.

(In reply to comment #8)
> Where is the extra reference to the JSContextRef coming from? Maybe the ref count is zero, but you're not sweeping anything so finalization never happens? Does the IncrementalSweeper work on Windows?
I'm not sure, but the ref count of the JSContextRef is always between 2 and 4.  Doing extra releases causes crashes, so there's probably something global that I wasn't expecting to be global.  
> 
> collectAllGarbage also forces eager sweeping, which is why it would cause finalization to happen.

[Mark Hahnenberg]

I would do some further investigation into whether or not sweeping is being triggered in a timely manner. Try adding a printf or breakpoint inside MarkedBlock::sweep and see if things are being swept after the WebView is closed. Sweeping is when finalizers are run, so if a finalizer isn't running that should be the first thing to check.

[Alex Christensen]

Comment on attachment 221181 [details]
Patch

I'm cleaning up old bugs.  This does not need to go in.