Bug 154852 – NetworkCache: Web process leaks resource buffer when using shareable reasources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 272558 [details]
Patch

[Darin Adler]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r197402: <http://trac.webkit.org/changeset/197402>

[Alexey Proskuryakov]

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().

[Carlos Garcia Campos]

(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.