Speculative revalidated request returns 200 instead of 304

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

I noticed that after enabling speculative revalidation in the GTk+ port the unit test /webkit2/WebKitWebResource/response started to fail in the bots:

  /webkit2/WebKitWebResource/response:                                 **

ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestResources.cpp:417:void testWebResourceResponse(SingleResourceLoadTest*, gconstpointer): assertion failed (webkit_uri_response_get_status_code(response) == SOUP_STATUS_NOT_MODIFIED): (200 == 304)

FAIL

We are simply doing a reload and checking that the response code we get for a subresource is 304, but now is 200, because the network process is using a preloaded entry. I would expect that in case of successful speculative revalidation, 304 was returned like a normal revalidation request, and the resource be loaded from the memory cache.

[Chris Dumez]

I am investigating but I will likely need help testing a fix.

[Chris Dumez]

Created attachment 280244 [details]
Patch

[Chris Dumez]

Created attachment 280245 [details]
Patch

[Chris Dumez]

Comment on attachment 280245 [details]
Patch

Carlos, could you confirm this fixes the GTK unit test?

[Chris Dumez]

Created attachment 280290 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 280245 [details]
> Patch
> 
> Carlos, could you confirm this fixes the GTK unit test?

/webkit2/WebKitWebResource/response: OK

Yes, thanks for the quick fix!

[Carlos Garcia Campos]

Comment on attachment 280290 [details]
Patch

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

Patch looks good to me. I'm not wk2 owner so please, wait until an owner approves this before cq+ it. Thanks!

> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:278
> -    static const HTTPHeaderName headersAllowedToMismatch[] = {
> -        HTTPHeaderName::IfMatch,
> -        HTTPHeaderName::IfModifiedSince,
> -        HTTPHeaderName::IfNoneMatch,
> -        HTTPHeaderName::IfRange,
> -        HTTPHeaderName::IfUnmodifiedSince,
> -        HTTPHeaderName::CacheControl
> -    };
> -
> -    HTTPHeaderMap headersA = a.httpHeaderFields();
> -    HTTPHeaderMap headersB = b.httpHeaderFields();
> -    for (auto headerName : headersAllowedToMismatch) {
> -        headersA.remove(headerName);
> -        headersB.remove(headerName);
> -    }
> +    ASSERT(!actualRequest.isConditional());
> +    ResourceRequest speculativeRequest = speculativeValidationRequest;
> +    speculativeRequest.makeUnconditional();
>  
> -    if (headersA != headersB) {
> +    if (speculativeRequest.httpHeaderFields() != actualRequest.httpHeaderFields()) {

Nice cleanup!

[Antti Koivisto]

Comment on attachment 280290 [details]
Patch

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

Looks good

> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:-280
> -        HTTPHeaderName::CacheControl

Why no allow CacheControl mismatch? Wouldn't that disallow some cases that would be fine to use?

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 280290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280290&action=review
> 
> Looks good
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:-280
> > -        HTTPHeaderName::CacheControl
> 
> Why no allow CacheControl mismatch? Wouldn't that disallow some cases that
> would be fine to use?

CacheControl on requests is very rarely used. The only reason I had this code was for the reload case because WebCore sets a "Cache-Control: max-age=0" in some cases. Right now, there is no way WebCore sets "Cache-Control: max-age=0" without also setting conditional headers so keeping this code won't normally help. It could in theory be useful for XHR requests but I doubt Cache-Control is used a lot there. We can add it back if we see XHR requests using Cache-Control header.

[Chris Dumez]

Committed r201600: <http://trac.webkit.org/changeset/201600>