1 files changed, 224 insertions, 230 deletions

diff --git a/core/isolate.rs b/core/isolate.rs
index d67ed9331..4a1c1bbd4 100644
--- a/core/isolate.rs
+++ b/core/isolate.rs
@@ -26,6 +26,7 @@ use std::collections::HashMap;
 use std::convert::From;
 use std::error::Error;
 use std::fmt;
+use std::mem::forget;
 use std::ops::{Deref, DerefMut};
 use std::option::Option;
 use std::pin::Pin;
@@ -169,7 +170,7 @@ pub struct Isolate {
   pub(crate) js_recv_cb: v8::Global<v8::Function>,
   pub(crate) pending_promise_exceptions: HashMap<i32, v8::Global<v8::Value>>,
   shared_isolate_handle: Arc<Mutex<Option<*mut v8::Isolate>>>,
-  js_error_create: Arc<JSErrorCreateFn>,
+  pub(crate) js_error_create_fn: Arc<JSErrorCreateFn>,
   needs_init: bool,
   pub(crate) shared: SharedQueue,
   pending_ops: FuturesUnordered<PendingOpFuture>,
@@ -181,42 +182,28 @@ pub struct Isolate {
   error_handler: Option<Box<IsolateErrorHandleFn>>,
 }
 
-// TODO(ry) this shouldn't be necessary, v8::OwnedIsolate should impl Send.
+// TODO(ry): a V8 Isolate cannot actually be moved between threads without the
+// use of a Locker, therefore Isolate should not implement the `Send` trait.
 unsafe impl Send for Isolate {}
 
 impl Drop for Isolate {
   fn drop(&mut self) {
-    // remove shared_libdeno_isolate reference
-    *self.shared_isolate_handle.lock().unwrap() = None;
-
-    // TODO Too much boiler plate.
-    // <Boilerplate>
-    let isolate = self.v8_isolate.take().unwrap();
-    // Clear persistent handles we own.
-    {
-      let mut locker = v8::Locker::new(&isolate);
-      let mut hs = v8::HandleScope::new(locker.enter());
-      let scope = hs.enter();
-      // </Boilerplate>
-      self.global_context.reset(scope);
-      self.shared_ab.reset(scope);
-      self.js_recv_cb.reset(scope);
-      for (_key, handle) in self.pending_promise_exceptions.iter_mut() {
-        handle.reset(scope);
-      }
-    }
     if let Some(creator) = self.snapshot_creator.take() {
+      // TODO(ry): in rusty_v8, `SnapShotCreator::get_owned_isolate()` returns
+      // a `struct OwnedIsolate` which is not actually owned, hence the need
+      // here to leak the `OwnedIsolate` in order to avoid a double free and
+      // the segfault that it causes.
+      let v8_isolate = self.v8_isolate.take().unwrap();
+      forget(v8_isolate);
+
       // TODO(ry) V8 has a strange assert which prevents a SnapshotCreator from
       // being deallocated if it hasn't created a snapshot yet.
       // https://github.com/v8/v8/blob/73212783fbd534fac76cc4b66aac899c13f71fc8/src/api.cc#L603
       // If that assert is removed, this if guard could be removed.
       // WARNING: There may be false positive LSAN errors here.
-      std::mem::forget(isolate);
       if self.has_snapshotted {
         drop(creator);
       }
-    } else {
-      drop(isolate);
     }
   }
 }
@@ -272,12 +259,9 @@ impl Isolate {
       let mut creator =
         v8::SnapshotCreator::new(Some(&bindings::EXTERNAL_REFERENCES));
       let isolate = unsafe { creator.get_owned_isolate() };
-      let isolate = Isolate::setup_isolate(isolate);
-
-      let mut locker = v8::Locker::new(&isolate);
-      let scope = locker.enter();
+      let mut isolate = Isolate::setup_isolate(isolate);
 
-      let mut hs = v8::HandleScope::new(scope);
+      let mut hs = v8::HandleScope::new(&mut isolate);
       let scope = hs.enter();
 
       let context = bindings::initialize_context(scope);
@@ -294,12 +278,9 @@ impl Isolate {
       }
 
       let isolate = v8::Isolate::new(params);
-      let isolate = Isolate::setup_isolate(isolate);
+      let mut isolate = Isolate::setup_isolate(isolate);
 
-      let mut locker = v8::Locker::new(&isolate);
-      let scope = locker.enter();
-
-      let mut hs = v8::HandleScope::new(scope);
+      let mut hs = v8::HandleScope::new(&mut isolate);
       let scope = hs.enter();
 
       let context = match load_snapshot {
@@ -329,7 +310,7 @@ impl Isolate {
       snapshot: load_snapshot,
       has_snapshotted: false,
       shared_isolate_handle: Arc::new(Mutex::new(None)),
-      js_error_create: Arc::new(CoreJSError::from_v8_exception),
+      js_error_create_fn: Arc::new(CoreJSError::from_v8_exception),
       shared,
       needs_init,
       pending_ops: FuturesUnordered::new(),
@@ -362,62 +343,6 @@ impl Isolate {
     isolate
   }
 
-  pub fn exception_to_err_result<'a, T>(
-    &mut self,
-    scope: &mut (impl v8::ToLocal<'a> + v8::InContext),
-    exception: v8::Local<v8::Value>,
-  ) -> Result<T, ErrBox> {
-    self.handle_exception(scope, exception);
-    self.check_last_exception().map(|_| unreachable!())
-  }
-
-  pub fn handle_exception<'a>(
-    &mut self,
-    scope: &mut (impl v8::ToLocal<'a> + v8::InContext),
-    exception: v8::Local<v8::Value>,
-  ) {
-    // Use a HandleScope because the  functions below create a lot of
-    // local handles (in particular, `encode_message_as_json()` does).
-    let mut hs = v8::HandleScope::new(scope);
-    let scope = hs.enter();
-
-    let is_terminating_exception = scope.isolate().is_execution_terminating();
-    let mut exception = exception;
-
-    if is_terminating_exception {
-      // TerminateExecution was called. Cancel exception termination so that the
-      // exception can be created..
-      scope.isolate().cancel_terminate_execution();
-
-      // Maybe make a new exception object.
-      if exception.is_null_or_undefined() {
-        let exception_str =
-          v8::String::new(scope, "execution terminated").unwrap();
-        exception = v8::Exception::error(scope, exception_str);
-      }
-    }
-
-    let message = v8::Exception::create_message(scope, exception);
-    let json_str = self.encode_message_as_json(scope, message);
-    self.last_exception = Some(json_str);
-
-    if is_terminating_exception {
-      // Re-enable exception termination.
-      scope.isolate().terminate_execution();
-    }
-  }
-
-  pub fn encode_message_as_json<'a>(
-    &mut self,
-    scope: &mut (impl v8::ToLocal<'a> + v8::InContext),
-    message: v8::Local<v8::Message>,
-  ) -> String {
-    let context = scope.isolate().get_current_context();
-    let json_obj = bindings::encode_message_as_object(scope, message);
-    let json_string = v8::json::stringify(context, json_obj.into()).unwrap();
-    json_string.to_rust_string_lossy(scope)
-  }
-
   /// Defines the how Deno.core.dispatch() acts.
   /// Called whenever Deno.core.dispatch() is called in JavaScript. zero_copy_buf
   /// corresponds to the second argument of Deno.core.dispatch().
@@ -437,14 +362,7 @@ impl Isolate {
   where
     F: Fn(V8Exception) -> ErrBox + 'static,
   {
-    self.js_error_create = Arc::new(f);
-  }
-
-  /// Get a thread safe handle on the isolate.
-  pub fn shared_isolate_handle(&mut self) -> IsolateHandle {
-    IsolateHandle {
-      shared_isolate: self.shared_isolate_handle.clone(),
-    }
+    self.js_error_create_fn = Arc::new(f);
   }
 
   /// Executes a bit of built-in JavaScript to provide Deno.sharedQueue.
@@ -463,7 +381,7 @@ impl Isolate {
 
   pub fn dispatch_op<'s>(
     &mut self,
-    scope: &mut (impl v8::ToLocal<'s> + v8::InContext),
+    scope: &mut impl v8::ToLocal<'s>,
     op_id: OpId,
     control_buf: &[u8],
     zero_copy_buf: Option<ZeroCopyBuf>,
@@ -516,11 +434,12 @@ impl Isolate {
   ) -> Result<(), ErrBox> {
     self.shared_init();
 
-    let isolate = self.v8_isolate.as_ref().unwrap();
-    let mut locker = v8::Locker::new(isolate);
-    assert!(!self.global_context.is_empty());
-    let mut hs = v8::HandleScope::new(locker.enter());
+    let js_error_create_fn = &*self.js_error_create_fn;
+    let v8_isolate = self.v8_isolate.as_mut().unwrap();
+
+    let mut hs = v8::HandleScope::new(v8_isolate);
     let scope = hs.enter();
+    assert!(!self.global_context.is_empty());
     let context = self.global_context.get(scope).unwrap();
     let mut cs = v8::ContextScope::new(scope, context);
     let scope = cs.enter();
@@ -539,75 +458,8 @@ impl Isolate {
       None => {
         assert!(tc.has_caught());
         let exception = tc.exception().unwrap();
-        self.exception_to_err_result(scope, exception)
-      }
-    }
-  }
-
-  pub(crate) fn check_last_exception(&mut self) -> Result<(), ErrBox> {
-    match self.last_exception.take() {
-      None => Ok(()),
-      Some(json_str) => {
-        let v8_exception = V8Exception::from_json(&json_str).unwrap();
-        let js_error = (self.js_error_create)(v8_exception);
-        Err(js_error)
-      }
-    }
-  }
-
-  pub(crate) fn attach_handle_to_error(
-    &mut self,
-    scope: &mut impl v8::InIsolate,
-    err: ErrBox,
-    handle: v8::Local<v8::Value>,
-  ) -> ErrBox {
-    ErrWithV8Handle::new(scope, err, handle).into()
-  }
-
-  fn check_promise_exceptions<'s>(
-    &mut self,
-    scope: &mut (impl v8::ToLocal<'s> + v8::InContext),
-  ) -> Result<(), ErrBox> {
-    if let Some(&key) = self.pending_promise_exceptions.keys().next() {
-      let mut handle = self.pending_promise_exceptions.remove(&key).unwrap();
-      let exception = handle.get(scope).expect("empty error handle");
-      handle.reset(scope);
-      self.exception_to_err_result(scope, exception)
-    } else {
-      Ok(())
-    }
-  }
-
-  fn async_op_response<'s>(
-    &mut self,
-    scope: &mut (impl v8::ToLocal<'s> + v8::InContext),
-    maybe_buf: Option<(OpId, Box<[u8]>)>,
-  ) -> Result<(), ErrBox> {
-    let context = scope.isolate().get_current_context();
-    let global: v8::Local<v8::Value> = context.global(scope).into();
-    let js_recv_cb = self
-      .js_recv_cb
-      .get(scope)
-      .expect("Deno.core.recv has not been called.");
-
-    // TODO(piscisaureus): properly integrate TryCatch in the scope chain.
-    let mut try_catch = v8::TryCatch::new(scope);
-    let tc = try_catch.enter();
-
-    match maybe_buf {
-      Some((op_id, buf)) => {
-        let op_id: v8::Local<v8::Value> =
-          v8::Integer::new(scope, op_id as i32).into();
-        let ui8: v8::Local<v8::Value> =
-          bindings::boxed_slice_to_uint8array(scope, buf).into();
-        js_recv_cb.call(scope, context, global, &[op_id, ui8])
+        exception_to_err_result(scope, exception, js_error_create_fn)
       }
-      None => js_recv_cb.call(scope, context, global, &[]),
-    };
-
-    match tc.exception() {
-      None => Ok(()),
-      Some(exception) => self.exception_to_err_result(scope, exception),
     }
   }
 
@@ -620,9 +472,11 @@ impl Isolate {
   pub fn snapshot(&mut self) -> Result<v8::OwnedStartupData, ErrBox> {
     assert!(self.snapshot_creator.is_some());
 
-    let isolate = self.v8_isolate.as_ref().unwrap();
-    let mut locker = v8::Locker::new(isolate);
-    let mut hs = v8::HandleScope::new(locker.enter());
+    let v8_isolate = self.v8_isolate.as_mut().unwrap();
+    let js_error_create_fn = &*self.js_error_create_fn;
+    let last_exception = &mut self.last_exception;
+
+    let mut hs = v8::HandleScope::new(v8_isolate);
     let scope = hs.enter();
     self.global_context.reset(scope);
 
@@ -631,7 +485,8 @@ impl Isolate {
       .create_blob(v8::FunctionCodeHandling::Keep)
       .unwrap();
     self.has_snapshotted = true;
-    self.check_last_exception().map(|_| snapshot)
+
+    check_last_exception(last_exception, js_error_create_fn).map(|_| snapshot)
   }
 }
 
@@ -643,14 +498,22 @@ impl Future for Isolate {
     inner.waker.register(cx.waker());
     inner.shared_init();
 
-    let mut locker = v8::Locker::new(&*inner.v8_isolate.as_mut().unwrap());
-    let mut hs = v8::HandleScope::new(locker.enter());
+    let v8_isolate = inner.v8_isolate.as_mut().unwrap();
+    let js_error_create_fn = &*inner.js_error_create_fn;
+    let js_recv_cb = &inner.js_recv_cb;
+    let pending_promise_exceptions = &mut inner.pending_promise_exceptions;
+
+    let mut hs = v8::HandleScope::new(v8_isolate);
     let scope = hs.enter();
     let context = inner.global_context.get(scope).unwrap();
     let mut cs = v8::ContextScope::new(scope, context);
     let scope = cs.enter();
 
-    inner.check_promise_exceptions(scope)?;
+    check_promise_exceptions(
+      scope,
+      pending_promise_exceptions,
+      js_error_create_fn,
+    )?;
 
     let mut overflow_response: Option<(OpId, Buf)> = None;
 
@@ -678,17 +541,26 @@ impl Future for Isolate {
     }
 
     if inner.shared.size() > 0 {
-      inner.async_op_response(scope, None)?;
+      async_op_response(scope, None, js_recv_cb, js_error_create_fn)?;
       // The other side should have shifted off all the messages.
       assert_eq!(inner.shared.size(), 0);
     }
 
     if overflow_response.is_some() {
       let (op_id, buf) = overflow_response.take().unwrap();
-      inner.async_op_response(scope, Some((op_id, buf)))?;
+      async_op_response(
+        scope,
+        Some((op_id, buf)),
+        js_recv_cb,
+        js_error_create_fn,
+      )?;
     }
 
-    inner.check_promise_exceptions(scope)?;
+    check_promise_exceptions(
+      scope,
+      pending_promise_exceptions,
+      js_error_create_fn,
+    )?;
 
     // We're idle if pending_ops is empty.
     if inner.pending_ops.is_empty() {
@@ -702,26 +574,147 @@ 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_isolate: Arc<Mutex<Option<*mut v8::Isolate>>>,
+fn async_op_response<'s>(
+  scope: &mut impl v8::ToLocal<'s>,
+  maybe_buf: Option<(OpId, Box<[u8]>)>,
+  js_recv_cb: &v8::Global<v8::Function>,
+  js_error_create_fn: &JSErrorCreateFn,
+) -> Result<(), ErrBox> {
+  let context = scope.get_current_context().unwrap();
+  let global: v8::Local<v8::Value> = context.global(scope).into();
+  let js_recv_cb = js_recv_cb
+    .get(scope)
+    .expect("Deno.core.recv has not been called.");
+
+  // TODO(piscisaureus): properly integrate TryCatch in the scope chain.
+  let mut try_catch = v8::TryCatch::new(scope);
+  let tc = try_catch.enter();
+
+  match maybe_buf {
+    Some((op_id, buf)) => {
+      let op_id: v8::Local<v8::Value> =
+        v8::Integer::new(scope, op_id as i32).into();
+      let ui8: v8::Local<v8::Value> =
+        bindings::boxed_slice_to_uint8array(scope, buf).into();
+      js_recv_cb.call(scope, context, global, &[op_id, ui8])
+    }
+    None => js_recv_cb.call(scope, context, global, &[]),
+  };
+
+  match tc.exception() {
+    None => Ok(()),
+    Some(exception) => {
+      exception_to_err_result(scope, exception, js_error_create_fn)
+    }
+  }
 }
 
-unsafe impl Send for IsolateHandle {}
+pub(crate) fn attach_handle_to_error(
+  scope: &mut impl v8::InIsolate,
+  err: ErrBox,
+  handle: v8::Local<v8::Value>,
+) -> ErrBox {
+  ErrWithV8Handle::new(scope, err, handle).into()
+}
 
-impl IsolateHandle {
-  /// Terminate the execution of any currently running javascript.
-  /// After terminating execution it is probably not wise to continue using
-  /// the isolate.
-  pub fn terminate_execution(&self) {
-    if let Some(isolate) = *self.shared_isolate.lock().unwrap() {
-      let isolate = unsafe { &mut *isolate };
-      isolate.terminate_execution();
+pub(crate) fn exception_to_err_result<'a, T>(
+  scope: &mut impl v8::ToLocal<'a>,
+  exception: v8::Local<v8::Value>,
+  js_error_create_fn: &JSErrorCreateFn,
+) -> Result<T, ErrBox> {
+  let mut last_exception = Option::<String>::None;
+  handle_exception(scope, exception, &mut last_exception);
+  check_last_exception(&mut last_exception, js_error_create_fn)
+    .map(|_| unreachable!())
+}
+
+pub(crate) fn handle_exception<'a>(
+  scope: &mut impl v8::ToLocal<'a>,
+  exception: v8::Local<v8::Value>,
+  last_exception: &mut Option<String>, // Out parameter.
+) {
+  // Use a HandleScope because the  functions below create a lot of
+  // local handles (in particular, `encode_message_as_json()` does).
+  let mut hs = v8::HandleScope::new(scope);
+  let scope = hs.enter();
+
+  // TODO(piscisaureus): in rusty_v8, `is_execution_terminating()` should
+  // also be implemented on `struct Isolate`.
+  let is_terminating_exception = scope
+    .isolate()
+    .thread_safe_handle()
+    .is_execution_terminating();
+  let mut exception = exception;
+
+  if is_terminating_exception {
+    // TerminateExecution was called. Cancel exception termination so that the
+    // exception can be created..
+    // TODO(piscisaureus): in rusty_v8, `cancel_terminate_execution()` should
+    // also be implemented on `struct Isolate`.
+    scope
+      .isolate()
+      .thread_safe_handle()
+      .cancel_terminate_execution();
+
+    // Maybe make a new exception object.
+    if exception.is_null_or_undefined() {
+      let exception_str =
+        v8::String::new(scope, "execution terminated").unwrap();
+      exception = v8::Exception::error(scope, exception_str);
+    }
+  }
+
+  let message = v8::Exception::create_message(scope, exception);
+  let json_str = encode_message_as_json(scope, message);
+  let prev_last_exception = last_exception.replace(json_str);
+  assert_eq!(prev_last_exception, None);
+
+  if is_terminating_exception {
+    // Re-enable exception termination.
+    // TODO(piscisaureus): in rusty_v8, `terminate_execution()` should also
+    // be implemented on `struct Isolate`.
+    scope.isolate().thread_safe_handle().terminate_execution();
+  }
+}
+
+pub(crate) fn check_last_exception(
+  last_exception: &mut Option<String>,
+  js_error_create_fn: &JSErrorCreateFn,
+) -> Result<(), ErrBox> {
+  match last_exception.take() {
+    None => Ok(()),
+    Some(json_str) => {
+      let v8_exception = V8Exception::from_json(&json_str).unwrap();
+      let js_error = (js_error_create_fn)(v8_exception);
+      Err(js_error)
     }
   }
 }
 
+fn check_promise_exceptions<'s>(
+  scope: &mut impl v8::ToLocal<'s>,
+  pending_promise_exceptions: &mut HashMap<i32, v8::Global<v8::Value>>,
+  js_error_create_fn: &JSErrorCreateFn,
+) -> Result<(), ErrBox> {
+  if let Some(&key) = pending_promise_exceptions.keys().next() {
+    let handle = pending_promise_exceptions.remove(&key).unwrap();
+    let exception = handle.get(scope).expect("empty error handle");
+    exception_to_err_result(scope, exception, js_error_create_fn)
+  } else {
+    Ok(())
+  }
+}
+
+pub fn encode_message_as_json<'a>(
+  scope: &mut impl v8::ToLocal<'a>,
+  message: v8::Local<v8::Message>,
+) -> String {
+  let context = scope.get_current_context().unwrap();
+  let json_obj = bindings::encode_message_as_object(scope, message);
+  let json_string = v8::json::stringify(context, json_obj.into()).unwrap();
+  json_string.to_rust_string_lossy(scope)
+}
+
 pub fn js_check<T>(r: Result<T, ErrBox>) -> T {
   if let Err(e) = r {
     panic!(e.to_string());
@@ -940,60 +933,61 @@ pub mod tests {
 
   #[test]
   fn terminate_execution() {
-    let (tx, rx) = std::sync::mpsc::channel::<bool>();
-    let tx_clone = tx.clone();
-
     let (mut isolate, _dispatch_count) = setup(Mode::Async);
-    let shared = isolate.shared_isolate_handle();
+    // TODO(piscisaureus): in rusty_v8, the `thread_safe_handle()` method
+    // should not require a mutable reference to `struct rusty_v8::Isolate`.
+    let v8_isolate_handle =
+      isolate.v8_isolate.as_mut().unwrap().thread_safe_handle();
 
-    let t1 = std::thread::spawn(move || {
+    let terminator_thread = std::thread::spawn(move || {
       // allow deno to boot and run
       std::thread::sleep(std::time::Duration::from_millis(100));
 
       // terminate execution
-      shared.terminate_execution();
-
-      // allow shutdown
-      std::thread::sleep(std::time::Duration::from_millis(200));
-
-      // unless reported otherwise the test should fail after this point
-      tx_clone.send(false).ok();
+      let ok = v8_isolate_handle.terminate_execution();
+      assert!(ok, true);
     });
 
-    let t2 = std::thread::spawn(move || {
-      // Rn an infinite loop, which should be terminated.
-      match isolate.execute("infinite_loop.js", "for(;;) {}") {
-        Ok(_) => panic!("execution should be terminated"),
-        Err(e) => {
-          assert_eq!(e.to_string(), "Uncaught Error: execution terminated")
-        }
-      };
-
-      // `execute()` returned, which means `terminate_execution()` worked.
-      tx.send(true).ok();
-
-      // Make sure the isolate unusable again.
-      isolate
-        .execute("simple.js", "1 + 1")
-        .expect("execution should be possible again");
-    });
+    // Rn an infinite loop, which should be terminated.
+    match isolate.execute("infinite_loop.js", "for(;;) {}") {
+      Ok(_) => panic!("execution should be terminated"),
+      Err(e) => {
+        assert_eq!(e.to_string(), "Uncaught Error: execution terminated")
+      }
+    };
 
-    rx.recv().expect("execution should be terminated");
+    // Cancel the execution-terminating exception in order to allow script
+    // execution again.
+    // TODO(piscisaureus): in rusty_v8, `cancel_terminate_execution()` should
+    // also be implemented on `struct Isolate`.
+    let ok = isolate
+      .v8_isolate
+      .as_mut()
+      .unwrap()
+      .thread_safe_handle()
+      .cancel_terminate_execution();
+    assert!(ok);
+
+    // Verify that the isolate usable again.
+    isolate
+      .execute("simple.js", "1 + 1")
+      .expect("execution should be possible again");
 
-    t1.join().unwrap();
-    t2.join().unwrap();
+    terminator_thread.join().unwrap();
   }
 
   #[test]
   fn dangling_shared_isolate() {
-    let shared = {
+    let v8_isolate_handle = {
       // isolate is dropped at the end of this block
       let (mut isolate, _dispatch_count) = setup(Mode::Async);
-      isolate.shared_isolate_handle()
+      // TODO(piscisaureus): in rusty_v8, the `thread_safe_handle()` method
+      // should not require a mutable reference to `struct rusty_v8::Isolate`.
+      isolate.v8_isolate.as_mut().unwrap().thread_safe_handle()
     };
 
     // this should not SEGFAULT
-    shared.terminate_execution();
+    v8_isolate_handle.terminate_execution();
   }
 
   #[test]