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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(6 files, 1 obsolete file)
4.16 KB,
patch
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.36 KB,
patch
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.56 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.40 KB,
patch
|
bkelly
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.49 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 | ||
Comment 2•8 years ago
|
||
Attachment #8715455 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Attachment #8715455 -
Attachment description: bug1244764_p1_cacheaddfail.patch → P1 Make Cache .add()/.addAll() fail if a Response.ok() is false. r=ehsan
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8715456 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8715457 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8715458 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7eee175cc9f
Assignee | ||
Comment 7•8 years ago
|
||
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 | ||
Comment 8•8 years ago
|
||
Try showed another devtools test that needs to get fixed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=922de55e874a
Attachment #8715507 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 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•8 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•8 years ago
|
Attachment #8715456 -
Flags: review?(ehsan) → review+
Updated•8 years ago
|
Attachment #8715457 -
Flags: review?(ehsan) → review+
Updated•8 years ago
|
Attachment #8715458 -
Flags: review?(ehsan) → review+
Updated•8 years ago
|
Attachment #8715507 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 10•8 years ago
|
||
I fixed the comment, although I meant "allow" instead of "call".
Attachment #8715455 -
Attachment is obsolete: true
Attachment #8715844 -
Flags: review+
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d39088bf6b05 https://hg.mozilla.org/integration/mozilla-inbound/rev/fefb8f3312f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/0517781f5ff7 https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e84bd32396 https://hg.mozilla.org/integration/mozilla-inbound/rev/30210bbd013e
Assignee | ||
Comment 12•8 years ago
|
||
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?
Assignee | ||
Comment 13•8 years ago
|
||
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?
Assignee | ||
Comment 14•8 years ago
|
||
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?
Assignee | ||
Comment 15•8 years ago
|
||
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•8 years ago
|
status-firefox44:
--- → wontfix
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Version: 32 Branch → unspecified
Assignee | ||
Comment 16•8 years ago
|
||
See comment 12.
Attachment #8716025 -
Flags: approval-mozilla-aurora?
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d39088bf6b05 https://hg.mozilla.org/mozilla-central/rev/fefb8f3312f2 https://hg.mozilla.org/mozilla-central/rev/0517781f5ff7 https://hg.mozilla.org/mozilla-central/rev/c9e84bd32396 https://hg.mozilla.org/mozilla-central/rev/30210bbd013e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Keywords: site-compat
Assignee | ||
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 18•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/cache-api-now-rejects-unsuccessful-responses/
Comment 19•8 years ago
|
||
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?
Keywords: dev-doc-needed → dev-doc-complete
Comment 20•8 years ago
|
||
Tracking this for 46+ since we're aiming this feature at 46.
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
Comment 21•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8715456 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8715457 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #8715507 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
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)
Assignee | ||
Comment 23•8 years ago
|
||
Since bug 1003860 is not in aurora, we don't need the P5 patch at all. That devtools test isn't using cache API. I went ahead and landed. remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/26c22e6b4ef5 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/93b6cbe48abd remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/1689beaabd59 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/bafa23fb5915
Flags: needinfo?(bkelly)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•