Fix FileAPI's refcount implementation

components/net/filemanager_thread.rs

@@ -178,7 +178,7 @@ impl<UI: 'static + UIProvider> FileManager<UI> {
                     if let Ok(id) = Uuid::parse_str(&id.0) {
                         spawn_named("revoke blob url".to_owned(), move || {
                             // Since it is revocation, unset_url_validity is true
-                            let _ = sender.send(store.dec_ref(&id, &origin, true));
+                            let _ = sender.send(store.set_blob_url_validity(false, &id, &origin));
                         })
                     } else {
                         let _ = sender.send(Err(BlobURLStoreError::InvalidFileID));
@@ -189,23 +189,16 @@ impl<UI: 'static + UIProvider> FileManager<UI> {
                         spawn_named("dec ref".to_owned(), move || {
                             // Since it is simple DecRef (possibly caused by close/drop),
                             // unset_url_validity is false
-                            let _ = sender.send(store.dec_ref(&id, &origin, false));
+                            let _ = sender.send(store.dec_ref(&id, &origin));
                         })
                     } else {
                         let _ = sender.send(Err(BlobURLStoreError::InvalidFileID));
                     }
                 }
-                FileManagerThreadMsg::IncRef(id, origin) => {
-                    if let Ok(id) = Uuid::parse_str(&id.0) {
-                        spawn_named("inc ref".to_owned(), move || {
-                            let _ = store.inc_ref(&id, &origin);
-                        })
-                    }
-                }
                 FileManagerThreadMsg::ActivateBlobURL(id, sender, origin) => {
                     if let Ok(id) = Uuid::parse_str(&id.0) {
                         spawn_named("activate blob url".to_owned(), move || {
-                            let _ = sender.send(store.activate_blob_url(&id, &origin));
+                            let _ = sender.send(store.set_blob_url_validity(true, &id, &origin));
                         });
                     } else {
                         let _ = sender.send(Err(BlobURLStoreError::InvalidFileID));
@@ -287,9 +280,11 @@ impl <UI: 'static + UIProvider> FileManagerStore<UI> {
                         file_impl: FileImpl::Sliced(parent_id, rel_pos),
                         refs: AtomicUsize::new(1),
                         // Valid here since AddSlicedURLEntry implies URL creation
+                        // from a BlobImpl::Sliced
                         is_valid_url: AtomicBool::new(true),
                     });
 
+                    // We assume that the returned id will be held by BlobImpl::File
                     let _ = sender.send(Ok(SelectedFileId(new_id.simple().to_string())));
                 }
                 Err(e) => {
@@ -475,17 +470,12 @@ impl <UI: 'static + UIProvider> FileManagerStore<UI> {
         self.get_blob_buf(&id, &origin_in, RelativePos::full_range(), check_url_validity)
     }
 
-    fn dec_ref(&self, id: &Uuid, origin_in: &FileOrigin,
+    fn dec_ref(&self, id: &Uuid, origin_in: &FileOrigin) -> Result<(), BlobURLStoreError> {
-               unset_url_validity: bool) -> Result<(), BlobURLStoreError> {
         let (do_remove, opt_parent_id) = match self.entries.read().unwrap().get(id) {
             Some(entry) => {
                 if *entry.origin == *origin_in {
                     let old_refs = entry.refs.fetch_sub(1, Ordering::Release);
 
-                    if unset_url_validity {
-                        entry.is_valid_url.store(false, Ordering::Release);
-                    }
-
                     if old_refs > 1 {
                         // not the last reference, no need to touch parent
                         (false, None)
@@ -515,7 +505,7 @@ impl <UI: 'static + UIProvider> FileManagerStore<UI> {
             if let Some(parent_id) = opt_parent_id {
                 // unset_url_validity for parent is false since we only need
                 // to unset the initial requesting URL
-                return self.dec_ref(&parent_id, origin_in, false);
+                return self.dec_ref(&parent_id, origin_in);
             }
         }
 
@@ -543,22 +533,46 @@ impl <UI: 'static + UIProvider> FileManagerStore<UI> {
         }
     }
 
-    fn activate_blob_url(&self, id: &Uuid, origin_in: &FileOrigin) -> Result<(), BlobURLStoreError> {
+    fn set_blob_url_validity(&self, validity: bool, id: &Uuid,
-        match self.entries.read().unwrap().get(id) {
+                             origin_in: &FileOrigin) -> Result<(), BlobURLStoreError> {
+        let (do_remove, opt_parent_id, res) = match self.entries.read().unwrap().get(id) {
             Some(entry) => {
                 if *entry.origin == *origin_in {
-                    entry.is_valid_url.store(true, Ordering::Release);
+                    entry.is_valid_url.store(validity, Ordering::Release);
-                    Ok(())
+
+                    if !validity {
+                        // Check if it is the last possible reference
+                        // since refs only accounts for blob id holders
+                        // and store entry id holders
+                        let zero_refs =  entry.refs.load(Ordering::Acquire) == 0;
+
+                        if let FileImpl::Sliced(ref parent_id, _) = entry.file_impl {
+                            (zero_refs, Some(parent_id.clone()), Ok(()))
+                        } else {
+                            (zero_refs, None, Ok(()))
+                        }
+                    } else {
+                        (false, None, Ok(()))
+                    }
                 } else {
-                    Err(BlobURLStoreError::InvalidOrigin)
+                    (false, None, Err(BlobURLStoreError::InvalidOrigin))
                 }
             }
-            None => Err(BlobURLStoreError::InvalidFileID)
+            None => (false, None, Err(BlobURLStoreError::InvalidFileID))
+        };
+
+        if do_remove {
+            atomic::fence(Ordering::Acquire);
+            self.remove(id);
+
+            if let Some(parent_id) = opt_parent_id {
+                return self.dec_ref(&parent_id, origin_in);
+            }
         }
+        res
     }
 }
 
-
 fn select_files_pref_enabled() -> bool {
     PREFS.get("dom.testing.htmlinputelement.select_files.enabled")
          .as_boolean().unwrap_or(false)

components/net_traits/filemanager_thread.rs

@@ -139,18 +139,15 @@ pub enum FileManagerThreadMsg {
     /// as part of a valid Blob URL
     AddSlicedURLEntry(SelectedFileId, RelativePos, IpcSender<Result<SelectedFileId, BlobURLStoreError>>, FileOrigin),
 
-    /// Revoke Blob URL and send back the acknowledgement
-    RevokeBlobURL(SelectedFileId, FileOrigin, IpcSender<Result<(), BlobURLStoreError>>),
-
     /// Decrease reference count and send back the acknowledgement
     DecRef(SelectedFileId, FileOrigin, IpcSender<Result<(), BlobURLStoreError>>),
 
-    /// Increase reference count
-    IncRef(SelectedFileId, FileOrigin),
-
     /// Activate an internal FileID so it becomes valid as part of a Blob URL
     ActivateBlobURL(SelectedFileId, IpcSender<Result<(), BlobURLStoreError>>, FileOrigin),
 
+    /// Revoke Blob URL and send back the acknowledgement
+    RevokeBlobURL(SelectedFileId, FileOrigin, IpcSender<Result<(), BlobURLStoreError>>),
+
     /// Shut down this thread
     Exit,
 }

components/script/dom/blob.rs

@@ -18,6 +18,7 @@ use net_traits::IpcSend;
 use net_traits::blob_url_store::{BlobBuf, get_blob_origin};
 use net_traits::filemanager_thread::{FileManagerThreadMsg, SelectedFileId, RelativePos};
 use std::cell::Cell;
+use std::mem;
 use std::ops::Index;
 use std::path::PathBuf;
 use uuid::Uuid;
@@ -26,7 +27,7 @@ use uuid::Uuid;
 #[derive(JSTraceable)]
 pub struct FileBlob {
     id: SelectedFileId,
-    name: PathBuf,
+    name: Option<PathBuf>,
     cache: DOMRefCell<Option<Vec<u8>>>,
     size: u64,
 }
@@ -57,7 +58,7 @@ impl BlobImpl {
     pub fn new_from_file(file_id: SelectedFileId, name: PathBuf, size: u64) -> BlobImpl {
         BlobImpl::File(FileBlob {
             id: file_id,
-            name: name,
+            name: Some(name),
             cache: DOMRefCell::new(None),
             size: size,
         })
@@ -98,9 +99,7 @@ impl Blob {
                   relativeContentType: DOMString) -> Root<Blob> {
         let global = parent.global();
         let blob_impl = match *parent.blob_impl.borrow() {
-            BlobImpl::File(ref f) => {
+            BlobImpl::File(_) => {
-                inc_ref_id(global.r(), f.id.clone());
-
                 // Create new parent node
                 BlobImpl::Sliced(JS::from_ref(parent), rel_pos)
             }
@@ -110,13 +109,7 @@ impl Blob {
             }
             BlobImpl::Sliced(ref grandparent, ref old_rel_pos) => {
                 // Adjust the slicing position, using same parent
-                let new_rel_pos = old_rel_pos.slice_inner(&rel_pos);
+                BlobImpl::Sliced(grandparent.clone(), old_rel_pos.slice_inner(&rel_pos))
-
-                if let BlobImpl::File(ref f) = *grandparent.blob_impl.borrow() {
-                    inc_ref_id(global.r(), f.id.clone());
-                }
-
-                BlobImpl::Sliced(grandparent.clone(), new_rel_pos)
             }
         };
 
@@ -173,47 +166,56 @@ impl Blob {
     /// Get a FileID representing the Blob content,
     /// used by URL.createObjectURL
     pub fn get_blob_url_id(&self) -> SelectedFileId {
-        match *self.blob_impl.borrow() {
+        let opt_sliced_parent = match *self.blob_impl.borrow() {
-            BlobImpl::File(ref f) => {
-                let global = self.global();
-                let origin = get_blob_origin(&global.r().get_url());
-                let filemanager = global.r().resource_threads().sender();
-                let (tx, rx) = ipc::channel().unwrap();
-
-                let _ = filemanager.send(FileManagerThreadMsg::ActivateBlobURL(f.id.clone(), tx, origin.clone()));
-
-                match rx.recv().unwrap() {
-                    Ok(_) => f.id.clone(),
-                    Err(_) => SelectedFileId(Uuid::new_v4().simple().to_string()) // Return a dummy id on error
-                }
-            }
-            BlobImpl::Memory(ref slice) => {
-                self.promote(slice, true)
-            }
             BlobImpl::Sliced(ref parent, ref rel_pos) => {
-                match *parent.blob_impl.borrow() {
+                Some((parent.promote(/* set_valid is */ false), rel_pos.clone(), parent.Size()))
-                    BlobImpl::Sliced(_, _) => {
-                        debug!("Sliced can't have a sliced parent");
-                        // Return dummy id
-                        SelectedFileId(Uuid::new_v4().simple().to_string())
-                    }
-                    BlobImpl::File(ref f) =>
-                        self.create_sliced_url_id(&f.id, rel_pos),
-                    BlobImpl::Memory(ref bytes) => {
-                        let parent_id = parent.promote(bytes, false);
-                        *self.blob_impl.borrow_mut() = BlobImpl::Sliced(parent.clone(), rel_pos.clone());
-                        self.create_sliced_url_id(&parent_id, rel_pos)
-                    }
-                }
             }
+            _ => None
+        };
+
+        match opt_sliced_parent {
+            Some((parent_id, rel_pos, size)) => self.create_sliced_url_id(&parent_id, &rel_pos, size),
+            None => self.promote(/* set_valid is */ true),
         }
     }
 
-    /// Promite memory-based Blob to file-based,
+    /// Promote non-Slice blob:
-    /// The bytes in data slice will be transferred to file manager thread.
+    /// 1. Memory-based: The bytes in data slice will be transferred to file manager thread.
+    /// 2. File-based: Activation
     /// Depending on set_valid, the returned FileID can be part of
     /// valid or invalid Blob URL.
-    fn promote(&self, bytes: &[u8], set_valid: bool) -> SelectedFileId {
+    fn promote(&self, set_valid: bool) -> SelectedFileId {
+        let mut bytes = vec![];
+
+        match *self.blob_impl.borrow_mut() {
+            BlobImpl::Sliced(_, _) => {
+                debug!("Sliced can't have a sliced parent");
+                // Return dummy id
+                return SelectedFileId(Uuid::new_v4().simple().to_string());
+            }
+            BlobImpl::File(ref f) => {
+                if set_valid {
+                    let global = self.global();
+                    let origin = get_blob_origin(&global.r().get_url());
+                    let filemanager = global.r().resource_threads().sender();
+                    let (tx, rx) = ipc::channel().unwrap();
+
+                    let msg = FileManagerThreadMsg::ActivateBlobURL(f.id.clone(), tx, origin.clone());
+                    let _ = filemanager.send(msg);
+
+                    match rx.recv().unwrap() {
+                        Ok(_) => return f.id.clone(),
+                        // Return a dummy id on error
+                        Err(_) => return SelectedFileId(Uuid::new_v4().simple().to_string())
+                    }
+                } else {
+                    // no need to activate
+                    return f.id.clone();
+                }
+            }
+            BlobImpl::Memory(ref mut bytes_in) => mem::swap(bytes_in, &mut bytes),
+        };
+
         let global = self.global();
         let origin = get_blob_origin(&global.r().get_url());
         let filemanager = global.r().resource_threads().sender();
@@ -221,7 +223,7 @@ impl Blob {
         let blob_buf = BlobBuf {
             filename: None,
             type_string: self.typeString.clone(),
-            size: self.Size(),
+            size: bytes.len() as u64,
             bytes: bytes.to_vec(),
         };
 
@@ -229,7 +231,16 @@ impl Blob {
         let _ = filemanager.send(FileManagerThreadMsg::PromoteMemory(blob_buf, set_valid, tx, origin.clone()));
 
         match rx.recv().unwrap() {
-            Ok(new_id) => SelectedFileId(new_id.0),
+            Ok(id) => {
+                let id = SelectedFileId(id.0);
+                *self.blob_impl.borrow_mut() = BlobImpl::File(FileBlob {
+                    id: id.clone(),
+                    name: None,
+                    cache: DOMRefCell::new(Some(bytes.to_vec())),
+                    size: bytes.len() as u64,
+                });
+                id
+            }
             // Dummy id
             Err(_) => SelectedFileId(Uuid::new_v4().simple().to_string()),
         }
@@ -237,7 +248,7 @@ impl Blob {
 
     /// Get a FileID representing sliced parent-blob content
     fn create_sliced_url_id(&self, parent_id: &SelectedFileId,
-                            rel_pos: &RelativePos) -> SelectedFileId {
+                            rel_pos: &RelativePos, parent_len: u64) -> SelectedFileId {
         let global = self.global();
 
         let origin = get_blob_origin(&global.r().get_url());
@@ -248,10 +259,25 @@ impl Blob {
                                                           rel_pos.clone(),
                                                           tx, origin.clone());
         let _ = filemanager.send(msg);
-        let new_id = rx.recv().unwrap().unwrap();
+        match rx.recv().expect("File manager thread is down") {
+            Ok(new_id) => {
+                let new_id = SelectedFileId(new_id.0);
 
-        // Return the indirect id reference
+                *self.blob_impl.borrow_mut() = BlobImpl::File(FileBlob {
-        SelectedFileId(new_id.0)
+                    id: new_id.clone(),
+                    name: None,
+                    cache: DOMRefCell::new(None),
+                    size: rel_pos.to_abs_range(parent_len as usize).len() as u64,
+                });
+
+                // Return the indirect id reference
+                new_id
+            }
+            Err(_) => {
+                // Return dummy id
+                SelectedFileId(Uuid::new_v4().simple().to_string())
+            }
+        }
     }
 
     /// Cleanups at the time of destruction/closing
@@ -380,11 +406,3 @@ fn is_ascii_printable(string: &str) -> bool {
     // https://w3c.github.io/FileAPI/#constructorBlob
     string.chars().all(|c| c >= '\x20' && c <= '\x7E')
 }
-
-/// Bump the reference counter in file manager thread
-fn inc_ref_id(global: GlobalRef, id: SelectedFileId) {
-    let file_manager = global.filemanager_thread();
-    let origin = get_blob_origin(&global.get_url());
-    let msg = FileManagerThreadMsg::IncRef(id, origin);
-    let _ = file_manager.send(msg);
-}