refactor: v8::Isolate::snapshot_creator instead of v8::SnapshotCreator (

denoland#1098)

This commit removes "v8::SnapshotCreator" in favor of
"v8::Isolate::snapshot_creator" API. All methods from
"v8::SnapshotCreator" are now available on the "v8::Isolate".
If a method is called and isolate was not set up as a snapshot
creator then it will panic.

This commit gets rid of long standing unsoundness in typing
that allowed to easily induce a panic or segfault if a user didn't
use snapshot creator API properly.

src/isolate.rs

@@ -6,6 +6,7 @@ use crate::isolate_create_params::raw;
 use crate::isolate_create_params::CreateParams;
 use crate::promise::PromiseRejectMessage;
 use crate::scope::data::ScopeData;
+use crate::snapshot::SnapshotCreator;
 use crate::support::MapFnFrom;
 use crate::support::MapFnTo;
 use crate::support::Opaque;
@@ -17,15 +18,18 @@ use crate::Array;
 use crate::CallbackScope;
 use crate::Context;
 use crate::Data;
+use crate::ExternalReferences;
 use crate::FixedArray;
 use crate::Function;
+use crate::FunctionCodeHandling;
 use crate::HandleScope;
 use crate::Local;
 use crate::Message;
 use crate::Module;
 use crate::Object;
 use crate::Promise;
 use crate::PromiseResolver;
+use crate::StartupData;
 use crate::String;
 use crate::Value;
 
@@ -532,6 +536,13 @@ impl Isolate {
  owned_isolate
  }
 
+ #[allow(clippy::new_ret_no_self)]
+ pub fn snapshot_creator(
+ external_references: Option<&'static ExternalReferences>,
+ ) -> OwnedIsolate {
+ SnapshotCreator::new(external_references)
+ }
+
  /// Initial configuration parameters for a new Isolate.
  #[inline(always)]
  pub fn create_params() -> CreateParams {
@@ -587,6 +598,17 @@ impl Isolate {
  unsafe { &mut *annex_ptr }
  }
 
+ pub(crate) fn set_snapshot_creator(
+ &mut self,
+ snapshot_creator: SnapshotCreator,
+ ) {
+ let prev = self
+ .get_annex_mut()
+ .maybe_snapshot_creator
+ .replace(snapshot_creator);
+ assert!(prev.is_none());
+ }
+
  pub(crate) fn get_finalizer_map(&self) -> &FinalizerMap {
  &self.get_annex().finalizer_map
  }
@@ -1031,9 +1053,7 @@ impl Isolate {
  unsafe { v8__Isolate__HasPendingBackgroundTasks(self) }
  }
 
- /// Disposes the isolate. The isolate must not be entered by any
- /// thread to be disposable.
- unsafe fn dispose(&mut self) {
+ unsafe fn clear_scope_and_annex(&mut self) {
  // Drop the scope stack.
  ScopeData::drop_root(self);
 
@@ -1069,7 +1089,11 @@ impl Isolate {
  // Subtract one from the Arc<IsolateAnnex> reference count.
  Arc::from_raw(annex);
  self.set_data(0, null_mut());
+ }
 
+ /// Disposes the isolate. The isolate must not be entered by any
+ /// thread to be disposable.
+ unsafe fn dispose(&mut self) {
  // No test case in rusty_v8 show this, but there have been situations in
  // deno where dropping Annex before the states causes a segfault.
  v8__Isolate__Dispose(self)
@@ -1102,12 +1126,76 @@ impl Isolate {
  let arg = &mut callback as *mut F as *mut c_void;
  unsafe { v8__HeapProfiler__TakeHeapSnapshot(self, trampoline::<F>, arg) }
  }
+
+ /// Set the default context to be included in the snapshot blob.
+ /// The snapshot will not contain the global proxy, and we expect one or a
+ /// global object template to create one, to be provided upon deserialization.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the isolate was not created using [`Isolate::snapshot_creator`]
+ #[inline(always)]
+ pub fn set_default_context(&mut self, context: Local<Context>) {
+ let snapshot_creator = self
+ .get_annex_mut()
+ .maybe_snapshot_creator
+ .as_mut()
+ .unwrap();
+ snapshot_creator.set_default_context(context);
+ }
+
+ /// Attach arbitrary `v8::Data` to the isolate snapshot, which can be
+ /// retrieved via `HandleScope::get_context_data_from_snapshot_once()` after
+ /// deserialization. This data does not survive when a new snapshot is created
+ /// from an existing snapshot.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the isolate was not created using [`Isolate::snapshot_creator`]
+ #[inline(always)]
+ pub fn add_isolate_data<T>(&mut self, data: Local<T>) -> usize
+ where
+ for<'l> Local<'l, T>: Into<Local<'l, Data>>,
+ {
+ let snapshot_creator = self
+ .get_annex_mut()
+ .maybe_snapshot_creator
+ .as_mut()
+ .unwrap();
+ snapshot_creator.add_isolate_data(data)
+ }
+
+ /// Attach arbitrary `v8::Data` to the context snapshot, which can be
+ /// retrieved via `HandleScope::get_context_data_from_snapshot_once()` after
+ /// deserialization. This data does not survive when a new snapshot is
+ /// created from an existing snapshot.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the isolate was not created using [`Isolate::snapshot_creator`]
+ #[inline(always)]
+ pub fn add_context_data<T>(
+ &mut self,
+ context: Local<Context>,
+ data: Local<T>,
+ ) -> usize
+ where
+ for<'l> Local<'l, T>: Into<Local<'l, Data>>,
+ {
+ let snapshot_creator = self
+ .get_annex_mut()
+ .maybe_snapshot_creator
+ .as_mut()
+ .unwrap();
+ snapshot_creator.add_context_data(context, data)
+ }
 }
 
 pub(crate) struct IsolateAnnex {
  create_param_allocations: Box<dyn Any>,
  slots: HashMap<TypeId, RawSlot, BuildTypeIdHasher>,
  finalizer_map: FinalizerMap,
+ maybe_snapshot_creator: Option<SnapshotCreator>,
  // The `isolate` and `isolate_mutex` fields are there so an `IsolateHandle`
  // (which may outlive the isolate itself) can determine whether the isolate
  // is still alive, and if so, get a reference to it. Safety rules:
@@ -1128,6 +1216,7 @@ impl IsolateAnnex {
  create_param_allocations,
  slots: HashMap::default(),
  finalizer_map: FinalizerMap::default(),
+ maybe_snapshot_creator: None,
  isolate,
  isolate_mutex: Mutex::new(()),
  }
@@ -1272,8 +1361,14 @@ impl OwnedIsolate {
 impl Drop for OwnedIsolate {
  fn drop(&mut self) {
  unsafe {
+ let snapshot_creator = self.get_annex_mut().maybe_snapshot_creator.take();
+ assert!(
+ snapshot_creator.is_none(),
+ "If isolate was created using v8::Isolate::snapshot_creator, you should use v8::OwnedIsolate::create_blob before dropping an isolate."
+ );
  self.exit();
- self.cxx_isolate.as_mut().dispose()
+ self.cxx_isolate.as_mut().clear_scope_and_annex();
+ self.cxx_isolate.as_mut().dispose();
  }
  }
 }
@@ -1303,6 +1398,29 @@ impl AsMut<Isolate> for Isolate {
  }
 }
 
+impl OwnedIsolate {
+ /// Creates a snapshot data blob.
+ /// This must not be called from within a handle scope.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the isolate was not created using [`Isolate::snapshot_creator`]
+ #[inline(always)]
+ pub fn create_blob(
+ mut self,
+ function_code_handling: FunctionCodeHandling,
+ ) -> Option<StartupData> {
+ let mut snapshot_creator =
+ self.get_annex_mut().maybe_snapshot_creator.take().unwrap();
+ ScopeData::get_root_mut(&mut self);
+ unsafe { self.cxx_isolate.as_mut().clear_scope_and_annex() };
+ // The isolate is owned by the snapshot creator; we need to forget it
+ // here as the snapshot creator will drop it when running the destructor.
+ std::mem::forget(self);
+ snapshot_creator.create_blob(function_code_handling)
+ }
+}
+
 impl HeapStatistics {
  #[inline(always)]
  pub fn total_heap_size(&self) -> usize {

src/lib.rs

@@ -131,7 +131,6 @@ pub use scope::TryCatch;
 pub use script::ScriptOrigin;
 pub use script_compiler::CachedData;
 pub use snapshot::FunctionCodeHandling;
-pub use snapshot::SnapshotCreator;
 pub use snapshot::StartupData;
 pub use string::NewStringType;
 pub use string::WriteOptions;

src/snapshot.rs

@@ -89,26 +89,37 @@ pub enum FunctionCodeHandling {
 /// Helper class to create a snapshot data blob.
 #[repr(C)]
 #[derive(Debug)]
-pub struct SnapshotCreator([usize; 1]);
+pub(crate) struct SnapshotCreator([usize; 1]);
 
 impl SnapshotCreator {
  /// Create and enter an isolate, and set it up for serialization.
  /// The isolate is created from scratch.
  #[inline(always)]
- pub fn new(external_references: Option<&'static ExternalReferences>) -> Self {
+ #[allow(clippy::new_ret_no_self)]
+ pub(crate) fn new(
+ external_references: Option<&'static ExternalReferences>,
+ ) -> OwnedIsolate {
  let mut snapshot_creator: MaybeUninit<Self> = MaybeUninit::uninit();
  let external_references_ptr = if let Some(er) = external_references {
  er.as_ptr()
  } else {
  std::ptr::null()
  };
- unsafe {
+ let snapshot_creator = unsafe {
  v8__SnapshotCreator__CONSTRUCT(
  &mut snapshot_creator,
  external_references_ptr,
  );
  snapshot_creator.assume_init()
- }
+ };
+
+ let isolate_ptr =
+ unsafe { v8__SnapshotCreator__GetIsolate(&snapshot_creator) };
+ let mut owned_isolate = OwnedIsolate::new(isolate_ptr);
+ ScopeData::new_root(&mut owned_isolate);
+ owned_isolate.create_annex(Box::new(()));
+ owned_isolate.set_snapshot_creator(snapshot_creator);
+ owned_isolate
  }
 }
 
@@ -123,7 +134,7 @@ impl SnapshotCreator {
  /// The snapshot will not contain the global proxy, and we expect one or a
  /// global object template to create one, to be provided upon deserialization.
  #[inline(always)]
- pub fn set_default_context(&mut self, context: Local<Context>) {
+ pub(crate) fn set_default_context(&mut self, context: Local<Context>) {
  unsafe { v8__SnapshotCreator__SetDefaultContext(self, &*context) };
  }
 
@@ -132,7 +143,7 @@ impl SnapshotCreator {
  /// deserialization. This data does not survive when a new snapshot is created
  /// from an existing snapshot.
  #[inline(always)]
- pub fn add_isolate_data<T>(&mut self, data: Local<T>) -> usize
+ pub(crate) fn add_isolate_data<T>(&mut self, data: Local<T>) -> usize
  where
  for<'l> Local<'l, T>: Into<Local<'l, Data>>,
  {
@@ -144,7 +155,7 @@ impl SnapshotCreator {
  /// deserialization. This data does not survive when a new snapshot is
  /// created from an existing snapshot.
  #[inline(always)]
- pub fn add_context_data<T>(
+ pub(crate) fn add_context_data<T>(
  &mut self,
  context: Local<Context>,
  data: Local<T>,
@@ -160,14 +171,10 @@ impl SnapshotCreator {
  /// Creates a snapshot data blob.
  /// This must not be called from within a handle scope.
  #[inline(always)]
- pub fn create_blob(
+ pub(crate) fn create_blob(
  &mut self,
  function_code_handling: FunctionCodeHandling,
  ) -> Option<StartupData> {
- {
- let isolate = unsafe { &mut *v8__SnapshotCreator__GetIsolate(self) };
- ScopeData::get_root_mut(isolate);
- }
  let blob =
  unsafe { v8__SnapshotCreator__CreateBlob(self, function_code_handling) };
  if blob.data.is_null() {
@@ -178,18 +185,4 @@ impl SnapshotCreator {
  Some(blob)
  }
  }
-
- /// This is marked unsafe because it should be called at most once per
- /// snapshot creator.
- // TODO Because the SnapshotCreator creates its own isolate, we need a way to
- // get an owned handle to it. This is a questionable design which ought to be
- // revisited after the libdeno integration is complete.
- #[inline(always)]
- pub unsafe fn get_owned_isolate(&mut self) -> OwnedIsolate {
- let isolate_ptr = v8__SnapshotCreator__GetIsolate(self);
- let mut owned_isolate = OwnedIsolate::new(isolate_ptr);
- ScopeData::new_root(&mut owned_isolate);
- owned_isolate.create_annex(Box::new(()));
- owned_isolate
- }
 }

tests/slots.rs

@@ -326,23 +326,19 @@ fn dropped_context_slots_on_kept_context() {
 fn clear_all_context_slots() {
  setup();
 
- let mut snapshot_creator = v8::SnapshotCreator::new(None);
- let mut isolate = unsafe { snapshot_creator.get_owned_isolate() };
+ let mut snapshot_creator = v8::Isolate::snapshot_creator(None);
 
  {
- let scope = &mut v8::HandleScope::new(&mut isolate);
+ let scope = &mut v8::HandleScope::new(&mut snapshot_creator);
  let context = v8::Context::new(scope);
  let scope = &mut v8::ContextScope::new(scope, context);
 
  context.set_slot(scope, TestState(0));
  context.clear_all_slots(scope);
  assert!(context.get_slot::<TestState>(scope).is_none());
-
- snapshot_creator.set_default_context(context);
+ scope.set_default_context(context);
  }
 
- std::mem::forget(isolate);
-
  snapshot_creator
  .create_blob(v8::FunctionCodeHandling::Keep)
  .unwrap();