Closed Bug 1272436 Opened 9 years ago Closed 8 years ago

Implement "only-if-cached" RequestCache mode

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: asuth)

References

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active)

Attachments

(3 files, 3 obsolete files)

This is being spec'ed in https://github.com/whatwg/fetch/issues/159. Note that we have once removed this in https://hg.mozilla.org/mozilla-central/rev/8f569dd0a9eb, it should be possible to bring it back without requiring a cache migration. Andrew, is this something that interests you? :-)
Flags: needinfo?(bugmail)
Ben, what should be done about the existing 17To18 migration given that it/bug 1240161's fix is present in the current release, Firefox 46? It seems simplest to leave the migration intact until the next time the schema gets fully nuked and all migrations get cleared out, but it could also be hollowed out to just bump the schema.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(bugmail) → needinfo?(bkelly)
I guess the main benefit here of changing the migration to just bump the schema version is that we could then change test_migration.js to show that we support only-if-cached. I doubt it would affect many, if any, profiles in the wild though. I could go either way. It seems slightly more correct to just leave the migration.
Flags: needinfo?(bkelly)
Depends on: 1120715
r? ehsan for DOM stuff; feel free to delegate to bkelly if appropriate, but he's not formally a peer at this time. r? mayhemer for the necko stuff, based on prior review of bug 1120715. I think this is right per the note below, but I don't want to make dump assumptions. The choice of the combination of load flags LOAD_FROM_CACHE and nsICachingChannel::LOAD_ONLY_FROM_CACHE was based on the documentation for the flags and confirmed by archaeology as reasonable choices by :mayhemer in https://bugzilla.mozilla.org/show_bug.cgi?id=1120715#c11 discussing the original implementation of cache mode.
Attachment #8753727 - Flags: review?(honzab.moz)
Attachment #8753727 - Flags: review?(ehsan)
r? ehsan with deferral being okay again, although maybe you have deeper meta thoughts about the request-cache tests since you created them originally. Note: These are web-platform tests, but it seems like magic upstreaming is okay for these? Also, I haven't done the manifest updating yet. I got burned last time I tried that because mach seems to commit stuff automatically by default, so I figured I'd put these patches up first before I destroy my checkout. request-cache.html: - Add comment block explaining the test dictionary arguments. - Eliminate duplicated code in populate_cache which seems to have existed only for historical reasons and is now moot. - Add `response` optional test dictionary array for cases where a network error is expected as the result of the fetch. Normalize server results to handle this case since test cases may no longer talk to the server at all. - Add only-if-cached tests that verify the network is not touched in any of the fresh cached, stale cached, and not cached cases. request-error.html: - Add test verifying we enforce only-if-cached requires same-origin mode.
Attachment #8753728 - Flags: review?(ehsan)
Comment on attachment 8753727 [details] [diff] [review] P0: Implement "only-if-cached" RequestCache mode. Review of attachment 8753727 [details] [diff] [review]: ----------------------------------------------------------------- I've also read something about not caching redirects in the #159 issue. Where is that going to be addressed? I don't see it here (unless I miss something). r+ for the HttpChannel bits mainly. ::: dom/fetch/Request.cpp @@ +428,5 @@ > + if (cache == RequestCache::Only_if_cached && > + request->Mode() != RequestMode::Same_origin) { > + aRv.ThrowTypeError<MSG_ONLY_IF_CACHED_WITHOUT_SAME_ORIGIN>(); > + return nullptr; > + } nit: indention?
Attachment #8753727 - Flags: review?(honzab.moz) → review+
Whiteboard: btpp-active
(In reply to Honza Bambas (:mayhemer) from comment #5) > I've also read something about not caching redirects in the #159 issue. > Where is that going to be addressed? I don't see it here (unless I miss > something). Thank you for noticing and raising this! I've inquired on the issue. It seems like the most pragmatic solution at this time is to require that the "redirect" mode be set to "error", and I've proposed that there. Before realizing this was the best approach, I prepared the tests that assumed it was enforced by the fetch() algorithm rather than the Request constructor throwing. Those tests may no longer be necessary if we just do the constructor thing. > nit: indention? Yeah, that indentation was really messed up. Not sure what happened, thanks.
(fixed whitespace, carrying forward :mayhemer r+ for now, clearing ehsan review flag until spec issue resolved.)
Attachment #8753727 - Attachment is obsolete: true
Attachment #8753727 - Flags: review?(ehsan)
Attachment #8756743 - Flags: review+
Attachment #8753728 - Flags: review?(ehsan)
Comment on attachment 8756743 [details] [diff] [review] P0: Implement "only-if-cached" RequestCache mode. On the github issue :annevk is cool with the "same-origin" enforcement being sufficient, so the current patch does all we need implementation-wise and I have revised the P1b tests to have coverage that verifies same-origin redirects work fine with only-if-cached and non-same-origin redirects fail with a rejection, as expected.
Attachment #8756743 - Flags: review?(bkelly)
This goes on top of P1 which I left separate for review since they do sort-of build and I'm trying to build up my split-patches muscles. Try run is pending: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a3abd360869
Attachment #8756746 - Attachment is obsolete: true
Attachment #8757172 - Flags: review?(bkelly)
Attachment #8753728 - Flags: review?(bkelly)
Comment on attachment 8753728 [details] [diff] [review] P1 Fetch "only-if-cached" cache mode tests. Review of attachment 8753728 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: testing/web-platform/tests/fetch/api/request/request-cache.html @@ +14,5 @@ > var now = new Date(); > + /** > + * Each test is run twice: once using etag/If-None-Match and once with > + * date/If-Modified-Since. Each test run gets its own URL and randomized > + * content and operates independently. Nice documentation! @@ -397,5 @@ > }).then(function(text) { > - return JSON.parse(text); > - }); > - } > - function populate_cache(url, content, info, type, identifier) { I guess this function was not needed? Can you explain why it was removed? ::: testing/web-platform/tests/fetch/api/request/request-error.html @@ +63,5 @@ > },"RequestInit's mode is no-cors and integrity is not empty"); > > test(function() { > + assert_throws(new TypeError() , > + function() { new Request("", {"mode" : "cors", "cache" : "same-origin"}); }, This is setting cache to 'same-origin'? Shouldn't it be 'only-if-cached'?
Attachment #8753728 - Flags: review?(bkelly) → review+
Comment on attachment 8757172 [details] [diff] [review] P1b "only-if-cached" tests verify the same-origin enforcement applies to redirects. Review of attachment 8757172 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: testing/web-platform/tests/fetch/api/request/request-cache.html @@ +522,5 @@ > if ("request_headers" in info) { > init.headers = info.request_headers[idx]; > } > + if ("redirect" in info) { > + init.redirect = "follow"; Why is this necessary? 'follow' should be the default. ::: testing/web-platform/tests/fetch/api/request/resources/cache.py @@ +56,5 @@ > + > + if redirect == "same-origin": > + netloc = request.url_parts.netloc > + else: > + netloc = request.server.config["domains"]["www"] + ":" + str(request.server.config["ports"]["http"][0]) I think its more typical to pass the redirect origin or URL as an encoded query param from the test script. Then we can more easily substitute the cross-origin locations that the test runner is configured with.
Attachment #8757172 - Flags: review?(bkelly) → review+
Comment on attachment 8756743 [details] [diff] [review] P0: Implement "only-if-cached" RequestCache mode. Review of attachment 8756743 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Note, you will need to get an actual DOM peer to review the webidl change. ::: dom/bindings/Errors.msg @@ +97,5 @@ > MSG_DEF(MSG_INVALID_EASING_ERROR, 1, JSEXN_TYPEERR, "Invalid easing '{0}'.") > MSG_DEF(MSG_USELESS_SETTIMEOUT, 1, JSEXN_TYPEERR, "Useless {0} call (missing quotes around argument?)") > MSG_DEF(MSG_TOKENLIST_NO_SUPPORTED_TOKENS, 2, JSEXN_TYPEERR, "{0} attribute of <{1}> does not define any supported tokens") > MSG_DEF(MSG_CACHE_STREAM_CLOSED, 0, JSEXN_TYPEERR, "Response body is a cache file stream that has already been closed.") > +MSG_DEF(MSG_ONLY_IF_CACHED_WITHOUT_SAME_ORIGIN, 0, JSEXN_TYPEERR, "Request cache mode 'only-if-cached' can only be used with a request mode of 'same-origin'.") It would be nice to include what the bad request mode being used is. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +2382,5 @@ > *aFetchCacheMode = nsIHttpChannelInternal::FETCH_CACHE_MODE_RELOAD; > } else if (mLoadFlags & VALIDATE_ALWAYS) { > *aFetchCacheMode = nsIHttpChannelInternal::FETCH_CACHE_MODE_NO_CACHE; > } else if (mLoadFlags & LOAD_FROM_CACHE) { > + if (mLoadFlags & nsICachingChannel::LOAD_ONLY_FROM_CACHE) { nit: The style of this if/else sequence is to check the combination of flags instead of using a nested if-statement. See how we check INHIBIT_CACHING | LOAD_BYPASS_CACHE for no-store above. I think it makes it a bit easier to see the flag combinations.
Attachment #8756743 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [:bkelly] from comment #11) > @@ -397,5 @@ > > }).then(function(text) { > > - return JSON.parse(text); > > - }); > > - } > > - function populate_cache(url, content, info, type, identifier) { > > I guess this function was not needed? Can you explain why it was removed? (From commit message since I wrote it and want credit ;) - Eliminate duplicated code in populate_cache which seems to have existed only for historical reasons and is now moot. Elaborating: I had done some git log digging prior to removing request-cache.html. It looked like in the patch that created the tests it was relevant, but refactoring was done in a subsequent patch in the patch set that made the code literally identical between the two cases. The populate_cache name did seem informative but otherwise useless, especially as the per-test info dictionaries did not differentiate between the setup fetch and the test fetches. > This is setting cache to 'same-origin'? Shouldn't it be 'only-if-cached'? ...I'm not sure what happened there. But, uh, you passed my test to make sure you were actually reviewing things? Yeah, that's the ticket.
(In reply to Ben Kelly [:bkelly] from comment #12) > Review of attachment 8757172 [details] [diff] [review]: > Why is this necessary? 'follow' should be the default. It wasn't, removed. > ::: testing/web-platform/tests/fetch/api/request/resources/cache.py > @@ +56,5 @@ > > + > > + if redirect == "same-origin": > > + netloc = request.url_parts.netloc > > + else: > > + netloc = request.server.config["domains"]["www"] + ":" + str(request.server.config["ports"]["http"][0]) > > I think its more typical to pass the redirect origin or URL as an encoded > query param from the test script. Then we can more easily substitute the > cross-origin locations that the test runner is configured with. Agreed, this is weird. This was a result of patch evolution and not wanting to worry about hg blame lossage if I renamed the file to ".sub.html" or moving the JS test code into its own file that can use the pipe/sub mechanism. I'll: - move the file to be ".sub.html" - have cache.py take a Location to repeat back verbatim which avoids having to re-encode arguments, etc. - leave the test dictionary arguments as { redirect: "same-origin"/"cross-origin" } since I think that's easier to understand the intent of the test case when skimming the tests than directly doing "{{domains[www]}}:{{ports[http[0]}}". (The test's driver logic will do that stuff.) (In reply to Ben Kelly [:bkelly] from comment #13) > Review of attachment 8756743 [details] [diff] [review]: > It would be nice to include what the bad request mode being used is. Done. Examples from console: I: new Request("", {"mode" : "cors", "cache" : "only-if-cached"}); O: TypeError: Request mode 'cors' was used, but request cache mode 'only-if-cached' can only be used with request mode 'same-origin'. I: new Request("", {"mode" : "no-cors", "cache" : "only-if-cached"}); O: TypeError: Request mode 'no-cors' was used, but request cache mode 'only-if-cached' can only be used with request mode 'same-origin'. > nit: The style of this if/else sequence is to check the combination of flags > instead of using a nested if-statement. See how we check INHIBIT_CACHING | > LOAD_BYPASS_CACHE for no-store above. I think it makes it a bit easier to > see the flag combinations. Changed. I was on the fence about this; I think trying to stick to 80 characters is what pushed me to what I had used.
(In reply to Andrew Sutherland [:asuth] from comment #15) > - move the file to be ".sub.html" You don't need to do this. See: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html#16 And: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html#34 > leave the test dictionary arguments as { redirect: "same-origin"/"cross-origin" } You can have a function in js that converts from this to the encoded arg pretty easily. I do that in the code linked above. This is kind of a style thing, but its easier to understand the test if we minmize the number of idioms in use. So making it look like the existing redirect.py would be nice: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/redirect.py
inter-diff (before squash): https://github.com/xeonchen/gecko/commit/0b8b24abbfc71bf3863667f1be8fc02b88a4b38f carrying over r=mayhemer, r=bkelly, begging dom peer on IRC for webidl signoff now.
Attachment #8756743 - Attachment is obsolete: true
Attachment #8757535 - Flags: review+
Comment on attachment 8757535 [details] [diff] [review] P0: Implement "only-if-cached" RequestCache mode. v2 w/review fixes r=me on the webidl bit.
Attachment #8757535 - Flags: review+
Pushed by bugmail@asutherland.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/253d159498a6 P0: Implement "only-if-cached" RequestCache mode. r=mayhemer, r=bkelly, r=bzbarsky for WebIDL change https://hg.mozilla.org/integration/mozilla-inbound/rev/d325ced29e5b P1 Fetch "only-if-cached" cache mode tests. r=bkelly
Most recent try run with the last test issues updated and squashed tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8494e75fde8c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
-> https://developer.mozilla.org/en-US/docs/Web/API/Request/cache I updated the page with more info about the other options, mostly copied from a blog post by some guy named Ehsan. Does it look OK to you?
Flags: needinfo?(bugmail)
Yes, looks great, thank you. The comments on the code samples are quite nice in particular. Apologies for the delay.
Flags: needinfo?(bugmail)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: