Bug 162281 - [Fetch API] Support Request cache mode
Summary: [Fetch API] Support Request cache mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937 159683
  Show dependency treegraph
 
Reported: 2016-09-20 09:06 PDT by youenn fablet
Modified: 2016-10-12 08:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (42.31 KB, patch)
2016-09-20 09:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (853.26 KB, application/zip)
2016-09-20 10:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.48 MB, application/zip)
2016-09-20 10:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (847.01 KB, application/zip)
2016-09-20 10:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (8.37 MB, application/zip)
2016-09-20 10:47 PDT, Build Bot
no flags Details
Patch (48.78 KB, patch)
2016-09-21 06:52 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Removing WK2 changes (47.46 KB, patch)
2016-09-21 09:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (50.17 KB, patch)
2016-10-11 03:21 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-09-20 09:06:21 PDT
We should support cache mode.
Comment 1 youenn fablet 2016-09-20 09:24:49 PDT
Created attachment 289358 [details]
Patch
Comment 2 Build Bot 2016-09-20 10:19:49 PDT
Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2113101

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html
Comment 3 Build Bot 2016-09-20 10:19:53 PDT
Created attachment 289373 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Build Bot 2016-09-20 10:22:06 PDT
Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2113094

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html
Comment 5 Build Bot 2016-09-20 10:22:09 PDT
Created attachment 289374 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-09-20 10:30:39 PDT
Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2113172

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html
Comment 7 Build Bot 2016-09-20 10:30:43 PDT
Created attachment 289376 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 8 Build Bot 2016-09-20 10:47:36 PDT
Comment on attachment 289358 [details]
Patch

Attachment 289358 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2113183

New failing tests:
imported/w3c/web-platform-tests/fetch/api/request/request-error.html
Comment 9 Build Bot 2016-09-20 10:47:40 PDT
Created attachment 289378 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 10 youenn fablet 2016-09-21 06:52:33 PDT
Created attachment 289448 [details]
Patch
Comment 11 youenn fablet 2016-09-21 09:07:04 PDT
Created attachment 289456 [details]
Removing WK2 changes
Comment 12 youenn fablet 2016-09-22 01:10:13 PDT
Comment on attachment 289456 [details]
Removing WK2 changes

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

> Source/WebCore/loader/FrameLoader.cpp:2564
> +ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const ResourceRequest& request, FrameLoadType loadType, bool isMainResource)

The integration of fetch tasks with navigation/frame load tasks is far from perfect and makes the loading code difficult to grasp.
Future refactoring steps should be done to clarify this, without breaking behavior...

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-cache-expected.txt:99
> +FAIL RequestCache "reload" mode does store the response in the cache with date and fresh response assert_equals: expected 1 but got 2

These two tests are not passing as the CFNetwork cache does not contain the reloaded entry.
The reason is unclear to me and will need further investigations.

> LayoutTests/platform/wk2/TestExpectations:694
> +imported/w3c/web-platform-tests/fetch/api/request/request-cache.html [ Skip ]

We skip it for the moment as wk2 test result is different from wk1.
In particular, some console log messages are added and they change for every run. We need something like bug 161310 to land first.
We also need network cache to be skipped in case request cache policy is reload.
Comment 13 youenn fablet 2016-10-04 07:56:21 PDT
Ping review?
Comment 14 Brady Eidson 2016-10-04 14:32:10 PDT
(In reply to comment #12)
> Comment on attachment 289456 [details]
> Removing WK2 changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289456&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2564
> > +ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const ResourceRequest& request, FrameLoadType loadType, bool isMainResource)
> 
> The integration of fetch tasks with navigation/frame load tasks is far from
> perfect and makes the loading code difficult to grasp.
> Future refactoring steps should be done to clarify this, without breaking
> behavior...
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-cache-expected.txt:99
> > +FAIL RequestCache "reload" mode does store the response in the cache with date and fresh response assert_equals: expected 1 but got 2
> 
> These two tests are not passing as the CFNetwork cache does not contain the
> reloaded entry.
> The reason is unclear to me and will need further investigations.

We don't use the CFNetwork cache in WK2. How is it that this expectation change applies to both WK1 and WK2?

> 
> > LayoutTests/platform/wk2/TestExpectations:694
> > +imported/w3c/web-platform-tests/fetch/api/request/request-cache.html [ Skip ]
> 
> We skip it for the moment as wk2 test result is different from wk1.
> In particular, some console log messages are added and they change for every
> run. We need something like bug 161310 to land first.
> We also need network cache to be skipped in case request cache policy is
> reload.

Where is the bug to re-enable once 161310 lands, or something else allows re-enabling?
We tend to skip a lot of tests without tracking re-enabling them later, which is bad.
Comment 15 Brady Eidson 2016-10-04 14:34:26 PDT
Comment on attachment 289456 [details]
Removing WK2 changes

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:605
> +static inline void updateRequestAccordingCacheMode(CachedResourceRequest& request)

Are all of the options in here from the spec?

If not, where are they from?
Comment 16 youenn fablet 2016-10-04 22:51:16 PDT
Comment on attachment 289456 [details]
Removing WK2 changes

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

>>> Source/WebCore/loader/FrameLoader.cpp:2564
>>> +ResourceRequestCachePolicy FrameLoader::defaultRequestCachingPolicy(const ResourceRequest& request, FrameLoadType loadType, bool isMainResource)
>> 
>> The integration of fetch tasks with navigation/frame load tasks is far from perfect and makes the loading code difficult to grasp.
>> Future refactoring steps should be done to clarify this, without breaking behavior...
> 
> We don't use the CFNetwork cache in WK2. How is it that this expectation change applies to both WK1 and WK2?

I am not sure to understand your question.
In WK2, the network session cache is using the request policy cache to decide whether to serve content or not from it.
The results for request-cache.html being different between WK1 and WK2 implementation and request-cache.html being flaky for WK2, we rebase request-cache.html expectation using WK1 implementation results.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:605
>> +static inline void updateRequestAccordingCacheMode(CachedResourceRequest& request)
> 
> Are all of the options in here from the spec?
> 
> If not, where are they from?

The cache options are defined at https://fetch.spec.whatwg.org/#concept-request-cache-mode
This specific processing is defined in steps 10-12 of https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

>>> LayoutTests/platform/wk2/TestExpectations:694
>>> +imported/w3c/web-platform-tests/fetch/api/request/request-cache.html [ Skip ]
>> 
>> We skip it for the moment as wk2 test result is different from wk1.
>> In particular, some console log messages are added and they change for every run. We need something like bug 161310 to land first.
>> We also need network cache to be skipped in case request cache policy is reload.
> 
> Where is the bug to re-enable once 161310 lands, or something else allows re-enabling?
> We tend to skip a lot of tests without tracking re-enabling them later, which is bad.

I can create a bug for it specifically at landing time, but this can be added to bug 161310 as well if this patch lands first.

In lieu of skipping this test, I can make it [Pass Failure], this would check for unexpected crash/timeout.

Also, it is not only about reactivating this test but also to pass all tests of request-cache.html.
For instance, we should be able to bypass WK2 network session cache entirely which is not supported yet.
I plan to work on that once this patch lands
Comment 17 youenn fablet 2016-10-04 22:56:33 PDT
Also bug 159683 is tracking request-cache.html specifically.
Comment 18 youenn fablet 2016-10-10 08:20:40 PDT
Ping review?
Comment 19 Alex Christensen 2016-10-10 11:22:56 PDT
Comment on attachment 289456 [details]
Removing WK2 changes

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

r=me.  This is a big progression with some things that will be further investigated.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:624
> +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::Pragma, ASCIILiteral("no-cache"));
> +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::CacheControl, ASCIILiteral("no-cache"));

Strings can go in HTTPHeaderValues.h, right?
Comment 20 youenn fablet 2016-10-11 03:21:12 PDT
Created attachment 291246 [details]
Patch for landing
Comment 21 youenn fablet 2016-10-11 03:55:51 PDT
Thanks for the review.

(In reply to comment #19)
> Comment on attachment 289456 [details]
> Removing WK2 changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289456&action=review
> 
> r=me.  This is a big progression with some things that will be further
> investigated.
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:624
> > +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::Pragma, ASCIILiteral("no-cache"));
> > +        request.mutableResourceRequest().addHTTPHeaderFieldIfNotPresent(HTTPHeaderName::CacheControl, ASCIILiteral("no-cache"));
> 
> Strings can go in HTTPHeaderValues.h, right?

Yep.
Comment 22 WebKit Commit Bot 2016-10-11 03:55:58 PDT
Comment on attachment 291246 [details]
Patch for landing

Clearing flags on attachment: 291246

Committed r207086: <http://trac.webkit.org/changeset/207086>
Comment 23 WebKit Commit Bot 2016-10-11 03:56:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 youenn fablet 2016-10-12 08:19:26 PDT
WebKit2 follow-up patch is tracked by bug 163332