Auto merge of #487 - jdm:shutdown, r=asajeffrey

Prevent initializing and shutting down SpiderMonkey from different threads

Based on [this comment](https://github.com/servo/mozjs/blob/e21c05b415dfc246175ff8d5fc48b0e8c5b4e9e9/mozjs/js/public/Initialization.h#L19-L23), it appears that JS_Init and JS_ShutDown need to be called on the same thread. These changes make JSEngine a non-sendable type, and add JSEngineHandle as a sendable type that allows verifying that all outstanding consumers of the engine are dropped before shutting down.

Author: bors-servo

src/rust.rs

@@ -23,6 +23,7 @@ use std::os::raw::c_void;
 use std::cell::Cell;
 use std::marker::PhantomData;
 use std::sync::{Arc, Mutex};
+use std::sync::atomic::{AtomicU32, Ordering};
 
 use consts::{JSCLASS_RESERVED_SLOTS_MASK, JSCLASS_GLOBAL_SLOT_COUNT};
 use consts::{JSCLASS_IS_DOMJSCLASS, JSCLASS_IS_GLOBAL};
@@ -172,11 +173,31 @@ pub enum JSEngineError {
 /// A handle that must be kept alive in order to create new Runtimes.
 /// When this handle is dropped, the engine is shut down and cannot
 /// be reinitialized.
-pub struct JSEngine(());
+pub struct JSEngine {
+    /// The count of alive handles derived from this initialized instance.
+    outstanding_handles: Arc<AtomicU32>,
+    // Ensure this type cannot be sent between threads.
+    marker: PhantomData<*mut ()>,
+}
+
+pub struct JSEngineHandle(Arc<AtomicU32>);
+
+impl Clone for JSEngineHandle {
+    fn clone(&self) -> JSEngineHandle {
+        self.0.fetch_add(1, Ordering::SeqCst);
+        JSEngineHandle(self.0.clone())
+    }
+}
+
+impl Drop for JSEngineHandle {
+    fn drop(&mut self) {
+        self.0.fetch_sub(1, Ordering::SeqCst);
+    }
+}
 
 impl JSEngine {
     /// Initialize the JS engine to prepare for creating new JS runtimes.
-    pub fn init() -> Result<Arc<JSEngine>, JSEngineError> {
+    pub fn init() -> Result<JSEngine, JSEngineError> {
         let mut state = ENGINE_STATE.lock().unwrap();
         match *state {
             EngineState::Initialized => return Err(JSEngineError::AlreadyInitialized),
@@ -189,9 +210,22 @@ impl JSEngine {
             Err(JSEngineError::InitFailed)
         } else {
             *state = EngineState::Initialized;
-            Ok(Arc::new(JSEngine(())))
+            Ok(JSEngine {
+                outstanding_handles: Arc::new(AtomicU32::new(0)),
+                marker: PhantomData,
+            })
         }
     }
+
+    pub fn can_shutdown(&self) -> bool {
+        self.outstanding_handles.load(Ordering::SeqCst) == 0
+    }
+
+    /// Create a handle to this engine.
+    pub fn handle(&self) -> JSEngineHandle {
+        self.outstanding_handles.fetch_add(1, Ordering::SeqCst);
+        JSEngineHandle(self.outstanding_handles.clone())
+    }
 }
 
 /// Shut down the JS engine, invalidating any existing runtimes and preventing
@@ -200,6 +234,11 @@ impl Drop for JSEngine {
     fn drop(&mut self) {
         let mut state = ENGINE_STATE.lock().unwrap();
         if *state == EngineState::Initialized {
+            assert_eq!(
+                self.outstanding_handles.load(Ordering::SeqCst),
+                0,
+                "There are outstanding JS engine handles"
+            );
             *state = EngineState::ShutDown;
             unsafe {
                 JS_ShutDown();
@@ -215,7 +254,7 @@ pub struct ParentRuntime {
     /// Raw pointer to the underlying SpiderMonkey runtime.
     parent: *mut JSRuntime,
     /// Handle to ensure the JS engine remains running while this handle exists.
-    engine: Arc<JSEngine>,
+    engine: JSEngineHandle,
     /// The number of children of the runtime that created this ParentRuntime value.
     children_of_parent: Arc<()>,
 }
@@ -226,7 +265,7 @@ pub struct Runtime {
     /// Raw pointer to the underlying SpiderMonkey context.
     cx: *mut JSContext,
     /// The engine that this runtime is associated with.
-    engine: Arc<JSEngine>,
+    engine: JSEngineHandle,
     /// If this Runtime was created with a parent, this member exists to ensure
     /// that that parent's count of outstanding children (see [outstanding_children])
     /// remains accurate and will be automatically decreased when this Runtime value
@@ -251,7 +290,7 @@ impl Runtime {
     }
 
     /// Creates a new `JSContext`.
-    pub fn new(engine: Arc<JSEngine>) -> Runtime {
+    pub fn new(engine: JSEngineHandle) -> Runtime {
         unsafe { Self::create(engine, None) }
     }
 
@@ -279,7 +318,7 @@ impl Runtime {
         Self::create(parent.engine.clone(), Some(parent))
     }
 
-    unsafe fn create(engine: Arc<JSEngine>, parent: Option<ParentRuntime>) -> Runtime {
+    unsafe fn create(engine: JSEngineHandle, parent: Option<ParentRuntime>) -> Runtime {
         let parent_runtime = parent.as_ref().map_or(
             ptr::null_mut(),
             |r| r.parent,

tests/callback.rs

@@ -25,7 +25,7 @@ use std::str;
 #[test]
 fn callback() {
     let engine = JSEngine::init().unwrap();
-    let runtime = Runtime::new(engine);
+    let runtime = Runtime::new(engine.handle());
     let context = runtime.cx();
     let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;
     let c_option = RealmOptions::default();

tests/capture_stack.rs

@@ -22,7 +22,7 @@ use std::ptr;
 #[test]
 fn capture_stack() {
     let engine = JSEngine::init().unwrap();
-    let runtime = Runtime::new(engine);
+    let runtime = Runtime::new(engine.handle());
     let context = runtime.cx();
     let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;
     let c_option = RealmOptions::default();

tests/custom_auto_rooter.rs

@@ -34,7 +34,7 @@ unsafe impl CustomTrace for TraceCheck {
 #[test]
 fn virtual_trace_called() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
 
     let mut rooter = CustomAutoRooter::new(TraceCheck::new());

tests/custom_auto_rooter_macro.rs

@@ -31,7 +31,7 @@ unsafe impl CustomTrace for TraceCheck {
 #[test]
 fn custom_auto_rooter_macro() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
 
     auto_root!(in(cx) let vec = vec![TraceCheck::new(), TraceCheck::new()]);

tests/enumerate.rs

@@ -23,7 +23,7 @@ use std::ptr;
 #[test]
 fn enumerate() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
     let options = RealmOptions::default();
 

tests/evaluate.rs

@@ -15,7 +15,7 @@ use std::ptr;
 #[test]
 fn evaluate() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
 
     unsafe {

tests/panic.rs

@@ -20,7 +20,7 @@ use std::ptr;
 #[should_panic]
 fn test_panic() {
     let engine = JSEngine::init().unwrap();
-    let runtime = Runtime::new(engine);
+    let runtime = Runtime::new(engine.handle());
     let context = runtime.cx();
     let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;
     let c_option = RealmOptions::default();

tests/rooting.rs

@@ -29,7 +29,7 @@ use std::ptr;
 fn rooting() {
     unsafe {
         let engine = JSEngine::init().unwrap();
-        let runtime = Runtime::new(engine);
+        let runtime = Runtime::new(engine.handle());
         let cx = runtime.cx();
         JS_SetGCZeal(cx, 2, 1);
 

tests/runtime.rs

@@ -24,7 +24,7 @@ use std::sync::mpsc::channel;
 fn runtime() {
     let engine = JSEngine::init().unwrap();
     assert!(JSEngine::init().is_err());
-    let runtime = Runtime::new(engine);
+    let runtime = Runtime::new(engine.handle());
     unsafe {
         let cx = runtime.cx();
         let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;

tests/runtime_no_outlive.rs

@@ -10,7 +10,7 @@ use mozjs::rust::{JSEngine, Runtime};
 #[should_panic]
 fn runtime() {
     let engine = JSEngine::init().unwrap();
-    let runtime = Runtime::new(engine);
+    let runtime = Runtime::new(engine.handle());
     let _parent = runtime.prepare_for_new_child();
     drop(runtime);
 }

tests/stack_limit.rs

@@ -15,7 +15,7 @@ use std::ptr;
 #[test]
 fn stack_limit() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
 
     unsafe {

tests/typedarray.rs

@@ -21,7 +21,7 @@ use std::ptr;
 #[test]
 fn typedarray() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
 
     unsafe {

tests/typedarray_panic.rs

@@ -20,7 +20,7 @@ use std::ptr;
 #[should_panic]
 fn typedarray_update_panic() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
     let options = RealmOptions::default();
 

tests/vec_conversion.rs

@@ -21,7 +21,7 @@ use std::ptr;
 #[test]
 fn vec_conversion() {
     let engine = JSEngine::init().unwrap();
-    let rt = Runtime::new(engine);
+    let rt = Runtime::new(engine.handle());
     let cx = rt.cx();
 
     let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;