From 8a4a5c0cb515242823289691a67fc553244eaa2e Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 2 Dec 2016 09:47:22 +0100 Subject: [PATCH 1/9] Avoid unlocking the response body while it is in an inconsistent state. --- components/net/http_loader.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index f42d49617c1..2c745d7e5ec 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -42,7 +42,7 @@ use std::collections::HashSet; use std::error::Error; use std::io::{self, Read, Write}; use std::iter::FromIterator; -use std::mem::swap; +use std::mem; use std::ops::Deref; use std::rc::Rc; use std::sync::{Arc, RwLock}; @@ -1101,16 +1101,14 @@ fn http_network_fetch(request: Rc, } }, Ok(Data::Done) | Err(_) => { - let mut empty_vec = Vec::new(); - let completed_body = match *res_body.lock().unwrap() { + let mut body = res_body.lock().unwrap(); + let completed_body = match *body { ResponseBody::Receiving(ref mut body) => { - // avoid cloning the body - swap(body, &mut empty_vec); - empty_vec + mem::replace(body, vec![]) }, - _ => empty_vec, + _ => vec![], }; - *res_body.lock().unwrap() = ResponseBody::Done(completed_body); + *body = ResponseBody::Done(completed_body); let _ = done_sender.send(Data::Done); break; } From 8d34ef109daf0ec77f837671708dc3e0c1944763 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sat, 3 Dec 2016 00:23:32 +0100 Subject: [PATCH 2/9] Qualify the fetch() calls in unit tests. --- tests/unit/net/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index cb8bc2edf89..b19ebb1b154 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -36,7 +36,7 @@ extern crate url; use devtools_traits::DevtoolsControlMsg; use hyper::server::{Handler, Listening, Server}; -use net::fetch::methods::{FetchContext, fetch}; +use net::fetch::methods::{self, FetchContext}; use net::filemanager_thread::FileManager; use net::test::HttpState; use net_traits::FetchTaskTarget; @@ -74,12 +74,12 @@ impl FetchTaskTarget for FetchResponseCollector { fn fetch_async(request: Request, target: Box, dc: Option>) { thread::spawn(move || { - fetch(Rc::new(request), &mut Some(target), &new_fetch_context(dc)); + methods::fetch(Rc::new(request), &mut Some(target), &new_fetch_context(dc)); }); } fn fetch_sync(request: Request, dc: Option>) -> Response { - fetch(Rc::new(request), &mut None, &new_fetch_context(dc)) + methods::fetch(Rc::new(request), &mut None, &new_fetch_context(dc)) } fn make_server(handler: H) -> (Listening, ServoUrl) { From 9d9f048b3b5f707803085d5b345372a5fa8404f4 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 2 Dec 2016 11:52:44 +0100 Subject: [PATCH 3/9] Introduce fetch_with_context() to reduce repetition. --- tests/unit/net/fetch.rs | 5 +++-- tests/unit/net/http_loader.rs | 23 +++++++++++------------ tests/unit/net/lib.rs | 6 +++++- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/tests/unit/net/fetch.rs b/tests/unit/net/fetch.rs index c5eb51b69f8..6abdd61cf0b 100644 --- a/tests/unit/net/fetch.rs +++ b/tests/unit/net/fetch.rs @@ -6,6 +6,7 @@ use {DEFAULT_USER_AGENT, FetchResponseCollector, new_fetch_context, fetch_async, use devtools_traits::DevtoolsControlMsg; use devtools_traits::HttpRequest as DevtoolsHttpRequest; use devtools_traits::HttpResponse as DevtoolsHttpResponse; +use fetch_with_context; use http_loader::{expect_devtools_http_request, expect_devtools_http_response}; use hyper::LanguageTag; use hyper::header::{Accept, AccessControlAllowCredentials, AccessControlAllowHeaders, AccessControlAllowOrigin}; @@ -21,7 +22,7 @@ use hyper::status::StatusCode; use hyper::uri::RequestUri; use msg::constellation_msg::TEST_PIPELINE_ID; use net::fetch::cors_cache::CorsCache; -use net::fetch::methods::{fetch, fetch_with_cors_cache}; +use net::fetch::methods::fetch_with_cors_cache; use net_traits::ReferrerPolicy; use net_traits::request::{Origin, RedirectMode, Referrer, Request, RequestMode}; use net_traits::response::{CacheState, Response, ResponseBody, ResponseType}; @@ -118,7 +119,7 @@ fn test_fetch_blob() { let request = Request::new(url, Some(Origin::Origin(origin.origin())), false, None); - let fetch_response = fetch(Rc::new(request), &mut None, &context); + let fetch_response = fetch_with_context(request, &context); assert!(!fetch_response.is_network_error()); diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index c2af331b348..27a46005c8e 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -8,6 +8,7 @@ use devtools_traits::{ChromeToDevtoolsControlMsg, DevtoolsControlMsg, NetworkEve use devtools_traits::HttpRequest as DevtoolsHttpRequest; use devtools_traits::HttpResponse as DevtoolsHttpResponse; use fetch_sync; +use fetch_with_context; use flate2::Compression; use flate2::write::{DeflateEncoder, GzEncoder}; use hyper::LanguageTag; @@ -24,7 +25,6 @@ use make_server; use msg::constellation_msg::TEST_PIPELINE_ID; use net::cookie::Cookie; use net::cookie_storage::CookieStorage; -use net::fetch::methods::fetch; use net::resource_thread::AuthCacheEntry; use net_traits::{CookieSource, NetworkError}; use net_traits::hosts::replace_host_table; @@ -34,7 +34,6 @@ use new_fetch_context; use servo_url::ServoUrl; use std::collections::HashMap; use std::io::{Read, Write}; -use std::rc::Rc; use std::sync::{Arc, Mutex, RwLock, mpsc}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::Receiver; @@ -496,7 +495,7 @@ fn test_load_doesnt_add_host_to_sts_list_when_url_is_http_even_if_sts_headers_ar .. RequestInit::default() }); let context = new_fetch_context(None); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -526,7 +525,7 @@ fn test_load_sets_cookies_in_the_resource_manager_when_it_get_set_cookie_header_ credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -566,7 +565,7 @@ fn test_load_sets_requests_cookies_header_for_url_by_getting_cookies_from_the_re credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -604,7 +603,7 @@ fn test_load_sends_cookie_if_nonhttp() { credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -635,7 +634,7 @@ fn test_cookie_set_with_httponly_should_not_be_available_using_getcookiesforurl( credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -670,7 +669,7 @@ fn test_when_cookie_received_marked_secure_is_ignored_for_http() { credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -1002,7 +1001,7 @@ fn test_redirect_from_x_to_y_provides_y_cookies_from_y() { credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -1087,7 +1086,7 @@ fn test_if_auth_creds_not_in_url_but_in_cache_it_sets_it() { context.state.auth_cache.write().unwrap().entries.insert(url.origin().clone().ascii_serialization(), auth_entry); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -1145,7 +1144,7 @@ fn test_content_blocked() { .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); @@ -1188,7 +1187,7 @@ fn test_cookies_blocked() { .. RequestInit::default() }); - let response = fetch(Rc::new(request), &mut None, &context); + let response = fetch_with_context(request, &context); let _ = server.close(); diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index b19ebb1b154..5c364832063 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -79,7 +79,11 @@ fn fetch_async(request: Request, target: Box, dc: Option } fn fetch_sync(request: Request, dc: Option>) -> Response { - methods::fetch(Rc::new(request), &mut None, &new_fetch_context(dc)) + fetch_with_context(request, &new_fetch_context(dc)) +} + +fn fetch_with_context(request: Request, context: &FetchContext) -> Response { + methods::fetch(Rc::new(request), &mut None, context) } fn make_server(handler: H) -> (Listening, ServoUrl) { From c03cd45258c1878e49413b5a46f7764306143bc2 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 2 Dec 2016 12:04:49 +0100 Subject: [PATCH 4/9] Replace fetch_async() by a new fetch() function. fetch() returns immediately after processing EOF, at the latest, so not spinning up a thread should not cause noticeable delays. OTOH, it might reduce the contention for cores, and reduce the overall time needed. --- tests/unit/net/fetch.rs | 23 ++++------------------- tests/unit/net/lib.rs | 14 +++++++++----- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/tests/unit/net/fetch.rs b/tests/unit/net/fetch.rs index 6abdd61cf0b..b4223b2f7f4 100644 --- a/tests/unit/net/fetch.rs +++ b/tests/unit/net/fetch.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use {DEFAULT_USER_AGENT, FetchResponseCollector, new_fetch_context, fetch_async, fetch_sync, make_server}; +use {DEFAULT_USER_AGENT, new_fetch_context, fetch, fetch_sync, make_server}; use devtools_traits::DevtoolsControlMsg; use devtools_traits::HttpRequest as DevtoolsHttpRequest; use devtools_traits::HttpResponse as DevtoolsHttpResponse; @@ -678,13 +678,8 @@ fn test_fetch_async_returns_complete_response() { let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let (tx, rx) = channel(); - let listener = Box::new(FetchResponseCollector { - sender: tx.clone() - }); + let fetch_response = fetch(request, None); - fetch_async(request, listener, None); - let fetch_response = rx.recv().unwrap(); let _ = server.close(); assert_eq!(response_is_done(&fetch_response), true); @@ -703,13 +698,8 @@ fn test_opaque_filtered_fetch_async_returns_complete_response() { let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let (tx, rx) = channel(); - let listener = Box::new(FetchResponseCollector { - sender: tx.clone() - }); + let fetch_response = fetch(request, None); - fetch_async(request, listener, None); - let fetch_response = rx.recv().unwrap(); let _ = server.close(); assert_eq!(fetch_response.response_type, ResponseType::Opaque); @@ -744,13 +734,8 @@ fn test_opaque_redirect_filtered_fetch_async_returns_complete_response() { *request.referrer.borrow_mut() = Referrer::NoReferrer; request.redirect_mode.set(RedirectMode::Manual); - let (tx, rx) = channel(); - let listener = Box::new(FetchResponseCollector { - sender: tx.clone() - }); + let fetch_response = fetch(request, None); - fetch_async(request, listener, None); - let fetch_response = rx.recv().unwrap(); let _ = server.close(); assert_eq!(fetch_response.response_type, ResponseType::OpaqueRedirect); diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index 5c364832063..17877e6d0cb 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -44,8 +44,7 @@ use net_traits::request::Request; use net_traits::response::Response; use servo_url::ServoUrl; use std::rc::Rc; -use std::sync::mpsc::Sender; -use std::thread; +use std::sync::mpsc::{Sender, channel}; const DEFAULT_USER_AGENT: &'static str = "Such Browser. Very Layout. Wow."; @@ -72,10 +71,15 @@ impl FetchTaskTarget for FetchResponseCollector { } } -fn fetch_async(request: Request, target: Box, dc: Option>) { - thread::spawn(move || { - methods::fetch(Rc::new(request), &mut Some(target), &new_fetch_context(dc)); +fn fetch(request: Request, dc: Option>) -> Response { + let (sender, receiver) = channel(); + let target = Box::new(FetchResponseCollector { + sender: sender, }); + + methods::fetch(Rc::new(request), &mut Some(target), &new_fetch_context(dc)); + + receiver.recv().unwrap() } fn fetch_sync(request: Request, dc: Option>) -> Response { From a5efc01b5f6b9654b1bac4ebf63bd2627b665f3b Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Mon, 5 Dec 2016 11:38:37 -1000 Subject: [PATCH 5/9] Use the asynchronous fetching code in unit tests. This is the only code that is used in Servo proper, so it's a more useful thing to test. --- tests/unit/net/data_loader.rs | 4 ++-- tests/unit/net/fetch.rs | 34 ++++++++++++++-------------- tests/unit/net/http_loader.rs | 42 +++++++++++++++++------------------ tests/unit/net/lib.rs | 4 ---- 4 files changed, 40 insertions(+), 44 deletions(-) diff --git a/tests/unit/net/data_loader.rs b/tests/unit/net/data_loader.rs index 93c625f30d9..8cd9ddbb953 100644 --- a/tests/unit/net/data_loader.rs +++ b/tests/unit/net/data_loader.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use fetch_sync; +use fetch; use hyper::header::ContentType; use hyper::mime::{Attr, Mime, SubLevel, TopLevel, Value}; use hyper_serde::Serde; @@ -21,7 +21,7 @@ fn assert_parse(url: &'static str, let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); - let response = fetch_sync(request, None); + let response = fetch(request, None); match data { Some(data) => { diff --git a/tests/unit/net/fetch.rs b/tests/unit/net/fetch.rs index b4223b2f7f4..ee6f425cd51 100644 --- a/tests/unit/net/fetch.rs +++ b/tests/unit/net/fetch.rs @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use {DEFAULT_USER_AGENT, new_fetch_context, fetch, fetch_sync, make_server}; +use {DEFAULT_USER_AGENT, new_fetch_context, fetch, make_server}; use devtools_traits::DevtoolsControlMsg; use devtools_traits::HttpRequest as DevtoolsHttpRequest; use devtools_traits::HttpResponse as DevtoolsHttpResponse; @@ -51,7 +51,7 @@ fn test_fetch_response_is_not_network_error() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); if fetch_response.is_network_error() { @@ -70,7 +70,7 @@ fn test_fetch_response_body_matches_const_message() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(!fetch_response.is_network_error()); @@ -90,7 +90,7 @@ fn test_fetch_aboutblank() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); assert!(!fetch_response.is_network_error()); assert!(*fetch_response.body.lock().unwrap() == ResponseBody::Done(vec![])); } @@ -144,7 +144,7 @@ fn test_fetch_file() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); assert!(!fetch_response.is_network_error()); assert_eq!(fetch_response.headers.len(), 1); let content_type: &ContentType = fetch_response.headers.get().unwrap(); @@ -169,7 +169,7 @@ fn test_fetch_ftp() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); assert!(fetch_response.is_network_error()); } @@ -179,7 +179,7 @@ fn test_fetch_bogus_scheme() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); assert!(fetch_response.is_network_error()); } @@ -210,7 +210,7 @@ fn test_cors_preflight_fetch() { *request.referrer_policy.get_mut() = Some(ReferrerPolicy::Origin); request.use_cors_preflight = true; request.mode = RequestMode::CorsMode; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(!fetch_response.is_network_error()); @@ -299,7 +299,7 @@ fn test_cors_preflight_fetch_network_error() { *request.referrer.borrow_mut() = Referrer::NoReferrer; request.use_cors_preflight = true; request.mode = RequestMode::CorsMode; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(fetch_response.is_network_error()); @@ -320,7 +320,7 @@ fn test_fetch_response_is_basic_filtered() { let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(!fetch_response.is_network_error()); @@ -366,7 +366,7 @@ fn test_fetch_response_is_cors_filtered() { let mut request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; request.mode = RequestMode::CorsMode; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(!fetch_response.is_network_error()); @@ -397,7 +397,7 @@ fn test_fetch_response_is_opaque_filtered() { let origin = Origin::Origin(UrlOrigin::new_opaque()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(!fetch_response.is_network_error()); @@ -445,7 +445,7 @@ fn test_fetch_response_is_opaque_redirect_filtered() { let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; request.redirect_mode.set(RedirectMode::Manual); - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); assert!(!fetch_response.is_network_error()); @@ -482,7 +482,7 @@ fn test_fetch_with_local_urls_only() { // Set the flag. request.local_urls_only = true; - fetch_sync(request, None) + fetch(request, None) }; let local_url = ServoUrl::parse("about:blank").unwrap(); @@ -519,7 +519,7 @@ fn setup_server_and_fetch(message: &'static [u8], redirect_cap: u32) -> Response let origin = Origin::Origin(url.origin()); let request = Request::new(url, Some(origin), false, None); *request.referrer.borrow_mut() = Referrer::NoReferrer; - let fetch_response = fetch_sync(request, None); + let fetch_response = fetch(request, None); let _ = server.close(); fetch_response } @@ -604,7 +604,7 @@ fn test_fetch_redirect_updates_method_runner(tx: Sender, status_code: Stat *request.referrer.borrow_mut() = Referrer::NoReferrer; *request.method.borrow_mut() = method; - let _ = fetch_sync(request, None); + let _ = fetch(request, None); let _ = server.close(); } @@ -757,7 +757,7 @@ fn test_fetch_with_devtools() { let (devtools_chan, devtools_port) = channel::(); - let _ = fetch_sync(request, Some(devtools_chan)); + let _ = fetch(request, Some(devtools_chan)); let _ = server.close(); // notification received from devtools diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 27a46005c8e..c2b720517f7 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -7,7 +7,7 @@ use cookie_rs::Cookie as CookiePair; use devtools_traits::{ChromeToDevtoolsControlMsg, DevtoolsControlMsg, NetworkEvent}; use devtools_traits::HttpRequest as DevtoolsHttpRequest; use devtools_traits::HttpResponse as DevtoolsHttpResponse; -use fetch_sync; +use fetch; use fetch_with_context; use flate2::Compression; use flate2::write::{DeflateEncoder, GzEncoder}; @@ -142,7 +142,7 @@ fn test_check_default_headers_loaded_in_every_request() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); assert!(response.status.unwrap().is_success()); // Testing for method.POST @@ -156,7 +156,7 @@ fn test_check_default_headers_loaded_in_every_request() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); assert!(response.status.unwrap().is_success()); let _ = server.close(); @@ -178,7 +178,7 @@ fn test_load_when_request_is_not_get_or_head_and_there_is_no_body_content_length pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); assert!(response.status.unwrap().is_success()); let _ = server.close(); @@ -205,7 +205,7 @@ fn test_request_and_response_data_with_network_messages() { .. RequestInit::default() }); let (devtools_chan, devtools_port) = mpsc::channel(); - let response = fetch_sync(request, Some(devtools_chan)); + let response = fetch(request, Some(devtools_chan)); assert!(response.status.unwrap().is_success()); let _ = server.close(); @@ -292,7 +292,7 @@ fn test_request_and_response_message_from_devtool_without_pipeline_id() { .. RequestInit::default() }); let (devtools_chan, devtools_port) = mpsc::channel(); - let response = fetch_sync(request, Some(devtools_chan)); + let response = fetch(request, Some(devtools_chan)); assert!(response.status.unwrap().is_success()); let _ = server.close(); @@ -327,7 +327,7 @@ fn test_redirected_request_to_devtools() { .. RequestInit::default() }); let (devtools_chan, devtools_port) = mpsc::channel(); - let response = fetch_sync(request, Some(devtools_chan)); + let response = fetch(request, Some(devtools_chan)); let _ = pre_server.close(); let _ = post_server.close(); @@ -374,7 +374,7 @@ fn test_load_when_redirecting_from_a_post_should_rewrite_next_request_as_get() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = pre_server.close(); let _ = post_server.close(); @@ -402,7 +402,7 @@ fn test_load_should_decode_the_response_as_deflate_when_response_headers_have_co pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -431,7 +431,7 @@ fn test_load_should_decode_the_response_as_gzip_when_response_headers_have_conte pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -469,7 +469,7 @@ fn test_load_doesnt_send_request_body_on_any_redirect() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = pre_server.close(); let _ = post_server.close(); @@ -697,7 +697,7 @@ fn test_load_sets_content_length_to_length_of_request_body() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -725,7 +725,7 @@ fn test_load_uses_explicit_accept_from_headers_in_load_data() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -752,7 +752,7 @@ fn test_load_sets_default_accept_to_html_xhtml_xml_and_then_anything_else() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -780,7 +780,7 @@ fn test_load_uses_explicit_accept_encoding_from_load_data_headers() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -807,7 +807,7 @@ fn test_load_sets_default_accept_encoding_to_gzip_and_deflate() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -843,7 +843,7 @@ fn test_load_errors_when_there_a_redirect_loop() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server_a.close(); let _ = server_b.close(); @@ -886,7 +886,7 @@ fn test_load_succeeds_with_a_redirect_loop() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server_a.close(); let _ = server_b.close(); @@ -923,7 +923,7 @@ fn test_load_follows_a_redirect() { pipeline_id: Some(TEST_PIPELINE_ID), .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = pre_server.close(); let _ = post_server.close(); @@ -1045,7 +1045,7 @@ fn test_redirect_from_x_to_x_provides_x_with_cookie_from_first_response() { credentials_mode: CredentialsMode::Include, .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); @@ -1112,7 +1112,7 @@ fn test_auth_ui_needs_www_auth() { .. RequestInit::default() }); - let response = fetch_sync(request, None); + let response = fetch(request, None); let _ = server.close(); diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index 17877e6d0cb..d01bffbcb14 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -82,10 +82,6 @@ fn fetch(request: Request, dc: Option>) -> Response { receiver.recv().unwrap() } -fn fetch_sync(request: Request, dc: Option>) -> Response { - fetch_with_context(request, &new_fetch_context(dc)) -} - fn fetch_with_context(request: Request, context: &FetchContext) -> Response { methods::fetch(Rc::new(request), &mut None, context) } From 306905a6319a9ff956512cc254932573ff654214 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Mon, 5 Dec 2016 11:41:07 -1000 Subject: [PATCH 6/9] Use the asynchronous fetching code for fetch_with_context(). --- tests/unit/net/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index d01bffbcb14..1f35c2fda3a 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -72,20 +72,20 @@ impl FetchTaskTarget for FetchResponseCollector { } fn fetch(request: Request, dc: Option>) -> Response { + fetch_with_context(request, &new_fetch_context(dc)) +} + +fn fetch_with_context(request: Request, context: &FetchContext) -> Response { let (sender, receiver) = channel(); let target = Box::new(FetchResponseCollector { sender: sender, }); - methods::fetch(Rc::new(request), &mut Some(target), &new_fetch_context(dc)); + methods::fetch(Rc::new(request), &mut Some(target), context); receiver.recv().unwrap() } -fn fetch_with_context(request: Request, context: &FetchContext) -> Response { - methods::fetch(Rc::new(request), &mut None, context) -} - fn make_server(handler: H) -> (Listening, ServoUrl) { // this is a Listening server because of handle_threads() let server = Server::http("0.0.0.0:0").unwrap().handle_threads(handler, 1).unwrap(); From 6f2606cdee153674dd1dda48405d2c2d7578b228 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 2 Dec 2016 12:16:27 +0100 Subject: [PATCH 7/9] Introduce fetch_with_cors_cache() API for tests. --- tests/unit/net/fetch.rs | 8 +++----- tests/unit/net/lib.rs | 12 ++++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/unit/net/fetch.rs b/tests/unit/net/fetch.rs index ee6f425cd51..b2047056253 100644 --- a/tests/unit/net/fetch.rs +++ b/tests/unit/net/fetch.rs @@ -7,6 +7,7 @@ use devtools_traits::DevtoolsControlMsg; use devtools_traits::HttpRequest as DevtoolsHttpRequest; use devtools_traits::HttpResponse as DevtoolsHttpResponse; use fetch_with_context; +use fetch_with_cors_cache; use http_loader::{expect_devtools_http_request, expect_devtools_http_response}; use hyper::LanguageTag; use hyper::header::{Accept, AccessControlAllowCredentials, AccessControlAllowHeaders, AccessControlAllowOrigin}; @@ -22,7 +23,6 @@ use hyper::status::StatusCode; use hyper::uri::RequestUri; use msg::constellation_msg::TEST_PIPELINE_ID; use net::fetch::cors_cache::CorsCache; -use net::fetch::methods::fetch_with_cors_cache; use net_traits::ReferrerPolicy; use net_traits::request::{Origin, RedirectMode, Referrer, Request, RequestMode}; use net_traits::response::{CacheState, Response, ResponseBody, ResponseType}; @@ -250,10 +250,8 @@ fn test_cors_preflight_cache_fetch() { let wrapped_request0 = Rc::new(request.clone()); let wrapped_request1 = Rc::new(request); - let fetch_response0 = fetch_with_cors_cache(wrapped_request0.clone(), &mut cache, - &mut None, &new_fetch_context(None)); - let fetch_response1 = fetch_with_cors_cache(wrapped_request1.clone(), &mut cache, - &mut None, &new_fetch_context(None)); + let fetch_response0 = fetch_with_cors_cache(wrapped_request0.clone(), &mut cache); + let fetch_response1 = fetch_with_cors_cache(wrapped_request1.clone(), &mut cache); let _ = server.close(); assert!(!fetch_response0.is_network_error() && !fetch_response1.is_network_error()); diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index 1f35c2fda3a..c698145a074 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -36,6 +36,7 @@ extern crate url; use devtools_traits::DevtoolsControlMsg; use hyper::server::{Handler, Listening, Server}; +use net::fetch::cors_cache::CorsCache; use net::fetch::methods::{self, FetchContext}; use net::filemanager_thread::FileManager; use net::test::HttpState; @@ -86,6 +87,17 @@ fn fetch_with_context(request: Request, context: &FetchContext) -> Response { receiver.recv().unwrap() } +fn fetch_with_cors_cache(request: Rc, cache: &mut CorsCache) -> Response { + let (sender, receiver) = channel(); + let target = Box::new(FetchResponseCollector { + sender: sender, + }); + + methods::fetch_with_cors_cache(request, cache, &mut Some(target), &new_fetch_context(None)); + + receiver.recv().unwrap() +} + fn make_server(handler: H) -> (Listening, ServoUrl) { // this is a Listening server because of handle_threads() let server = Server::http("0.0.0.0:0").unwrap().handle_threads(handler, 1).unwrap(); From 217f44b67af541635822dfeb41bf1345c029381d Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 2 Dec 2016 15:49:41 +0100 Subject: [PATCH 8/9] Make the fetch target non-optional. --- components/net/fetch/methods.rs | 58 +++++++++++-------------------- components/net/http_loader.rs | 4 +-- components/net/resource_thread.rs | 7 ++-- tests/unit/net/lib.rs | 12 +++---- 4 files changed, 32 insertions(+), 49 deletions(-) diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index 052e5e31ced..1a2c4ae0f4b 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -25,7 +25,7 @@ use std::mem; use std::rc::Rc; use std::sync::mpsc::{Sender, Receiver}; -pub type Target = Option>; +pub type Target<'a> = &'a mut (FetchTaskTarget + Send); pub enum Data { Payload(Vec), @@ -43,7 +43,7 @@ pub type DoneChannel = Option<(Sender, Receiver)>; /// [Fetch](https://fetch.spec.whatwg.org#concept-fetch) pub fn fetch(request: Rc, - target: &mut Target, + target: Target, context: &FetchContext) -> Response { fetch_with_cors_cache(request, &mut CorsCache::new(), target, context) @@ -51,7 +51,7 @@ pub fn fetch(request: Rc, pub fn fetch_with_cors_cache(request: Rc, cache: &mut CorsCache, - target: &mut Target, + target: Target, context: &FetchContext) -> Response { // Step 1 @@ -120,7 +120,7 @@ pub fn main_fetch(request: Rc, cache: &mut CorsCache, cors_flag: bool, recursive_flag: bool, - target: &mut Target, + target: Target, done_chan: &mut DoneChannel, context: &FetchContext) -> Response { @@ -297,20 +297,16 @@ pub fn main_fetch(request: Rc, // Step 19 if request.synchronous { - if let Some(ref mut target) = *target { - // process_response is not supposed to be used - // by sync fetch, but we overload it here for simplicity - target.process_response(&response); - } + // process_response is not supposed to be used + // by sync fetch, but we overload it here for simplicity + target.process_response(&response); if let Some(ref ch) = *done_chan { loop { match ch.1.recv() .expect("fetch worker should always send Done before terminating") { Data::Payload(vec) => { - if let Some(ref mut target) = *target { - target.process_response_chunk(vec); - } + target.process_response_chunk(vec); } Data::Done => break, } @@ -321,37 +317,29 @@ pub fn main_fetch(request: Rc, // in case there was no channel to wait for, the body was // obtained synchronously via basic_fetch for data/file/about/etc // We should still send the body across as a chunk - if let Some(ref mut target) = *target { - target.process_response_chunk(vec.clone()); - } + target.process_response_chunk(vec.clone()); } else { assert!(*body == ResponseBody::Empty) } } // overloaded similarly to process_response - if let Some(ref mut target) = *target { - target.process_response_eof(&response); - } + target.process_response_eof(&response); return response; } // Step 20 if request.body.borrow().is_some() && matches!(request.current_url().scheme(), "http" | "https") { - if let Some(ref mut target) = *target { - // XXXManishearth: We actually should be calling process_request - // in http_network_fetch. However, we can't yet follow the request - // upload progress, so I'm keeping it here for now and pretending - // the body got sent in one chunk - target.process_request_body(&request); - target.process_request_eof(&request); - } + // XXXManishearth: We actually should be calling process_request + // in http_network_fetch. However, we can't yet follow the request + // upload progress, so I'm keeping it here for now and pretending + // the body got sent in one chunk + target.process_request_body(&request); + target.process_request_eof(&request); } // Step 21 - if let Some(ref mut target) = *target { - target.process_response(&response); - } + target.process_response(&response); // Step 22 if let Some(ref ch) = *done_chan { @@ -359,14 +347,12 @@ pub fn main_fetch(request: Rc, match ch.1.recv() .expect("fetch worker should always send Done before terminating") { Data::Payload(vec) => { - if let Some(ref mut target) = *target { - target.process_response_chunk(vec); - } + target.process_response_chunk(vec); } Data::Done => break, } } - } else if let Some(ref mut target) = *target { + } else { let body = response.body.lock().unwrap(); if let ResponseBody::Done(ref vec) = *body { // in case there was no channel to wait for, the body was @@ -379,9 +365,7 @@ pub fn main_fetch(request: Rc, } // Step 24 - if let Some(ref mut target) = *target { - target.process_response_eof(&response); - } + target.process_response_eof(&response); // TODO remove this line when only asynchronous fetches are used return response; @@ -390,7 +374,7 @@ pub fn main_fetch(request: Rc, /// [Basic fetch](https://fetch.spec.whatwg.org#basic-fetch) fn basic_fetch(request: Rc, cache: &mut CorsCache, - target: &mut Target, + target: Target, done_chan: &mut DoneChannel, context: &FetchContext) -> Response { diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 2c745d7e5ec..7904e9f995f 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -533,7 +533,7 @@ pub fn http_fetch(request: Rc, cors_flag: bool, cors_preflight_flag: bool, authentication_fetch_flag: bool, - target: &mut Target, + target: Target, done_chan: &mut DoneChannel, context: &FetchContext) -> Response { @@ -707,7 +707,7 @@ fn http_redirect_fetch(request: Rc, cache: &mut CorsCache, response: Response, cors_flag: bool, - target: &mut Target, + target: Target, done_chan: &mut DoneChannel, context: &FetchContext) -> Response { diff --git a/components/net/resource_thread.rs b/components/net/resource_thread.rs index 7055f928ad7..6eeb3aa6122 100644 --- a/components/net/resource_thread.rs +++ b/components/net/resource_thread.rs @@ -18,7 +18,7 @@ use hyper::header::{Header, SetCookie}; use hyper_serde::Serde; use ipc_channel::ipc::{self, IpcReceiver, IpcReceiverSet, IpcSender}; use net_traits::{CookieSource, CoreResourceThread}; -use net_traits::{CoreResourceMsg, FetchResponseMsg, FetchTaskTarget}; +use net_traits::{CoreResourceMsg, FetchResponseMsg}; use net_traits::{CustomResponseMediator, ResourceId}; use net_traits::{ResourceThreads, WebSocketCommunicate, WebSocketConnectData}; use net_traits::request::{Request, RequestInit}; @@ -332,7 +332,7 @@ impl CoreResourceManager { fn fetch(&self, init: RequestInit, - sender: IpcSender, + mut sender: IpcSender, group: &ResourceGroup) { let http_state = HttpState { hsts_list: group.hsts_list.clone(), @@ -349,14 +349,13 @@ impl CoreResourceManager { // todo load context / mimesniff in fetch // todo referrer policy? // todo service worker stuff - let mut target = Some(Box::new(sender) as Box); let context = FetchContext { state: http_state, user_agent: ua, devtools_chan: dc, filemanager: filemanager, }; - fetch(Rc::new(request), &mut target, &context); + fetch(Rc::new(request), &mut sender, &context); }).expect("Thread spawning failed"); } diff --git a/tests/unit/net/lib.rs b/tests/unit/net/lib.rs index c698145a074..6c2f8c7036f 100644 --- a/tests/unit/net/lib.rs +++ b/tests/unit/net/lib.rs @@ -78,22 +78,22 @@ fn fetch(request: Request, dc: Option>) -> Response { fn fetch_with_context(request: Request, context: &FetchContext) -> Response { let (sender, receiver) = channel(); - let target = Box::new(FetchResponseCollector { + let mut target = FetchResponseCollector { sender: sender, - }); + }; - methods::fetch(Rc::new(request), &mut Some(target), context); + methods::fetch(Rc::new(request), &mut target, context); receiver.recv().unwrap() } fn fetch_with_cors_cache(request: Rc, cache: &mut CorsCache) -> Response { let (sender, receiver) = channel(); - let target = Box::new(FetchResponseCollector { + let mut target = FetchResponseCollector { sender: sender, - }); + }; - methods::fetch_with_cors_cache(request, cache, &mut Some(target), &new_fetch_context(None)); + methods::fetch_with_cors_cache(request, cache, &mut target, &new_fetch_context(None)); receiver.recv().unwrap() } From 1e0ab08c421b9cccbf07244ad5b37e5b95a60ead Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Mon, 5 Dec 2016 14:12:36 -1000 Subject: [PATCH 9/9] Stop returning the response from fetch(). --- components/net/fetch/methods.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index 1a2c4ae0f4b..9eedd748c2d 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -44,16 +44,14 @@ pub type DoneChannel = Option<(Sender, Receiver)>; /// [Fetch](https://fetch.spec.whatwg.org#concept-fetch) pub fn fetch(request: Rc, target: Target, - context: &FetchContext) - -> Response { - fetch_with_cors_cache(request, &mut CorsCache::new(), target, context) + context: &FetchContext) { + fetch_with_cors_cache(request, &mut CorsCache::new(), target, context); } pub fn fetch_with_cors_cache(request: Rc, cache: &mut CorsCache, target: Target, - context: &FetchContext) - -> Response { + context: &FetchContext) { // Step 1 if request.window.get() == Window::Client { // TODO: Set window to request's client object if client is a Window object @@ -112,7 +110,7 @@ pub fn fetch_with_cors_cache(request: Rc, } // Step 7 - main_fetch(request, cache, false, false, target, &mut None, &context) + main_fetch(request, cache, false, false, target, &mut None, &context); } /// [Main fetch](https://fetch.spec.whatwg.org/#concept-main-fetch)