Bug 139339 – [Curl] Cache entry is sometimes deleted when request receives a not modified response.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

Sometimes it happens that a request receives a not modified response, but the cache entry has already been deleted by another request.
This can be avoided by locking a cache entry while there are pending requests for the cache entry's url.

Created attachment 242710 [details]
Patch

Created attachment 242807 [details]
Patch

[Alex Christensen]

Using a hash set as a mutex seems quite excessive. There are mutex implementations in webkit. I know this is not designed for multi threaded use, but I think it would be better to use a mutex as a mutex.

Created attachment 242927 [details]
Patch

(In reply to comment #3)
> Using a hash set as a mutex seems quite excessive. There are mutex
> implementations in webkit. I know this is not designed for multi threaded
> use, but I think it would be better to use a mutex as a mutex.

Thanks for reviewing :)

Updated patch.

(In reply to comment #4)
> Created attachment 242927 [details]
> Patch

I'm not sure that it's correct to add the cache headers to the header map in the ResourceRequest object (changed in the last patch).

[Alex Christensen]

Comment on attachment 242927 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        but the cache entry has already been deleted by another request.

How would another request have already deleted the cached response? I'm not entirely convinced that locking will solve this problem.

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:83
> +    int m_lockCount;

Why is this not a bool? How would more than one request get the lock?

Created attachment 243016 [details]
Patch

(In reply to comment #7)
> Comment on attachment 242927 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242927&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        but the cache entry has already been deleted by another request.
> 
> How would another request have already deleted the cached response? I'm not
> entirely convinced that locking will solve this problem.
>

Another request will delete and rebuild the entry if it receives a 200 OK response for the same url. The problem is usually seen with css files, which often are loaded by several requests concurrently, resulting in documents without styling. 
 
> > Source/WebCore/platform/network/curl/CurlCacheEntry.h:83
> > +    int m_lockCount;
> 
> Why is this not a bool? How would more than one request get the lock?

I have probably used the wrong term here. It's not an exclusive lock, it's more of a reference count of the cache entry, to make sure nobody is deleting the cache entry when there are pending request, which will need the entry soon.

[Alex Christensen]

Comment on attachment 243016 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlCacheEntry.h:85
> +    ListHashSet<ResourceHandle*> m_clients;

Right now this could be an int count, but this isn't many more operations and it could be useful to have a record of exactly which clients.

[WebKit Commit Bot]

Comment on attachment 243016 [details]
Patch

Clearing flags on attachment: 243016

Committed r177080: <http://trac.webkit.org/changeset/177080>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.