Bug 136855 – [Curl] Sometimes incomplete or empty content can be loaded from cache.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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.

Created attachment 238177 [details]
Patch

[Radu Stavila]

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.

Created attachment 238184 [details]
Patch

(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.

[Alex Christensen]

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?

Created attachment 238189 [details]
Patch

(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.

[Alex Christensen]

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?

[Alex Christensen]

Comment on attachment 238189 [details]
Patch

Oh, ok.

[WebKit Commit Bot]

Comment on attachment 238189 [details]
Patch

Clearing flags on attachment: 238189

Committed r173666: <http://trac.webkit.org/changeset/173666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.