Bug 136855 - [Curl] Sometimes incomplete or empty content can be loaded from cache.
Summary: [Curl] Sometimes incomplete or empty content can be loaded from cache.
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-09-16 05:43 PDT by peavo
Modified: 2014-09-16 12:22 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.82 KB, patch)
2014-09-16 06:01 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2014-09-16 09:52 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2014-09-16 11:31 PDT, 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-09-16 05:43:16 PDT
Sometimes, when two requests with the same url are started at the same time, there is a possibility of loading incomplete or empty content from the cache. The first request will create the new cache entry, and start loading the content into the cache. The second request will assume the content is cached, and start loading from the cache. But if the first request is not finished yet, empty or imcomplete content will be loaded.
This happens because the method CurlCacheEntry::isLoading() is returning the wrong status in the time period between the headers are received, and the content data is received.
Comment 1 peavo 2014-09-16 06:01:15 PDT
Created attachment 238177 [details]
Patch
Comment 2 Radu Stavila 2014-09-16 08:47:41 PDT
Comment on attachment 238177 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:75
> +    return m_isLoading;

This method could now become const.
Comment 3 peavo 2014-09-16 09:52:28 PDT
Created attachment 238184 [details]
Patch
Comment 4 peavo 2014-09-16 09:52:56 PDT
(In reply to comment #2)
> (From update of attachment 238177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238177&action=review
> 
> > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:75
> > +    return m_isLoading;
> 
> This method could now become const.

Thanks :) Updated patch.
Comment 5 Alex Christensen 2014-09-16 11:24:44 PDT
Comment on attachment 238184 [details]
Patch

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

This looks mostly good.  r- only because it needs another patch and I have a question.

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:119
> +    if (buffer.size() > 0)

is buffer.size() signed?  I think this should just be if (buffer.size()).

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:221
> +            cacheEntry->setIsLoading(true);

This not only sets the loading flag, but it calls openContentFile.  Is this intentional?
Comment 6 peavo 2014-09-16 11:31:42 PDT
Created attachment 238189 [details]
Patch
Comment 7 peavo 2014-09-16 11:39:08 PDT
(In reply to comment #5)
> (From update of attachment 238184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238184&action=review
> 
> This looks mostly good.  r- only because it needs another patch and I have a question.
> 

Thanks for the review :) I have updated the patch.

> > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:119
> > +    if (buffer.size() > 0)
> 
> is buffer.size() signed?  I think this should just be if (buffer.size()).
> 
> > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:221
> > +            cacheEntry->setIsLoading(true);
> 
> This not only sets the loading flag, but it calls openContentFile.  Is this intentional?

Yes, when there are responses with only headers, and no content, we need to create the content file (which will be empty). Otherwise it will never be created since we never receive any content data. When another request for the same url tries to retrieve the content data from the cache, the file will not exist, and the request will fail.
Comment 8 Alex Christensen 2014-09-16 11:46:40 PDT
Comment on attachment 238189 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlCacheManager.cpp:221
> +            cacheEntry->setIsLoading(true);

Why is this line added here?  Should it be added in other places?  Did it work without this?
Comment 9 Alex Christensen 2014-09-16 11:47:35 PDT
Comment on attachment 238189 [details]
Patch

Oh, ok.
Comment 10 WebKit Commit Bot 2014-09-16 12:22:47 PDT
Comment on attachment 238189 [details]
Patch

Clearing flags on attachment: 238189

Committed r173666: <http://trac.webkit.org/changeset/173666>
Comment 11 WebKit Commit Bot 2014-09-16 12:22:50 PDT
All reviewed patches have been landed.  Closing bug.