See: https://github.com/slightlyoff/ServiceWorker/issues/710#issuecomment-235896038

I would like to take this bug.

Modify the check from both GET & HEAD requests to only GET request can query cache. After applying this patch, match(), mathcAll(), delete() and put() are aligned with spec [1], but I'm not sure about Put(), Add() and AddAll() since they need return TypeError when non-GET request (when option.ignoreMethod is false). I'll implement returning TypeError for Put(), Add() adn AddAll() when the input request is not GET and ignoreMethod is false tomorrow. [1] https://w3c.github.io/ServiceWorker/#cache-objects

Fix the mochitest after applying Part 1.

This patch is only for running local/try test to ensure the behavior is aligned with spec. I've found someone in google has already written similar tests [1] for this issue, so I'll not send this patch for review. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=633358

(In reply to Tom Tung [:tt] from comment #2) I find out that we've already check the request's method for add(), addAll() and put() [1], so send the Part 1 and Part 2 to review. [1] http://searchfox.org/mozilla-central/source/dom/cache/Cache.cpp#58

Comment on attachment 8818499 [details] [diff] [review] Part 1 - Cache API should not match() non-GET requests. Hi Ben, I implement this patch for preventing HEAD(non-GET) request to match() in Cache. Could you help me to review this patch? Thanks! I leave P2 patch for fixing mochitest and P3 patch for testing behavior in wpt.

Comment on attachment 8818500 [details] [diff] [review] Part 2 - fix the exist mochitest. Patch for fixing existing mochitests.

Tom, can you add a check that HEAD is not matched in these two WPT test files as well? https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/cache-storage/script-tests/cache-match.js https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/cache-storage/script-tests/cache-matchAll.js Thanks!

(In reply to Ben Kelly [:bkelly] from comment #8) Thanks for the review, Ben! > Tom, can you add a check that HEAD is not matched in these two WPT test > files as well? Actually, I had written three WPT tests(cache-match.js, cache-matchAll.js and cache-storage-match.js) in patch Part 3 (attachment 8818501 [details] [diff] [review]) and then I found out that someone in google had written similar WPT tests [1]. These WPT tests which are written by google people are not updated to our code base yet. Hence, I only uploaded my patch for adding WPT test but not sending to review. Should I ask for reviewing attachment 8818501 [details] [diff] [review] and land it after r+ as well? [1] https://bugs.chromium.org/p/chromium/issues/detail?id=633358

Update the summary of the patch since r+.

Update patch

try for applying Part 1, 2 and 3. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1578b1139e802d19066a0df120798a669fa005b8

Can you tweak your patch to match the code the blink tests added? Then we can land and it will get auto-upstreamed.

(In reply to Ben Kelly [:bkelly] from comment #13) > Can you tweak your patch to match the code the blink tests added? Then we > can land and it will get auto-upstreamed. Sure, I update blink's WPT tests which is related to github issue 710 [1]. Could you help me to review this patch? Thanks! [1] https://github.com/w3c/ServiceWorker/issues/710#issuecomment-235896038 try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20a69d815eddc38a9587588ff2d4b56f07b22ce7

Comment on attachment 8819244 [details] [diff] [review] Part 3 - Update blink's WPT tests which are related to do not match non-GET requests. Review of attachment 8819244 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/service-workers/cache-storage/resources/test-helpers.js @@ +247,5 @@ > + assert_header_equals(actual.headers, expected.headers, description); > +} > + > +// TODO(zino): Should remove this function once keys() returns request > +// keys in key insertion order. Please see http://crbug.com/627821. I don't think we should incorporate or uplift this blink specific work-around. Can you remove this help and then replace with assert_request_array_equals()? Alternatively, you don't need to import ablink's keys() tests in this bug at all, right? I only really meant to sync the HEAD related tests here. ::: testing/web-platform/tests/service-workers/cache-storage/worker/cache-keys.https.html @@ +1,4 @@ > +<!DOCTYPE html> > +<title>Cache.keys</title> > +<link rel="help" href="https://w3c.github.io/ServiceWorker/#cache-keys"> > +<meta name="timeout" content="long"> We do we have a long timeout on the worker version of this test, but not the window version?

(In reply to Ben Kelly [:bkelly] from comment #15) Thanks for the review! I rewrite a patch to remove the tests for cache-keys. Could you help me to review it again? Thanks! > Comment on attachment 8819244 [details] [diff] [review] > Part 3 - Update blink's WPT tests which are related to do not match non-GET > requests. > > Review of attachment 8819244 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > testing/web-platform/tests/service-workers/cache-storage/resources/test- > helpers.js > @@ +247,5 @@ > > + assert_header_equals(actual.headers, expected.headers, description); > > +} > > + > > +// TODO(zino): Should remove this function once keys() returns request > > +// keys in key insertion order. Please see http://crbug.com/627821. > > I don't think we should incorporate or uplift this blink specific > work-around. > > Can you remove this help and then replace with assert_request_array_equals()? > > Alternatively, you don't need to import ablink's keys() tests in this bug at > all, right? I only really meant to sync the HEAD related tests here. Right, I was thinking to update all the tests which they updated in that issue and update the test that we don't have yet. I remove the tests in cache-keys from the patch since we haven't uplift this whole testcase yet. > ::: > testing/web-platform/tests/service-workers/cache-storage/worker/cache-keys. > https.html > @@ +1,4 @@ > > +<!DOCTYPE html> > > +<title>Cache.keys</title> > > +<link rel="help" href="https://w3c.github.io/ServiceWorker/#cache-keys"> > > +<meta name="timeout" content="long"> > > We do we have a long timeout on the worker version of this test, but not the > window version? Sorry, it should not have a long timeout on both test. However, since I remove the tests in cache-keys, I will remove it as well.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d50c417aa131de4ea70d0ef48c7ce91ac7697c5d

Comment on attachment 8820161 [details] [diff] [review] Part 3 - Update blink's WPT tests which are related to do not match non-GET requests. Review of attachment 8820161 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

(In reply to Ben Kelly [:bkelly] from comment #18) Thanks for the review!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/39222b3e7ed3 Part 1 - Cache API should not match() non-GET requests. r=bkelly. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae61f13238a8 Part 2 - fix the exist mochitest. r=bkelly. https://hg.mozilla.org/integration/mozilla-inbound/rev/9589c4a6677c Part 3 - Update blink's WPT tests which are related to do not match non-GET requests. r=bkelly.

https://hg.mozilla.org/mozilla-central/rev/39222b3e7ed3 https://hg.mozilla.org/mozilla-central/rev/ae61f13238a8 https://hg.mozilla.org/mozilla-central/rev/9589c4a6677c