Set response status for range requests to file and blob urls

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<u64>) -> RelativePos {
+    pub fn get_final(&self, len: Option<u64>) -> Result<RelativePos, ()> {
         match self {
-            RangeRequestBounds::Final(pos) => pos.clone(),
+            RangeRequestBounds::Final(pos) => {
-            RangeRequestBounds::Pending(offset) => RelativePos::from_opts(
+                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<Range>) -> 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::<Range>());
+
-                    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::<Range>();
+                    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::<Range>());
+            let range_header = request.headers.typed_get::<Range>();
+            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));
             };
 

components/net/filemanager_thread.rs

@@ -114,18 +114,16 @@ impl FileManager {
         origin: FileOrigin,
         response: &mut Response,
         range: RangeRequestBounds,
-    ) -> Result<(), String> {
+    ) -> Result<(), BlobURLStoreError> {
-        self.store
+        self.store.fetch_blob_buf(
-            .fetch_blob_buf(
+            done_sender,
-                done_sender,
+            cancellation_listener,
-                cancellation_listener,
+            &id,
-                &id,
+            &origin,
-                &origin,
+            range,
-                range,
+            check_url_validity,
-                check_url_validity,
+            response,
-                response,
+        )
-            )
-            .map_err(|e| format!("{:?}", e))
     }
 
     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,
                 );

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),
 }

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": [

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);
-//      assert_equals(response.status, test.status);
+      if (response.status != 206) {
-//      if (response.status != 206) {
+        return "";
-//        return "";
+      }
-//      }
       return response.text();
     })
     .then(response => {