From e46537e6c2a990ffa161b27ea73324f3e26337bd Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 10 Oct 2018 12:50:54 -0700 Subject: [PATCH] Ensure that `JsValue` isn't considered `Send` The `JsValue` type wraps a slab/heap of js objects which is managed by the wasm-bindgen shim, and everything here is not actually able to cross any thread boundaries. When wasm actually has threads, for example, each thread will have to have its own slab of objects generated by wasm-bindgen, and indices in one slab aren't valid in any other slabs. This is technically a breaking change because `JsValue` was previously `Send` and `Sync`, but I'm hoping that in practice this isn't actually a breaking change because nothing in wasm can be using threads which in theory shouldn't activate the `Send` and/or `Sync` bounds. --- crates/backend/src/codegen.rs | 6 +-- src/closure.rs | 2 +- src/convert/impls.rs | 4 +- src/lib.rs | 90 ++++++++++++++++------------------- 4 files changed, 46 insertions(+), 56 deletions(-) diff --git a/crates/backend/src/codegen.rs b/crates/backend/src/codegen.rs index 644210de4..18143cb1c 100644 --- a/crates/backend/src/codegen.rs +++ b/crates/backend/src/codegen.rs @@ -1042,11 +1042,9 @@ impl ToTokens for ast::ImportStatic { fn init() -> #ty { panic!("cannot access imported statics on non-wasm targets") } - static mut _VAL: ::wasm_bindgen::__rt::core::cell::UnsafeCell> = - ::wasm_bindgen::__rt::core::cell::UnsafeCell::new(None); + thread_local!(static _VAL: #ty = init();); ::wasm_bindgen::JsStatic { - __inner: unsafe { &_VAL }, - __init: init, + __inner: &_VAL, } }; }).to_tokens(into); diff --git a/src/closure.rs b/src/closure.rs index 49e68b309..16c080879 100644 --- a/src/closure.rs +++ b/src/closure.rs @@ -214,7 +214,7 @@ impl Closure }; Closure { - js: ManuallyDrop::new(JsValue { idx }), + js: ManuallyDrop::new(JsValue::_new(idx)), data: ManuallyDrop::new(data), } } diff --git a/src/convert/impls.rs b/src/convert/impls.rs index 0c17f22e5..ec66a9aa4 100644 --- a/src/convert/impls.rs +++ b/src/convert/impls.rs @@ -337,7 +337,7 @@ impl FromWasmAbi for JsValue { #[inline] unsafe fn from_abi(js: u32, _extra: &mut Stack) -> JsValue { - JsValue { idx: js } + JsValue::_new(js) } } @@ -356,7 +356,7 @@ impl RefFromWasmAbi for JsValue { #[inline] unsafe fn ref_from_abi(js: u32, _extra: &mut Stack) -> Self::Anchor { - ManuallyDrop::new(JsValue { idx: js }) + ManuallyDrop::new(JsValue::_new(js)) } } diff --git a/src/lib.rs b/src/lib.rs index 2973233c7..2b89cb918 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,8 +16,8 @@ extern crate serde_json; extern crate wasm_bindgen_macro; -use core::cell::UnsafeCell; use core::fmt; +use core::marker; use core::mem; use core::ops::{Deref, DerefMut}; use core::ptr; @@ -64,6 +64,7 @@ if_std! { /// but for now it may be slightly slow. pub struct JsValue { idx: u32, + _marker: marker::PhantomData<*mut u8>, // not at all threadsafe } const JSIDX_UNDEFINED: u32 = 0; @@ -74,18 +75,36 @@ const JSIDX_RESERVED: u32 = 8; impl JsValue { /// The `null` JS value constant. - pub const NULL: JsValue = JsValue { idx: JSIDX_NULL }; + pub const NULL: JsValue = JsValue { + idx: JSIDX_NULL, + _marker: marker::PhantomData, + }; /// The `undefined` JS value constant. pub const UNDEFINED: JsValue = JsValue { idx: JSIDX_UNDEFINED, + _marker: marker::PhantomData, }; + /// The `true` JS value constant. - pub const TRUE: JsValue = JsValue { idx: JSIDX_TRUE }; + pub const TRUE: JsValue = JsValue { + idx: JSIDX_TRUE, + _marker: marker::PhantomData, + }; /// The `false` JS value constant. - pub const FALSE: JsValue = JsValue { idx: JSIDX_FALSE }; + pub const FALSE: JsValue = JsValue { + idx: JSIDX_FALSE, + _marker: marker::PhantomData, + }; + + fn _new(idx: u32) -> JsValue { + JsValue { + idx, + _marker: marker::PhantomData, + } + } /// Creates a new JS value which is a string. /// @@ -94,9 +113,7 @@ impl JsValue { #[inline] pub fn from_str(s: &str) -> JsValue { unsafe { - JsValue { - idx: __wbindgen_string_new(s.as_ptr(), s.len()), - } + JsValue::_new(__wbindgen_string_new(s.as_ptr(), s.len())) } } @@ -107,9 +124,7 @@ impl JsValue { #[inline] pub fn from_f64(n: f64) -> JsValue { unsafe { - JsValue { - idx: __wbindgen_number_new(n), - } + JsValue::_new(__wbindgen_number_new(n)) } } @@ -142,9 +157,7 @@ impl JsValue { unsafe { let ptr = description.map(|s| s.as_ptr()).unwrap_or(ptr::null()); let len = description.map(|s| s.len()).unwrap_or(0); - JsValue { - idx: __wbindgen_symbol_new(ptr, len), - } + JsValue::_new(__wbindgen_symbol_new(ptr, len)) } } @@ -170,9 +183,7 @@ impl JsValue { { let s = serde_json::to_string(t)?; unsafe { - Ok(JsValue { - idx: __wbindgen_json_parse(s.as_ptr(), s.len()), - }) + Ok(JsValue::_new(__wbindgen_json_parse(s.as_ptr(), s.len()))) } } @@ -486,7 +497,7 @@ impl Clone for JsValue { fn clone(&self) -> JsValue { unsafe { let idx = __wbindgen_object_clone_ref(self.idx); - JsValue { idx } + JsValue::_new(idx) } } } @@ -552,43 +563,26 @@ impl Drop for JsValue { /// /// This type implements `Deref` to the inner type so it's typically used as if /// it were `&T`. +#[cfg(feature = "std")] pub struct JsStatic { #[doc(hidden)] - pub __inner: &'static UnsafeCell>, - #[doc(hidden)] - pub __init: fn() -> T, + pub __inner: &'static std::thread::LocalKey, } -unsafe impl Sync for JsStatic {} -unsafe impl Send for JsStatic {} - +#[cfg(feature = "std")] impl Deref for JsStatic { type Target = T; fn deref(&self) -> &T { + // We know that our tls key is never overwritten after initialization, + // so it should be safe (on that axis at least) to hand out a reference + // that lives longer than the closure below. + // + // FIXME: this is not sound if we ever implement thread exit hooks on + // wasm, as the pointer will eventually be invalidated but you can get + // `&'static T` from this interface. We... probably need to deprecate + // and/or remove this interface nowadays. unsafe { - // Ideally we want to use `get_or_insert_with` here but - // unfortunately that has subpar codegen for now. - // - // If we get past the `Some` branch here LLVM statically - // knows that we're `None`, but the after the call to the `__init` - // function LLVM can no longer know this because `__init` could - // recursively call this function again (aka if JS came back to Rust - // and Rust referenced this static). - // - // We know, however, that cannot happen. As a result we can - // conclude that even after the call to `__init` our `ptr` still - // points to `None` (and a debug assertion to this effect). Then - // using `ptr::write` should tell rustc to not run destuctors - // (as one isn't there) and this should tighten up codegen for - // `JsStatic` a bit as well. - let ptr = self.__inner.get(); - if let Some(ref t) = *ptr { - return t; - } - let init = Some((self.__init)()); - debug_assert!((*ptr).is_none()); - ptr::write(ptr, init); - (*ptr).as_ref().unwrap() + self.__inner.with(|ptr| &*(ptr as *const T)) } } } @@ -642,9 +636,7 @@ pub fn throw_val(s: JsValue) -> ! { /// Returns a handle to this wasm instance's `WebAssembly.Memory` pub fn memory() -> JsValue { unsafe { - JsValue { - idx: __wbindgen_memory(), - } + JsValue::_new(__wbindgen_memory()) } }