Closed Bug 1536511 Opened 5 years ago Closed 5 years ago

Implement stale-while-revalidate c-c response directive handling in HTTP cache validation code

Categories

(Core :: Networking: HTTP, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [necko-triaged][wptsync upstream])

Attachments

(1 file, 2 obsolete files)

See https://tools.ietf.org/html/rfc5861#section-3

If c-c: s-w-r is present on the cached response and the entry is not fresh enough but the request manages to fall into the revalidate window, serve from the cache and then start an independent, same principal http channel instructed to revalidate the entry and potentially refresh with a new response in background, given the lowest possible priority.

Tentative P1 as it would be nice to have this in this cycle, Chrome is shiping this in M75 (June 4th).

FYI, google doesn't plan to shorten their max-age time in favor of the revalidation window use.

Honza, I'd like to assign this to you since this is a P1 bug.
If you think someone else should work on this, please put this back to triage list. Thanks.

Assignee: nobody → honzab.moz
Whiteboard: [necko-triaged]

I'm changing a bit how the expiration time is stored on a cache entry to allow calculation of the stale window for most cases it can be calculated (like max-age=0), let's see how the try push goes

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

The expiration time change probably causes a perma fail of:

TEST-UNEXPECTED-FAIL | /fetch/stale-while-revalidate/fetch.tentative.html | Second fetch does not return same response - assert_not_equals: got disallowed value "weyjkbkkgbldasdsozay"
TEST-UNEXPECTED-PASS | /fetch/stale-while-revalidate/stale-image.tentative.html | Cache returns stale resource - expected FAIL
TEST-UNEXPECTED-PASS | /fetch/stale-while-revalidate/stale-script.tentative.html | Cache returns stale resource - expected FAIL
TEST-UNEXPECTED-TIMEOUT | /service-workers/service-worker/client-navigate.https.html | expected OK

Blocks: 1540216

(In reply to Honza Bambas (:mayhemer) from comment #4)

The expiration time change probably causes a perma fail of:

ok, I'm apparently blind - those are wpt tests touching exactly this feature. patch will be updated.

Status: NEW → ASSIGNED

After reading actual changes to the fetch() spec, there are two bits missing in the patch:

  • service workers has to be explicitly disabled for the background revalidate request
  • the revalidate requests has to be part of the browser context (it is - same principal) but also has to be aborted when the context goes away; hence, we have to add it to the same load group, which (unless I'm mistaken) allows us to use request tailing ensuring way small effect on page load time

going to update

Blocks: 1542322
Depends on: 1543742
Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/728aa342ad35
Cache-control: stale-while-revalidate response header implementation,
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1544918
Regressions: 1545023
Blocks: 1543742
No longer depends on: 1543742
Attachment #9054480 - Attachment is obsolete: true
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/34059c6188e5
Backed out changeset 728aa342ad35 for causing crashes on Bug 1545023. a=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---
Status: REOPENED → ASSIGNED
Attachment #9058541 - Attachment is obsolete: true

In case it's useful: in bug 1545023 comment 13, I posted some STR that reliably trigger the crash there ([@ mozilla::net::LoadInfo::LoadInfo]) in Nightly builds from before this was backed out.

Pushed by honzab.moz@firemni.cz:
https://hg.mozilla.org/integration/autoland/rev/46ecb4e343db
Cache-control: stale-while-revalidate response header implementation, r=kershaw,asuth
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16529 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged] → [necko-triaged][wptsync upstream]

(In reply to gwarser from comment #19)

Compatibility table links to 995651 https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#Browser_compatibility

Thanks for pointing this out! Created https://github.com/mdn/browser-compat-data/pull/4004

Honza, can you take a look at this last small change on the spec PR so we can close the loop there?

https://github.com/whatwg/fetch/pull/853#discussion_r278136865

Thanks! (And thanks for the quick implementation!)

Flags: needinfo?(honzab.moz)

(In reply to Ben Kelly [:bkelly, not reviewing] from comment #21)

Honza, can you take a look at this last small change on the spec PR so we can close the loop there?

https://github.com/whatwg/fetch/pull/853#discussion_r278136865

I commented there, but we need some feedback from Anne.

Thanks! (And thanks for the quick implementation!)

There are followups pending, so don't cheer too soon ;)

Flags: needinfo?(honzab.moz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: