Bug 143953 – [Curl] Favicons loaded from disc cache are ignored.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

When a favicon is loaded from the Curl disc cache, the icon data is thrown away. This happens because we give a 304 response, which makes the icon loader ignore the response:

void IconLoader::notifyFinished(CachedResource* resource)
{
    ASSERT(resource == m_resource);

    // If we got a status code indicating an invalid response, then lets
    // ignore the data and not try to decode the error page as an icon.
    auto* data = resource->resourceBuffer();
    int status = resource->response().httpStatusCode();
    if (status && (status < 200 || status > 299))
        data = nullptr;

We can solve this by responding with 200 OK.

Created attachment 251165 [details]
Patch

[Alex Christensen]

Comment on attachment 251165 [details]
Patch

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

> Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:177
>  void CurlCacheEntry::setResponseFromCachedHeaders(ResourceResponse& response)
>  {
> -    response.setHTTPStatusCode(304);
> +    response.setHTTPStatusCode(200);
> +    response.setHTTPStatusText("OK");

304 is the status of the response from the servers.  If a 304 is received from a server, then we as the browser want to tell the webpage that we got a 200 OK, right?  This makes sense, but it seems like it would've been a bigger problem.  Why wasn't this noticed before?

(In reply to comment #2)
> Comment on attachment 251165 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251165&action=review
> 
> > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:177
> >  void CurlCacheEntry::setResponseFromCachedHeaders(ResourceResponse& response)
> >  {
> > -    response.setHTTPStatusCode(304);
> > +    response.setHTTPStatusCode(200);
> > +    response.setHTTPStatusText("OK");
> 
> 304 is the status of the response from the servers.  If a 304 is received
> from a server, then we as the browser want to tell the webpage that we got a
> 200 OK, right?  This makes sense, but it seems like it would've been a
> bigger problem.  Why wasn't this noticed before?

Yes, it seems like it is only the icon loader which does not accept a 304 response. There is no problem responding with 304 for a cached document or image.

[Alex Christensen]

(In reply to comment #3)
If we changed this, though, would there be XMLHttpRequests that used to have a status of 304 that now have a status of 200?

(In reply to comment #4)
> (In reply to comment #3)
> If we changed this, though, would there be XMLHttpRequests that used to have
> a status of 304 that now have a status of 200?

Yes, I believe we then would respond with "200 OK" for all requests, regardless of whether they come from the cache or the network.

Or maybe it makes more sense to change the icon loader?

[Alex Christensen]

http://www.w3.org/TR/XMLHttpRequest/ says:
For 304 Not Modified responses that are a result of a user agent generated conditional request the user agent must act as if the server gave a 200 OK response with the appropriate content. The user agent must allow author request headers to override automatic cache validation (e.g. If-None-Match or If-Modified-Since), in which case 304 Not Modified responses must be passed through.

Created attachment 251346 [details]
Patch

(In reply to comment #7)
> http://www.w3.org/TR/XMLHttpRequest/ says:
> For 304 Not Modified responses that are a result of a user agent generated
> conditional request the user agent must act as if the server gave a 200 OK
> response with the appropriate content. The user agent must allow author
> request headers to override automatic cache validation (e.g. If-None-Match
> or If-Modified-Since), in which case 304 Not Modified responses must be
> passed through.

Thanks for looking into this and providing this information.

I updated the patch to handle this. I'm now running the http tests to check for regressions.

Have finished running the http tests, there are no regressions caused by this patch.

[Alex Christensen]

Comment on attachment 251346 [details]
Patch

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

> Source/WebCore/platform/network/ResourceHandleInternal.h:167
> +        bool m_addedCacheValidationHeaders;

There is a trend towards using syntax like this:
bool m_addedCacheValidationHeaders { false };
If this is done with all the variables here, we could completely remove the other #if USE(CURL) above, but let's not touch the soup or cfnetwork code in this patch.

(In reply to comment #11)
> Comment on attachment 251346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251346&action=review
> 
> > Source/WebCore/platform/network/ResourceHandleInternal.h:167
> > +        bool m_addedCacheValidationHeaders;
> 
> There is a trend towards using syntax like this:
> bool m_addedCacheValidationHeaders { false };
> If this is done with all the variables here, we could completely remove the
> other #if USE(CURL) above, but let's not touch the soup or cfnetwork code in
> this patch.

Thanks for reviewing :) Should I make this change for all the CURL members before landing?

[Alex Christensen]

(In reply to comment #12)
> Thanks for reviewing :) Should I make this change for all the CURL members
> before landing?
Yep.

Committed r183407: <http://trac.webkit.org/changeset/183407>