Bug 139339 - [Curl] Cache entry is sometimes deleted when request receives a not modified response.
Summary: [Curl] Cache entry is sometimes deleted when request receives a not modified ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-06 04:10 PST by peavo
Modified: 2014-12-10 10:53 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.57 KB, patch)
2014-12-06 04:42 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2014-12-08 06:48 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (7.19 KB, patch)
2014-12-09 09:02 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2014-12-10 07:33 PST, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2014-12-06 04:10:09 PST
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.
Comment 1 peavo 2014-12-06 04:42:54 PST
Created attachment 242710 [details]
Patch
Comment 2 peavo 2014-12-08 06:48:52 PST
Created attachment 242807 [details]
Patch
Comment 3 Alex Christensen 2014-12-08 10:14:59 PST
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.
Comment 4 peavo 2014-12-09 09:02:44 PST
Created attachment 242927 [details]
Patch
Comment 5 peavo 2014-12-09 09:03:18 PST
(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.
Comment 6 peavo 2014-12-09 12:23:16 PST
(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).
Comment 7 Alex Christensen 2014-12-09 18:39:28 PST
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?
Comment 8 peavo 2014-12-10 07:33:06 PST
Created attachment 243016 [details]
Patch
Comment 9 peavo 2014-12-10 07:48:01 PST
(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.
Comment 10 Alex Christensen 2014-12-10 10:11:43 PST
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.
Comment 11 WebKit Commit Bot 2014-12-10 10:53:48 PST
Comment on attachment 243016 [details]
Patch

Clearing flags on attachment: 243016

Committed r177080: <http://trac.webkit.org/changeset/177080>
Comment 12 WebKit Commit Bot 2014-12-10 10:53:52 PST
All reviewed patches have been landed.  Closing bug.