From a29eeaf326681fc44eefa099ba04c525e5af5010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Sun, 5 Jan 2020 20:52:03 +0100 Subject: [PATCH] libdeno: don't pass pointers between core::Isolate and libdeno (#3602) --- core/isolate.rs | 101 ++++++++++++++++-------------------------------- core/libdeno.rs | 99 +++++++++-------------------------------------- 2 files changed, 53 insertions(+), 147 deletions(-) diff --git a/core/isolate.rs b/core/isolate.rs index f15f0856c6..9d1b5bb8b3 100644 --- a/core/isolate.rs +++ b/core/isolate.rs @@ -164,8 +164,8 @@ type IsolateErrorHandleFn = dyn FnMut(ErrBox) -> Result<(), ErrBox>; /// by implementing dispatcher function that takes control buffer and optional zero copy buffer /// as arguments. An async Op corresponds exactly to a Promise in JavaScript. pub struct Isolate { - libdeno_isolate: *mut libdeno::isolate, - shared_libdeno_isolate: Arc>>, + libdeno_isolate: Box, + shared_libdeno_isolate: Arc>>, dyn_import: Option>, js_error_create: Arc, needs_init: bool, @@ -185,8 +185,6 @@ impl Drop for Isolate { fn drop(&mut self) { // remove shared_libdeno_isolate reference *self.shared_libdeno_isolate.lock().unwrap() = None; - - unsafe { libdeno::deno_delete(self.libdeno_isolate) } } } @@ -229,10 +227,12 @@ impl Isolate { }; let libdeno_isolate = unsafe { libdeno::deno_new(libdeno_config) }; + let shared_libdeno_isolate = Arc::new(Mutex::new(Some(libdeno_isolate))); + let libdeno_isolate = unsafe { Box::from_raw(libdeno_isolate) }; Self { libdeno_isolate, - shared_libdeno_isolate: Arc::new(Mutex::new(Some(libdeno_isolate))), + shared_libdeno_isolate, dyn_import: None, js_error_create: Arc::new(CoreJSError::from_v8_exception), shared, @@ -388,27 +388,21 @@ impl Isolate { js_source: &str, ) -> Result<(), ErrBox> { self.shared_init(); - unsafe { - libdeno::deno_execute( - self.libdeno_isolate, - self as *mut _ as *mut c_void, - js_filename, - js_source, - ) - }; + let core_i = self as *mut _ as *mut c_void; + let libdeno_i = &mut self.libdeno_isolate; + unsafe { libdeno::deno_execute(libdeno_i, core_i, js_filename, js_source) }; self.check_last_exception() } fn check_last_exception(&mut self) -> Result<(), ErrBox> { - let maybe_err = - unsafe { libdeno::deno_last_exception(self.libdeno_isolate) }; + let maybe_err = self.libdeno_isolate.last_exception_.clone(); match maybe_err { None => Ok(()), Some(json_str) => { let js_error_create = &*self.js_error_create; if self.error_handler.is_some() { // We need to clear last exception to avoid double handling. - unsafe { libdeno::deno_clear_last_exception(self.libdeno_isolate) }; + self.libdeno_isolate.last_exception_ = None; let v8_exception = V8Exception::from_json(&json_str).unwrap(); let js_error = js_error_create(v8_exception); let handler = self.error_handler.as_mut().unwrap(); @@ -422,15 +416,15 @@ impl Isolate { } } - fn check_promise_errors(&self) { + fn check_promise_errors(&mut self) { unsafe { - libdeno::deno_check_promise_errors(self.libdeno_isolate); + libdeno::deno_check_promise_errors(&mut self.libdeno_isolate); } } fn throw_exception(&mut self, exception_text: &str) { unsafe { - libdeno::deno_throw_exception(self.libdeno_isolate, exception_text) + libdeno::deno_throw_exception(&mut self.libdeno_isolate, exception_text) } } @@ -442,9 +436,9 @@ impl Isolate { None => (0, deno_buf::empty()), Some((op_id, r)) => (op_id, deno_buf::from(r)), }; - unsafe { - libdeno::deno_respond(self.libdeno_isolate, self.as_raw_ptr(), op_id, buf) - } + let core_i = self.as_raw_ptr(); + let libdeno_i = &mut self.libdeno_isolate; + unsafe { libdeno::deno_respond(libdeno_i, core_i, op_id, buf) } self.check_last_exception() } @@ -455,21 +449,17 @@ impl Isolate { name: &str, source: &str, ) -> Result { - let id = unsafe { - libdeno::deno_mod_new(self.libdeno_isolate, main, name, source) - }; - + let id = self.libdeno_isolate.register_module(main, name, source); self.check_last_exception().map(|_| id) } pub fn mod_get_imports(&self, id: deno_mod) -> Vec { - let len = - unsafe { libdeno::deno_mod_imports_len(self.libdeno_isolate, id) }; + let info = self.libdeno_isolate.get_module_info(id).unwrap(); + let len = info.import_specifiers.len(); let mut out = Vec::new(); for i in 0..len { - let specifier = - unsafe { libdeno::deno_mod_imports_get(self.libdeno_isolate, id, i) } - .unwrap(); + let info = self.libdeno_isolate.get_module_info(id).unwrap(); + let specifier = info.import_specifiers.get(i).unwrap().to_string(); out.push(specifier); } out @@ -482,7 +472,7 @@ impl Isolate { /// the V8 exception. By default this type is CoreJSError, however it may be a /// different type if Isolate::set_js_error_create() has been used. pub fn snapshot(&mut self) -> Result { - let snapshot = libdeno::deno_snapshot_new(self.libdeno_isolate); + let snapshot = libdeno::deno_snapshot_new(&mut self.libdeno_isolate); match self.check_last_exception() { Ok(..) => Ok(snapshot), Err(err) => Err(err), @@ -500,10 +490,13 @@ impl Isolate { Err(None) => (0, None), Err(Some(err_str)) => (0, Some(err_str)), }; + let core_i = self.as_raw_ptr(); + let libdeno_i = &mut self.libdeno_isolate; + unsafe { libdeno::deno_dyn_import_done( - self.libdeno_isolate, - self.as_raw_ptr(), + libdeno_i, + core_i, id, mod_id, maybe_err_str, @@ -588,11 +581,10 @@ impl Isolate { id: deno_mod, resolve_fn: &mut ResolveFn, ) -> Result<(), ErrBox> { - let libdeno_isolate = self.libdeno_isolate; let mut ctx = ResolveContext { resolve_fn }; unsafe { libdeno::deno_mod_instantiate( - libdeno_isolate, + &mut self.libdeno_isolate, ctx.as_raw_ptr(), id, Self::resolve_cb, @@ -622,30 +614,13 @@ impl Isolate { /// different type if Isolate::set_js_error_create() has been used. pub fn mod_evaluate(&mut self, id: deno_mod) -> Result<(), ErrBox> { self.shared_init(); - unsafe { - libdeno::deno_mod_evaluate(self.libdeno_isolate, self.as_raw_ptr(), id) - }; + let core_i = self.as_raw_ptr(); + let libdeno_i = &mut self.libdeno_isolate; + unsafe { libdeno::deno_mod_evaluate(libdeno_i, core_i, id) }; self.check_last_exception() } } -struct LockerScope { - libdeno_isolate: *mut libdeno::isolate, -} - -impl LockerScope { - fn new(libdeno_isolate: *mut libdeno::isolate) -> LockerScope { - unsafe { libdeno::deno_lock(libdeno_isolate) } - LockerScope { libdeno_isolate } - } -} - -impl Drop for LockerScope { - fn drop(&mut self) { - unsafe { libdeno::deno_unlock(self.libdeno_isolate) } - } -} - impl Future for Isolate { type Output = Result<(), ErrBox>; @@ -686,20 +661,14 @@ impl Future for Isolate { } if inner.shared.size() > 0 { - // Lock the current thread for V8. - let locker = LockerScope::new(inner.libdeno_isolate); inner.respond(None)?; // The other side should have shifted off all the messages. assert_eq!(inner.shared.size(), 0); - drop(locker); } if overflow_response.is_some() { - // Lock the current thread for V8. - let locker = LockerScope::new(inner.libdeno_isolate); let (op_id, buf) = overflow_response.take().unwrap(); inner.respond(Some((op_id, &buf)))?; - drop(locker); } inner.check_promise_errors(); @@ -720,7 +689,7 @@ impl Future for Isolate { /// IsolateHandle is a thread safe handle on an Isolate. It exposed thread safe V8 functions. #[derive(Clone)] pub struct IsolateHandle { - shared_libdeno_isolate: Arc>>, + shared_libdeno_isolate: Arc>>, } unsafe impl Send for IsolateHandle {} @@ -730,10 +699,8 @@ impl IsolateHandle { /// After terminating execution it is probably not wise to continue using /// the isolate. pub fn terminate_execution(&self) { - unsafe { - if let Some(isolate) = *self.shared_libdeno_isolate.lock().unwrap() { - libdeno::deno_terminate_execution(isolate) - } + if let Some(isolate) = *self.shared_libdeno_isolate.lock().unwrap() { + unsafe { libdeno::deno_terminate_execution(isolate) } } } } diff --git a/core/libdeno.rs b/core/libdeno.rs index c6e47d0bb9..eea8cfd419 100644 --- a/core/libdeno.rs +++ b/core/libdeno.rs @@ -27,16 +27,16 @@ pub type OpId = u32; #[allow(non_camel_case_types)] pub type isolate = DenoIsolate; -struct ModuleInfo { - main: bool, - name: String, - handle: v8::Global, - import_specifiers: Vec, +pub struct ModuleInfo { + pub main: bool, + pub name: String, + pub handle: v8::Global, + pub import_specifiers: Vec, } pub struct DenoIsolate { isolate_: Option, - last_exception_: Option, + pub last_exception_: Option, last_exception_handle_: v8::Global, context_: v8::Global, mods_: HashMap, @@ -208,7 +208,7 @@ impl DenoIsolate { id } - fn get_module_info(&self, id: deno_mod) -> Option<&ModuleInfo> { + pub fn get_module_info(&self, id: deno_mod) -> Option<&ModuleInfo> { if id == 0 { return None; } @@ -925,7 +925,7 @@ lazy_static! { ]); } -pub unsafe fn deno_new_snapshotter(config: deno_config) -> *mut isolate { +pub unsafe fn deno_new_snapshotter(config: deno_config) -> *mut DenoIsolate { assert_ne!(config.will_snapshot, 0); // TODO(ry) Support loading snapshots before snapshotting. assert!(config.load_snapshot.is_none()); @@ -1382,7 +1382,7 @@ fn initialize_context<'a>( context.exit(); } -pub unsafe fn deno_new(config: deno_config) -> *mut isolate { +pub unsafe fn deno_new(config: deno_config) -> *mut DenoIsolate { if config.will_snapshot != 0 { return deno_new_snapshotter(config); } @@ -1416,21 +1416,7 @@ pub unsafe fn deno_new(config: deno_config) -> *mut isolate { Box::into_raw(d) } -pub unsafe fn deno_delete(i: *mut DenoIsolate) { - let deno_isolate = unsafe { Box::from_raw(i as *mut DenoIsolate) }; - drop(deno_isolate); -} - -pub unsafe fn deno_last_exception(i: *mut DenoIsolate) -> Option { - (*i).last_exception_.clone() -} - -pub unsafe fn deno_clear_last_exception(i: *mut DenoIsolate) { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; - i_mut.last_exception_ = None; -} - -pub unsafe fn deno_check_promise_errors(i: *mut DenoIsolate) { +pub unsafe fn deno_check_promise_errors(i_mut: &mut DenoIsolate) { /* if (d->pending_promise_map_.size() > 0) { auto* isolate = d->isolate_; @@ -1448,7 +1434,6 @@ pub unsafe fn deno_check_promise_errors(i: *mut DenoIsolate) { } } */ - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; let isolate = i_mut.isolate_.as_ref().unwrap(); if i_mut.pending_promise_map_.is_empty() { @@ -1473,20 +1458,7 @@ pub unsafe fn deno_check_promise_errors(i: *mut DenoIsolate) { context.exit(); } -pub unsafe fn deno_lock(i: *mut DenoIsolate) { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; - assert!(i_mut.locker_.is_none()); - let mut locker = v8::Locker::new(i_mut.isolate_.as_ref().unwrap()); - i_mut.locker_ = Some(locker); -} - -pub unsafe fn deno_unlock(i: *mut DenoIsolate) { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; - i_mut.locker_.take().unwrap(); -} - -pub unsafe fn deno_throw_exception(i: *mut DenoIsolate, text: &str) { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; +pub unsafe fn deno_throw_exception(i_mut: &mut DenoIsolate, text: &str) { let isolate = i_mut.isolate_.as_ref().unwrap(); let mut locker = v8::Locker::new(isolate); let mut hs = v8::HandleScope::new(&mut locker); @@ -1551,7 +1523,7 @@ pub unsafe fn deno_import_buf<'sc>( } pub unsafe fn deno_respond( - i: *mut isolate, + deno_isolate: &mut DenoIsolate, core_isolate: *const c_void, op_id: OpId, buf: deno_buf, @@ -1569,7 +1541,6 @@ pub unsafe fn deno_respond( return; } */ - let deno_isolate: &mut DenoIsolate = unsafe { &mut *i }; if !deno_isolate.current_args_.is_null() { // Synchronous response. @@ -1673,12 +1644,11 @@ pub unsafe fn deno_respond( } pub unsafe fn deno_execute( - i: *mut DenoIsolate, + i_mut: &mut DenoIsolate, core_isolate: *mut c_void, js_filename: &str, js_source: &str, ) { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; i_mut.core_isolate_ = core_isolate; let isolate = i_mut.isolate_.as_ref().unwrap(); // println!("deno_execute -> Isolate ptr {:?}", isolate); @@ -1737,35 +1707,6 @@ pub unsafe fn deno_run_microtasks(i: *mut isolate, core_isolate: *mut c_void) { // Modules -pub unsafe fn deno_mod_new( - i: *mut DenoIsolate, - main: bool, - name: &str, - source: &str, -) -> deno_mod { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; - i_mut.register_module(main, name, source) -} - -pub unsafe fn deno_mod_imports_len(i: *mut DenoIsolate, id: deno_mod) -> usize { - let info = (*i).get_module_info(id).unwrap(); - info.import_specifiers.len() -} - -pub unsafe fn deno_mod_imports_get( - i: *mut DenoIsolate, - id: deno_mod, - index: size_t, -) -> Option { - match (*i).get_module_info(id) { - Some(info) => match info.import_specifiers.get(index) { - Some(specifier) => Some(specifier.to_string()), - None => None, - }, - None => None, - } -} - fn resolve_callback( context: v8::Local, specifier: v8::Local, @@ -1822,12 +1763,11 @@ fn resolve_callback( } pub unsafe fn deno_mod_instantiate( - i: *mut DenoIsolate, + i_mut: &mut DenoIsolate, resolve_context: *mut c_void, id: deno_mod, resolve_cb: deno_resolve_cb, ) { - let i_mut: &mut DenoIsolate = unsafe { &mut *i }; i_mut.resolve_context_ = resolve_context; let isolate = i_mut.isolate_.as_ref().unwrap(); let mut locker = v8::Locker::new(isolate); @@ -1868,11 +1808,10 @@ pub unsafe fn deno_mod_instantiate( } pub unsafe fn deno_mod_evaluate( - i: *mut DenoIsolate, + deno_isolate: &mut DenoIsolate, core_isolate: *const c_void, id: deno_mod, ) { - let deno_isolate: &mut DenoIsolate = unsafe { &mut *i }; let core_isolate: *mut c_void = unsafe { std::mem::transmute(core_isolate) }; deno_isolate.core_isolate_ = core_isolate; let isolate = deno_isolate.isolate_.as_ref().unwrap(); @@ -1920,13 +1859,12 @@ pub unsafe fn deno_mod_evaluate( /// Call exactly once for every deno_dyn_import_cb. pub unsafe fn deno_dyn_import_done( - i: *mut DenoIsolate, + deno_isolate: &mut DenoIsolate, core_isolate: *const c_void, id: deno_dyn_import_id, mod_id: deno_mod, error_str: Option, ) { - let deno_isolate: &mut DenoIsolate = unsafe { &mut *i }; assert!( (mod_id == 0 && error_str.is_some()) || (mod_id != 0 && error_str.is_none()) @@ -1981,8 +1919,9 @@ pub unsafe fn deno_dyn_import_done( deno_isolate.core_isolate_ = std::ptr::null_mut(); } -pub fn deno_snapshot_new(i: *mut DenoIsolate) -> v8::OwnedStartupData { - let deno_isolate: &mut DenoIsolate = unsafe { &mut *i }; +pub fn deno_snapshot_new( + deno_isolate: &mut DenoIsolate, +) -> v8::OwnedStartupData { assert!(deno_isolate.snapshot_creator_.is_some()); let isolate = deno_isolate.isolate_.as_ref().unwrap();