Skip to content
This repository has been archived by the owner on Nov 12, 2022. It is now read-only.

Replace return_address usage in Rooted with a stack guard and a rooted! macro. #272

Merged
merged 2 commits into from
Jun 27, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions src/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use jsapi::JSPROP_ENUMERATE;
use jsapi::{JSContext, JSObject, JSString, HandleValue, MutableHandleValue};
use jsapi::{JS_NewUCStringCopyN, JS_StringHasLatin1Chars, JS_WrapValue};
use jsapi::{JS_GetLatin1StringCharsAndLength, JS_GetTwoByteStringCharsAndLength};
use jsapi::{JS_NewArrayObject1, JS_DefineElement, RootedValue, RootedObject};
use jsapi::{JS_NewArrayObject1, JS_DefineElement, RootedObject};
use jsapi::{ForOfIterator, ForOfIterator_NonIterableBehavior};
use jsval::{BooleanValue, Int32Value, NullValue, UInt32Value, UndefinedValue};
use jsval::{JSVal, ObjectValue, ObjectOrNullValue, StringValue};
Expand Down Expand Up @@ -473,10 +473,10 @@ impl<T: FromJSValConvertible> FromJSValConvertible for Option<T> {
// https://heycam.github.io/webidl/#es-sequence
impl<T: ToJSValConvertible> ToJSValConvertible for Vec<T> {
unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
let js_array = RootedObject::new(cx, JS_NewArrayObject1(cx, self.len() as libc::size_t));
rooted!(in(cx) let js_array = JS_NewArrayObject1(cx, self.len() as libc::size_t));
assert!(!js_array.handle().is_null());

let mut val = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut val = UndefinedValue());
for (index, obj) in self.iter().enumerate() {
obj.to_jsval(cx, val.handle_mut());

Expand All @@ -488,6 +488,33 @@ impl<T: ToJSValConvertible> ToJSValConvertible for Vec<T> {
}
}

/// Rooting guard for the iterator field of ForOfIterator.
/// Behaves like RootedGuard (roots on creation, unroots on drop),
/// but borrows and allows access to the whole ForOfIterator, so
/// that methods on ForOfIterator can still be used through it.
struct ForOfIteratorGuard<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documentation on this? (explaining why it is necessary perhaps)

root: &'a mut ForOfIterator
}

impl<'a> ForOfIteratorGuard<'a> {
fn new(cx: *mut JSContext, root: &'a mut ForOfIterator) -> Self {
unsafe {
root.iterator.add_to_root_stack(cx);
}
ForOfIteratorGuard {
root: root
}
}
}

impl<'a> Drop for ForOfIteratorGuard<'a> {
fn drop(&mut self) {
unsafe {
self.root.iterator.remove_from_root_stack();
}
}
}

impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T> {
type Config = C;

Expand All @@ -497,9 +524,11 @@ impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T
-> Result<Vec<T>, ()> {
let mut iterator = ForOfIterator {
cx_: cx,
iterator: RootedObject::new(cx, ptr::null_mut()),
iterator: RootedObject::new_unrooted(ptr::null_mut()),
index: ::std::u32::MAX, // NOT_ARRAY
};
let mut iterator = ForOfIteratorGuard::new(cx, &mut iterator);
let iterator = &mut *iterator.root;

if !iterator.init(value, ForOfIterator_NonIterableBehavior::ThrowOnNonIterable) {
return Err(())
Expand All @@ -509,7 +538,7 @@ impl<C: Clone, T: FromJSValConvertible<Config=C>> FromJSValConvertible for Vec<T

loop {
let mut done = false;
let mut val = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut val = UndefinedValue());
if !iterator.next(val.handle_mut(), &mut done) {
return Err(())
}
Expand Down
6 changes: 4 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#![crate_name = "js"]
#![crate_type = "rlib"]

#![feature(core_intrinsics)]
#![feature(filling_drop)]
#![feature(link_args)]
#![feature(unsafe_no_drop_flag)]
Expand Down Expand Up @@ -78,12 +77,15 @@ pub mod jsapi {
include!("jsapi_linux_32_debug.rs");
}

#[macro_use]
pub mod rust;

mod consts;
pub mod conversions;
pub mod error;
pub mod glue;
pub mod jsval;
pub mod rust;


pub use consts::*;

Expand Down
155 changes: 82 additions & 73 deletions src/rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

//! Rust wrappers around the raw JS apis

use libc::{size_t, c_uint, c_char, ptrdiff_t};
use libc::{size_t, c_uint, c_char};
use heapsize::HeapSizeOf;
use std::char;
use std::ffi;
Expand All @@ -13,7 +13,6 @@ use std::slice;
use std::mem;
use std::u32;
use std::default::Default;
use std::intrinsics::return_address;
use std::ops::{Deref, DerefMut};
use std::cell::UnsafeCell;
use std::marker::PhantomData;
Expand All @@ -26,7 +25,6 @@ use jsapi::{JS_SetErrorReporter, Evaluate2, JSErrorReport};
use jsapi::{JS_SetGCParameter, JSGCParamKey};
use jsapi::{Heap, HeapObjectPostBarrier, HeapValuePostBarrier};
use jsapi::{ContextFriendFields};
use jsapi::{CustomAutoRooter, AutoGCRooter, _vftable_CustomAutoRooter, AutoGCRooter_jspubtd_h_unnamed_1};
use jsapi::{Rooted, Handle, MutableHandle, MutableHandleBase, RootedBase};
use jsapi::{MutableHandleValue, HandleValue, HandleObject, HandleBase};
use jsapi::AutoObjectVector;
Expand Down Expand Up @@ -270,41 +268,103 @@ impl RootKind for Value {
fn rootKind() -> jsapi::RootKind { jsapi::RootKind::Value }
}

impl<T: RootKind + Copy> Rooted<T> {
pub fn new_with_addr(cx: *mut JSContext, initial: T, addr: *const u8) -> Rooted<T> {
let ctxfriend: &mut ContextFriendFields = unsafe {
mem::transmute(cx)
};

let kind = T::rootKind() as usize;
let root = Rooted::<T> {
impl<T> Rooted<T> {
pub fn new_unrooted(initial: T) -> Rooted<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut reaction to seeing an unrooted "Rooted<T>" is terror: it means you could pass around references to Rooted<T> (what would be [Mutable]Handle<T> in js/) that are not in fact rooted.

Is there any way we could use the type system to differentiate between things that are actually rooted vs those that are not (hashtag session types?), or maybe even just rename these types? It seems to me that Rooted<T> is really Rootable<T> and RootedGuard<T> is the real Rooted<T>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot create a Handle safely from it. A reference to Rooted means nothing by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to use better names but Rooted is an FFI type (which is required because it's a node in a linked list) so we cannot do too much about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the initial assignment to RootedGuard::new()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose so? But what do you put there in Rooted::new_unrooted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That trait seems like a good choice to have as a bound in the struct itself.
Alright, other changes suggested on IRC were to have struct Rootable<T>(Rooted<T>); which could only be created empty and not accessed, and it's what RootedGuard would accept.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do the wrapper that cannot be accessed and fill it with initial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, that wrapper would cause issues with ForOfIterator, although that's internal to rust-mozjs and perhaps could be accommodated for with a pub(crate) unsafe method.

Rooted {
_base: RootedBase { _phantom0: PhantomData },
stack: &mut ctxfriend.roots.stackRoots_[kind] as *mut _ as *mut _,
prev: ctxfriend.roots.stackRoots_[kind] as *mut _,
stack: ptr::null_mut(),
prev: ptr::null_mut(),
ptr: initial,
};
}
}

pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) where T: RootKind {
let ctxfriend: &mut ContextFriendFields = mem::transmute(cx);

let kind = T::rootKind() as usize;
self.stack = &mut ctxfriend.roots.stackRoots_[kind] as *mut _ as *mut _;
self.prev = ctxfriend.roots.stackRoots_[kind] as *mut _;

ctxfriend.roots.stackRoots_[kind] = unsafe { mem::transmute(addr) };
root
ctxfriend.roots.stackRoots_[kind] = self as *mut _ as usize as _;
}

pub fn new(cx: *mut JSContext, initial: T) -> Rooted<T> {
Rooted::new_with_addr(cx, initial, unsafe { return_address() })
pub unsafe fn remove_from_root_stack(&mut self) {
assert!(*self.stack == mem::transmute(&*self));
*self.stack = self.prev;
}
}

pub fn handle(&self) -> Handle<T> {
/// Rust API for keeping a Rooted value in the context's root stack.
/// Example usage: `rooted!(in(cx) let x = UndefinedValue());`.
/// `RootedGuard::new` also works, but the macro is preferred.
pub struct RootedGuard<'a, T: 'a> {
root: &'a mut Rooted<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that the Rooted<T> can safely be added to the root stack because it cannot be moved by the compiler while there is an active &mut reference to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, borrowed lvalues cannot be moved from in Rust. That's a core guarantee.

}

impl<'a, T> RootedGuard<'a, T> {
pub fn new(cx: *mut JSContext, root: &'a mut Rooted<T>) -> Self where T: RootKind {
unsafe {
Handle::from_marked_location(&self.ptr)
root.add_to_root_stack(cx);
}
RootedGuard {
root: root
}
}

pub fn handle(&self) -> Handle<T> where T: Copy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the Copy bound here and on handle_mut?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods to create those types requires it.

unsafe {
Handle::from_marked_location(&self.root.ptr)
}
}

pub fn handle_mut(&mut self) -> MutableHandle<T> where T: Copy {
unsafe {
MutableHandle::from_marked_location(&mut self.root.ptr)
}
}

pub fn get(&self) -> T where T: Copy {
self.root.ptr
}

pub fn set(&mut self, v: T) {
self.root.ptr = v;
}
}

impl<'a, T> Deref for RootedGuard<'a, T> {
type Target = T;
fn deref(&self) -> &T {
&self.root.ptr
}
}

impl<'a, T> DerefMut for RootedGuard<'a, T> {
fn deref_mut(&mut self) -> &mut T {
&mut self.root.ptr
}
}

pub fn handle_mut(&mut self) -> MutableHandle<T> {
impl<'a, T> Drop for RootedGuard<'a, T> {
fn drop(&mut self) {
unsafe {
MutableHandle::from_marked_location(&mut self.ptr)
self.root.remove_from_root_stack();
}
}
}

#[macro_export]
macro_rules! rooted {
(in($cx:expr) let $name:ident = $init:expr) => {
let mut __root = $crate::jsapi::Rooted::new_unrooted($init);
let $name = $crate::rust::RootedGuard::new($cx, &mut __root);
};
(in($cx:expr) let mut $name:ident = $init:expr) => {
let mut __root = $crate::jsapi::Rooted::new_unrooted($init);
let mut $name = $crate::rust::RootedGuard::new($cx, &mut __root);
}
}

impl<T: Copy> Handle<T> {
pub fn get(&self) -> T {
unsafe { *self.ptr }
Expand Down Expand Up @@ -391,57 +451,6 @@ impl<T: Copy> MutableHandle<T> {
}
}

impl<T> Drop for Rooted<T> {
fn drop(&mut self) {
unsafe {
if self.stack as usize == mem::POST_DROP_USIZE {
return;
}
assert!(*self.stack == mem::transmute(&*self));
*self.stack = self.prev;
}
}
}

impl CustomAutoRooter {
pub fn new(cx: *mut JSContext, vftable: &'static _vftable_CustomAutoRooter)
-> CustomAutoRooter {
CustomAutoRooter::new_with_addr(cx, vftable, unsafe { return_address() })
}

pub fn new_with_addr(cx: *mut JSContext, vftable: &'static _vftable_CustomAutoRooter, addr: *const u8) -> CustomAutoRooter {
let ctxfriend: &mut ContextFriendFields = unsafe {
&mut *(cx as *mut ContextFriendFields)
};

let rooter = CustomAutoRooter {
_vftable: vftable as *const _,
_base: AutoGCRooter {
down: ctxfriend.roots.autoGCRooters_,
tag_: AutoGCRooter_jspubtd_h_unnamed_1::CUSTOM as ptrdiff_t,
stackTop: &mut ctxfriend.roots.autoGCRooters_ as *mut _,
}
};

ctxfriend.roots.autoGCRooters_ = unsafe {
(addr as *const *const u8).offset(1) as *const AutoGCRooter as *mut _
};
rooter
}
}

impl Drop for CustomAutoRooter {
fn drop(&mut self) {
if self._base.stackTop as usize == mem::POST_DROP_USIZE {
return;
}
unsafe {
assert!(*self._base.stackTop == mem::transmute(&self._base));
*self._base.stackTop = self._base.down;
}
}
}

impl Default for jsid {
fn default() -> jsid { JSID_VOID }
}
Expand Down
9 changes: 4 additions & 5 deletions tests/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#[macro_use]
extern crate js;
extern crate libc;

Expand All @@ -14,8 +15,6 @@ use js::jsapi::JS_EncodeStringToUTF8;
use js::jsapi::JS_NewGlobalObject;
use js::jsapi::JS_ReportError;
use js::jsapi::OnNewGlobalHookOption;
use js::jsapi::Rooted;
use js::jsapi::RootedValue;
use js::jsapi::Value;
use js::jsval::UndefinedValue;
use js::rust::{Runtime, SIMPLE_GLOBAL_CLASS};
Expand All @@ -33,14 +32,14 @@ fn callback() {

unsafe {
let global = JS_NewGlobalObject(context, &SIMPLE_GLOBAL_CLASS, ptr::null_mut(), h_option, &c_option);
let global_root = Rooted::new(context, global);
rooted!(in(context) let global_root = global);
let global = global_root.handle();
let _ac = JSAutoCompartment::new(context, global.get());
let function = JS_DefineFunction(context, global, b"puts\0".as_ptr() as *const libc::c_char,
Some(puts), 1, 0);
assert!(!function.is_null());
let javascript = "puts('Test Iñtërnâtiônàlizætiøn ┬─┬ノ( º _ ºノ) ');";
let mut rval = RootedValue::new(runtime.cx(), UndefinedValue());
rooted!(in(context) let mut rval = UndefinedValue());
let _ = runtime.evaluate_script(global, javascript, "test.js", 0, rval.handle_mut());
}
}
Expand All @@ -55,7 +54,7 @@ unsafe extern "C" fn puts(context: *mut JSContext, argc: u32, vp: *mut Value) ->

let arg = args.get(0);
let js = js::rust::ToString(context, arg);
let message_root = Rooted::new(context, js);
rooted!(in(context) let message_root = js);
let message = JS_EncodeStringToUTF8(context, message_root.handle());
let message = CStr::from_ptr(message);
println!("{}", str::from_utf8(message.to_bytes()).unwrap());
Expand Down
7 changes: 3 additions & 4 deletions tests/evaluate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

#[macro_use]
extern crate js;

use js::jsapi::CompartmentOptions;
use js::jsapi::JS_NewGlobalObject;
use js::jsapi::OnNewGlobalHookOption;
use js::jsapi::RootedObject;
use js::jsapi::RootedValue;
use js::jsval::UndefinedValue;
use js::rust::{Runtime, SIMPLE_GLOBAL_CLASS};

Expand All @@ -21,12 +20,12 @@ fn evaluate() {

unsafe {

let global = RootedObject::new(cx,
rooted!(in(cx) let global =
JS_NewGlobalObject(cx, &SIMPLE_GLOBAL_CLASS, ptr::null_mut(),
OnNewGlobalHookOption::FireOnNewGlobalHook,
&CompartmentOptions::default())
);
let mut rval = RootedValue::new(cx, UndefinedValue());
rooted!(in(cx) let mut rval = UndefinedValue());
assert!(rt.evaluate_script(global.handle(), "1 + 1",
"test", 1, rval.handle_mut()).is_ok());
}
Expand Down
Loading