Auto merge of #24388 - jdm:cache-fun, r=<try>

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24388)
<!-- Reviewable:end -->

components/net/http_cache.rs

@@ -223,13 +223,15 @@ fn get_response_expiry(response: &Response) -> Duration {
             // If the response has a Last-Modified header field,
             // caches are encouraged to use a heuristic expiration value
             // that is no more than some fraction of the interval since that time.
-            response.headers.typed_get::<LastModified>() {
+            response.headers.typed_get::<LastModified>()
+        {
             let current = time::now().to_timespec();
             let last_modified: SystemTime = last_modified.into();
             let last_modified = last_modified.duration_since(SystemTime::UNIX_EPOCH).unwrap();
             let last_modified = Timespec::new(last_modified.as_secs() as i64, 0);
             // A typical setting of this fraction might be 10%.
             let raw_heuristic_calc = (current - last_modified) / 10;
+            trace!("calculated {:?} vs. {:?} ({:?})", current, last_modified, raw_heuristic_calc);
             let result = if raw_heuristic_calc < max_heuristic {
                 raw_heuristic_calc
             } else {
@@ -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;
+    trace!(
+        "time_since_validated: {:?}, adjusted_expires: {:?}",
+        time_since_validated,
+        adjusted_expires
+    );
     CachedResponse {
         response: response,
         needs_validation: has_expired,
@@ -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!(
+                "there are {} cached responses for {:?}",
+                cached_resources.len(),
+                request.url()
+            );
             for cached_resource in cached_resources.iter_mut() {
                 // done_chan will have been set to Some(..) by http_network_fetch.
                 // If the body is not receiving data, set the done_chan back to None.
@@ -756,10 +767,12 @@ impl HttpCache {
                 constructed_response.referrer_policy = request.referrer_policy.clone();
                 constructed_response.raw_status = cached_resource.data.raw_status.clone();
                 constructed_response.url_list = cached_resource.data.url_list.clone();
+                {
+                    let mut stored_headers = cached_resource.data.metadata.headers.lock().unwrap();
+                    stored_headers.extend(response.headers);
+                    constructed_response.headers = stored_headers.clone();
+                }
                 cached_resource.data.expires = get_response_expiry(&constructed_response);
-                let mut stored_headers = cached_resource.data.metadata.headers.lock().unwrap();
-                stored_headers.extend(response.headers);
-                constructed_response.headers = stored_headers.clone();
                 return Some(constructed_response);
             }
         }
@@ -832,6 +845,7 @@ impl HttpCache {
             return;
         }
         let expiry = get_response_expiry(&response);
+        debug!("new cached response has expiry of {:?}", expiry);
         let cacheable_metadata = CachedMetadata {
             headers: Arc::new(Mutex::new(response.headers.clone())),
             data: Measurable(MeasurableCachedMetadata {
@@ -857,7 +871,32 @@ impl HttpCache {
                 last_validated: time::now(),
             }),
         };
+        debug!("storing new cached response for {:?}", request.url());
         let entry = self.entries.entry(entry_key).or_insert(vec![]);
+
+        if response
+            .status
+            .as_ref()
+            .map_or(false, |s| s.0 == StatusCode::OK)
+        {
+            // Ensure that any existing complete response is overwritten by the new
+            // complete response.
+            let existing_complete_response = entry.iter().position(|response| {
+                response
+                    .data
+                    .status
+                    .as_ref()
+                    .map_or(false, |s| s.0 == StatusCode::OK)
+            });
+            if let Some(idx) = existing_complete_response {
+                debug!(
+                    "Removing existing cached 200 OK response for {:?}",
+                    request.url()
+                );
+                entry.remove(idx);
+            }
+        }
+
         entry.push(entry_resource);
         // TODO: Complete incomplete responses, including 206 response, when stored here.
         // See A cache MAY complete a stored incomplete response by making a subsequent range request

components/net/http_loader.rs

@@ -1040,6 +1040,7 @@ fn http_network_or_cache_fetch(
                     ),
                 };
             if needs_revalidation {
+                debug!("Need to revalidate {:?}", http_request.url());
                 revalidating_flag = true;
                 // Substep 5
                 if let Some(http_date) = response_headers.typed_get::<LastModified>() {
@@ -1054,6 +1055,7 @@ fn http_network_or_cache_fetch(
                         .insert(header::IF_NONE_MATCH, entity_tag.clone());
                 }
             } else {
+                debug!("Using cached response for {:?}", http_request.url());
                 // Substep 6
                 response = cached_response;
             }
@@ -1108,6 +1110,10 @@ fn http_network_or_cache_fetch(
         // Substep 3
         if let Some((200..=399, _)) = forward_response.raw_status {
             if !http_request.method.is_safe() {
+                debug!(
+                    "Unsafe method for response; invalidating cached response for {:?}",
+                    http_request.url(),
+                );
                 if let Ok(mut http_cache) = context.state.http_cache.write() {
                     http_cache.invalidate(&http_request, &forward_response);
                 }
@@ -1120,6 +1126,7 @@ fn http_network_or_cache_fetch(
                 .as_ref()
                 .map_or(false, |s| s.0 == StatusCode::NOT_MODIFIED)
         {
+            debug!("Refreshing cached response for {:?}", http_request.url());
             if let Ok(mut http_cache) = context.state.http_cache.write() {
                 response = http_cache.refresh(&http_request, forward_response.clone(), done_chan);
                 wait_for_cached_response(done_chan, &mut response);
@@ -1129,10 +1136,16 @@ fn http_network_or_cache_fetch(
         // Substep 5
         if response.is_none() {
             if http_request.cache_mode != CacheMode::NoStore {
+                debug!("Storing new cached response for {:?}", http_request.url());
                 // Subsubstep 2, doing it first to avoid a clone of forward_response.
                 if let Ok(mut http_cache) = context.state.http_cache.write() {
                     http_cache.store(&http_request, &forward_response);
                 }
+            } else {
+                debug!(
+                    "Can't store new response for {:?} in cached",
+                    http_request.url()
+                );
             }
             // Subsubstep 1
             response = Some(forward_response);