Cache .add() and .addAll() should reject if any response is not ok()

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45 wontfix, firefox46+ fixed, firefox47+ fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Decision from the f2f to make cache .add() and .addAll() block not ok() responses.  This will block opaque responses as a side effect:

https://github.com/slightlyoff/ServiceWorker/issues/823#issuecomment-175320500

Note, google checked telemetry to verify opaque responses are not frequently stored in cache currently.
We need the new cache wpt test changes from blink before this can land.  Mainly to avoid forcing blink to re-merge their test changes.
Depends on: 1245460
(Assignee)

Updated

3 years ago
Attachment #8715455 - Attachment description: bug1244764_p1_cacheaddfail.patch → P1 Make Cache .add()/.addAll() fail if a Response.ok() is false. r=ehsan
Comment on attachment 8715458 [details] [diff] [review]
P4 Update cache wpt tests for new add()/addAll() behavior. r=ehsan

Review of attachment 8715458 [details] [diff] [review]:
-----------------------------------------------------------------

Note that these tests changes were provided by the blink team.  I propose we just adopt them.
(Assignee)

Updated

3 years ago
Attachment #8715507 - Attachment description: bug1244764_p5_devtoolstest.patch → P5 Fix devtools test to work with new Cache add()/addAll() behavior. r=ehsan

Comment 9

3 years ago
Comment on attachment 8715455 [details] [diff] [review]
P1 Make Cache .add()/.addAll() fail if a Response.ok() is false. r=ehsan

Review of attachment 8715455 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/Cache.cpp
@@ +157,5 @@
>          Fail();
>          return;
>        }
>  
> +      // Do not all the convenience methods .add()/.addAll() to cache failed

*call
Attachment #8715455 - Flags: review?(ehsan) → review+

Updated

3 years ago
Attachment #8715456 - Flags: review?(ehsan) → review+

Updated

3 years ago
Attachment #8715457 - Flags: review?(ehsan) → review+

Updated

3 years ago
Attachment #8715458 - Flags: review?(ehsan) → review+

Updated

3 years ago
Attachment #8715507 - Flags: review?(ehsan) → review+
I fixed the comment, although I meant "allow" instead of "call".
Attachment #8715455 - Attachment is obsolete: true
Attachment #8715844 - Flags: review+
Comment on attachment 8715844 [details] [diff] [review]
P1 Make Cache .add()/.addAll() fail if a Response.ok() is false. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service Worker Cache API non-backward compatible change.
[User impact if declined]: I would like to uplift this change so that it ships in the same timeframe as chrome.  They indicate it will ship in April.  Uplifting to 46 would result in us shipping in April as well.
[Describe test coverage new/current, TreeHerder]: Tests included.  The wpt test patch will need to be rebased for aurora.
[Risks and why]: Minimal, only affects cache API.
[String/UUID change made/needed]: None
Attachment #8715844 - Flags: approval-mozilla-aurora?
Comment on attachment 8715456 [details] [diff] [review]
P2 Make dom/cache mochitests pass with new add()/addAll() behavior. r=ehsan

See comment 12.
Attachment #8715456 - Flags: approval-mozilla-aurora?
Comment on attachment 8715457 [details] [diff] [review]
P3 Make service worker tests pass with new Cache add()/addAll() behavior. r=ehsan

See comment 12.
Attachment #8715457 - Flags: approval-mozilla-aurora?
Comment on attachment 8715507 [details] [diff] [review]
P5 Fix devtools test to work with new Cache add()/addAll() behavior. r=ehsan

See comment 12.
Attachment #8715507 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Version: 32 Branch → unspecified
See comment 12.
Attachment #8716025 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Keywords: dev-doc-needed
I've noted this in an exceptions table on

https://developer.mozilla.org/en-US/docs/Web/API/Cache/add
https://developer.mozilla.org/en-US/docs/Web/API/Cache/addAll

And added a line to the browser compat tables to make it clear what version this change is available in.

Is that ok?
Tracking this for 46+ since we're aiming this feature at 46.
Comment on attachment 8715844 [details] [diff] [review]
P1 Make Cache .add()/.addAll() fail if a Response.ok() is false. r=ehsan

SW changes aimed at 46, includes tests.
OK to uplift to aurora.
Attachment #8715844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8715456 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8715457 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8715507 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8716025 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm hitting conflicts uplifting part 5. Looks like you'll need to rebase around the lack of bug 1003860.
Flags: needinfo?(bkelly)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.