Summary: | [Curl] Empty headers in request response. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | peavo | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, alex.christensen, bfulgham, commit-queue, galpeter, mmatyas | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
peavo
2014-06-03 09:51:09 PDT
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! |