Bug 154852

Summary: NetworkCache: Web process leaks resource buffer when using shareable reasources
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, ap, bugs-noreply, cdumez, darin, koivisto
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 152316    
Attachments:
Description Flags
Patch darin: review+

Description Carlos Garcia Campos 2016-03-01 04:05:20 PST
This is causing us running out of fds when using a web process limit of one after long time running, since we never release the shareable resources.
Comment 1 Carlos Garcia Campos 2016-03-01 04:10:56 PST
Created attachment 272558 [details]
Patch
Comment 2 Darin Adler 2016-03-01 08:33:56 PST
Comment on attachment 272558 [details]
Patch

Better still to replace PassRefPtr with RefPtr&& or Ref&&, but I’m sure someone will come along to do that later.
Comment 3 Carlos Garcia Campos 2016-03-01 08:58:09 PST
(In reply to comment #2)
> Comment on attachment 272558 [details]
> Patch
> 
> Better still to replace PassRefPtr with RefPtr&& or Ref&&, but I’m sure
> someone will come along to do that later.

I thought about that, there's tricky part in SubresourceLoader::didReceiveDataOrBuffer that calls ResourceLoader::didReceiveDataOrBuffer, but yes, it can be done. I decided to leave that for a follow up patch, to ensure the leak is fixed first.
Comment 4 Carlos Garcia Campos 2016-03-01 09:42:35 PST
Committed r197402: <http://trac.webkit.org/changeset/197402>
Comment 5 Alexey Proskuryakov 2016-03-01 22:39:59 PST
I can see how this avoids refcount churn, however I don't see how this can fix a leak. What am I missing?

+        ResourceLoader::didReceiveBuffer() expects a PassRefPtr, but we
+        are passing a raw pointer making PassRefPtr to take another
+        reference instead of transfering the ownership as expected.

This was compensated by the original RefPtr still having a pointer to the object, so there was also one more deref().
Comment 6 Carlos Garcia Campos 2016-03-01 23:11:11 PST
(In reply to comment #5)
> I can see how this avoids refcount churn, however I don't see how this can
> fix a leak. What am I missing?
> 
> +        ResourceLoader::didReceiveBuffer() expects a PassRefPtr, but we
> +        are passing a raw pointer making PassRefPtr to take another
> +        reference instead of transfering the ownership as expected.
> 
> This was compensated by the original RefPtr still having a pointer to the
> object, so there was also one more deref().

hmm, you are indeed right. When passing the raw pointer, the PassRefPtr takes another ref but the original RefPtr releases its own when didReceiveResource() finishes. I was doing a lot of tests and didn't see any ShareableResource freed before applying this patch, but I guess I didn't disable the memory cache either. So, maybe there's no leak but when using a single web process we end up with a lot of resources cached in memory keeping their fds alive. We might consider copying the data in the web process and releasing the mmap.