Auto merge of #450 - jdm:parent-refactor, r=asajeffrey

Make runtime creation safe

The fundamental problem exposed in servo/servo#22342 is that our concept of a parent runtime did not match reality. Using the first JSContext's runtime as the global parent for all subsequent contexts only makes sense if that JSContext outlives every other context. This is not guaranteed, leading to crashes when trying to use those contexts if the first context (and therefore its runtime) was destroyed.

The new design incorporates several changes for safer, more clear context and runtime management:
* in order to create a new context, either a handle to an initialized JS engine is required or a handle to an existing runtime
* all runtimes track outstanding handles that have been created, and assert if a runtime is destroyed before all of its child runtimes
* overall initialization and shutdown of the engine is controlled by the lifetime of a JSEngine value, so creating a Runtime value is now infallible

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

Cargo.toml

@@ -2,7 +2,7 @@
 name = "mozjs"
 description = "Rust bindings to the Mozilla SpiderMonkey JavaScript engine."
 repository = "https://github.com/servo/rust-mozjs"
-version = "0.9.5"
+version = "0.10.0"
 authors = ["The Servo Project Developers"]
 build = "build.rs"
 license = "MPL-2.0"
@@ -29,6 +29,8 @@ name = "rooting"
 [[test]]
 name = "runtime"
 [[test]]
+name = "runtime_no_outlive"
+[[test]]
 name = "typedarray"
 [[test]]
 name = "typedarray_panic"
@@ -42,7 +44,6 @@ doctest = false
 
 [features]
 debugmozjs = ['mozjs_sys/debugmozjs']
-init_once = []
 
 [dependencies]
 lazy_static = "1"

src/rust.rs

@@ -4,7 +4,6 @@
 
 //! Rust wrappers around the raw JS apis
 
-
 use libc::{size_t, c_uint};
 
 use mozjs_sys::jsgc::CustomAutoRooterVFTable;
@@ -23,7 +22,7 @@ use std::ops::{Deref, DerefMut};
 use std::os::raw::c_void;
 use std::cell::Cell;
 use std::marker::PhantomData;
-use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicUsize, Ordering};
+use std::sync::{Arc, Mutex};
 
 use consts::{JSCLASS_RESERVED_SLOTS_MASK, JSCLASS_GLOBAL_SLOT_COUNT};
 use consts::{JSCLASS_IS_DOMJSCLASS, JSCLASS_IS_GLOBAL};
@@ -124,16 +123,94 @@ impl ToResult for bool {
 
 thread_local!(static CONTEXT: Cell<*mut JSContext> = Cell::new(ptr::null_mut()));
 
+#[derive(PartialEq)]
+enum EngineState {
+    Uninitialized,
+    InitFailed,
+    Initialized,
+    ShutDown,
+}
+
 lazy_static! {
-    static ref PARENT: AtomicPtr<JSRuntime> = AtomicPtr::new(ptr::null_mut());
-    static ref OUTSTANDING_RUNTIMES: AtomicUsize = AtomicUsize::new(0);
-    static ref SHUT_DOWN: AtomicBool = AtomicBool::new(false);
-    static ref JS_INIT_CALLED: AtomicBool = AtomicBool::new(false);
+    static ref ENGINE_STATE: Mutex<EngineState> = Mutex::new(EngineState::Uninitialized);
+}
+
+#[derive(Debug)]
+pub enum JSEngineError {
+    AlreadyInitialized,
+    AlreadyShutDown,
+    InitFailed,
+}
+
+/// 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(());
+
+impl JSEngine {
+    /// Initialize the JS engine to prepare for creating new JS runtimes.
+    pub fn init() -> Result<Arc<JSEngine>, JSEngineError> {
+        let mut state = ENGINE_STATE.lock().unwrap();
+        match *state {
+            EngineState::Initialized => return Err(JSEngineError::AlreadyInitialized),
+            EngineState::InitFailed => return Err(JSEngineError::InitFailed),
+            EngineState::ShutDown => return Err(JSEngineError::AlreadyShutDown),
+            EngineState::Uninitialized => (),
+        }
+        if unsafe { !JS_Init() } {
+            *state = EngineState::InitFailed;
+            Err(JSEngineError::InitFailed)
+        } else {
+            *state = EngineState::Initialized;
+            Ok(Arc::new(JSEngine(())))
+        }
+    }
+}
+
+/// Shut down the JS engine, invalidating any existing runtimes and preventing
+/// any new ones from being created.
+impl Drop for JSEngine {
+    fn drop(&mut self) {
+        let mut state = ENGINE_STATE.lock().unwrap();
+        if *state == EngineState::Initialized {
+            *state = EngineState::ShutDown;
+            unsafe {
+                JS_ShutDown();
+            }
+        }
+    }
+}
+
+/// A handle to a Runtime that will be used to create a new runtime in another
+/// thread. This handle and the new runtime must be destroyed before the original
+/// runtime can be dropped.
+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>,
+    /// The number of children of the runtime that created this ParentRuntime value.
+    children_of_parent: Arc<()>,
 }
+unsafe impl Send for ParentRuntime {}
 
 /// A wrapper for the `JSContext` structure in SpiderMonkey.
 pub struct Runtime {
+    /// Raw pointer to the underlying SpiderMonkey context.
     cx: *mut JSContext,
+    /// The engine that this runtime is associated with.
+    engine: Arc<JSEngine>,
+    /// 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
+    /// is dropped.
+    _parent_child_count: Option<Arc<()>>,
+    /// The strong references to this value represent the number of child runtimes
+    /// that have been created using this Runtime as a parent. Since Runtime values
+    /// must be associated with a particular thread, we cannot simply use Arc<Runtime>
+    /// to represent the resulting ownership graph and risk destroying a Runtime on
+    /// the wrong thread.
+    outstanding_children: Arc<()>,
 }
 
 impl Runtime {
@@ -147,76 +224,85 @@ impl Runtime {
     }
 
     /// Creates a new `JSContext`.
-    pub fn new() -> Result<Runtime, ()> {
-        unsafe {
-            if SHUT_DOWN.load(Ordering::SeqCst) {
-                return Err(());
-            }
-
-            let outstanding = OUTSTANDING_RUNTIMES.fetch_add(1, Ordering::SeqCst);
-
-            let js_context = if outstanding == 0 {
-                // We are creating the first JSContext, so we need to initialize
-                // the runtime.
-                if cfg!(not(feature = "init_once")) || !JS_INIT_CALLED.load(Ordering::SeqCst) {
-                    assert!(JS_Init());
-                    JS_INIT_CALLED.store(true, Ordering::SeqCst);
-                }
-                let js_context = JS_NewContext(default_heapsize, ChunkSize as u32, ptr::null_mut());
-                let parent_runtime = JS_GetRuntime(js_context);
-                assert!(!parent_runtime.is_null());
-                let old_runtime = PARENT.compare_and_swap(ptr::null_mut(), parent_runtime, Ordering::SeqCst);
-                assert!(old_runtime.is_null());
-                // TODO: should we use the internal job queues or not?
-                // assert!(UseInternalJobQueues(js_context, false));
-                js_context
-            } else {
-                let parent_runtime = PARENT.load(Ordering::SeqCst);
-                assert!(!parent_runtime.is_null());
-                JS_NewContext(default_heapsize, ChunkSize as u32, parent_runtime)
-            };
-
-            assert!(!js_context.is_null());
-
-            // Unconstrain the runtime's threshold on nominal heap size, to avoid
-            // triggering GC too often if operating continuously near an arbitrary
-            // finite threshold. This leaves the maximum-JS_malloc-bytes threshold
-            // still in effect to cause periodical, and we hope hygienic,
-            // last-ditch GCs from within the GC's allocator.
-            JS_SetGCParameter(
-                js_context, JSGCParamKey::JSGC_MAX_BYTES, u32::MAX);
-
-            JS_SetNativeStackQuota(
-                js_context,
-                STACK_QUOTA,
-                STACK_QUOTA - SYSTEM_CODE_BUFFER,
-                STACK_QUOTA - SYSTEM_CODE_BUFFER - TRUSTED_SCRIPT_BUFFER);
+    pub fn new(engine: Arc<JSEngine>) -> Runtime {
+        unsafe { Self::create(engine, None) }
+    }
+
+    /// Signal that a new child runtime will be created in the future, and ensure
+    /// that this runtime will not allow itself to be destroyed before the new
+    /// child runtime. Returns a handle that can be passed to `create_with_parent`
+    /// in order to create a new runtime on another thread that is associated with
+    /// this runtime.
+    pub fn prepare_for_new_child(&self) -> ParentRuntime {
+        ParentRuntime {
+            parent: self.rt(),
+            engine: self.engine.clone(),
+            children_of_parent: self.outstanding_children.clone(),
+        }
+    }
 
-            CONTEXT.with(|context| {
-                assert!(context.get().is_null());
-                context.set(js_context);
-            });
+    /// Creates a new `JSContext` with a parent runtime. If the parent does not outlive
+    /// the new runtime, its destructor will assert.
+    ///
+    /// Unsafety:
+    /// If panicking does not abort the program, any threads with child runtimes will
+    /// continue executing after the thread with the parent runtime panics, but they
+    /// will be in an invalid and undefined state.
+    pub unsafe fn create_with_parent(parent: ParentRuntime) -> Runtime {
+        Self::create(parent.engine.clone(), Some(parent))
+    }
+
+    unsafe fn create(engine: Arc<JSEngine>, parent: Option<ParentRuntime>) -> Runtime {
+        let parent_runtime = parent.as_ref().map_or(
+            ptr::null_mut(),
+            |r| r.parent,
+        );
+        let js_context = JS_NewContext(default_heapsize, ChunkSize as u32, parent_runtime);
+        assert!(!js_context.is_null());
+
+        // Unconstrain the runtime's threshold on nominal heap size, to avoid
+        // triggering GC too often if operating continuously near an arbitrary
+        // finite threshold. This leaves the maximum-JS_malloc-bytes threshold
+        // still in effect to cause periodical, and we hope hygienic,
+        // last-ditch GCs from within the GC's allocator.
+        JS_SetGCParameter(
+            js_context, JSGCParamKey::JSGC_MAX_BYTES, u32::MAX);
+
+        JS_SetNativeStackQuota(
+            js_context,
+            STACK_QUOTA,
+            STACK_QUOTA - SYSTEM_CODE_BUFFER,
+            STACK_QUOTA - SYSTEM_CODE_BUFFER - TRUSTED_SCRIPT_BUFFER);
+
+        CONTEXT.with(|context| {
+            assert!(context.get().is_null());
+            context.set(js_context);
+        });
 
-            InitSelfHostedCode(js_context);
+        InitSelfHostedCode(js_context);
 
-            let contextopts = ContextOptionsRef(js_context);
-            (*contextopts).set_baseline_(true);
-            (*contextopts).set_ion_(true);
-            (*contextopts).set_nativeRegExp_(true);
+        let contextopts = ContextOptionsRef(js_context);
+        (*contextopts).set_baseline_(true);
+        (*contextopts).set_ion_(true);
+        (*contextopts).set_nativeRegExp_(true);
 
-            SetWarningReporter(js_context, Some(report_warning));
+        SetWarningReporter(js_context, Some(report_warning));
 
-            JS_BeginRequest(js_context);
+        JS_BeginRequest(js_context);
 
-            Ok(Runtime {
-                cx: js_context,
-            })
+        Runtime {
+            engine,
+            _parent_child_count: parent.map(|p| p.children_of_parent),
+            cx: js_context,
+            outstanding_children: Arc::new(()),
         }
     }
 
     /// Returns the `JSRuntime` object.
     pub fn rt(&self) -> *mut JSRuntime {
-        PARENT.load(Ordering::SeqCst)
+        unsafe {
+            JS_GetRuntime(self.cx)
+        }
     }
 
     /// Returns the `JSContext` object.
@@ -258,6 +344,9 @@ impl Runtime {
 
 impl Drop for Runtime {
     fn drop(&mut self) {
+        assert_eq!(Arc::strong_count(&self.outstanding_children),
+                   1,
+                   "This runtime still has live children.");
         unsafe {
             JS_EndRequest(self.cx);
             JS_DestroyContext(self.cx);
@@ -266,14 +355,6 @@ impl Drop for Runtime {
                 assert_eq!(context.get(), self.cx);
                 context.set(ptr::null_mut());
             });
-
-            if OUTSTANDING_RUNTIMES.fetch_sub(1, Ordering::SeqCst) == 1 {
-                PARENT.store(ptr::null_mut(), Ordering::SeqCst);
-                if cfg!(not(feature = "init_once")) {
-                    SHUT_DOWN.store(true, Ordering::SeqCst);
-                    JS_ShutDown();
-                }
-            }
         }
     }
 }

tests/callback.rs

@@ -17,15 +17,16 @@ use mozjs::jsapi::JS_ReportErrorASCII;
 use mozjs::jsapi::OnNewGlobalHookOption;
 use mozjs::jsapi::Value;
 use mozjs::jsval::UndefinedValue;
-use mozjs::rust::{Runtime, SIMPLE_GLOBAL_CLASS};
+use mozjs::rust::{JSEngine, Runtime, SIMPLE_GLOBAL_CLASS};
 
 use std::ffi::CStr;
 use std::ptr;
 use std::str;
 
 #[test]
 fn callback() {
-    let runtime = Runtime::new().unwrap();
+    let engine = JSEngine::init().unwrap();
+    let runtime = Runtime::new(engine);
     let context = runtime.cx();
     let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;
     let c_option = CompartmentOptions::default();

tests/capture_stack.rs

@@ -16,13 +16,14 @@ use mozjs::jsapi::OnNewGlobalHookOption;
 use mozjs::jsapi::StackFormat;
 use mozjs::jsapi::Value;
 use mozjs::jsval::UndefinedValue;
-use mozjs::rust::{Runtime, SIMPLE_GLOBAL_CLASS};
+use mozjs::rust::{JSEngine, Runtime, SIMPLE_GLOBAL_CLASS};
 
 use std::ptr;
 
 #[test]
 fn capture_stack() {
-    let runtime = Runtime::new().unwrap();
+    let engine = JSEngine::init().unwrap();
+    let runtime = Runtime::new(engine);
     let context = runtime.cx();
     let h_option = OnNewGlobalHookOption::FireOnNewGlobalHook;
     let c_option = CompartmentOptions::default();

tests/custom_auto_rooter.rs

@@ -5,6 +5,7 @@
 extern crate mozjs;
 use mozjs::jsapi::JSTracer;
 use mozjs::jsapi::JS_GC;
+use mozjs::rust::JSEngine;
 use mozjs::rust::Runtime;
 use mozjs::rust::CustomTrace;
 use mozjs::rust::CustomAutoRooter;
@@ -31,7 +32,8 @@ unsafe impl CustomTrace for TraceCheck {
 /// by checking if appropriate virtual trace function was called.
 #[test]
 fn virtual_trace_called() {
-    let rt = Runtime::new().unwrap();
+    let engine = JSEngine::init().unwrap();
+    let rt = Runtime::new(engine);
     let cx = rt.cx();
 
     let mut rooter = CustomAutoRooter::new(TraceCheck::new());

tests/custom_auto_rooter_macro.rs

@@ -6,6 +6,7 @@
 extern crate mozjs;
 use mozjs::jsapi::JSTracer;
 use mozjs::jsapi::JS_GC;
+use mozjs::rust::JSEngine;
 use mozjs::rust::Runtime;
 use mozjs::rust::CustomTrace;
 use std::cell::Cell;
@@ -28,7 +29,8 @@ unsafe impl CustomTrace for TraceCheck {
 
 #[test]
 fn custom_auto_rooter_macro() {
-    let rt = Runtime::new().unwrap();
+    let engine = JSEngine::init().unwrap();
+    let rt = Runtime::new(engine);
     let cx = rt.cx();
 
     auto_root!(in(cx) let vec = vec![TraceCheck::new(), TraceCheck::new()]);