servo: Merge #9298 - Implement a basic test for Fetch (from nikkisqua…

…red:test_fetch); r=KiChjang

As per jdm's suggestion that I start minimally testing the Fetch protocol to catch any errors, I wrote a very simple test that just calls Fetch and checks that the response isn't a network error. I've made changes as necessary for every failure I encountered, although this doesn't mean the implementation is faultless yet.

As always, I look forward to any feedback for improvements regarding the test itself, the changes to the fetch files I've made, and anything that I missed and should update.

Source-Repo: https://github.com/servo/servo
Source-Revision: 9c713cb4688f1a1729ada64846fac2d8426b1ef4

UltraBlame original commit: 2b7cfbc101b78151607bf66bea2632543bfed083

servo/components/net/fetch/request.rs

@@ -116,7 +116,7 @@ pub struct Request {
     pub origin: Option<Url>, 
     pub force_origin_header: bool,
     pub omit_origin_header: bool,
-    pub same_origin_data: bool,
+    pub same_origin_data: Cell<bool>,
     pub referer: Referer,
     pub authentication: bool,
     pub sync: bool,
@@ -145,7 +145,7 @@ impl Request {
             origin: None,
             force_origin_header: false,
             omit_origin_header: false,
-            same_origin_data: false,
+            same_origin_data: Cell::new(false),
             referer: Referer::Client,
             authentication: false,
             sync: false,
@@ -235,8 +235,8 @@ pub fn fetch(request: Rc<Request>, cors_flag: bool) -> Response {
 
 fn main_fetch(request: Rc<Request>, _cors_flag: bool) -> Response {
 
-    let _ = basic_fetch(request);
-    Response::network_error()
+    let response = basic_fetch(request);
+    response
 }
 
 
@@ -305,6 +305,7 @@ fn http_fetch(request: Rc<Request>,
     if !request.skip_service_worker.get() && !request.is_service_worker_global_scope {
 
 
+
         if let Some(ref res) = response {
 
 
@@ -361,7 +362,7 @@ fn http_fetch(request: Rc<Request>,
                     destination: url.clone(),
                     credentials: credentials
                 }, view.name()) && !is_simple_header(&view)
-                );
+            );
 
             if method_mismatch || header_mismatch {
                 let preflight_result = preflight_fetch(request.clone());
@@ -397,9 +398,10 @@ fn http_fetch(request: Rc<Request>,
     }
 
 
-    let actual_response = Rc::try_unwrap(actual_response.unwrap()).ok().unwrap();
-    let mut response = Rc::try_unwrap(response.unwrap()).ok().unwrap();
+    let mut response = response.unwrap();
+    let actual_response = actual_response.unwrap();
 
+
     match actual_response.status.unwrap() {
 
 
@@ -413,11 +415,14 @@ fn http_fetch(request: Rc<Request>,
 
 
             if !actual_response.headers.has::<Location>() {
-                return actual_response;
+                drop(actual_response);
+                return Rc::try_unwrap(response).ok().unwrap();
             }
 
+
             let location = match actual_response.headers.get::<Location>() {
                 Some(&Location(ref location)) => location.clone(),
+
                 _ => return Response::network_error(),
             };
 
@@ -426,7 +431,6 @@ fn http_fetch(request: Rc<Request>,
 
 
             let location_url = match location_url {
-                Ok(ref url) if url.scheme == "data" => { return Response::network_error(); }
                 Ok(url) => url,
                 _ => { return Response::network_error(); }
             };
@@ -439,11 +443,14 @@ fn http_fetch(request: Rc<Request>,
 
             request.redirect_count.set(request.redirect_count.get() + 1);
 
+
+            request.same_origin_data.set(false);
+
             match request.redirect_mode {
 
 
                 RedirectMode::Manual => {
-                    response = actual_response.to_filtered(ResponseType::Opaque);
+                    response = Rc::new(Response::to_filtered(actual_response, ResponseType::Opaque));
                 }
 
 
@@ -493,7 +500,8 @@ fn http_fetch(request: Rc<Request>,
 
 
             if cors_flag {
-                return response;
+                drop(actual_response);
+                return Rc::try_unwrap(response).ok().unwrap();
             }
 
 
@@ -526,7 +534,7 @@ fn http_fetch(request: Rc<Request>,
                               authentication_fetch_flag);
         }
 
-        _ => { }
+        _ => drop(actual_response)
     }
 
 
@@ -535,7 +543,7 @@ fn http_fetch(request: Rc<Request>,
     }
 
 
-    response
+    Rc::try_unwrap(response).ok().unwrap()
 }
 
 
@@ -744,10 +752,7 @@ fn http_network_fetch(request: Rc<Request>,
     let cancellation_listener = CancellationListener::new(None);
 
     let wrapped_response = obtain_response(&factory, &url, &request.method.borrow(),
-
-
-
-                                           &mut *request.headers.borrow_mut(),
+                                           &request.headers.borrow(),
                                            &cancellation_listener, &None, &request.method.borrow(),
                                            &None, request.redirect_count.get(), &None, "");
 

servo/components/net/fetch/response.rs

@@ -13,7 +13,7 @@ use url::Url;
 
 pub trait ResponseMethods {
     fn new() -> Response;
-    fn to_filtered(self, ResponseType) -> Response;
+    fn to_filtered(Rc<Response>, ResponseType) -> Response;
 }
 
 impl ResponseMethods for Response {
@@ -34,17 +34,23 @@ impl ResponseMethods for Response {
 
 
 
-    fn to_filtered(self, filter_type: ResponseType) -> Response {
+    fn to_filtered(old_response: Rc<Response>, filter_type: ResponseType) -> Response {
+
         assert!(filter_type != ResponseType::Error);
         assert!(filter_type != ResponseType::Default);
-        if self.is_network_error() {
-            return self;
+
+        if Response::is_network_error(&old_response) {
+            return Response::network_error();
         }
-        let old_headers = self.headers.clone();
-        let mut response = self.clone();
-        response.internal_response = Some(Rc::new(self));
+
+        let old_headers = old_response.headers.clone();
+        let mut response = (*old_response).clone();
+        response.internal_response = Some(old_response);
+
         match filter_type {
+
             ResponseType::Default | ResponseType::Error => unreachable!(),
+
             ResponseType::Basic => {
                 let headers = old_headers.iter().filter(|header| {
                     match &*header.name().to_ascii_lowercase() {
@@ -55,6 +61,7 @@ impl ResponseMethods for Response {
                 response.headers = headers;
                 response.response_type = filter_type;
             },
+
             ResponseType::CORS => {
                 let headers = old_headers.iter().filter(|header| {
                     match &*header.name().to_ascii_lowercase() {
@@ -67,13 +74,15 @@ impl ResponseMethods for Response {
                 response.headers = headers;
                 response.response_type = filter_type;
             },
+
             ResponseType::Opaque |
             ResponseType::OpaqueRedirect => {
                 response.headers = Headers::new();
                 response.status = None;
                 response.body = ResponseBody::Empty;
             }
         }
+
         response
     }
 }

servo/components/net/http_loader.rs

@@ -554,7 +554,7 @@ pub fn process_response_headers(response: &HttpResponse,
 pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
                           url: &Url,
                           method: &Method,
-                          request_headers: &mut Headers,
+                          request_headers: &Headers,
                           cancel_listener: &CancellationListener,
                           data: &Option<Vec<u8>>,
                           load_data_method: &Method,
@@ -706,7 +706,7 @@ pub fn load<A>(load_data: LoadData,
 
         modify_request_headers(&mut request_headers, &doc_url, &user_agent, &cookie_jar, &load_data);
 
-        let response = try!(obtain_response(request_factory, &url, &method, &mut request_headers,
+        let response = try!(obtain_response(request_factory, &url, &method, &request_headers,
                                             &cancel_listener, &load_data.data, &load_data.method,
                                             &load_data.pipeline_id, iters, &devtools_chan, &request_id));
 

servo/tests/unit/net/fetch.rs

@@ -0,0 +1,44 @@
+
+
+
+
+use hyper::server::{Listening, Server};
+use hyper::server::{Request as HyperRequest, Response as HyperResponse};
+use net::fetch::request::{Context, fetch, Referer, Request};
+use net_traits::response::{Response};
+use std::rc::Rc;
+use url::Url;
+
+fn make_server(message: &'static [u8]) -> (Listening, Url) {
+
+    let handler = move |_: HyperRequest, response: HyperResponse| {
+        response.send(message).unwrap();
+    };
+
+
+    let server = Server::http("0.0.0.0:0").unwrap().handle_threads(handler, 1).unwrap();
+    let port = server.socket.port().to_string();
+    let mut url_string = "http://localhost:".to_owned();
+    url_string.push_str(&port);
+    let url = Url::parse(&url_string).unwrap();
+    (server, url)
+}
+
+
+#[test]
+fn test_fetch_response_is_not_network_error() {
+
+    static MESSAGE: &'static [u8] = b"";
+    let (mut server, url) = make_server(MESSAGE);
+
+    let mut request = Request::new(url, Context::Fetch, false);
+    request.referer = Referer::NoReferer;
+    let wrapped_request = Rc::new(request);
+
+    let fetch_response = fetch(wrapped_request, false);
+    let _ = server.close();
+
+    if Response::is_network_error(&fetch_response) {
+        panic!("fetch response shouldn't be a network error");
+    }
+}

servo/tests/unit/net/lib.rs

@@ -19,6 +19,7 @@ extern crate util;
 
 #[cfg(test)] mod cookie;
 #[cfg(test)] mod data_loader;
+#[cfg(test)] mod fetch;
 #[cfg(test)] mod mime_classifier;
 #[cfg(test)] mod resource_thread;
 #[cfg(test)] mod hsts;