When a request is taken from the cache, it's cached response headers are empty, if the cache entry was created in the same session. It is only when the cache entry is loaded from disc, that the response headers are properly set. We need to set the cached response headers in both cases. There is also an issue if two jobs are loading the same url at the same time. Both jobs will then write to the cache content file, and create invalid content. This can be fixed by only letting the first request write to the content file.
Created attachment 232431 [details] Patch
You should add checks to see if job is null before dereferencing it. Are raw pointers really the best thing to use here?
Created attachment 232553 [details] Patch
(In reply to comment #2) > You should add checks to see if job is null before dereferencing it. Are raw pointers really the best thing to use here? Thanks for looking into this :) Added null pointer checks.
Comment on attachment 232553 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232553&action=review > Source/WebCore/ChangeLog:13 > + This can be fixed by only letting the first request write to the content file. Does this also protect against another process writing to the file? > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:142 > + std::unique_ptr<CurlCacheEntry> cacheEntry(new CurlCacheEntry(url, 0, m_cacheDir)); This 0 should be a nullptr. Also, try using std::make_unique instead of new. > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:219 > + std::unique_ptr<CurlCacheEntry> cacheEntry(new CurlCacheEntry(url, job, m_cacheDir)); make_unique
Created attachment 232573 [details] Patch
(In reply to comment #5) > (From update of attachment 232553 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232553&action=review > > > Source/WebCore/ChangeLog:13 > > + This can be fixed by only letting the first request write to the content file. > > Does this also protect against another process writing to the file? > No, I don't believe it's designed to handle access by multiple processes. > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:142 > > + std::unique_ptr<CurlCacheEntry> cacheEntry(new CurlCacheEntry(url, 0, m_cacheDir)); > > This 0 should be a nullptr. Also, try using std::make_unique instead of new. > > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:219 > > + std::unique_ptr<CurlCacheEntry> cacheEntry(new CurlCacheEntry(url, job, m_cacheDir)); > > make_unique Done.
This looks good to me, but a reviewer will have to r+ it.
Comment on attachment 232573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232573&action=review I think this looks good, but it could be made a bit tighter. > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:48 > +CurlCacheEntry::CurlCacheEntry(const String& url, ResourceHandle* job, const String& cacheDir) This could be a reference... > Source/WebCore/platform/network/curl/CurlCacheEntry.h:64 > + const ResourceHandle* getJob() const { return m_job; } This could then return a reference... > Source/WebCore/platform/network/curl/CurlCacheEntry.h:80 > + ResourceHandle* m_job; This could just be a const reference, since m_job doesn't appear to be allowed to be null, does it? > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:142 > + std::unique_ptr<CurlCacheEntry> cacheEntry = std::make_unique<CurlCacheEntry>(url, nullptr, m_cacheDir); auto cacheEntry! > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:213 > + HashMap<String, std::unique_ptr<CurlCacheEntry>>::iterator it = m_index.find(url); This would be better as auto to avoid having to type all that stuff out. > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:219 > + std::unique_ptr<CurlCacheEntry> cacheEntry = std::make_unique<CurlCacheEntry>(url, job, m_cacheDir); auto cacheEntry > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:230 > +void CurlCacheManager::didFinishLoading(ResourceHandle* job) Make this a ResourceHandle& ... > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:236 > + return; ... and get rid of this null check! > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:276 > +void CurlCacheManager::didReceiveData(ResourceHandle* job, const char* data, size_t size) Make this a reference ... > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:282 > + return; ... and get rid of this null check. > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:331 > +void CurlCacheManager::didFail(ResourceHandle *job) Make this a reference ... > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:334 > + return; ... and get rid of this null check. > Source/WebCore/platform/network/curl/CurlCacheManager.h:55 > + void didFail(ResourceHandle*); These could all be (const ResourceHandle&), since 'job' is never NULL in the use cases you are changing. > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:328 > + CurlCacheManager::getInstance().didReceiveData(job, static_cast<char*>(ptr), totalSize); 'job' was always non-null here, so why not pass it as a reference? > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:692 > + CurlCacheManager::getInstance().didFinishLoading(job); ditto: 'job' was always non-null here, so why not pass it as a reference? > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:704 > + CurlCacheManager::getInstance().didFail(job); ditto: 'job' was always non-null here, so why not pass it as a reference?
Created attachment 232615 [details] Patch
(In reply to comment #9) > (From update of attachment 232573 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232573&action=review > Thanks for reviewing :) > I think this looks good, but it could be made a bit tighter. > > > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:48 > > +CurlCacheEntry::CurlCacheEntry(const String& url, ResourceHandle* job, const String& cacheDir) > > This could be a reference... > > > Source/WebCore/platform/network/curl/CurlCacheEntry.h:64 > > + const ResourceHandle* getJob() const { return m_job; } > > This could then return a reference... > > > Source/WebCore/platform/network/curl/CurlCacheEntry.h:80 > > + ResourceHandle* m_job; > > This could just be a const reference, since m_job doesn't appear to be allowed to be null, does it? > I left this as is, since m_job will be null if the entry is loaded from disc, there will be no associated job then. > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:142 > > + std::unique_ptr<CurlCacheEntry> cacheEntry = std::make_unique<CurlCacheEntry>(url, nullptr, m_cacheDir); > > auto cacheEntry! > > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:213 > > + HashMap<String, std::unique_ptr<CurlCacheEntry>>::iterator it = m_index.find(url); > > This would be better as auto to avoid having to type all that stuff out. > > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:219 > > + std::unique_ptr<CurlCacheEntry> cacheEntry = std::make_unique<CurlCacheEntry>(url, job, m_cacheDir); > > auto cacheEntry > > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:230 > > +void CurlCacheManager::didFinishLoading(ResourceHandle* job) > > Make this a ResourceHandle& ... > > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:236 > > + return; > > ... and get rid of this null check! > > > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:276 > > +void CurlCacheManager::didReceiveData(ResourceHandle* job, const char* data, size_t size) > Done.
Comment on attachment 232615 [details] Patch r=me
Comment on attachment 232615 [details] Patch Clearing flags on attachment: 232615 Committed r169811: <http://trac.webkit.org/changeset/169811>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > (From update of attachment 232615 [details]) > r=me Thanks!