Cache API should not match() HEAD requests

RESOLVED FIXED in Firefox 53

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: bkelly, Assigned: tt)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Priority: -- → P2
Assignee

Comment 1

3 years ago
I would like to take this bug.
Assignee: nobody → ttung
Assignee

Comment 2

3 years ago
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
Assignee

Comment 3

3 years ago
Fix the mochitest after applying Part 1.
Assignee

Comment 4

3 years ago
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
Assignee

Comment 5

3 years ago
(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
Assignee

Comment 6

3 years ago
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.
Attachment #8818499 - Attachment description: [WIP] Part 1 - Cache API should not match() non-GET requests. → Part 1 - Cache API should not match() non-GET requests.
Attachment #8818499 - Flags: review?(bkelly)
Assignee

Comment 7

3 years ago
Comment on attachment 8818500 [details] [diff] [review]
Part 2 - fix the exist mochitest.

Patch for fixing existing mochitests.
Attachment #8818500 - Attachment description: [WIP] Part 2 - fix the exist mochitest. → Part 2 - fix the exist mochitest.
Attachment #8818500 - Flags: review?(bkelly)
Assignee

Updated

3 years ago
Attachment #8818501 - Attachment description: [WIP, local test only] Part 3 - add few wpt tests. → [local test only] Part 3 - add few wpt tests.
Attachment #8818499 - Flags: review?(bkelly) → review+
Attachment #8818500 - Flags: review?(bkelly) → review+
Assignee

Comment 9

3 years ago
(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
Flags: needinfo?(ttung) → needinfo?(bkelly)
Assignee

Comment 10

3 years ago
Update the summary of the patch since r+.
Attachment #8818499 - Attachment is obsolete: true
Assignee

Comment 11

3 years ago
Update patch
Attachment #8818500 - Attachment is obsolete: true
Can you tweak your patch to match the code the blink tests added?  Then we can land and it will get auto-upstreamed.
Flags: needinfo?(bkelly)
Assignee

Comment 14

3 years ago
(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
Attachment #8818501 - Attachment is obsolete: true
Attachment #8819244 - Flags: review?(bkelly)
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?
Attachment #8819244 - Flags: review?(bkelly) → review-
Assignee

Comment 16

3 years ago
(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.
Attachment #8819244 - Attachment is obsolete: true
Attachment #8820161 - Flags: review?(bkelly)
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!
Attachment #8820161 - Flags: review?(bkelly) → review+
Assignee

Comment 19

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #18)

Thanks for the review!
Attachment #8820161 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 20

3 years ago
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.
Keywords: checkin-needed

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39222b3e7ed3
https://hg.mozilla.org/mozilla-central/rev/ae61f13238a8
https://hg.mozilla.org/mozilla-central/rev/9589c4a6677c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.