Auto merge of #18676 - gterzian:continue_http_cache_work, r=jdm

Continue http cache work  Work in progress, and not quite worth a review yet. (Continuation of #4117) TODO - [ ] cache metadata (find some subset of`net_traits::Metadata` that can be shared across threads, it seems the problem is mainly stuff inside `hyper::header` in the `headers` field) - [ ] determine which other fields of a `Response` need to be cached, so a full and valid one can be returned upon a cache hit. - [ ] determine how to best share the cache across fetch threads (inside HttpState like I tried now?) - [ ] Spend more time reading the spec and make sure the cache follows it where it matters. - [ ] Make the current wpt tests pass. - [ ] More... ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #12972 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18676)
servo · Nov 21, 2017 · e2bc0f0 · e2bc0f0
2 parents 7217232 + 6196a6e
commit e2bc0f0
Show file tree

Hide file tree

Showing 37 changed files with 1,238 additions and 631 deletions.
diff --git a/components/net/http_cache.rs b/components/net/http_cache.rs
diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs
@@ -13,6 +13,7 @@ use fetch::methods::{Data, DoneChannel, FetchContext, Target};
 use fetch::methods::{is_cors_safelisted_request_header, is_cors_safelisted_method, main_fetch};
 use flate2::read::{DeflateDecoder, GzDecoder};
 use hsts::HstsList;
+use http_cache::HttpCache;
 use hyper::Error as HttpError;
 use hyper::LanguageTag;
 use hyper::client::{Pool, Request as HyperRequest, Response as HyperResponse};
@@ -22,7 +23,7 @@ use hyper::header::{AccessControlMaxAge, AccessControlRequestHeaders};
 use hyper::header::{AccessControlRequestMethod, AcceptEncoding, AcceptLanguage};
 use hyper::header::{Authorization, Basic, CacheControl, CacheDirective};
 use hyper::header::{ContentEncoding, ContentLength, Encoding, Header, Headers};
-use hyper::header::{Host, Origin as HyperOrigin, IfMatch, IfRange};
+use hyper::header::{Host, HttpDate, Origin as HyperOrigin, IfMatch, IfRange};
 use hyper::header::{IfUnmodifiedSince, IfModifiedSince, IfNoneMatch, Location};
 use hyper::header::{Pragma, Quality, QualityItem, Referer, SetCookie};
 use hyper::header::{UserAgent, q, qitem};
@@ -45,6 +46,7 @@ use std::io::{self, Read, Write};
 use std::iter::FromIterator;
 use std::mem;
 use std::ops::Deref;
+use std::str::FromStr;
 use std::sync::RwLock;
 use std::sync::mpsc::{channel, Sender};
 use std::thread;
@@ -69,6 +71,7 @@ fn read_block<R: Read>(reader: &mut R) -> Result<Data, ()> {
 pub struct HttpState {
     pub hsts_list: RwLock<HstsList>,
     pub cookie_jar: RwLock<CookieStorage>,
+    pub http_cache: RwLock<HttpCache>,
     pub auth_cache: RwLock<AuthCache>,
     pub ssl_client: OpensslClient,
     pub connector: Pool<Connector>,
@@ -80,6 +83,7 @@ impl HttpState {
             hsts_list: RwLock::new(HstsList::new()),
             cookie_jar: RwLock::new(CookieStorage::new(150)),
             auth_cache: RwLock::new(AuthCache::new()),
+            http_cache: RwLock::new(HttpCache::new()),
             ssl_client: ssl_client.clone(),
             connector: create_http_connector(ssl_client),
         }
@@ -895,34 +899,35 @@ fn http_network_or_cache_fetch(request: &mut Request,
     let mut revalidating_flag = false;
 
     // Step 21
-    // TODO have a HTTP cache to check for a completed response
-    let complete_http_response_from_cache: Option<Response> = None;
-    if http_request.cache_mode != CacheMode::NoStore &&
-       http_request.cache_mode != CacheMode::Reload &&
-       complete_http_response_from_cache.is_some() {
-        // TODO Substep 1 and 2. Select a response from HTTP cache.
-
-        // Substep 3
-        if let Some(ref response) = response {
-            revalidating_flag = response_needs_revalidation(&response);
-        };
-
-        // Substep 4
-        if http_request.cache_mode == CacheMode::ForceCache ||
-           http_request.cache_mode == CacheMode::OnlyIfCached {
-            // TODO pull response from HTTP cache
-            // response = http_request
-        }
+    if let Ok(http_cache) = context.state.http_cache.read() {
+        if let Some(response_from_cache) = http_cache.construct_response(&http_request) {
+            let response_headers = response_from_cache.response.headers.clone();
+            // Substep 1, 2, 3, 4
+            let (cached_response, needs_revalidation) = match (http_request.cache_mode, &http_request.mode) {
+                (CacheMode::ForceCache, _) => (Some(response_from_cache.response), false),
+                (CacheMode::OnlyIfCached, &RequestMode::SameOrigin) => (Some(response_from_cache.response), false),
+                (CacheMode::OnlyIfCached, _) | (CacheMode::NoStore, _) | (CacheMode::Reload, _) => (None, false),
+                (_, _) => (Some(response_from_cache.response), response_from_cache.needs_validation)
+            };
+            if needs_revalidation {
+                revalidating_flag = true;
+                // Substep 5
+                // TODO: find out why the typed header getter return None from the headers of cached responses.
+                if let Some(date_slice) = response_headers.get_raw("Last-Modified") {
+                    let date_string = String::from_utf8_lossy(&date_slice[0]);
+                    if let Ok(http_date) = HttpDate::from_str(&date_string) {
+                        http_request.headers.set(IfModifiedSince(http_date));
+                    }
+                }
+                if let Some(entity_tag) =
+                    response_headers.get_raw("ETag") {
+                    http_request.headers.set_raw("If-None-Match", entity_tag.to_vec());
 
-        if revalidating_flag {
-            // Substep 5
-            // TODO set If-None-Match and If-Modified-Since according to cached
-            //      response headers.
-        } else {
-            // Substep 6
-            // TODO pull response from HTTP cache
-            // response = http_request
-            // response.cache_state = CacheState::Local;
+                }
+            } else {
+                // Substep 6
+                response = cached_response;
+            }
         }
     }
 
@@ -933,26 +938,37 @@ fn http_network_or_cache_fetch(request: &mut Request,
             return Response::network_error(
                 NetworkError::Internal("Couldn't find response in cache".into()))
         }
+    }
+    // More Step 22
+    if response.is_none() {
         // Substep 2
         let forward_response = http_network_fetch(http_request, credentials_flag,
                                                   done_chan, context);
         // Substep 3
         if let Some((200...399, _)) = forward_response.raw_status {
             if !http_request.method.safe() {
-                // TODO Invalidate HTTP cache response
+                if let Ok(mut http_cache) = context.state.http_cache.write() {
+                    http_cache.invalidate(&http_request, &forward_response);
+                }
             }
         }
         // Substep 4
         if revalidating_flag && forward_response.status.map_or(false, |s| s == StatusCode::NotModified) {
-            // TODO update forward_response headers with cached response headers
+            if let Ok(mut http_cache) = context.state.http_cache.write() {
+                response = http_cache.refresh(&http_request, forward_response.clone(), done_chan);
+            }
         }
 
         // Substep 5
         if response.is_none() {
+            if http_request.cache_mode != CacheMode::NoStore {
+                // 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);
+                }
+            }
             // Subsubstep 1
             response = Some(forward_response);
-            // Subsubstep 2
-            // TODO: store http_request and forward_response in cache
         }
     }
 
@@ -1170,7 +1186,9 @@ fn http_network_fetch(request: &Request,
 
     // Step 14
     if !response.is_network_error() && request.cache_mode != CacheMode::NoStore {
-        // TODO update response in the HTTP cache for request
+        if let Ok(mut http_cache) = context.state.http_cache.write() {
+            http_cache.store(&request, &response);
+        }
     }
 
     // TODO this step isn't possible yet
@@ -1368,11 +1386,6 @@ fn is_no_store_cache(headers: &Headers) -> bool {
     headers.has::<IfRange>()
 }
 
-fn response_needs_revalidation(_response: &Response) -> bool {
-    // TODO this function
-    false
-}
-
 /// <https://fetch.spec.whatwg.org/#redirect-status>
 pub fn is_redirect_status(status: StatusCode) -> bool {
     match status {

diff --git a/components/net/lib.rs b/components/net/lib.rs
@@ -48,6 +48,7 @@ mod data_loader;
 pub mod filemanager_thread;
 mod hosts;
 pub mod hsts;
+pub mod http_cache;
 pub mod http_loader;
 pub mod image_cache;
 pub mod mime_classifier;

diff --git a/components/net/resource_thread.rs b/components/net/resource_thread.rs
@@ -12,6 +12,7 @@ use fetch::cors_cache::CorsCache;
 use fetch::methods::{FetchContext, fetch};
 use filemanager_thread::{FileManager, TFDProvider};
 use hsts::HstsList;
+use http_cache::HttpCache;
 use http_loader::{HttpState, http_redirect_fetch};
 use hyper_serde::Serde;
 use ipc_channel::ipc::{self, IpcReceiver, IpcReceiverSet, IpcSender};
@@ -91,6 +92,7 @@ struct ResourceChannelManager {
 fn create_http_states(config_dir: Option<&Path>) -> (Arc<HttpState>, Arc<HttpState>) {
     let mut hsts_list = HstsList::from_servo_preload();
     let mut auth_cache = AuthCache::new();
+    let http_cache = HttpCache::new();
     let mut cookie_jar = CookieStorage::new(150);
     if let Some(config_dir) = config_dir {
         read_json_from_file(&mut auth_cache, config_dir, "auth_cache.json");
@@ -109,6 +111,7 @@ fn create_http_states(config_dir: Option<&Path>) -> (Arc<HttpState>, Arc<HttpSta
     let http_state = HttpState {
         cookie_jar: RwLock::new(cookie_jar),
         auth_cache: RwLock::new(auth_cache),
+        http_cache: RwLock::new(http_cache),
         hsts_list: RwLock::new(hsts_list),
         ssl_client: ssl_client.clone(),
         connector: create_http_connector(ssl_client),

diff --git a/components/script/dom/headers.rs b/components/script/dom/headers.rs
@@ -234,6 +234,12 @@ impl Headers {
         *self.header_list.borrow_mut() = hyper_headers;
     }
 
+    pub fn get_headers_list(&self) -> HyperHeaders {
+        let mut headers = HyperHeaders::new();
+        headers.extend(self.header_list.borrow_mut().iter());
+        headers
+    }
+
     // https://fetch.spec.whatwg.org/#concept-header-extract-mime-type
     pub fn extract_mime_type(&self) -> Vec<u8> {
         self.header_list.borrow().get_raw("content-type").map_or(vec![], |v| v[0].clone())

diff --git a/components/script/dom/request.rs b/components/script/dom/request.rs
@@ -339,6 +339,9 @@ impl Request {
             _ => {},
         }
 
+        // Copy the headers list onto the headers of net_traits::Request
+        r.request.borrow_mut().headers = r.Headers().get_headers_list();
+
         // Step 32
         let mut input_body = if let RequestInfo::Request(ref input_request) = input {
             let input_request_request = input_request.request.borrow();
@@ -459,17 +462,7 @@ fn normalize_method(m: &str) -> HttpMethod {
 
 // https://fetch.spec.whatwg.org/#concept-method
 fn is_method(m: &ByteString) -> bool {
-    match m.to_lower().as_str() {
-        Some("get") => true,
-        Some("head") => true,
-        Some("post") => true,
-        Some("put") => true,
-        Some("delete") => true,
-        Some("connect") => true,
-        Some("options") => true,
-        Some("trace") => true,
-        _ => false,
-    }
+    m.as_str().is_some()
 }
 
 // https://fetch.spec.whatwg.org/#forbidden-method

diff --git a/components/script/fetch.rs b/components/script/fetch.rs
@@ -60,6 +60,7 @@ fn request_init_from_request(request: NetTraitsRequest) -> NetTraitsRequestInit
         referrer_policy: request.referrer_policy,
         pipeline_id: request.pipeline_id,
         redirect_mode: request.redirect_mode,
+        cache_mode: request.cache_mode,
         ..NetTraitsRequestInit::default()
     }
 }

diff --git a/resources/prefs.json b/resources/prefs.json
@@ -66,6 +66,7 @@
   "layout.text-orientation.enabled": false,
   "layout.viewport.enabled": false,
   "layout.writing-mode.enabled": false,
+  "network.http-cache.disabled": false,
   "network.mime.sniff": false,
   "session-history.max-length": 20,
   "shell.builtin-key-shortcuts.enabled": true,

diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json
@@ -528584,15 +528584,15 @@
    "support"
   ],
   "fetch/http-cache/cc-request.html": [
-   "d4417b8fd444362a3f217d1c95d37811a608e1a7",
+   "2002d341679139428e164cfe916dd39b9b664a3e",
    "testharness"
   ],
   "fetch/http-cache/freshness.html": [
    "84016c3d56e01f8f6be52cf4d26a0e4e860b9147",
    "testharness"
   ],
   "fetch/http-cache/heuristic.html": [
-   "5b0d55f891cb2e235456cd65f4e9f63e07999410",
+   "63837026eb6085fc7d6220c3dcab200b4bcd1eca",
    "testharness"
   ],
   "fetch/http-cache/http-cache.js": [
@@ -528604,7 +528604,7 @@
    "testharness"
   ],
   "fetch/http-cache/partial.html": [
-   "243e57c39f9e45e3e2acf845b36f3a140e3763bc",
+   "685057fe8876321a5d42bcf1e7582e6f0b745f85",
    "testharness"
   ],
   "fetch/http-cache/resources/http-cache.py": [
@@ -528616,7 +528616,7 @@
    "testharness"
   ],
   "fetch/http-cache/vary.html": [
-   "fa9a2e0554671bf2de5826e66ac0ea73de28d530",
+   "45f337270cfa90932c7469802655e313367ac92f",
    "testharness"
   ],
   "fetch/nosniff/image.html": [

diff --git a/tests/wpt/metadata/cors/304.htm.ini b/tests/wpt/metadata/cors/304.htm.ini
diff --git a/tests/wpt/metadata/fetch/api/basic/accept-header.any.js.ini b/tests/wpt/metadata/fetch/api/basic/accept-header.any.js.ini
diff --git a/tests/wpt/metadata/fetch/api/basic/request-headers.any.js.ini b/tests/wpt/metadata/fetch/api/basic/request-headers.any.js.ini
@@ -39,12 +39,6 @@
   [Fetch with Chicken with body]
     expected: FAIL
 
-  [Fetch with TacO and mode "same-origin" needs an Origin header]
-    expected: FAIL
-
-  [Fetch with TacO and mode "cors" needs an Origin header]
-    expected: FAIL
-
 
 [request-headers.any.worker.html]
   type: testharness

diff --git a/tests/wpt/metadata/fetch/api/cors/cors-preflight-star.any.js.ini b/tests/wpt/metadata/fetch/api/cors/cors-preflight-star.any.js.ini
@@ -1,17 +1,8 @@
 [cors-preflight-star.any.html]
   type: testharness
-  [CORS that succeeds with credentials: false; method: SUPER (allowed: *); header: X-Test,1 (allowed: x-test)]
-    expected: FAIL
-
   [CORS that succeeds with credentials: false; method: OK (allowed: *); header: X-Test,1 (allowed: *)]
     expected: FAIL
 
-  [CORS that fails with credentials: true; method: GET (allowed: get); header: X-Test,1 (allowed: *)]
-    expected: FAIL
-
-  [CORS that fails with credentials: true; method: GET (allowed: *); header: X-Test,1 (allowed: *)]
-    expected: FAIL
-
   [CORS that succeeds with credentials: true; method: PUT (allowed: PUT); header:  (allowed: *)]
     expected: FAIL