From eba98e7a5e76854adbe5766794082cb24c628c4c Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Tue, 25 Feb 2020 16:22:32 -0800 Subject: [PATCH] Revert "Add ability to attach arbitrary state to Isolate (#282)" This patch introduces a bug that breaks Deno. This reverts commit 457f7ae77951a1dd22ea3722587122a934451a17. --- src/context.rs | 4 +- src/isolate.rs | 84 +---------- tests/isolate_with_state.rs | 271 ------------------------------------ 3 files changed, 8 insertions(+), 351 deletions(-) delete mode 100644 tests/isolate_with_state.rs diff --git a/src/context.rs b/src/context.rs index b635d375..2aea63e9 100644 --- a/src/context.rs +++ b/src/context.rs @@ -64,14 +64,14 @@ impl Context { /// and run is compiled and run in this context. If another context /// is already entered, this old context is saved so it can be /// restored when the new context is exited. - pub fn enter(&mut self) { + pub(crate) fn enter(&mut self) { // TODO: enter/exit should be controlled by a scope. unsafe { v8__Context__Enter(self) }; } /// Exit this context. Exiting the current context restores the /// context that was in place when entering the current context. - pub fn exit(&mut self) { + pub(crate) fn exit(&mut self) { // TODO: enter/exit should be controlled by a scope. unsafe { v8__Context__Exit(self) }; } diff --git a/src/isolate.rs b/src/isolate.rs index b39a3741..d338489c 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -19,24 +19,15 @@ use crate::StartupData; use crate::String; use crate::Value; -use std::any::Any; -use std::any::TypeId; -use std::cell::RefCell; -use std::collections::HashMap; use std::ffi::c_void; use std::mem::replace; use std::ops::Deref; use std::ops::DerefMut; use std::ptr::null_mut; use std::ptr::NonNull; -use std::rc::Rc; use std::sync::Arc; use std::sync::Mutex; -/// Because there are only 3 embedder slots, we use this store multiple states. -/// We have many different types of states in Deno due to the core/cli split. -type States = HashMap>>; - pub type MessageCallback = extern "C" fn(Local, Local); pub type PromiseRejectCallback = extern "C" fn(PromiseRejectMessage); @@ -142,12 +133,6 @@ extern "C" { ); } -enum InternalSlots { - Annex = 0, - States, - Total, -} - #[repr(C)] /// Isolate represents an isolated instance of the V8 engine. V8 isolates have /// completely separate states. Objects from one isolate must not be used in @@ -182,79 +167,29 @@ impl Isolate { } unsafe fn set_annex(&mut self, ptr: *mut IsolateAnnex) { - self.set_data_int(InternalSlots::Annex as _, ptr as *mut c_void); + v8__Isolate__SetData(self, 0, ptr as *mut c_void) } fn get_annex(&self) -> *mut IsolateAnnex { - self.get_data_int(InternalSlots::Annex as _) as *mut _ + unsafe { v8__Isolate__GetData(self, 0) as *mut _ } } /// Associate embedder-specific data with the isolate. |slot| has to be /// between 0 and GetNumberOfDataSlots() - 1. pub unsafe fn set_data(&mut self, slot: u32, ptr: *mut c_void) { - self.set_data_int(slot + InternalSlots::Total as u32, ptr) - } - - unsafe fn set_data_int(&mut self, slot: u32, ptr: *mut c_void) { - v8__Isolate__SetData(self, slot, ptr) + v8__Isolate__SetData(self, slot + 1, ptr) } /// Retrieve embedder-specific data from the isolate. /// Returns NULL if SetData has never been called for the given |slot|. pub fn get_data(&self, slot: u32) -> *mut c_void { - self.get_data_int(slot + InternalSlots::Total as u32) - } - - fn get_data_int(&self, slot: u32) -> *mut c_void { - unsafe { v8__Isolate__GetData(self, slot) } + unsafe { v8__Isolate__GetData(self, slot + 1) } } /// Returns the maximum number of available embedder data slots. Valid slots /// are in the range of 0 - GetNumberOfDataSlots() - 1. pub fn get_number_of_data_slots(&self) -> u32 { - unsafe { - v8__Isolate__GetNumberOfDataSlots(self) - InternalSlots::Total as u32 - } - } - - pub fn state_add(&mut self, state: S) - where - S: 'static + Sized, - { - let mut ptr = self.get_data_int(InternalSlots::States as _) as *mut States; - if ptr.is_null() { - assert!(ptr.is_null()); - let states = Box::new(HashMap::new()); - ptr = Box::into_raw(states); - unsafe { self.set_data_int(InternalSlots::States as _, ptr as *mut _) }; - } - - let mut states = unsafe { Box::from_raw(ptr) }; - let type_id = TypeId::of::(); - let existing = states.insert(type_id, Rc::new(RefCell::new(state))); - assert!(existing.is_none()); - - // ptr is still stored in Isolate's embedder slot. - let ptr2 = Box::into_raw(states); - assert_eq!(ptr, ptr2); - } - - pub fn state_get(&self) -> Rc> - where - S: 'static + Sized, - { - let ptr = self.get_data_int(InternalSlots::States as _) as *mut States; - let states = unsafe { Box::from_raw(ptr) }; - - let type_id = TypeId::of::(); - let state = states.get(&type_id).unwrap(); - // TODO(ry) how to change Rc> into - // Rc> without transmute? - #[allow(clippy::transmute_ptr_to_ptr)] - let state = unsafe { std::mem::transmute::<_, &Rc>>(state) }; - let ptr2 = Box::into_raw(states); // because isolate slot still has ptr. - assert_eq!(ptr, ptr2); - state.clone() + unsafe { v8__Isolate__GetNumberOfDataSlots(self) - 1 } } /// Sets this isolate as the entered one for the current thread. @@ -361,13 +296,6 @@ impl Isolate { // No test case in rusty_v8 show this, but there have been situations in // deno where dropping Annex before the states causes a segfault. - let ptr = self.get_data_int(InternalSlots::States as _) as *mut States; - if !ptr.is_null() { - let states = Box::from_raw(ptr); - self.set_data_int(InternalSlots::States as _, std::ptr::null_mut()); - drop(states); - } - v8__Isolate__Dispose(self) } } @@ -422,7 +350,7 @@ impl IsolateHandle { unsafe { { let _lock = (*annex_ptr).mutex.lock().unwrap(); - isolate.set_data(InternalSlots::Annex as _, null_mut()); + isolate.set_data(0, null_mut()); let isolate_ptr = replace(&mut (*annex_ptr).isolate, null_mut()); assert_eq!(isolate as *mut _, isolate_ptr); } diff --git a/tests/isolate_with_state.rs b/tests/isolate_with_state.rs deleted file mode 100644 index caaece10..00000000 --- a/tests/isolate_with_state.rs +++ /dev/null @@ -1,271 +0,0 @@ -//! This shows a pattern for adding state to an Isolate. The pattern is used -//! extensively in Deno. - -use core::ops::Deref; -use core::ops::DerefMut; -use rusty_v8 as v8; - -#[test] -fn isolate_with_state1() { - init_v8(); - let mut isolate1 = Isolate1::new(); - isolate1.setup(); - assert_eq!(0, isolate1.count()); - assert_eq!(0, isolate1.js_count()); - isolate1.exec("change_state()"); - assert_eq!(1, isolate1.count()); - assert_eq!(1, isolate1.js_count()); - isolate1.exec("change_state()"); - assert_eq!(2, isolate1.count()); - assert_eq!(2, isolate1.js_count()); -} - -#[test] -fn isolate_with_state2() { - init_v8(); - let mut isolate2 = Isolate2::new(); - isolate2.setup(); - assert_eq!(0, isolate2.count()); - assert_eq!(0, isolate2.js_count()); - assert_eq!(0, isolate2.count2()); - assert_eq!(0, isolate2.js_count2()); - isolate2.exec("change_state2()"); - assert_eq!(0, isolate2.count()); - assert_eq!(0, isolate2.js_count()); - assert_eq!(1, isolate2.count2()); - assert_eq!(1, isolate2.js_count2()); - isolate2.exec("change_state2()"); - assert_eq!(0, isolate2.count()); - assert_eq!(0, isolate2.js_count()); - assert_eq!(2, isolate2.count2()); - assert_eq!(2, isolate2.js_count2()); - isolate2.exec("change_state()"); - assert_eq!(1, isolate2.count()); - assert_eq!(1, isolate2.js_count()); - assert_eq!(2, isolate2.count2()); - assert_eq!(2, isolate2.js_count2()); -} - -struct State1 { - pub magic_number: usize, - pub count: usize, - pub js_count: v8::Global, -} - -struct Isolate1(v8::OwnedIsolate); - -impl Isolate1 { - fn new() -> Isolate1 { - let mut params = v8::Isolate::create_params(); - params.set_array_buffer_allocator(v8::new_default_allocator()); - let mut isolate = v8::Isolate::new(params); - - let state = State1 { - magic_number: 0xCAFE_BABE, - count: 0, - js_count: v8::Global::new(), - }; - - isolate.state_add(state); - - Isolate1(isolate) - } - - pub fn setup(&mut self) { - let mut hs = v8::HandleScope::new(&mut self.0); - let scope = hs.enter(); - let mut context = v8::Context::new(scope); - context.enter(); - - let global = context.global(scope); - - { - let js_count = v8::Integer::new(scope, 0); - let state = scope.isolate().state_get::(); - assert_eq!(state.borrow().magic_number, 0xCAFE_BABE); - state.borrow_mut().js_count.set(scope, js_count); - } - - let mut change_state_tmpl = - v8::FunctionTemplate::new(scope, Self::change_state); - let change_state_val = - change_state_tmpl.get_function(scope, context).unwrap(); - global.set( - context, - v8::String::new(scope, "change_state").unwrap().into(), - change_state_val.into(), - ); - } - - pub fn exec(&mut self, src: &str) { - let mut hs = v8::HandleScope::new(&mut self.0); - let scope = hs.enter(); - let context = scope.get_current_context().unwrap(); - let source = v8::String::new(scope, src).unwrap(); - let mut script = v8::Script::compile(scope, context, source, None).unwrap(); - script.run(scope, context); - } - - fn change_state( - scope: v8::FunctionCallbackScope, - _args: v8::FunctionCallbackArguments, - _rv: v8::ReturnValue, - ) { - let state = scope.isolate().state_get::(); - let mut state = state.borrow_mut(); - assert_eq!(state.magic_number, 0xCAFE_BABE); - state.count += 1; - - let js_count = state.js_count.get(scope).unwrap(); - let js_count_value = js_count.value() as i32; - let js_count_next = v8::Integer::new(scope, js_count_value + 1); - state.js_count.set(scope, js_count_next); - } - - fn count(&self) -> usize { - let state = self.0.state_get::(); - let state = state.borrow(); - assert_eq!(state.magic_number, 0xCAFE_BABE); - state.count - } - - fn js_count(&mut self) -> i64 { - let mut hs = v8::HandleScope::new(&mut self.0); - let scope = hs.enter(); - - let state = scope.isolate().state_get::(); - let state = state.borrow_mut(); - let js_count = state.js_count.get(scope).unwrap(); - - js_count.value() - } -} - -// TODO(ry) Useless boilerplate! -impl v8::InIsolate for Isolate1 { - fn isolate(&mut self) -> &mut v8::Isolate { - &mut self.0 - } -} - -impl Deref for Isolate1 { - type Target = v8::Isolate; - fn deref(&self) -> &v8::Isolate { - &self.0 - } -} - -impl DerefMut for Isolate1 { - fn deref_mut(&mut self) -> &mut v8::Isolate { - &mut self.0 - } -} - -struct State2 { - pub count2: usize, - pub js_count2: v8::Global, - pub magic_number: u32, -} - -struct Isolate2(Isolate1); - -impl Isolate2 { - fn new() -> Isolate2 { - let mut isolate1 = Isolate1::new(); - let state2 = State2 { - count2: 0, - js_count2: v8::Global::new(), - magic_number: 0xDEAD_BEEF, - }; - isolate1.state_add(state2); - Isolate2(isolate1) - } - - fn setup(&mut self) { - self.0.setup(); - - let mut hs = v8::HandleScope::new(&mut self.0); - let scope = hs.enter(); - let context = scope.get_current_context().unwrap(); - - let global = context.global(scope); - - { - let js_count2 = v8::Integer::new(scope, 0); - let state = scope.isolate().state_get::(); - assert_eq!(state.borrow().magic_number, 0xDEAD_BEEF); - state.borrow_mut().js_count2.set(scope, js_count2); - } - - let mut change_state_tmpl = - v8::FunctionTemplate::new(scope, Self::change_state2); - let change_state_val = - change_state_tmpl.get_function(scope, context).unwrap(); - global.set( - context, - v8::String::new(scope, "change_state2").unwrap().into(), - change_state_val.into(), - ); - } - - pub fn exec(&mut self, src: &str) { - self.0.exec(src) - } - - fn change_state2( - scope: v8::FunctionCallbackScope, - _args: v8::FunctionCallbackArguments, - _rv: v8::ReturnValue, - ) { - let state = scope.isolate().state_get::(); - let mut state = state.borrow_mut(); - assert_eq!(state.magic_number, 0xDEAD_BEEF); - state.count2 += 1; - - let js_count2 = state.js_count2.get(scope).unwrap(); - let js_count2_value = js_count2.value() as i32; - let js_count2_next = v8::Integer::new(scope, js_count2_value + 1); - state.js_count2.set(scope, js_count2_next); - } - - fn count2(&self) -> usize { - let state = self.0.state_get::(); - let state = state.borrow(); - assert_eq!(state.magic_number, 0xDEAD_BEEF); - state.count2 - } - - fn js_count2(&mut self) -> i64 { - let mut hs = v8::HandleScope::new(&mut self.0); - let scope = hs.enter(); - - let state = scope.isolate().state_get::(); - let state = state.borrow_mut(); - assert_eq!(state.magic_number, 0xDEAD_BEEF); - let js_count2 = state.js_count2.get(scope).unwrap(); - - js_count2.value() - } -} - -impl Deref for Isolate2 { - type Target = Isolate1; - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl DerefMut for Isolate2 { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} - -fn init_v8() { - use std::sync::Once; - static INIT_V8: Once = Once::new(); - INIT_V8.call_once(|| { - v8::V8::initialize_platform(v8::new_default_platform()); - v8::V8::initialize(); - }); -}