Auto merge of #13106 - servo:trusted, r=nox

Remove mutex from Trusted

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13106)
<!-- Reviewable:end -->

components/script/dom/bindings/refcounted.rs

@@ -6,21 +6,21 @@
 //! between threads (or intra-thread for asynchronous events). Akin to Gecko's
 //! nsMainThreadPtrHandle, this uses thread-safe reference counting and ensures
 //! that the actual SpiderMonkey GC integration occurs on the script thread via
-//! message passing. Ownership of a `Trusted<T>` object means the DOM object of
+//! weak refcounts. Ownership of a `Trusted<T>` object means the DOM object of
 //! type T to which it points remains alive. Any other behaviour is undefined.
 //! To guarantee the lifetime of a DOM object when performing asynchronous operations,
 //! obtain a `Trusted<T>` from that object and pass it along with each operation.
 //! A usable pointer to the original DOM object can be obtained on the script thread
 //! from a `Trusted<T>` via the `to_temporary` method.
 //!
-//! The implementation of Trusted<T> is as follows:
+//! The implementation of `Trusted<T>` is as follows:
-//! A hashtable resides in the script thread, keyed on the pointer to the Rust DOM object.
+//! The `Trusted<T>` object contains an atomic reference counted pointer to the Rust DOM object.
-//! The values in this hashtable are atomic reference counts. When a Trusted<T> object is
+//! A hashtable resides in the script thread, keyed on the pointer.
-//! created or cloned, this count is increased. When a Trusted<T> is dropped, the count
+//! The values in this hashtable are weak reference counts. When a `Trusted<T>` object is
-//! decreases. If the count hits zero, a message is dispatched to the script thread to remove
+//! created or cloned, the reference count is increased. When a `Trusted<T>` is dropped, the count
-//! the entry from the hashmap if the count is still zero. The JS reflector for the DOM object
+//! decreases. If the count hits zero, the weak reference is emptied, and is removed from
-//! is rooted when a hashmap entry is first created, and unrooted when the hashmap entry
+//! its hash table during the next GC. During GC, the entries of the hash table are counted
-//! is removed.
+//! as JS roots.
 
 use core::nonzero::NonZero;
 use dom::bindings::js::Root;
@@ -28,13 +28,13 @@ use dom::bindings::reflector::{Reflectable, Reflector};
 use dom::bindings::trace::trace_reflector;
 use js::jsapi::JSTracer;
 use libc;
-use script_runtime::{CommonScriptMsg, ScriptChan};
 use std::cell::RefCell;
 use std::collections::hash_map::Entry::{Occupied, Vacant};
 use std::collections::hash_map::HashMap;
+use std::hash::Hash;
 use std::marker::PhantomData;
 use std::os;
-use std::sync::{Arc, Mutex};
+use std::sync::{Arc, Weak};
 
 
 #[allow(missing_docs)]  // FIXME
@@ -52,6 +52,12 @@ pub use self::dummy::LIVE_REFERENCES;
 pub struct TrustedReference(*const libc::c_void);
 unsafe impl Send for TrustedReference {}
 
+impl TrustedReference {
+    fn new<T: Reflectable>(ptr: *const T) -> TrustedReference {
+        TrustedReference(ptr as *const libc::c_void)
+    }
+}
+
 /// A safe wrapper around a raw pointer to a DOM object that can be
 /// shared among threads for use in asynchronous operations. The underlying
 /// DOM object is guaranteed to live at least as long as the last outstanding
@@ -60,9 +66,7 @@ unsafe impl Send for TrustedReference {}
 pub struct Trusted<T: Reflectable> {
     /// A pointer to the Rust DOM object of type T, but void to allow
     /// sending `Trusted<T>` between threads, regardless of T's sendability.
-    ptr: *const libc::c_void,
+    refcount: Arc<TrustedReference>,
-    refcount: Arc<Mutex<usize>>,
-    script_chan: Box<ScriptChan + Send>,
     owner_thread: *const libc::c_void,
     phantom: PhantomData<T>,
 }
@@ -74,15 +78,12 @@ impl<T: Reflectable> Trusted<T> {
     /// be prevented from being GCed for the duration of the resulting `Trusted<T>` object's
     /// lifetime.
     pub fn new(ptr: &T) -> Trusted<T> {
-        let script_chan = ptr.global().r().script_chan();
         LIVE_REFERENCES.with(|ref r| {
             let r = r.borrow();
             let live_references = r.as_ref().unwrap();
             let refcount = live_references.addref(&*ptr as *const T);
             Trusted {
-                ptr: &*ptr as *const T as *const libc::c_void,
                 refcount: refcount,
-                script_chan: script_chan.clone(),
                 owner_thread: (&*live_references) as *const _ as *const libc::c_void,
                 phantom: PhantomData,
             }
@@ -99,48 +100,26 @@ impl<T: Reflectable> Trusted<T> {
             self.owner_thread == (&*live_references) as *const _ as *const libc::c_void
         }));
         unsafe {
-            Root::new(NonZero::new(self.ptr as *const T))
+            Root::new(NonZero::new(self.refcount.0 as *const T))
         }
     }
 }
 
 impl<T: Reflectable> Clone for Trusted<T> {
     fn clone(&self) -> Trusted<T> {
-        {
-            let mut refcount = self.refcount.lock().unwrap();
-            *refcount += 1;
-        }
-
         Trusted {
-            ptr: self.ptr,
             refcount: self.refcount.clone(),
-            script_chan: self.script_chan.clone(),
             owner_thread: self.owner_thread,
             phantom: PhantomData,
         }
     }
 }
 
-impl<T: Reflectable> Drop for Trusted<T> {
-    fn drop(&mut self) {
-        let mut refcount = self.refcount.lock().unwrap();
-        assert!(*refcount > 0);
-        *refcount -= 1;
-        if *refcount == 0 {
-            // It's possible this send will fail if the script thread
-            // has already exited. There's not much we can do at this
-            // point though.
-            let msg = CommonScriptMsg::RefcountCleanup(TrustedReference(self.ptr));
-            let _ = self.script_chan.send(msg);
-        }
-    }
-}
-
 /// The set of live, pinned DOM objects that are currently prevented
 /// from being garbage collected due to outstanding references.
 pub struct LiveDOMReferences {
     // keyed on pointer to Rust DOM object
-    table: RefCell<HashMap<*const libc::c_void, Arc<Mutex<usize>>>>,
+    table: RefCell<HashMap<*const libc::c_void, Weak<TrustedReference>>>,
 }
 
 impl LiveDOMReferences {
@@ -153,59 +132,55 @@ impl LiveDOMReferences {
         });
     }
 
-    fn addref<T: Reflectable>(&self, ptr: *const T) -> Arc<Mutex<usize>> {
+    fn addref<T: Reflectable>(&self, ptr: *const T) -> Arc<TrustedReference> {
         let mut table = self.table.borrow_mut();
+        let capacity = table.capacity();
+        let len = table.len();
+        if (0 < capacity) && (capacity <= len) {
+            info!("growing refcounted references by {}", len);
+            remove_nulls(&mut table);
+            table.reserve(len);
+        }
         match table.entry(ptr as *const libc::c_void) {
-            Occupied(mut entry) => {
+            Occupied(mut entry) => match entry.get().upgrade() {
-                let refcount = entry.get_mut();
+                Some(refcount) => refcount,
-                *refcount.lock().unwrap() += 1;
+                None => {
-                refcount.clone()
+                    let refcount = Arc::new(TrustedReference::new(ptr));
-            }
+                    entry.insert(Arc::downgrade(&refcount));
+                    refcount
+                },
+            },
             Vacant(entry) => {
-                let refcount = Arc::new(Mutex::new(1));
+                let refcount = Arc::new(TrustedReference::new(ptr));
-                entry.insert(refcount.clone());
+                entry.insert(Arc::downgrade(&refcount));
                 refcount
             }
         }
     }
+}
 
-    /// Unpin the given DOM object if its refcount is 0.
+/// Remove null entries from the live references table
-    pub fn cleanup(raw_reflectable: TrustedReference) {
+fn remove_nulls<K: Eq + Hash + Clone, V> (table: &mut HashMap<K, Weak<V>>) {
-        let TrustedReference(raw_reflectable) = raw_reflectable;
+    let to_remove: Vec<K> =
-        LIVE_REFERENCES.with(|ref r| {
+        table.iter()
-            let r = r.borrow();
+        .filter(|&(_, value)| Weak::upgrade(value).is_none())
-            let live_references = r.as_ref().unwrap();
+        .map(|(key, _)| key.clone())
-            let mut table = live_references.table.borrow_mut();
+        .collect();
-            match table.entry(raw_reflectable) {
+    info!("removing {} refcounted references", to_remove.len());
-                Occupied(entry) => {
+    for key in to_remove {
-                    if *entry.get().lock().unwrap() != 0 {
+        table.remove(&key);
-                        // there could have been a new reference taken since
-                        // this message was dispatched.
-                        return;
-                    }
-
-                    let _ = entry.remove();
-                }
-                Vacant(_) => {
-                    // there could be a cleanup message dispatched, then a new
-                    // pinned reference obtained and released before the message
-                    // is processed, at which point there would be no matching
-                    // hashtable entry.
-                    info!("attempt to cleanup an unrecognized reflector");
-                }
-            }
-        })
     }
 }
 
 /// A JSTraceDataOp for tracing reflectors held in LIVE_REFERENCES
 pub unsafe extern "C" fn trace_refcounted_objects(tracer: *mut JSTracer,
                                                   _data: *mut os::raw::c_void) {
-    debug!("tracing live refcounted references");
+    info!("tracing live refcounted references");
     LIVE_REFERENCES.with(|ref r| {
         let r = r.borrow();
         let live_references = r.as_ref().unwrap();
-        let table = live_references.table.borrow();
+        let mut table = live_references.table.borrow_mut();
+        remove_nulls(&mut table);
         for obj in table.keys() {
             let reflectable = &*(*obj as *const Reflector);
             trace_reflector(tracer, "refcounted", reflectable);

components/script/dom/dedicatedworkerglobalscope.rs

@@ -14,7 +14,6 @@ use dom::bindings::error::ErrorResult;
 use dom::bindings::global::{GlobalRef, global_root_from_context};
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{Root, RootCollection};
-use dom::bindings::refcounted::LiveDOMReferences;
 use dom::bindings::reflector::Reflectable;
 use dom::bindings::str::DOMString;
 use dom::bindings::structuredclone::StructuredCloneData;
@@ -298,9 +297,6 @@ impl DedicatedWorkerGlobalScope {
             WorkerScriptMsg::Common(CommonScriptMsg::RunnableMsg(_, runnable)) => {
                 runnable.handler()
             },
-            WorkerScriptMsg::Common(CommonScriptMsg::RefcountCleanup(addr)) => {
-                LiveDOMReferences::cleanup(addr);
-            },
             WorkerScriptMsg::Common(CommonScriptMsg::CollectReports(reports_chan)) => {
                 let scope = self.upcast::<WorkerGlobalScope>();
                 let cx = scope.get_cx();

components/script/dom/serviceworkerglobalscope.rs

@@ -12,7 +12,6 @@ use dom::bindings::codegen::Bindings::ServiceWorkerGlobalScopeBinding::ServiceWo
 use dom::bindings::global::{GlobalRef, global_root_from_context};
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{Root, RootCollection};
-use dom::bindings::refcounted::LiveDOMReferences;
 use dom::bindings::reflector::Reflectable;
 use dom::bindings::str::DOMString;
 use dom::eventtarget::EventTarget;
@@ -238,9 +237,6 @@ impl ServiceWorkerGlobalScope {
             CommonWorker(WorkerScriptMsg::Common(CommonScriptMsg::RunnableMsg(_, runnable))) => {
                 runnable.handler()
             },
-            CommonWorker(WorkerScriptMsg::Common(CommonScriptMsg::RefcountCleanup(addr))) => {
-                LiveDOMReferences::cleanup(addr);
-            },
             CommonWorker(WorkerScriptMsg::Common(CommonScriptMsg::CollectReports(reports_chan))) => {
                 let scope = self.upcast::<WorkerGlobalScope>();
                 let cx = scope.get_cx();

components/script/script_runtime.rs

@@ -6,7 +6,7 @@
 //! script thread, the dom, and the worker threads.
 
 use dom::bindings::js::{RootCollection, RootCollectionPtr, trace_roots};
-use dom::bindings::refcounted::{LiveDOMReferences, TrustedReference, trace_refcounted_objects};
+use dom::bindings::refcounted::{LiveDOMReferences, trace_refcounted_objects};
 use dom::bindings::trace::trace_traceables;
 use dom::bindings::utils::DOM_CALLBACKS;
 use js::glue::CollectServoSizes;
@@ -35,8 +35,6 @@ pub enum CommonScriptMsg {
     /// Requests that the script thread measure its memory usage. The results are sent back via the
     /// supplied channel.
     CollectReports(ReportsChan),
-    /// A DOM object's last pinned reference was removed (dispatched to all threads).
-    RefcountCleanup(TrustedReference),
     /// Generic message that encapsulates event handling.
     RunnableMsg(ScriptThreadEventCategory, Box<Runnable + Send>),
 }

components/script/script_thread.rs

@@ -31,7 +31,7 @@ use dom::bindings::global::GlobalRef;
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, MutNullableHeap, Root, RootCollection};
 use dom::bindings::js::{RootCollectionPtr, RootedReference};
-use dom::bindings::refcounted::{LiveDOMReferences, Trusted};
+use dom::bindings::refcounted::Trusted;
 use dom::bindings::reflector::Reflectable;
 use dom::bindings::str::DOMString;
 use dom::bindings::trace::JSTraceable;
@@ -948,8 +948,6 @@ impl ScriptThread {
                     runnable.handler()
                 }
             }
-            MainThreadScriptMsg::Common(CommonScriptMsg::RefcountCleanup(addr)) =>
-                LiveDOMReferences::cleanup(addr),
             MainThreadScriptMsg::Common(CommonScriptMsg::CollectReports(reports_chan)) =>
                 self.collect_reports(reports_chan),
             MainThreadScriptMsg::DOMManipulation(task) =>