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

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Kershaw Chang [:kershaw]]

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.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: Bug 1536511 - Cache-control: stale-while-revalidate response header implementation, r=dragana!,michal!

Comment 3

[Honza Bambas (:mayhemer)]

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

Comment 4

[Honza Bambas (:mayhemer)]

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

Comment 5

[Honza Bambas (:mayhemer)]

(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.

Comment 6

[Honza Bambas (:mayhemer)]

rerun of affected wpt:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a41482d81790ed3ce5c5ea977de3cb4aaac8f86d

Comment 7

[Honza Bambas (:mayhemer)]

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

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: Bug 1536511 - Cache-control: stale-while-revalidate response header implementation, r=dragana,michal,asuth

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/728aa342ad35
Cache-control: stale-while-revalidate response header implementation,

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/728aa342ad35

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/34059c6188e5
Backed out changeset 728aa342ad35 for causing crashes on Bug 1545023. a=backout

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: Bug 1536511 - Cache-control: stale-while-revalidate response header implementation, r=kershaw!,asuth

Comment 13

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46ecb4e343db
Cache-control: stale-while-revalidate response header implementation, r=kershaw,asuth

Comment 16

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/46ecb4e343db

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16529 for changes under testing/web-platform/tests

Comment 19

[gwarser]

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

Comment 20

[Honza Bambas (:mayhemer)]

(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

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

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!)

Comment 22

[Honza Bambas (:mayhemer)]

(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 ;)

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged