Fix HTTP cache revalidation bugs #24388

jdm · 2019-10-07T21:22:46Z

This fixes two cache related issues:

old cached content could be viewed as a result of revalidating a newer cached response
the expiry of a cached response refreshed through a revalidation could be calculated without any of the original response's headers

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Reload goes back in time (http cache issue?) #24385
There are tests for these changes

This change is

…e's headers.

…onse.

highfive · 2019-10-07T21:22:51Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/http_loader.rs, components/net/http_cache.rs

highfive · 2019-10-07T21:22:53Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

jdm · 2019-10-07T21:23:06Z

r? @gterzian

gterzian

I agree with one change, and I'm less sure about the overwriting of existing cached resource. I'll do a wpt run since I expect at least one test to fail.

gterzian · 2019-10-08T06:40:22Z

components/net/http_cache.rs

+                    let mut stored_headers = cached_resource.data.metadata.headers.lock().unwrap();
+                    stored_headers.extend(response.headers);
+                    constructed_response.headers = stored_headers.clone();
+                }


This change makes a lot of sense, it seemed like it was a mistake to calculate the expiry without first replacing the headers.

gterzian · 2019-10-08T06:50:11Z

components/net/http_cache.rs

+        {
+            // Ensure that any existing complete response is overwritten by the new
+            // complete response.
+            let existing_complete_response = entry.iter().position(|response| {


I'm less sure about this, since in theory different complete responses could be cached and used to satisfy different requests, for example in the case of responses with different Vary headers. We can try running this test with those changes:

servo/tests/wpt/web-platform-tests/fetch/http-cache/vary.html

Line 77 in 2313f7f

name: "HTTP cache doesn't invalidate existing Vary response",

gterzian · 2019-10-08T06:55:12Z

@bors-servo try=wpt

Expectation: failure of "HTTP cache doesn't invalidate existing Vary response"

bors-servo · 2019-10-08T06:55:17Z

⌛ Trying commit 2313f7f with merge daa7a3a...

Fix HTTP cache revalidation bugs This fixes two cache related issues: * old cached content could be viewed as a result of revalidating a newer cached response * the expiry of a cached response refreshed through a revalidation could be calculated without any of the original response's headers --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24385 - [ ] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24388)

bors-servo · 2019-10-08T07:31:13Z

💔 Test failed - linux-rel-wpt

CYBAI · 2019-10-08T07:38:50Z

I think this is what @gterzian expected 👀

{
    "status": "FAIL", 
    "group": "default", 
    "message": "assert_less_than: Response 3 does not come from cache expected a number less than 3 but got 3", 
    "stack": "checkResponse@http://web-platform.test:8000/fetch/http-cache/http-cache.js:136:9\n", 
    "subtest": "HTTP cache doesn't invalidate existing Vary response", 
    "test": "/fetch/http-cache/vary.html", 
    "line": 168808, 
    "action": "test_result", 
    "expected": "PASS"
}

gterzian · 2019-10-08T08:54:46Z

components/net/http_cache.rs

+                "there are {} cached responses for {:?}",
+                cached_resources.len(),
+                request.url()
+            );
            for cached_resource in cached_resources.iter_mut() {


A simple, and perhaps effective, fix for the "refresh of older resource", for now, might be to sort those by their original expiry. Then we can leave a TODO regarding the full "selection" logic as described at https://tools.ietf.org/html/rfc7234#section-4.3.4

On second thought, I think it might be best to start with the strictest case, and default to None for the less certain ones, and then over time move down the ladder of selection.

The weak validator and no validator case mentioned at https://tools.ietf.org/html/rfc7234#section-4.3.4 are softer and require us to handle all other "strong validator" cases correctly, so we probably shouldn't even try to do those for now, since the risk of erroneous caching is not worth it.

As described at https://tools.ietf.org/html/rfc7232#section-2.2.2 , if the stored resource has a Date header, and the Last-Modified header of the 304 response is at least 60 seconds earlier than the stored date, the Last-Modified header becomes a strong validator.

And per the "strong validator" case at https://tools.ietf.org/html/rfc7234#section-4.3.4:

All of the stored responses with the
same strong validator are selected. If none of the stored
responses contain the same strong validator, then the cache MUST
NOT use the new response to update any stored responses.

So if we don't handle this case first, we are not following this MUST NOT.

Then we would need to cover all other "strong validator" cases, and only then can we start to try to use weak or no validators.

gterzian · 2019-10-08T10:31:02Z

components/net/http_cache.rs

@@ -331,8 +333,12 @@ fn create_cached_response(
    // TODO: take must-revalidate into account <https://tools.ietf.org/html/rfc7234#section-5.2.2.1>
    // TODO: if this cache is to be considered shared, take proxy-revalidate into account
    // <https://tools.ietf.org/html/rfc7234#section-5.2.2.7>
-    let has_expired =
-        (adjusted_expires < time_since_validated) || (adjusted_expires == time_since_validated);
+    let has_expired = adjusted_expires <= time_since_validated;


Also here, we could be returning None if has_expired is true, and the stored resource doesn't contain a Date and Last-Modified header, since we need those to do a successful refresh, and if we can't, we should not construct a response that requires validation, and instead just go to the network "normally".

We could even purge such an expired resource that can't be successfully refreshed.

Mmh, one issue here is that fetch doesn't have a hook to deal with the "could not select a stored response for update", as Step 7.4 seems to assume that a refresh is always a succesful operation.

I've filed whatwg/fetch#950

bors-servo · 2019-10-08T14:22:14Z

☔ The latest upstream changes (presumably #24318) made this pull request unmergeable. Please resolve the merge conflicts.

gterzian

So I think the easiest way to unblock this one, is to simply refresh the most recent entry and use it to construct a response.

gterzian · 2020-06-25T08:46:19Z

components/net/http_cache.rs

@@ -721,6 +727,11 @@ impl HttpCache {
        assert_eq!(response.status.map(|s| s.0), Some(StatusCode::NOT_MODIFIED));
        let entry_key = CacheKey::new(&request);
        if let Some(cached_resources) = self.entries.get_mut(&entry_key) {
+            trace!(


So I would say the easy way to fix the problem, is to choose the cached resources that is the most recent, and then select that one for update and construct a response from it.

This also matches the "weak validator" part of https://httpwg.org/http-core/draft-ietf-httpbis-cache-latest.html#rfc.section.4.3.4

We can do the strong validator checking and so on in a different PR, since the problem here was the cache "going back in time", choosing the most recent entry should fix it.

gterzian · 2020-06-25T08:47:05Z

components/net/http_cache.rs

+            .as_ref()
+            .map_or(false, |s| s.0 == StatusCode::OK)
+        {
+            // Ensure that any existing complete response is overwritten by the new


I think we should not overwrite entries, since it will make it harder to have a spec compliant cache(needs to support multiple entries, matching on Etag and so on).

jdm added 4 commits October 7, 2019 17:08

Format some cache code for readability.

ca32d2c

Add useful cache debugging messages.

e093eee

Calculate expiry for 304 response after merging with existing respons…

da66443

…e's headers.

Replace existing cached response when revalidation returns a new resp…

2313f7f

…onse.

highfive assigned SimonSapin Oct 7, 2019

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 7, 2019

jdm changed the title ~~Cache fun~~ Fix HTTP cache revalidation bugs Oct 7, 2019

highfive assigned gterzian and unassigned SimonSapin Oct 7, 2019

gterzian reviewed Oct 8, 2019

View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 8, 2019

gterzian reviewed Oct 8, 2019

View reviewed changes

gterzian requested changes Oct 8, 2019

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Oct 8, 2019

gterzian mentioned this pull request Jun 20, 2020

Fix HTTP-cache refresh bug #26999

Closed

5 tasks

gterzian requested changes Jun 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HTTP cache revalidation bugs #24388

Fix HTTP cache revalidation bugs #24388

jdm commented Oct 7, 2019 •

edited by SimonSapin

highfive commented Oct 7, 2019

highfive commented Oct 7, 2019

jdm commented Oct 7, 2019

gterzian left a comment

gterzian Oct 8, 2019

gterzian Oct 8, 2019 •

edited

gterzian commented Oct 8, 2019

bors-servo commented Oct 8, 2019

bors-servo commented Oct 8, 2019

CYBAI commented Oct 8, 2019

gterzian Oct 8, 2019 •

edited

gterzian Oct 8, 2019 •

edited

gterzian Oct 8, 2019 •

edited

gterzian Oct 9, 2019

bors-servo commented Oct 8, 2019

gterzian left a comment

gterzian Jun 25, 2020

gterzian Jun 25, 2020 •

edited

Fix HTTP cache revalidation bugs #24388

Are you sure you want to change the base?

Fix HTTP cache revalidation bugs #24388

Conversation

jdm commented Oct 7, 2019 • edited by SimonSapin

highfive commented Oct 7, 2019

highfive commented Oct 7, 2019

jdm commented Oct 7, 2019

gterzian left a comment

Choose a reason for hiding this comment

gterzian Oct 8, 2019

Choose a reason for hiding this comment

gterzian Oct 8, 2019 • edited

Choose a reason for hiding this comment

gterzian commented Oct 8, 2019

bors-servo commented Oct 8, 2019

bors-servo commented Oct 8, 2019

CYBAI commented Oct 8, 2019

gterzian Oct 8, 2019 • edited

Choose a reason for hiding this comment

gterzian Oct 8, 2019 • edited

Choose a reason for hiding this comment

gterzian Oct 8, 2019 • edited

Choose a reason for hiding this comment

gterzian Oct 9, 2019

Choose a reason for hiding this comment

bors-servo commented Oct 8, 2019

gterzian left a comment

Choose a reason for hiding this comment

gterzian Jun 25, 2020

Choose a reason for hiding this comment

gterzian Jun 25, 2020 • edited

Choose a reason for hiding this comment

jdm commented Oct 7, 2019 •

edited by SimonSapin

gterzian Oct 8, 2019 •

edited

gterzian Oct 8, 2019 •

edited

gterzian Oct 8, 2019 •

edited

gterzian Oct 8, 2019 •

edited

gterzian Jun 25, 2020 •

edited