Summary: | [Curl] Cache entry is sometimes deleted when request receives a not modified response. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | peavo | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alex.christensen, bfulgham, commit-queue, galpeter | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
peavo
2014-12-06 04:10:09 PST
Created attachment 242710 [details]
Patch
Created attachment 242807 [details]
Patch
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). 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. 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 on attachment 243016 [details] Patch Clearing flags on attachment: 243016 Committed r177080: <http://trac.webkit.org/changeset/177080> All reviewed patches have been landed. Closing bug. |