Bug 108960 – [GTK] Remove subresource leaks from WebKit1 and WebKit2

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Both WebKit1 and WebKit2 have an API in which you can get the subresources of a page. To do this WebKit keeps a list of all subresources. There are many pages that are just continuous JSON requests and no full-page reloads. This means that all resources will leak (with their data) until memory is filled up.

We cannot remove the WebKit1 API, but perhaps we can observe the destruction of the resource. For WebKit2, it's probably better that we remove this API completely.

[Martin Robinson]

We need to sort out whether or not it's possible to keep the webkit_web_view_get_subresources API in WebKit2 before we ship our first stable release. Perhaps it's better to remove the API now and re-add it when we are sure that we can do it without the memory leak.

[Martin Robinson]

Adding cgarcia, since he probably knows whether or not it's possible to maintain the API without leaking.

[Gustavo Noronha (kov)]

FWIW, this is also what requires the injected bundle feature that was removed by apple for being considered chatty. Maybe we should reconsider storing the resources by default and make this a setting that applications that really want it can turn on, or maybe we should keep them alive only while they are being loaded. A setting to disable this seems like the only option for wk1.

[Martin Robinson]

(In reply to comment #3)
> FWIW, this is also what requires the injected bundle feature that was removed by apple for being considered chatty. Maybe we should reconsider storing the resources by default and make this a setting that applications that really want it can turn on, or maybe we should keep them alive only while they are being loaded. A setting to disable this seems like the only option for wk1.

I think in WebKit1 we can probably just wait for them to be destroyed or modify the API to return an empty list.

[George McCollister]

Created attachment 186661 [details]
Simple test page uses XHR to request large JSON file every second. Will exhaust most systems of memory in a few hours or less.

[Martin Robinson]

WebKit2 may only be leaking the URIResponse, but get_subresources still seems to have the chance to return responses for old or invalid data (and the responses still seem to be leaked).

[Carlos Garcia Campos]

The subresources API was used by ephy in webkit1 to implement the save feature, saving the main resource and then all subresources in a folder. In WebKit2 we have a specific API for that webkit_web_view_save, that allows to save in mhtml format, so get_subresources() is not used anymore. AFAIK nobody is currently using the get_subresources() method, at least the apps I've ported so far. 

So, we could either remove the API, or try to reimplement it as an async method that reloads the page and collects the resources that would be returned transfering the ownership to the caller. This approach has several issues, though, not all resources are available after a reload (see bug https://bugs.webkit.org/show_bug.cgi?id=78510) and it's impossible to know when to finish the operation in some cases like the one attached to this bug. On the other hand, our API allows the user to collect all subresources during the load, so I think the easiest solution would be to not cache the resources, and simply remove the get_subresources() method.

For webkit1 we can do the same, but returning NULL from get_subresources() and marking it as deprecated in favor of a new save() method similar to the wk2 one.

[Carlos Garcia Campos]

(In reply to comment #6)
> WebKit2 may only be leaking the URIResponse, but get_subresources still seems to have the chance to return responses for old or invalid data (and the responses still seem to be leaked).

why only the URI response? everytime a resource load starts it's added to the loading resources map and when it finishes, it's moved to the subresources map. So all the resources are kept in memory, not only the responses. I guess I'm missing something.

[Martin Robinson]

(In reply to comment #7)

> For webkit1 we can do the same, but returning NULL from get_subresources() and marking it as deprecated in favor of a new save() method similar to the wk2 one.

It seems that in WebKit there's a getSubresources method we can call on the DocumentLoader. We'll likely use this and create new WebKitWebResources during calls to get_subresources. The resource load signals will work as usual, but the actual WebKitWebResource will change for a given WebCore resource upon calling get_subresources, since once a load finishes we'll deref the original WebKitWebResources.

[Martin Robinson]

(In reply to comment #8)
> (In reply to comment #6)
> > WebKit2 may only be leaking the URIResponse, but get_subresources still seems to have the chance to return responses for old or invalid data (and the responses still seem to be leaked).
> 
> why only the URI response? everytime a resource load starts it's added to the loading resources map and when it finishes, it's moved to the subresources map. So all the resources are kept in memory, not only the responses. I guess I'm missing something.

Hrm. I guess I meant the WebCore resource. The map stores a list of WebKitWebResources which only store a reference to the WebKitURIResponse. To get the data we call WebFrameProxy::getResourceData. I figured that we weren't actually maintaining a reference to the original resource in this case. I may be misunderstanding the code though.

This brings up the question of what happens when two of the resources have the same URL, but different data though...

[Xan Lopez]

(In reply to comment #7)
> The subresources API was used by ephy in webkit1 to implement the save feature, saving the main resource and then all subresources in a folder. In WebKit2 we have a specific API for that webkit_web_view_save, that allows to save in mhtml format, so get_subresources() is not used anymore. AFAIK nobody is currently using the get_subresources() method, at least the apps I've ported so far. 

(...)

> 
> For webkit1 we can do the same, but returning NULL from get_subresources() and marking it as deprecated in favor of a new save() method similar to the wk2 one.

This plan seems reasonable to me. Remove from WK2, do the same for WK1.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #7)
> 
> > For webkit1 we can do the same, but returning NULL from get_subresources() and marking it as deprecated in favor of a new save() method similar to the wk2 one.
> 
> It seems that in WebKit there's a getSubresources method we can call on the DocumentLoader.

If the document loader is caching the subresources, the problem is still there, no?

> We'll likely use this and create new WebKitWebResources during calls to get_subresources. The resource load signals will work as usual, but the actual WebKitWebResource will change for a given WebCore resource upon calling get_subresources, since once a load finishes we'll deref the original WebKitWebResources.

I'm not sure I understand this, we would create different WebKitWebResource objects, one to wrap the resource while loading to pass it to the signals and a different one to return in get_subresources. The former would be owned by webkit and released when the resource load finishes while the latter would be owned by the caller and not cached by webkit.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > WebKit2 may only be leaking the URIResponse, but get_subresources still seems to have the chance to return responses for old or invalid data (and the responses still seem to be leaked).
> > 
> > why only the URI response? everytime a resource load starts it's added to the loading resources map and when it finishes, it's moved to the subresources map. So all the resources are kept in memory, not only the responses. I guess I'm missing something.
> 
> Hrm. I guess I meant the WebCore resource. The map stores a list of WebKitWebResources which only store a reference to the WebKitURIResponse. To get the data we call WebFrameProxy::getResourceData. I figured that we weren't actually maintaining a reference to the original resource in this case. I may be misunderstanding the code though.
> 
> This brings up the question of what happens when two of the resources have the same URL, but different data though...

Resources are not identified by the URI, but by their numeric identifier. Every time a resource is loaded a new identifier is assigned, even if the URI is the same, so it will always be a new resource.

[Martin Robinson]

(In reply to comment #12)

> If the document loader is caching the subresources, the problem is still there, no?

I haven't confirmed this, but I suspect that the DocumentLoader has an idea of when a subresource is no longer used. In WebKit we are just saving all of them without any eviction policy.

> I'm not sure I understand this, we would create different WebKitWebResource objects, one to wrap the resource while loading to pass it to the signals and a different one to return in get_subresources. The former would be owned by webkit and released when the resource load finishes while the latter would be owned by the caller and not cached by webkit.

That's pretty close, but it seems like that would cause the subresources to continue to leak. Applications still aren't expecting to receive a transfer=full reference. What we could do is to deref any resources we create in a timeout though.

[Martin Robinson]

(In reply to comment #13)

> Resources are not identified by the URI, but by their numeric identifier. Every time a resource is loaded a new identifier is assigned, even if the URI is the 
same, so it will always be a new resource.

The call in webkit_web_resource_get_data seems to be passing the URL:

    if (resource->priv->isMainResource)
        resource->priv->frame->getMainResourceData(DataCallback::create(result, resourceDataCallback));
    else {
        String url = String::fromUTF8(resource->priv->uri.data());
        resource->priv->frame->getResourceData(WebURL::create(url).get(), DataCallback::create(result, resourceDataCallback));
    }

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #12)
> 
> > If the document loader is caching the subresources, the problem is still there, no?
> 
> I haven't confirmed this, but I suspect that the DocumentLoader has an idea of when a subresource is no longer used. In WebKit we are just saving all of them without any eviction policy.

In that case get_subresources might not work, if some subresources has been deleted by the document loader.

> > I'm not sure I understand this, we would create different WebKitWebResource objects, one to wrap the resource while loading to pass it to the signals and a different one to return in get_subresources. The former would be owned by webkit and released when the resource load finishes while the latter would be owned by the caller and not cached by webkit.
> 
> That's pretty close, but it seems like that would cause the subresources to continue to leak. Applications still aren't expecting to receive a transfer=full reference. What we could do is to deref any resources we create in a timeout though.

Well, I was thinking of deprecating the old api and adding a new async api to get the subresources that would transfer the ownership to the caller.

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #13)
> 
> > Resources are not identified by the URI, but by their numeric identifier. Every time a resource is loaded a new identifier is assigned, even if the URI is the 
> same, so it will always be a new resource.
> 
> The call in webkit_web_resource_get_data seems to be passing the URL:
> 
>     if (resource->priv->isMainResource)
>         resource->priv->frame->getMainResourceData(DataCallback::create(result, resourceDataCallback));
>     else {
>         String url = String::fromUTF8(resource->priv->uri.data());
>         resource->priv->frame->getResourceData(WebURL::create(url).get(), DataCallback::create(result, resourceDataCallback));
>     }

Right, the identifier is only used for loading resources, while loaded we are using the URI. In that case, maybe we could just check when moving the resources from loading map to subresources map, and remove any existing subresource for the same URI. That would fix the problem without changing the API.

[Martin Robinson]

(In reply to comment #16)

> In that case get_subresources might not work, if some subresources has been deleted by the document loader.

Hrm. I'm not sure what you mean here. I was thinking that maybe the loader doesn't keep a reference to subresources that aren't present somewhere in the DOM (things like AJAX replies). To be honest, I haven't confirmed this. It's the API used for [WebDataSource subresources].

> Well, I was thinking of deprecating the old api and adding a new async api to get the subresources that would transfer the ownership to the caller.

Fixing the old API probably has value too, because it will fix a memory leak in pre-existing applications.

[Carlos Garcia Campos]

(In reply to comment #18)
> (In reply to comment #16)
> 
> > In that case get_subresources might not work, if some subresources has been deleted by the document loader.
> 
> Hrm. I'm not sure what you mean here. I was thinking that maybe the loader doesn't keep a reference to subresources that aren't present somewhere in the DOM (things like AJAX replies). To be honest, I haven't confirmed this. It's the API used for [WebDataSource subresources].
>

Ah, that makes sense, anyway we would need to look at it in more detail.

> > Well, I was thinking of deprecating the old api and adding a new async api to get the subresources that would transfer the ownership to the caller.
> 
> Fixing the old API probably has value too, because it will fix a memory leak in pre-existing applications.

My idea was to make old API return NULL and not cache resources at all.

[George McCollister]

Created attachment 187920 [details]
wk1 leak patch for discussion purposes

This patch seems to plug the leak for wk1 but unittests/testwebdatasource seems to fail, comments welcome. Incidentally the test I attached also seems to cause another leak but I'll save the details for another bug report.

[Martin Robinson]

Carlos brought up a good point that the old API returned borrowed references, so we'd need to preserve that behavior to avoid further leaks. Perhaps you can create the resources for get_resources and then deref them in a timeout?

[George McCollister]

(In reply to comment #21)
> Carlos brought up a good point that the old API returned borrowed references, so we'd need to preserve that behavior to avoid further leaks. Perhaps you can create the resources for get_resources and then deref them in a timeout?

Looks like using a timeout might be the only option for derefing the objects although creating a race condition seems like a bad idea. How long of a timeout do you think would be required to minimize the likely hood of derefing the resources while they are still in use?

[Martin Robinson]

(In reply to comment #22)
> (In reply to comment #21)
> > Carlos brought up a good point that the old API returned borrowed references, so we'd need to preserve that behavior to avoid further leaks. Perhaps you can create the resources for get_resources and then deref them in a timeout?
> 
> Looks like using a timeout might be the only option for derefing the objects although creating a race condition seems like a bad idea. How long of a timeout do you think would be required to minimize the likely hood of derefing the resources while they are still in use?

That's a good question. I assumed that anyone wanting to use the resources "later" would have increased their reference count, but perhaps that isn't the case. Do you know if Midori uses them later?

[George McCollister]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Carlos brought up a good point that the old API returned borrowed references, so we'd need to preserve that behavior to avoid further leaks. Perhaps you can create the resources for get_resources and then deref them in a timeout?
> > 
> > Looks like using a timeout might be the only option for derefing the objects although creating a race condition seems like a bad idea. How long of a timeout do you think would be required to minimize the likely hood of derefing the resources while they are still in use?
> 
> That's a good question. I assumed that anyone wanting to use the resources "later" would have increased their reference count, but perhaps that isn't the case. Do you know if Midori uses them later?

No, looks like it just uses them within the signal handler so I think we could  just specify an interval of 1.

[George McCollister]

Created attachment 188401 [details]
patch with timeout cleanup

Let me know if you see any problems in this revision of the patch.

[George McCollister]

Created attachment 188402 [details]
previous patch with indentation problem fixed

Oops, used a tab to indent one line, fixed it.

[Gustavo Noronha (kov)]

Comment on attachment 188402 [details]
previous patch with indentation problem fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=188402&action=review

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5115
> +    GList* subResources = (GList*)data;
> +    g_list_foreach(subResources, (GFunc) g_object_unref, NULL);
> +    g_list_free(subResources);

These should use C++-style casts.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5123
> +    GList* subResources = 0;
> +    Vector<PassRefPtr<ArchiveResource> > subresources;

Maybe use subResources and coreSubResources, so it's easier to read, but otherwise I don't see a problem with this approach, even though there is an expectation being broken, I don't think anyone uses it so...

[George McCollister]

Created attachment 188598 [details]
patch with suggested changes

[Martin Robinson]

Comment on attachment 188598 [details]
patch with suggested changes

View in context: https://bugs.webkit.org/attachment.cgi?id=188598&action=review

Nice. Work. I think this just a few small adjustments.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5113
> +    GList* subResources = reinterpret_cast<GList*>(data);

You actually only need a static_cast here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5115
> +    g_list_free(subResources);

We have to be a bit careful here. The annotation for webkit_web_data_source_get_subresources: is "transfer container." That means that the application owns the container and not the contents. It seems that you'll need to put the resources into a container that you own to free them. I suggest something like this:

Vector<GRefPtr<WebKitWebResource> > gCachedResources;
static gboolean cleanupTemporarilyCachedSubresources()
{
    gCachedResources.clear();
}

And then in get_subresources the code would look like this:

for (unsigned int i = 0; i < coreSubResources.size(); i++) {
    WebKitWebResource* webResource = WEBKIT_WEB_RESOURCE(g_object_new(WEBKIT_TYPE_WEB_RESOURCE, NULL));
    webkit_web_resource_init_with_core_resource(webResource, coreSubResources[i]);
    subResources = g_list_append(subResources, webResource);
    gCachedResources.append(adoptGRef(webResorce));
}

Note that I'm using adoptGRef to avoid a reference leak and also that cleanupTemporarilyCachedSubresources follows Webkit naming conventions.

[George McCollister]

Created attachment 188624 [details]
patch with changed callback name and static cast

[WebKit Review Bot]

Comment on attachment 188624 [details]
patch with changed callback name and static cast

Rejecting attachment 188624 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 188624, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13634 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
53>At revision 13634.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16580987

[WebKit Review Bot]

Comment on attachment 188624 [details]
patch with changed callback name and static cast

Rejecting attachment 188624 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 188624, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13634 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
53>At revision 13634.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/16587287

[George McCollister]

Created attachment 188882 [details]
Updated ChangeLog, no longer remove main resource

Removing the main resource in dispatchDidFinishLoading and dispatchDidFailLoading was causing unittests/testwebdatasource to fail so I changed them not to remove the main resource. I also updated the ChangeLog.

[Martin Robinson]

Comment on attachment 188882 [details]
Updated ChangeLog, no longer remove main resource

Looks pretty good to me. Maybe Gustavo knows if there's a better way to sniff out the whether a resource is the main one.

[WebKit Review Bot]

Attachment 188882 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Source/WebKit/gtk/webkit/webkitwebview.cpp']" exit_code: 1
Source/WebKit/gtk/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/webkit/webkitwebview.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/gtk/webkit/webkitwebview.cpp:5127:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Looks like there are some style errors. George, you can ensure that your patch meets the style guidelines by uploading it with webkit-patch.

[George McCollister]

Created attachment 188917 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188917 [details]
Patch

Clearing flags on attachment: 188917

Committed r143619: <http://trac.webkit.org/changeset/143619>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.