Auto merge of #25740 - gterzian:per_fetch_filemanager_handle, r=Manishearth

Per fetch file token for blob url

<!-- Please describe your changes on the following line: -->

Depends on https://github.com/servo/servo/pull/25724

First step of https://github.com/servo/servo/issues/25226

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/net/fetch/methods.rs

@@ -18,7 +18,7 @@ use hyper::StatusCode;
 use ipc_channel::ipc::IpcReceiver;
 use mime::{self, Mime};
 use net_traits::blob_url_store::{parse_blob_url, BlobURLStoreError};
-use net_traits::filemanager_thread::RelativePos;
+use net_traits::filemanager_thread::{FileTokenCheck, RelativePos};
 use net_traits::request::{
     is_cors_safelisted_method, is_cors_safelisted_request_header, Origin, ResponseTainting, Window,
 };
@@ -56,6 +56,7 @@ pub struct FetchContext {
     pub user_agent: Cow<'static, str>,
     pub devtools_chan: Option<Sender<DevtoolsControlMsg>>,
     pub filemanager: FileManager,
+    pub file_token: FileTokenCheck,
     pub cancellation_listener: Arc<Mutex<CancellationListener>>,
     pub timing: ServoArc<Mutex<ResourceFetchTiming>>,
 }
@@ -756,12 +757,12 @@ fn scheme_fetch(
             let (done_sender, done_receiver) = unbounded();
             *done_chan = Some((done_sender.clone(), done_receiver));
             *response.body.lock().unwrap() = ResponseBody::Receiving(vec![]);
-            let check_url_validity = true;
+
             if let Err(err) = context.filemanager.fetch_file(
                 &done_sender,
                 context.cancellation_listener.clone(),
                 id,
-                check_url_validity,
+                &context.file_token,
                 origin,
                 &mut response,
                 range,

components/net/filemanager_thread.rs

@@ -11,14 +11,16 @@ use http::header::{self, HeaderValue};
 use ipc_channel::ipc::{self, IpcSender};
 use mime::{self, Mime};
 use net_traits::blob_url_store::{BlobBuf, BlobURLStoreError};
-use net_traits::filemanager_thread::{FileManagerResult, FileManagerThreadMsg, FileOrigin};
+use net_traits::filemanager_thread::{
+    FileManagerResult, FileManagerThreadMsg, FileOrigin, FileTokenCheck,
+};
 use net_traits::filemanager_thread::{
     FileManagerThreadError, ReadFileProgress, RelativePos, SelectedFile,
 };
 use net_traits::http_percent_encode;
 use net_traits::response::{Response, ResponseBody};
 use servo_arc::Arc as ServoArc;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::fs::File;
 use std::io::{BufRead, BufReader, Read, Seek, SeekFrom};
 use std::mem;
@@ -46,6 +48,9 @@ struct FileStoreEntry {
     /// by the user with createObjectURL. Validity can be revoked as well.
     /// (The UUID is the one that maps to this entry in `FileManagerStore`)
     is_valid_url: AtomicBool,
+    /// UUIDs of fetch instances that acquired an interest in this file,
+    /// when the url was still valid.
+    outstanding_tokens: HashSet<Uuid>,
 }
 
 #[derive(Clone)]
@@ -91,7 +96,6 @@ impl FileManager {
         &self,
         sender: IpcSender<FileManagerResult<ReadFileProgress>>,
         id: Uuid,
-        check_url_validity: bool,
         origin: FileOrigin,
     ) {
         let store = self.store.clone();
@@ -99,7 +103,7 @@ impl FileManager {
             .upgrade()
             .and_then(|pool| {
                 pool.spawn(move || {
-                    if let Err(e) = store.try_read_file(&sender, id, check_url_validity, origin) {
+                    if let Err(e) = store.try_read_file(&sender, id, origin) {
                         let _ = sender.send(Err(FileManagerThreadError::BlobURLStoreError(e)));
                     }
                 });
@@ -110,6 +114,14 @@ impl FileManager {
             });
     }
 
+    pub fn get_token_for_file(&self, file_id: &Uuid) -> FileTokenCheck {
+        self.store.get_token_for_file(file_id)
+    }
+
+    pub fn invalidate_token(&self, token: &FileTokenCheck, file_id: &Uuid) {
+        self.store.invalidate_token(token, file_id);
+    }
+
     // Read a file for the Fetch implementation.
     // It gets the required headers synchronously and reads the actual content
     // in a separate thread.
@@ -118,7 +130,7 @@ impl FileManager {
         done_sender: &Sender<Data>,
         cancellation_listener: Arc<Mutex<CancellationListener>>,
         id: Uuid,
-        check_url_validity: bool,
+        file_token: &FileTokenCheck,
         origin: FileOrigin,
         response: &mut Response,
         range: RangeRequestBounds,
@@ -127,9 +139,9 @@ impl FileManager {
             done_sender,
             cancellation_listener,
             &id,
+            file_token,
             &origin,
             range,
-            check_url_validity,
             response,
         )
     }
@@ -175,8 +187,8 @@ impl FileManager {
                         );
                     });
             },
-            FileManagerThreadMsg::ReadFile(sender, id, check_url_validity, origin) => {
+            FileManagerThreadMsg::ReadFile(sender, id, origin) => {
-                self.read_file(sender, id, check_url_validity, origin);
+                self.read_file(sender, id, origin);
             },
             FileManagerThreadMsg::PromoteMemory(id, blob_buf, set_valid, origin) => {
                 self.promote_memory(id, blob_buf, set_valid, origin);
@@ -273,12 +285,12 @@ impl FileManager {
         done_sender: &Sender<Data>,
         cancellation_listener: Arc<Mutex<CancellationListener>>,
         id: &Uuid,
+        file_token: &FileTokenCheck,
         origin_in: &FileOrigin,
         range: RangeRequestBounds,
-        check_url_validity: bool,
         response: &mut Response,
     ) -> Result<(), BlobURLStoreError> {
-        let file_impl = self.store.get_impl(id, origin_in, check_url_validity)?;
+        let file_impl = self.store.get_impl(id, file_token, origin_in)?;
         match file_impl {
             FileImpl::Memory(buf) => {
                 let range = match range.get_final(Some(buf.size)) {
@@ -362,11 +374,11 @@ impl FileManager {
                     done_sender,
                     cancellation_listener,
                     &parent_id,
+                    file_token,
                     origin_in,
                     RangeRequestBounds::Final(
                         RelativePos::full_range().slice_inner(&inner_rel_pos),
                     ),
-                    false,
                     response,
                 );
             },
@@ -392,19 +404,23 @@ impl FileManagerStore {
     pub fn get_impl(
         &self,
         id: &Uuid,
+        file_token: &FileTokenCheck,
         origin_in: &FileOrigin,
-        check_url_validity: bool,
     ) -> Result<FileImpl, BlobURLStoreError> {
         match self.entries.read().unwrap().get(id) {
             Some(ref entry) => {
                 if *origin_in != *entry.origin {
                     Err(BlobURLStoreError::InvalidOrigin)
                 } else {
-                    let is_valid = entry.is_valid_url.load(Ordering::Acquire);
+                    match file_token {
-                    if check_url_validity && !is_valid {
+                        FileTokenCheck::NotRequired => Ok(entry.file_impl.clone()),
-                        Err(BlobURLStoreError::InvalidFileID)
+                        FileTokenCheck::Required(token) => {
-                    } else {
+                            if entry.outstanding_tokens.contains(token) {
-                        Ok(entry.file_impl.clone())
+                                return Ok(entry.file_impl.clone());
+                            }
+                            Err(BlobURLStoreError::InvalidFileID)
+                        },
+                        FileTokenCheck::ShouldFail => Err(BlobURLStoreError::InvalidFileID),
                     }
                 }
             },
@@ -412,6 +428,58 @@ impl FileManagerStore {
         }
     }
 
+    pub fn invalidate_token(&self, token: &FileTokenCheck, file_id: &Uuid) {
+        if let FileTokenCheck::Required(token) = token {
+            let mut entries = self.entries.write().unwrap();
+            if let Some(entry) = entries.get_mut(file_id) {
+                entry.outstanding_tokens.remove(token);
+
+                // Check if there are references left.
+                let zero_refs = entry.refs.load(Ordering::Acquire) == 0;
+
+                // Check if no other fetch has acquired a token for this file.
+                let no_outstanding_tokens = entry.outstanding_tokens.len() == 0;
+
+                // Check if there is still a blob URL outstanding.
+                let valid = entry.is_valid_url.load(Ordering::Acquire);
+
+                // Can we remove this file?
+                let do_remove = zero_refs && no_outstanding_tokens && !valid;
+
+                if do_remove {
+                    entries.remove(&file_id);
+                }
+            }
+        }
+    }
+
+    pub fn get_token_for_file(&self, file_id: &Uuid) -> FileTokenCheck {
+        let mut entries = self.entries.write().unwrap();
+        let parent_id = match entries.get(file_id) {
+            Some(entry) => {
+                if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl {
+                    Some(parent_id.clone())
+                } else {
+                    None
+                }
+            },
+            None => return FileTokenCheck::ShouldFail,
+        };
+        let file_id = match parent_id.as_ref() {
+            Some(id) => id,
+            None => file_id,
+        };
+        if let Some(entry) = entries.get_mut(file_id) {
+            if !entry.is_valid_url.load(Ordering::Acquire) {
+                return FileTokenCheck::ShouldFail;
+            }
+            let token = Uuid::new_v4();
+            entry.outstanding_tokens.insert(token.clone());
+            return FileTokenCheck::Required(token);
+        }
+        FileTokenCheck::ShouldFail
+    }
+
     fn insert(&self, id: Uuid, entry: FileStoreEntry) {
         self.entries.write().unwrap().insert(id, entry);
     }
@@ -453,6 +521,7 @@ impl FileManagerStore {
                         // Valid here since AddSlicedURLEntry implies URL creation
                         // from a BlobImpl::Sliced
                         is_valid_url: AtomicBool::new(true),
+                        outstanding_tokens: Default::default(),
                     },
                 );
 
@@ -604,6 +673,7 @@ impl FileManagerStore {
                 refs: AtomicUsize::new(1),
                 // Invalid here since create_entry is called by file selection
                 is_valid_url: AtomicBool::new(false),
+                outstanding_tokens: Default::default(),
             },
         );
 
@@ -626,11 +696,11 @@ impl FileManagerStore {
         &self,
         sender: &IpcSender<FileManagerResult<ReadFileProgress>>,
         id: &Uuid,
+        file_token: &FileTokenCheck,
         origin_in: &FileOrigin,
         rel_pos: RelativePos,
-        check_url_validity: bool,
     ) -> Result<(), BlobURLStoreError> {
-        let file_impl = self.get_impl(id, origin_in, check_url_validity)?;
+        let file_impl = self.get_impl(id, file_token, origin_in)?;
         match file_impl {
             FileImpl::Memory(buf) => {
                 let range = rel_pos.to_abs_range(buf.size as usize);
@@ -686,9 +756,9 @@ impl FileManagerStore {
                 self.get_blob_buf(
                     sender,
                     &parent_id,
+                    file_token,
                     origin_in,
                     rel_pos.slice_inner(&inner_rel_pos),
-                    false,
                 )
             },
         }
@@ -699,15 +769,14 @@ impl FileManagerStore {
         &self,
         sender: &IpcSender<FileManagerResult<ReadFileProgress>>,
         id: Uuid,
-        check_url_validity: bool,
         origin_in: FileOrigin,
     ) -> Result<(), BlobURLStoreError> {
         self.get_blob_buf(
             sender,
             &id,
+            &FileTokenCheck::NotRequired,
             &origin_in,
             RelativePos::full_range(),
-            check_url_validity,
         )
     }
 
@@ -724,10 +793,17 @@ impl FileManagerStore {
                         // last reference, and if it has a reference to parent id
                         // dec_ref on parent later if necessary
                         let is_valid = entry.is_valid_url.load(Ordering::Acquire);
+
+                        // Check if no fetch has acquired a token for this file.
+                        let no_outstanding_tokens = entry.outstanding_tokens.len() == 0;
+
+                        // Can we remove this file?
+                        let do_remove = !is_valid && no_outstanding_tokens;
+
                         if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl {
-                            (!is_valid, Some(parent_id.clone()))
+                            (do_remove, Some(parent_id.clone()))
                         } else {
-                            (!is_valid, None)
+                            (do_remove, None)
                         }
                     }
                 } else {
@@ -762,6 +838,7 @@ impl FileManagerStore {
                         file_impl: FileImpl::Memory(blob_buf),
                         refs: AtomicUsize::new(1),
                         is_valid_url: AtomicBool::new(set_valid),
+                        outstanding_tokens: Default::default(),
                     },
                 );
             },
@@ -786,10 +863,16 @@ impl FileManagerStore {
                         // and store entry id holders
                         let zero_refs = entry.refs.load(Ordering::Acquire) == 0;
 
+                        // Check if no fetch has acquired a token for this file.
+                        let no_outstanding_tokens = entry.outstanding_tokens.len() == 0;
+
+                        // Can we remove this file?
+                        let do_remove = zero_refs && no_outstanding_tokens;
+
                         if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl {
-                            (zero_refs, Some(parent_id.clone()), Ok(()))
+                            (do_remove, Some(parent_id.clone()), Ok(()))
                         } else {
-                            (zero_refs, None, Ok(()))
+                            (do_remove, None, Ok(()))
                         }
                     } else {
                         (false, None, Ok(()))

components/net/resource_thread.rs

@@ -22,6 +22,8 @@ use embedder_traits::EmbedderProxy;
 use hyper_serde::Serde;
 use ipc_channel::ipc::{self, IpcReceiver, IpcReceiverSet, IpcSender};
 use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
+use net_traits::blob_url_store::parse_blob_url;
+use net_traits::filemanager_thread::FileTokenCheck;
 use net_traits::request::{Destination, RequestBuilder};
 use net_traits::response::{Response, ResponseInit};
 use net_traits::storage_thread::StorageThreadMsg;
@@ -621,8 +623,31 @@ impl CoreResourceManager {
             _ => ResourceTimingType::Resource,
         };
 
+        let mut request = request_builder.build();
+        let url = request.current_url();
+
+        // In the case of a valid blob URL, acquiring a token granting access to a file,
+        // regardless if the URL is revoked after token acquisition.
+        //
+        // TODO: to make more tests pass, acquire this token earlier,
+        // probably in a separate message flow.
+        //
+        // In such a setup, the token would not be acquired here,
+        // but could instead be contained in the actual CoreResourceMsg::Fetch message.
+        //
+        // See https://github.com/servo/servo/issues/25226
+        let (file_token, blob_url_file_id) = match url.scheme() {
+            "blob" => {
+                if let Ok((id, _)) = parse_blob_url(&url) {
+                    (self.filemanager.get_token_for_file(&id), Some(id))
+                } else {
+                    (FileTokenCheck::ShouldFail, None)
+                }
+            },
+            _ => (FileTokenCheck::NotRequired, None),
+        };
+
         self.thread_pool.spawn(move || {
-            let mut request = request_builder.build();
             // XXXManishearth: Check origin against pipeline id (also ensure that the mode is allowed)
             // todo load context / mimesniff in fetch
             // todo referrer policy?
@@ -632,6 +657,7 @@ impl CoreResourceManager {
                 user_agent: ua,
                 devtools_chan: dc,
                 filemanager: filemanager,
+                file_token,
                 cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(cancel_chan))),
                 timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new(request.timing_type()))),
             };
@@ -651,6 +677,13 @@ impl CoreResourceManager {
                 },
                 None => fetch(&mut request, &mut sender, &context),
             };
+
+            // Remove token after fetch.
+            if let Some(id) = blob_url_file_id.as_ref() {
+                context
+                    .filemanager
+                    .invalidate_token(&context.file_token, id);
+            }
         });
     }
 

components/net/tests/fetch.rs

@@ -29,6 +29,7 @@ use net::filemanager_thread::FileManager;
 use net::hsts::HstsEntry;
 use net::resource_thread::CoreResourceThreadPool;
 use net::test::HttpState;
+use net_traits::filemanager_thread::FileTokenCheck;
 use net_traits::request::{
     Destination, Origin, RedirectMode, Referrer, Request, RequestBuilder, RequestMode,
 };
@@ -683,6 +684,7 @@ fn test_fetch_with_hsts() {
         user_agent: DEFAULT_USER_AGENT.into(),
         devtools_chan: None,
         filemanager: FileManager::new(create_embedder_proxy(), Weak::new()),
+        file_token: FileTokenCheck::NotRequired,
         cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))),
         timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new(
             ResourceTimingType::Navigation,
@@ -735,6 +737,7 @@ fn test_load_adds_host_to_hsts_list_when_url_is_https() {
         user_agent: DEFAULT_USER_AGENT.into(),
         devtools_chan: None,
         filemanager: FileManager::new(create_embedder_proxy(), Weak::new()),
+        file_token: FileTokenCheck::NotRequired,
         cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))),
         timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new(
             ResourceTimingType::Navigation,

components/net/tests/filemanager_thread.rs

@@ -59,7 +59,6 @@ fn test_filemanager() {
             filemanager.handle(FileManagerThreadMsg::ReadFile(
                 tx2,
                 selected.id.clone(),
-                false,
                 origin.clone(),
             ));
 
@@ -113,7 +112,6 @@ fn test_filemanager() {
             filemanager.handle(FileManagerThreadMsg::ReadFile(
                 tx2,
                 selected.id.clone(),
-                false,
                 origin.clone(),
             ));
 

components/net/tests/main.rs

@@ -35,6 +35,7 @@ use net::fetch::methods::{self, CancellationListener, FetchContext};
 use net::filemanager_thread::FileManager;
 use net::resource_thread::CoreResourceThreadPool;
 use net::test::HttpState;
+use net_traits::filemanager_thread::FileTokenCheck;
 use net_traits::request::Request;
 use net_traits::response::Response;
 use net_traits::{FetchTaskTarget, ResourceFetchTiming, ResourceTimingType};
@@ -98,6 +99,7 @@ fn new_fetch_context(
         user_agent: DEFAULT_USER_AGENT.into(),
         devtools_chan: dc,
         filemanager: FileManager::new(sender, pool_handle.unwrap_or_else(|| Weak::new())),
+        file_token: FileTokenCheck::NotRequired,
         cancellation_listener: Arc::new(Mutex::new(CancellationListener::new(None))),
         timing: ServoArc::new(Mutex::new(ResourceFetchTiming::new(
             ResourceTimingType::Navigation,

components/net_traits/filemanager_thread.rs

@@ -16,6 +16,20 @@ use uuid::Uuid;
 /// File manager store entry's origin
 pub type FileOrigin = String;
 
+/// A token modulating access to a file for a blob URL.
+pub enum FileTokenCheck {
+    /// Checking against a token not required,
+    /// used for accessing a file
+    /// that isn't linked to from a blob URL.
+    NotRequired,
+    /// Checking against token required.
+    Required(Uuid),
+    /// Request should always fail,
+    /// used for cases when a check is required,
+    /// but no token could be acquired.
+    ShouldFail,
+}
+
 /// Relative slice positions of a sequence,
 /// whose semantic should be consistent with (start, end) parameters in
 /// <https://w3c.github.io/FileAPI/#dfn-slice>
@@ -125,7 +139,6 @@ pub enum FileManagerThreadMsg {
     ReadFile(
         IpcSender<FileManagerResult<ReadFileProgress>>,
         Uuid,
-        bool,
         FileOrigin,
     ),
 

components/script/dom/globalscope.rs

@@ -1675,8 +1675,7 @@ impl GlobalScope {
         let resource_threads = self.resource_threads();
         let (chan, recv) = profile_ipc::channel(self.time_profiler_chan().clone()).unwrap();
         let origin = get_blob_origin(&self.get_url());
-        let check_url_validity = false;
+        let msg = FileManagerThreadMsg::ReadFile(chan, id, origin);
-        let msg = FileManagerThreadMsg::ReadFile(chan, id, check_url_validity, origin);
         let _ = resource_threads.send(CoreResourceMsg::ToFileManager(msg));
         recv
     }

tests/wpt/metadata/FileAPI/url/url-in-tags-revoke.window.js.ini

@@ -1,5 +1,4 @@
 [url-in-tags-revoke.window.html]
-  expected: TIMEOUT
   [Fetching a blob URL immediately before revoking it works in an iframe.]
     expected: FAIL
 
@@ -15,6 +14,3 @@
   [Opening a blob URL in a new window by clicking an <a> tag works immediately before revoking the URL.]
     expected: FAIL
 
-  [Fetching a blob URL immediately before revoking it works in <script> tags.]
-    expected: TIMEOUT
-

tests/wpt/metadata/FileAPI/url/url-with-fetch.any.js.ini

@@ -2,14 +2,8 @@
   [Revoke blob URL after creating Request, will fetch]
     expected: FAIL
 
-  [Revoke blob URL after calling fetch, fetch should succeed]
-    expected: FAIL
-
 
 [url-with-fetch.any.html]
   [Revoke blob URL after creating Request, will fetch]
     expected: FAIL
 
-  [Revoke blob URL after calling fetch, fetch should succeed]
-    expected: FAIL
-