From 79d27cb7ca0267092496dbbef24844ca5ab14fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fernando=20Jim=C3=A9nez=20Moreno?= Date: Thu, 15 Nov 2018 06:45:13 +0100 Subject: [PATCH] Set response status for range requests to file and blob urls --- components/net/fetch/methods.rs | 70 ++++++++++++++++--- components/net/filemanager_thread.rs | 42 +++++++---- components/net_traits/blob_url_store.rs | 4 +- tests/wpt/mozilla/meta/MANIFEST.json | 2 +- .../tests/mozilla/range_request_blob_url.html | 15 ++-- 5 files changed, 96 insertions(+), 37 deletions(-) diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index a15045bef4b..19736de1136 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -18,7 +18,7 @@ use hyper::StatusCode; use ipc_channel::ipc::IpcReceiver; use mime::{self, Mime}; use mime_guess::guess_mime_type; -use net_traits::blob_url_store::parse_blob_url; +use net_traits::blob_url_store::{parse_blob_url, BlobURLStoreError}; use net_traits::filemanager_thread::RelativePos; use net_traits::request::{CredentialsMode, Destination, Referrer, Request, RequestMode}; use net_traits::request::{Origin, ResponseTainting, Window}; @@ -500,17 +500,24 @@ pub enum RangeRequestBounds { } impl RangeRequestBounds { - pub fn get_final(&self, len: Option) -> RelativePos { + pub fn get_final(&self, len: Option) -> Result { match self { - RangeRequestBounds::Final(pos) => pos.clone(), - RangeRequestBounds::Pending(offset) => RelativePos::from_opts( + RangeRequestBounds::Final(pos) => { + if let Some(len) = len { + if pos.start <= len as i64 { + return Ok(pos.clone()); + } + } + Err(()) + }, + RangeRequestBounds::Pending(offset) => Ok(RelativePos::from_opts( if let Some(len) = len { Some((len - u64::min(len, *offset)) as i64) } else { Some(0) }, None, - ), + )), } } } @@ -539,6 +546,18 @@ fn get_range_request_bounds(range: Option) -> RangeRequestBounds { } } +fn partial_content(response: &mut Response) { + let reason = "Partial Content".to_owned(); + response.status = Some((StatusCode::PARTIAL_CONTENT, reason.clone())); + response.raw_status = Some((StatusCode::PARTIAL_CONTENT.as_u16(), reason.into())); +} + +fn range_not_satisfiable_error(response: &mut Response) { + let reason = "Range Not Satisfiable".to_owned(); + response.status = Some((StatusCode::RANGE_NOT_SATISFIABLE, reason.clone())); + response.raw_status = Some((StatusCode::RANGE_NOT_SATISFIABLE.as_u16(), reason.into())); +} + /// [Scheme fetch](https://fetch.spec.whatwg.org#scheme-fetch) fn scheme_fetch( request: &mut Request, @@ -584,16 +603,24 @@ fn scheme_fetch( } if let Ok(file_path) = url.to_file_path() { if let Ok(file) = File::open(file_path.clone()) { - let mut response = Response::new(url, ResourceFetchTiming::new(request.timing_type())); - // Get range bounds (if any) and try to seek to the requested offset. // If seeking fails, bail out with a NetworkError. let file_size = match file.metadata() { Ok(metadata) => Some(metadata.len()), Err(_) => None, }; - let range = get_range_request_bounds(request.headers.typed_get::()); - let range = range.get_final(file_size); + + let mut response = Response::new(url, ResourceFetchTiming::new(request.timing_type())); + + let range_header = request.headers.typed_get::(); + let is_range_request = range_header.is_some(); + let range = match get_range_request_bounds(range_header).get_final(file_size) { + Ok(range) => range, + Err(_) => { + range_not_satisfiable_error(&mut response); + return response; + }, + }; let mut reader = BufReader::with_capacity(FILE_CHUNK_SIZE, file); if reader.seek(SeekFrom::Start(range.start as u64)).is_err() { return Response::network_error(NetworkError::Internal( @@ -601,6 +628,12 @@ fn scheme_fetch( )); } + // Set response status to 206 if Range header is present. + // At this point we should have already validated the header. + if is_range_request { + partial_content(&mut response); + } + // Set Content-Type header. let mime = guess_mime_type(file_path); response.headers.typed_insert(ContentType::from(mime)); @@ -631,7 +664,7 @@ fn scheme_fetch( }, "blob" => { - println!("Loading blob {}", url.as_str()); + debug!("Loading blob {}", url.as_str()); // Step 2. if request.method != Method::GET { return Response::network_error(NetworkError::Internal( @@ -639,7 +672,11 @@ fn scheme_fetch( )); } - let range = get_range_request_bounds(request.headers.typed_get::()); + let range_header = request.headers.typed_get::(); + let is_range_request = range_header.is_some(); + // We will get a final version of this range once we have + // the length of the data backing the blob. + let range = get_range_request_bounds(range_header); let (id, origin) = match parse_blob_url(&url) { Ok((id, origin)) => (id, origin), @@ -651,6 +688,10 @@ fn scheme_fetch( }; let mut response = Response::new(url); + if is_range_request { + partial_content(&mut response); + } + let (done_sender, done_receiver) = channel(); *done_chan = Some((done_sender.clone(), done_receiver)); *response.body.lock().unwrap() = ResponseBody::Receiving(vec![]); @@ -665,6 +706,13 @@ fn scheme_fetch( range, ) { let _ = done_sender.send(Data::Done); + let err = match err { + BlobURLStoreError::InvalidRange => { + range_not_satisfiable_error(&mut response); + return response; + }, + _ => format!("{:?}", err), + }; return Response::network_error(NetworkError::Internal(err)); }; diff --git a/components/net/filemanager_thread.rs b/components/net/filemanager_thread.rs index f70d774e95e..3a639cbb741 100644 --- a/components/net/filemanager_thread.rs +++ b/components/net/filemanager_thread.rs @@ -114,18 +114,16 @@ impl FileManager { origin: FileOrigin, response: &mut Response, range: RangeRequestBounds, - ) -> Result<(), String> { - self.store - .fetch_blob_buf( - done_sender, - cancellation_listener, - &id, - &origin, - range, - check_url_validity, - response, - ) - .map_err(|e| format!("{:?}", e)) + ) -> Result<(), BlobURLStoreError> { + self.store.fetch_blob_buf( + done_sender, + cancellation_listener, + &id, + &origin, + range, + check_url_validity, + response, + ) } pub fn promote_memory( @@ -539,7 +537,13 @@ impl FileManagerStore { let file_impl = self.get_impl(id, origin_in, check_url_validity)?; match file_impl { FileImpl::Memory(buf) => { - let range = range.get_final(Some(buf.size)); + let range = match range.get_final(Some(buf.size)) { + Ok(range) => range, + Err(_) => { + return Err(BlobURLStoreError::InvalidRange); + }, + }; + let range = range.to_abs_range(buf.size as usize); let len = range.len() as u64; @@ -568,7 +572,13 @@ impl FileManagerStore { let file = File::open(&metadata.path) .map_err(|e| BlobURLStoreError::External(e.to_string()))?; - let range = range.get_final(Some(metadata.size)); + let range = match range.get_final(Some(metadata.size)) { + Ok(range) => range, + Err(_) => { + return Err(BlobURLStoreError::InvalidRange); + }, + }; + let mut reader = BufReader::with_capacity(FILE_CHUNK_SIZE, file); if reader.seek(SeekFrom::Start(range.start as u64)).is_err() { return Err(BlobURLStoreError::External( @@ -607,7 +617,9 @@ impl FileManagerStore { cancellation_listener, &parent_id, origin_in, - RangeRequestBounds::Final(range.get_final(None).slice_inner(&inner_rel_pos)), + RangeRequestBounds::Final( + RelativePos::full_range().slice_inner(&inner_rel_pos), + ), false, response, ); diff --git a/components/net_traits/blob_url_store.rs b/components/net_traits/blob_url_store.rs index a429c6880bc..c9bb670ee29 100644 --- a/components/net_traits/blob_url_store.rs +++ b/components/net_traits/blob_url_store.rs @@ -9,7 +9,7 @@ use url::Url; use uuid::Uuid; /// Errors returned to Blob URL Store request -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub enum BlobURLStoreError { /// Invalid File UUID InvalidFileID, @@ -17,6 +17,8 @@ pub enum BlobURLStoreError { InvalidOrigin, /// Invalid entry content InvalidEntry, + /// Invalid range + InvalidRange, /// External error, from like file system, I/O etc. External(String), } diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 730701a7e78..5233572da7d 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -27246,7 +27246,7 @@ "testharness" ], "mozilla/range_request_blob_url.html": [ - "b41f0b5382e3994db35f38b2c358a41b75551fe2", + "075397620e989dafc814c0ed2bca46bd476bccf6", "testharness" ], "mozilla/range_request_file_url.html": [ diff --git a/tests/wpt/mozilla/tests/mozilla/range_request_blob_url.html b/tests/wpt/mozilla/tests/mozilla/range_request_blob_url.html index b41f0b5382e..075397620e9 100644 --- a/tests/wpt/mozilla/tests/mozilla/range_request_blob_url.html +++ b/tests/wpt/mozilla/tests/mozilla/range_request_blob_url.html @@ -23,12 +23,10 @@ range: "bytes=0-100", status: 206, expected: "abcdefghijklmnopqrstuvwxyz" -/*}, { - XXX(ferjm): Range requests with an out of bounds start throw an exception - rather than responding with 416. +}, { range: "bytes=100-", status: 416, - expected: ""*/ + expected: "" }, { range: "bytes=-100", status: 206, @@ -43,11 +41,10 @@ } }) .then(response => { -// XXX(ferjm): Responses to range requests to blob urls have status 200 rather than 206. -// assert_equals(response.status, test.status); -// if (response.status != 206) { -// return ""; -// } + assert_equals(response.status, test.status); + if (response.status != 206) { + return ""; + } return response.text(); }) .then(response => {