Closed Bug 1244764 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(6 files, 1 obsolete file)

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
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.
Attachment #8715507 - Attachment description: bug1244764_p5_devtoolstest.patch → P5 Fix devtools test to work with new Cache add()/addAll() behavior. r=ehsan
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+
Attachment #8715456 - Flags: review?(ehsan) → review+
Attachment #8715457 - Flags: review?(ehsan) → review+
Attachment #8715458 - Flags: review?(ehsan) → review+
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?
Version: 32 Branch → unspecified
Attached patch Aurora P4 PatchSplinter Review
See comment 12.
Attachment #8716025 - Flags: approval-mozilla-aurora?
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: