(no longer active) Reporter
	Description • 8 years ago

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? :-)

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 1 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 2 • 8 years ago

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.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 3 • 8 years ago

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.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 4 • 8 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 5 • 8 years ago

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?

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 6 • 8 years ago

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

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 7 • 8 years ago

(fixed whitespace, carrying forward :mayhemer r+ for now, clearing ehsan review flag until spec issue resolved.)

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 8 • 8 years ago

(details in patch commit comment)

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 9 • 8 years ago

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.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 10 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 8 years ago

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'?

	Ben Kelly [:bkelly, not reviewing]
	Comment 12 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 8 years ago

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.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 14 • 8 years ago

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

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 15 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 8 years ago

(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

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 17 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 8 years ago

Comment on attachment 8757535 [details] [diff] [review]
P0: Implement "only-if-cached" RequestCache mode. v2 w/review fixes

r=me on the webidl bit.

	Pulsebot
	Comment 19 • 8 years ago

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

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 20 • 8 years ago

Most recent try run with the last test issues updated and squashed tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8494e75fde8c

	Carsten Book [:Tomcat]
	Comment 21 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/253d159498a6
https://hg.mozilla.org/mozilla-central/rev/d325ced29e5b

	Will Bamberg [:wbamberg]
	Comment 22 • 8 years ago

-> 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?

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 23 • 8 years ago

Yes, looks great, thank you.  The comments on the code samples are quite nice in particular.  Apologies for the delay.