Abide by the rules when passing Isolate between c and rust

Ensure that at most one mutable Isolate reference exists at a time.

`deno_execute()` and `deno_respond()` now borrow a reference to the rust-side
isolate from the caller. When we need a reference to the isolate while one of
these functions is on the stack, `deno_get_data()` can be used to borrow back
that reference.

Roadmap.md

@@ -131,7 +131,7 @@ void deno_set_callback(Deno* deno, deno_sub_cb cb);
 // Get error text with deno_last_exception().
 // 0 = success, non-zero = failure.
 // TODO(ry) Currently the return code has opposite semantics.
-int deno_execute(Deno* d, const char* js_filename, const char* js_source);
+int deno_execute(Deno* d, void* user_data, const char* js_filename, const char* js_source);
 
 // This call doesn't go into JS. This is thread-safe.
 // TODO(ry) Currently this is called deno_pub. It should be renamed.

libdeno/binding.cc

@@ -401,6 +401,24 @@ void AddIsolate(Deno* d, v8::Isolate* isolate) {
   d->isolate->SetData(0, d);
 }
 
+class UserDataScope {
+  Deno* deno;
+  void* prev_data;
+  void* data;  // Not necessary; only for sanity checking.
+
+ public:
+  UserDataScope(Deno* deno_, void* data_) : deno(deno_), data(data_) {
+    CHECK(deno->user_data == nullptr || deno->user_data == data_);
+    prev_data = deno->user_data;
+    deno->user_data = data;
+  }
+
+  ~UserDataScope() {
+    CHECK(deno->user_data == data);
+    deno->user_data = prev_data;
+  }
+};
+
 }  // namespace deno
 
 extern "C" {
@@ -413,7 +431,10 @@ void deno_init() {
   v8::V8::Initialize();
 }
 
-void* deno_get_data(Deno* d) { return d->user_data; }
+void* deno_get_data(const Deno* d) {
+  CHECK(d->user_data != nullptr);
+  return d->user_data;
+}
 
 const char* deno_v8_version() { return v8::V8::GetVersion(); }
 
@@ -423,7 +444,9 @@ void deno_set_v8_flags(int* argc, char** argv) {
 
 const char* deno_last_exception(Deno* d) { return d->last_exception.c_str(); }
 
-int deno_execute(Deno* d, const char* js_filename, const char* js_source) {
+int deno_execute(Deno* d, void* user_data, const char* js_filename,
+                 const char* js_source) {
+  deno::UserDataScope user_data_scope(d, user_data);
   auto* isolate = d->isolate;
   v8::Locker locker(isolate);
   v8::Isolate::Scope isolate_scope(isolate);
@@ -432,7 +455,7 @@ int deno_execute(Deno* d, const char* js_filename, const char* js_source) {
   return deno::Execute(context, js_filename, js_source) ? 1 : 0;
 }
 
-int deno_respond(Deno* d, int32_t req_id, deno_buf buf) {
+int deno_respond(Deno* d, void* user_data, int32_t req_id, deno_buf buf) {
   if (d->currentArgs != nullptr) {
     // Synchronous response.
     auto ab = deno::ImportBuf(d->isolate, buf);
@@ -442,7 +465,7 @@ int deno_respond(Deno* d, int32_t req_id, deno_buf buf) {
   }
 
   // Asynchronous response.
-
+  deno::UserDataScope user_data_scope(d, user_data);
   v8::Locker locker(d->isolate);
   v8::Isolate::Scope isolate_scope(d->isolate);
   v8::HandleScope handle_scope(d->isolate);

libdeno/deno.h

@@ -30,7 +30,7 @@ void deno_init();
 const char* deno_v8_version();
 void deno_set_v8_flags(int* argc, char** argv);
 
-Deno* deno_new(void* user_data, deno_recv_cb cb);
+Deno* deno_new(deno_recv_cb cb);
 void deno_delete(Deno* d);
 
 // Returns the void* user_data provided in deno_new.
@@ -39,7 +39,8 @@ void* deno_get_data(Deno*);
 // Returns false on error.
 // Get error text with deno_last_exception().
 // 0 = fail, 1 = success
-int deno_execute(Deno* d, const char* js_filename, const char* js_source);
+int deno_execute(Deno* d, void* user_data, const char* js_filename,
+                 const char* js_source);
 
 // deno_respond sends up to one message back for every deno_recv_cb made.
 //
@@ -59,7 +60,7 @@ int deno_execute(Deno* d, const char* js_filename, const char* js_source);
 //
 // A non-zero return value, means a JS exception was encountered during the
 // libdeno.recv() callback. Check deno_last_exception() for exception text.
-int deno_respond(Deno* d, int32_t req_id, deno_buf buf);
+int deno_respond(Deno* d, void* user_data, int32_t req_id, deno_buf buf);
 
 const char* deno_last_exception(Deno* d);
 

libdeno/from_filesystem.cc

@@ -14,7 +14,7 @@
 
 namespace deno {
 
-Deno* NewFromFileSystem(void* user_data, deno_recv_cb cb) {
+Deno* NewFromFileSystem(deno_recv_cb cb) {
   std::string exe_path;
   CHECK(deno::ExePath(&exe_path));
   std::string exe_dir = deno::Dirname(exe_path);  // Always ends with a slash.
@@ -30,7 +30,7 @@ Deno* NewFromFileSystem(void* user_data, deno_recv_cb cb) {
   Deno* d = new Deno;
   d->currentArgs = nullptr;
   d->cb = cb;
-  d->user_data = user_data;
+  d->user_data = nullptr;
   v8::Isolate::CreateParams params;
   params.array_buffer_allocator =
       v8::ArrayBuffer::Allocator::NewDefaultAllocator();
@@ -55,7 +55,5 @@ Deno* NewFromFileSystem(void* user_data, deno_recv_cb cb) {
 }  // namespace deno
 
 extern "C" {
-Deno* deno_new(void* user_data, deno_recv_cb cb) {
-  return deno::NewFromFileSystem(user_data, cb);
-}
+Deno* deno_new(deno_recv_cb cb) { return deno::NewFromFileSystem(cb); }
 }

libdeno/from_snapshot.cc

@@ -43,11 +43,11 @@ void DeserializeInternalFields(v8::Local<v8::Object> holder, int index,
   deserialized_data.push_back(embedder_field);
 }
 
-Deno* NewFromSnapshot(void* user_data, deno_recv_cb cb) {
+Deno* NewFromSnapshot(deno_recv_cb cb) {
   Deno* d = new Deno;
   d->currentArgs = nullptr;
   d->cb = cb;
-  d->user_data = user_data;
+  d->user_data = nullptr;
   v8::Isolate::CreateParams params;
   params.array_buffer_allocator =
       v8::ArrayBuffer::Allocator::NewDefaultAllocator();
@@ -80,7 +80,5 @@ Deno* NewFromSnapshot(void* user_data, deno_recv_cb cb) {
 }  // namespace deno
 
 extern "C" {
-Deno* deno_new(void* user_data, deno_recv_cb cb) {
-  return deno::NewFromSnapshot(user_data, cb);
-}
+Deno* deno_new(deno_recv_cb cb) { return deno::NewFromSnapshot(cb); }
 }

libdeno/libdeno_test.cc

@@ -4,21 +4,21 @@
 #include "deno.h"
 
 TEST(LibDenoTest, InitializesCorrectly) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_TRUE(deno_execute(d, "a.js", "1 + 2"));
+  Deno* d = deno_new(nullptr);
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "1 + 2"));
   deno_delete(d);
 }
 
 TEST(LibDenoTest, CanCallFunction) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_TRUE(deno_execute(d, "a.js",
+  Deno* d = deno_new(nullptr);
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js",
                            "if (CanCallFunction() != 'foo') throw Error();"));
   deno_delete(d);
 }
 
 TEST(LibDenoTest, ErrorsCorrectly) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_FALSE(deno_execute(d, "a.js", "throw Error()"));
+  Deno* d = deno_new(nullptr);
+  EXPECT_FALSE(deno_execute(d, nullptr, "a.js", "throw Error()"));
   deno_delete(d);
 }
 
@@ -54,7 +54,7 @@ void assert_null(deno_buf b) {
 
 TEST(LibDenoTest, RecvReturnEmpty) {
   static int count = 0;
-  Deno* d = deno_new(nullptr, [](auto _, int req_id, auto buf, auto data_buf) {
+  Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) {
     assert_null(data_buf);
     count++;
     EXPECT_EQ(static_cast<size_t>(3), buf.data_len);
@@ -62,38 +62,36 @@ TEST(LibDenoTest, RecvReturnEmpty) {
     EXPECT_EQ(buf.data_ptr[1], 'b');
     EXPECT_EQ(buf.data_ptr[2], 'c');
   });
-  EXPECT_TRUE(deno_execute(d, "a.js", "RecvReturnEmpty()"));
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "RecvReturnEmpty()"));
   EXPECT_EQ(count, 2);
   deno_delete(d);
 }
 
 TEST(LibDenoTest, RecvReturnBar) {
   static int count = 0;
-  Deno* d =
-      deno_new(nullptr, [](auto deno, int req_id, auto buf, auto data_buf) {
+  Deno* d = deno_new([](auto deno, int req_id, auto buf, auto data_buf) {
     assert_null(data_buf);
     count++;
     EXPECT_EQ(static_cast<size_t>(3), buf.data_len);
     EXPECT_EQ(buf.data_ptr[0], 'a');
     EXPECT_EQ(buf.data_ptr[1], 'b');
     EXPECT_EQ(buf.data_ptr[2], 'c');
-        deno_respond(deno, req_id, strbuf("bar"));
+    deno_respond(deno, nullptr, req_id, strbuf("bar"));
   });
-  EXPECT_TRUE(deno_execute(d, "a.js", "RecvReturnBar()"));
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "RecvReturnBar()"));
   EXPECT_EQ(count, 1);
   deno_delete(d);
 }
 
 TEST(LibDenoTest, DoubleRecvFails) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_FALSE(deno_execute(d, "a.js", "DoubleRecvFails()"));
+  Deno* d = deno_new(nullptr);
+  EXPECT_FALSE(deno_execute(d, nullptr, "a.js", "DoubleRecvFails()"));
   deno_delete(d);
 }
 
 TEST(LibDenoTest, SendRecvSlice) {
   static int count = 0;
-  Deno* d =
-      deno_new(nullptr, [](auto deno, int req_id, auto buf, auto data_buf) {
+  Deno* d = deno_new([](auto deno, int req_id, auto buf, auto data_buf) {
     assert_null(data_buf);
     static const size_t alloc_len = 1024;
     size_t i = count++;
@@ -117,16 +115,16 @@ TEST(LibDenoTest, SendRecvSlice) {
     buf2.data_ptr[0] = 200 + i;
     buf2.data_ptr[buf2.data_len - 1] = 200 - i;
     // Send back.
-        deno_respond(deno, req_id, buf2);
+    deno_respond(deno, nullptr, req_id, buf2);
   });
-  EXPECT_TRUE(deno_execute(d, "a.js", "SendRecvSlice()"));
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "SendRecvSlice()"));
   EXPECT_EQ(count, 5);
   deno_delete(d);
 }
 
 TEST(LibDenoTest, JSSendArrayBufferViewTypes) {
   static int count = 0;
-  Deno* d = deno_new(nullptr, [](auto _, int req_id, auto buf, auto data_buf) {
+  Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) {
     assert_null(data_buf);
     count++;
     size_t data_offset = buf.data_ptr - buf.alloc_ptr;
@@ -135,47 +133,47 @@ TEST(LibDenoTest, JSSendArrayBufferViewTypes) {
     EXPECT_EQ(buf.alloc_len, 4321u);
     EXPECT_EQ(buf.data_ptr[0], count);
   });
-  EXPECT_TRUE(deno_execute(d, "a.js", "JSSendArrayBufferViewTypes()"));
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "JSSendArrayBufferViewTypes()"));
   EXPECT_EQ(count, 3);
   deno_delete(d);
 }
 
 TEST(LibDenoTest, TypedArraySnapshots) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_TRUE(deno_execute(d, "a.js", "TypedArraySnapshots()"));
+  Deno* d = deno_new(nullptr);
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "TypedArraySnapshots()"));
   deno_delete(d);
 }
 
 TEST(LibDenoTest, SnapshotBug) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_TRUE(deno_execute(d, "a.js", "SnapshotBug()"));
+  Deno* d = deno_new(nullptr);
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "SnapshotBug()"));
   deno_delete(d);
 }
 
 TEST(LibDenoTest, GlobalErrorHandling) {
   static int count = 0;
-  Deno* d = deno_new(nullptr, [](auto _, int req_id, auto buf, auto data_buf) {
+  Deno* d = deno_new([](auto _, int req_id, auto buf, auto data_buf) {
     assert_null(data_buf);
     count++;
     EXPECT_EQ(static_cast<size_t>(1), buf.data_len);
     EXPECT_EQ(buf.data_ptr[0], 42);
   });
-  EXPECT_FALSE(deno_execute(d, "a.js", "GlobalErrorHandling()"));
+  EXPECT_FALSE(deno_execute(d, nullptr, "a.js", "GlobalErrorHandling()"));
   EXPECT_EQ(count, 1);
   deno_delete(d);
 }
 
 TEST(LibDenoTest, DoubleGlobalErrorHandlingFails) {
-  Deno* d = deno_new(nullptr, nullptr);
-  EXPECT_FALSE(deno_execute(d, "a.js", "DoubleGlobalErrorHandlingFails()"));
+  Deno* d = deno_new(nullptr);
+  EXPECT_FALSE(
+      deno_execute(d, nullptr, "a.js", "DoubleGlobalErrorHandlingFails()"));
   deno_delete(d);
 }
 
 TEST(LibDenoTest, DataBuf) {
   static int count = 0;
   static deno_buf data_buf_copy;
-  Deno* d = deno_new(nullptr,
-                     [](auto _, int req_id, deno_buf buf, deno_buf data_buf) {
+  Deno* d = deno_new([](auto _, int req_id, deno_buf buf, deno_buf data_buf) {
     count++;
     data_buf.data_ptr[0] = 4;
     data_buf.data_ptr[1] = 2;
@@ -185,7 +183,7 @@ TEST(LibDenoTest, DataBuf) {
     EXPECT_EQ(buf.data_ptr[0], 1);
     EXPECT_EQ(buf.data_ptr[1], 2);
   });
-  EXPECT_TRUE(deno_execute(d, "a.js", "DataBuf()"));
+  EXPECT_TRUE(deno_execute(d, nullptr, "a.js", "DataBuf()"));
   EXPECT_EQ(count, 1);
   // data_buf was subsequently changed in JS, let's check that our copy reflects
   // that.

src/isolate.rs

@@ -14,7 +14,6 @@ use libc::c_void;
 use std;
 use std::ffi::CStr;
 use std::ffi::CString;
-use std::sync::atomic;
 use std::sync::mpsc;
 use std::sync::Arc;
 use std::sync::Mutex;
@@ -72,18 +71,18 @@ impl IsolateState {
 static DENO_INIT: std::sync::Once = std::sync::ONCE_INIT;
 
 impl Isolate {
-  pub fn new(argv: Vec<String>, dispatch: Dispatch) -> Box<Isolate> {
+  pub fn new(argv: Vec<String>, dispatch: Dispatch) -> Isolate {
     DENO_INIT.call_once(|| {
       unsafe { libdeno::deno_init() };
     });
 
     let (flags, argv_rest) = flags::set_flags(argv);
-
+    let libdeno_isolate = unsafe { libdeno::deno_new(pre_dispatch) };
     // This channel handles sending async messages back to the runtime.
     let (tx, rx) = mpsc::channel::<(i32, Buf)>();
 
-    let mut isolate = Box::new(Isolate {
-      libdeno_isolate: 0 as *const libdeno::isolate,
+    Isolate {
+      libdeno_isolate,
       dispatch,
       rx,
       ntasks: 0,
@@ -94,24 +93,20 @@ impl Isolate {
         flags,
         tx: Mutex::new(Some(tx)),
       }),
-    });
+    }
+  }
 
-    (*isolate).libdeno_isolate = unsafe {
-      libdeno::deno_new(isolate.as_mut() as *mut _ as *mut c_void, pre_dispatch)
-    };
-
-    isolate
+  pub fn as_void_ptr(&mut self) -> *mut c_void {
+    self as *mut _ as *mut c_void
   }
 
   pub fn from_c<'a>(d: *const libdeno::isolate) -> &'a mut Isolate {
-    let ptr = unsafe { libdeno::deno_get_data(d) };
-    let ptr = ptr as *mut Isolate;
-    let isolate_box = unsafe { Box::from_raw(ptr) };
-    Box::leak(isolate_box)
+    let ptr = unsafe { libdeno::deno_get_data(d) } as *mut _;
+    unsafe { &mut *ptr }
   }
 
   pub fn execute(
-    &self,
+    &mut self,
     js_filename: &str,
     js_source: &str,
   ) -> Result<(), DenoException> {
@@ -120,6 +115,7 @@ impl Isolate {
     let r = unsafe {
       libdeno::deno_execute(
         self.libdeno_isolate,
+        self.as_void_ptr(),
         filename.as_ptr(),
         source.as_ptr(),
       )
@@ -132,10 +128,17 @@ impl Isolate {
     Ok(())
   }
 
-  pub fn respond(&self, req_id: i32, buf: Buf) {
+  pub fn respond(&mut self, req_id: i32, buf: Buf) {
     // TODO(zero-copy) Use Buf::leak(buf) to leak the heap allocated buf. And
     // don't do the memcpy in ImportBuf() (in libdeno/binding.cc)
-    unsafe { libdeno::deno_respond(self.libdeno_isolate, req_id, buf.into()) }
+    unsafe {
+      libdeno::deno_respond(
+        self.libdeno_isolate,
+        self.as_void_ptr(),
+        req_id,
+        buf.into(),
+      )
+    }
   }
 
   fn complete_op(&mut self, req_id: i32, buf: Buf) {
@@ -146,14 +149,21 @@ impl Isolate {
     self.respond(req_id, buf);
   }
 
-  fn timeout(&self) {
+  fn timeout(&mut self) {
     let dummy_buf = libdeno::deno_buf {
       alloc_ptr: 0 as *mut u8,
       alloc_len: 0,
       data_ptr: 0 as *mut u8,
       data_len: 0,
     };
-    unsafe { libdeno::deno_respond(self.libdeno_isolate, -1, dummy_buf) }
+    unsafe {
+      libdeno::deno_respond(
+        self.libdeno_isolate,
+        self.as_void_ptr(),
+        -1,
+        dummy_buf,
+      )
+    }
   }
 
   // TODO Use Park abstraction? Note at time of writing Tokio default runtime
@@ -280,27 +290,6 @@ mod tests {
   use super::*;
   use futures;
 
-  #[test]
-  fn test_c_to_rust() {
-    let argv = vec![String::from("./deno"), String::from("hello.js")];
-    let isolate = Isolate::new(argv, unreachable_dispatch);
-    let isolate2 = Isolate::from_c(isolate.libdeno_isolate);
-    assert_eq!(isolate.libdeno_isolate, isolate2.libdeno_isolate);
-    assert_eq!(
-      isolate.state.dir.root.join("gen"),
-      isolate.state.dir.gen,
-      "Sanity check"
-    );
-  }
-
-  fn unreachable_dispatch(
-    _isolate: &mut Isolate,
-    _control: &[u8],
-    _data: &'static mut [u8],
-  ) -> (bool, Box<Op>) {
-    unreachable!();
-  }
-
   #[test]
   fn test_dispatch_sync() {
     let argv = vec![String::from("./deno"), String::from("hello.js")];

src/libdeno.rs

@@ -30,13 +30,19 @@ extern "C" {
   pub fn deno_init();
   pub fn deno_v8_version() -> *const c_char;
   pub fn deno_set_v8_flags(argc: *mut c_int, argv: *mut *mut c_char);
-  pub fn deno_new(user_data: *mut c_void, cb: DenoRecvCb) -> *const isolate;
+  pub fn deno_new(cb: DenoRecvCb) -> *const isolate;
   pub fn deno_delete(i: *const isolate);
   pub fn deno_last_exception(i: *const isolate) -> *const c_char;
   pub fn deno_get_data(i: *const isolate) -> *mut c_void;
-  pub fn deno_respond(i: *const isolate, req_id: i32, buf: deno_buf);
+  pub fn deno_respond(
+    i: *const isolate,
+    user_data: *mut c_void,
+    req_id: i32,
+    buf: deno_buf,
+  );
   pub fn deno_execute(
     i: *const isolate,
+    user_data: *mut c_void,
     js_filename: *const c_char,
     js_source: *const c_char,
   ) -> c_int;