fix(ext/node): fix vm memory usage and context initialization (#23976)

Fixes https://github.com/denoland/deno/issues/22441
Fixes https://github.com/denoland/deno/issues/23913
Fixes  https://github.com/denoland/deno/issues/23852
Fixes https://github.com/denoland/deno/issues/23917

.github/workflows/ci.generate.ts

@@ -5,7 +5,7 @@ import { stringify } from "jsr:@std/yaml@^0.221/stringify";
 // Bump this number when you want to purge the cache.
 // Note: the tools/release/01_bump_crate_versions.ts script will update this version
 // automatically via regex, so ensure that this line maintains this format.
-const cacheVersion = 95;
+const cacheVersion = 96;
 
 const ubuntuX86Runner = "ubuntu-22.04";
 const ubuntuX86XlRunner = "ubuntu-22.04-xl";
@@ -688,6 +688,16 @@ const ci = {
           ].join("\n"),
           env: { CARGO_PROFILE_DEV_DEBUG: 0 },
         },
+        // Uncomment for remote debugging
+        // {
+        //   name: "Setup tmate session",
+        //   if: [
+        //     "(matrix.job == 'test' || matrix.job == 'bench') &&",
+        //     "matrix.profile == 'release' && (matrix.use_sysroot ||",
+        //     "github.repository == 'denoland/deno')",
+        //   ].join("\n"),
+        //   uses: "mxschmitt/action-tmate@v3",
+        // },
         {
           name: "Build release",
           if: [

.github/workflows/ci.yml

@@ -367,8 +367,8 @@ jobs:
           path: |-
             ~/.cargo/registry/index
             ~/.cargo/registry/cache
-          key: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
+          key: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
-          restore-keys: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
+          restore-keys: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
         if: '!(matrix.skip)'
       - name: Restore cache build output (PR)
         uses: actions/cache/restore@v4
@@ -380,7 +380,7 @@ jobs:
             !./target/*/*.zip
             !./target/*/*.tar.gz
           key: never_saved
-          restore-keys: '95-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-'
+          restore-keys: '96-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-'
       - name: Apply and update mtime cache
         if: '!(matrix.skip) && (!startsWith(github.ref, ''refs/tags/''))'
         uses: ./.github/mtime_cache
@@ -669,7 +669,7 @@ jobs:
             !./target/*/gn_out
             !./target/*/*.zip
             !./target/*/*.tar.gz
-          key: '95-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
+          key: '96-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
   publish-canary:
     name: publish canary
     runs-on: ubuntu-22.04

ext/node/global.rs

@@ -275,7 +275,7 @@ fn current_mode(scope: &mut v8::HandleScope) -> Mode {
   };
   let mut buffer = [MaybeUninit::uninit(); 2048];
   let str = v8_string.to_rust_cow_lossy(scope, &mut buffer);
-  if node_resolver.in_npm_package_with_cache(str) {
+  if str.starts_with("node:") || node_resolver.in_npm_package_with_cache(str) {
     Mode::Node
   } else {
     Mode::Deno

ext/node/lib.rs

@@ -35,6 +35,10 @@ mod resolution;
 pub use ops::ipc::ChildPipeFd;
 pub use ops::ipc::IpcJsonStreamResource;
 pub use ops::v8::VM_CONTEXT_INDEX;
+use ops::vm;
+pub use ops::vm::create_v8_context;
+pub use ops::vm::init_global_template;
+pub use ops::vm::ContextInitMode;
 pub use package_json::PackageJson;
 pub use path::PathClean;
 pub use polyfill::is_builtin_node_module;
@@ -641,7 +645,64 @@ deno_core::extension!(deno_node,
   global_template_middleware = global_template_middleware,
   global_object_middleware = global_object_middleware,
   customizer = |ext: &mut deno_core::Extension| {
-    let mut external_references = Vec::with_capacity(7);
+    let mut external_references = Vec::with_capacity(14);
+
+    vm::GETTER_MAP_FN.with(|getter| {
+      external_references.push(ExternalReference {
+        named_getter: *getter,
+      });
+    });
+    vm::SETTER_MAP_FN.with(|setter| {
+      external_references.push(ExternalReference {
+        named_setter: *setter,
+      });
+    });
+    vm::DELETER_MAP_FN.with(|deleter| {
+      external_references.push(ExternalReference {
+        named_getter: *deleter,
+      },);
+    });
+    vm::ENUMERATOR_MAP_FN.with(|enumerator| {
+      external_references.push(ExternalReference {
+        enumerator: *enumerator,
+      });
+    });
+    vm::DEFINER_MAP_FN.with(|definer| {
+      external_references.push(ExternalReference {
+        named_definer: *definer,
+      });
+    });
+    vm::DESCRIPTOR_MAP_FN.with(|descriptor| {
+      external_references.push(ExternalReference {
+        named_getter: *descriptor,
+      });
+    });
+
+    vm::INDEXED_GETTER_MAP_FN.with(|getter| {
+      external_references.push(ExternalReference {
+        indexed_getter: *getter,
+      });
+    });
+    vm::INDEXED_SETTER_MAP_FN.with(|setter| {
+      external_references.push(ExternalReference {
+        indexed_setter: *setter,
+      });
+    });
+    vm::INDEXED_DELETER_MAP_FN.with(|deleter| {
+      external_references.push(ExternalReference {
+        indexed_getter: *deleter,
+      });
+    });
+    vm::INDEXED_DEFINER_MAP_FN.with(|definer| {
+      external_references.push(ExternalReference {
+        indexed_definer: *definer,
+      });
+    });
+    vm::INDEXED_DESCRIPTOR_MAP_FN.with(|descriptor| {
+      external_references.push(ExternalReference {
+        indexed_getter: *descriptor,
+      });
+    });
 
     global::GETTER_MAP_FN.with(|getter| {
       external_references.push(ExternalReference {

ext/node/ops/vm.rs

@@ -7,6 +7,23 @@ use deno_core::v8;
 
 use super::vm_internal as i;
 
+pub use i::create_v8_context;
+pub use i::init_global_template;
+pub use i::ContextInitMode;
+
+pub use i::DEFINER_MAP_FN;
+pub use i::DELETER_MAP_FN;
+pub use i::DESCRIPTOR_MAP_FN;
+pub use i::ENUMERATOR_MAP_FN;
+pub use i::GETTER_MAP_FN;
+pub use i::SETTER_MAP_FN;
+
+pub use i::INDEXED_DEFINER_MAP_FN;
+pub use i::INDEXED_DELETER_MAP_FN;
+pub use i::INDEXED_DESCRIPTOR_MAP_FN;
+pub use i::INDEXED_GETTER_MAP_FN;
+pub use i::INDEXED_SETTER_MAP_FN;
+
 pub struct Script {
   inner: i::ContextifyScript,
 }

ext/node/ops/vm_internal.rs

@@ -69,9 +69,9 @@ impl ContextifyContext {
     scope: &mut v8::HandleScope,
     sandbox_obj: v8::Local<v8::Object>,
   ) {
-    let tmp = init_global_template(scope);
+    let tmp = init_global_template(scope, ContextInitMode::UseSnapshot);
 
-    let context = create_v8_context(scope, tmp, None);
+    let context = create_v8_context(scope, tmp, ContextInitMode::UseSnapshot);
     Self::from_context(scope, context, sandbox_obj);
   }
 
@@ -84,6 +84,8 @@ impl ContextifyContext {
     let context_state = main_context.get_aligned_pointer_from_embedder_data(
       deno_core::CONTEXT_STATE_SLOT_INDEX,
     );
+    let module_map = main_context
+      .get_aligned_pointer_from_embedder_data(deno_core::MODULE_MAP_SLOT_INDEX);
 
     v8_context.set_security_token(main_context.get_security_token(scope));
     // SAFETY: set embedder data from the creation context
@@ -92,6 +94,10 @@ impl ContextifyContext {
         deno_core::CONTEXT_STATE_SLOT_INDEX,
         context_state,
       );
+      v8_context.set_aligned_pointer_in_embedder_data(
+        deno_core::MODULE_MAP_SLOT_INDEX,
+        module_map,
+      );
     }
 
     let context = v8::Global::new(scope, v8_context);
@@ -106,7 +112,7 @@ impl ContextifyContext {
     // lives longer than the execution context, so this should be safe.
     unsafe {
       v8_context.set_aligned_pointer_in_embedder_data(
-        2,
+        3,
         ptr as *const ContextifyContext as _,
       );
     }
@@ -168,7 +174,10 @@ impl ContextifyContext {
   ) -> Option<&'c ContextifyContext> {
     let context = object.get_creation_context(scope)?;
 
-    let context_ptr = context.get_aligned_pointer_from_embedder_data(2);
+    let context_ptr = context.get_aligned_pointer_from_embedder_data(3);
+    if context_ptr.is_null() {
+      return None;
+    }
     // SAFETY: We are storing a pointer to the ContextifyContext
     // in the embedder data of the v8::Context during creation.
     Some(unsafe { &*(context_ptr as *const ContextifyContext) })
@@ -177,17 +186,31 @@ impl ContextifyContext {
 
 pub const VM_CONTEXT_INDEX: usize = 0;
 
-fn create_v8_context<'a>(
+#[derive(PartialEq)]
-  scope: &mut v8::HandleScope<'a>,
+pub enum ContextInitMode {
+  ForSnapshot,
+  UseSnapshot,
+}
+
+pub fn create_v8_context<'a>(
+  scope: &mut v8::HandleScope<'a, ()>,
   object_template: v8::Local<v8::ObjectTemplate>,
-  snapshot_data: Option<&'static [u8]>,
+  mode: ContextInitMode,
 ) -> v8::Local<'a, v8::Context> {
   let scope = &mut v8::EscapableHandleScope::new(scope);
 
-  let context = if let Some(_snapshot_data) = snapshot_data {
+  let context = if mode == ContextInitMode::UseSnapshot {
     v8::Context::from_snapshot(scope, VM_CONTEXT_INDEX).unwrap()
   } else {
-    v8::Context::new_from_template(scope, object_template)
+    let ctx = v8::Context::new_from_template(scope, object_template);
+    // SAFETY: ContextifyContexts will update this to a pointer to the native object
+    unsafe {
+      ctx.set_aligned_pointer_in_embedder_data(1, std::ptr::null_mut());
+      ctx.set_aligned_pointer_in_embedder_data(2, std::ptr::null_mut());
+      ctx.set_aligned_pointer_in_embedder_data(3, std::ptr::null_mut());
+      ctx.clear_all_slots(scope);
+    };
+    ctx
   };
 
   scope.escape(context)
@@ -196,25 +219,31 @@ fn create_v8_context<'a>(
 #[derive(Debug, Clone)]
 struct SlotContextifyGlobalTemplate(v8::Global<v8::ObjectTemplate>);
 
-fn init_global_template<'a>(
+pub fn init_global_template<'a>(
-  scope: &mut v8::HandleScope<'a>,
+  scope: &mut v8::HandleScope<'a, ()>,
+  mode: ContextInitMode,
 ) -> v8::Local<'a, v8::ObjectTemplate> {
-  let mut maybe_object_template_slot =
+  let maybe_object_template_slot =
     scope.get_slot::<SlotContextifyGlobalTemplate>();
 
   if maybe_object_template_slot.is_none() {
-    init_global_template_inner(scope);
+    let global_object_template = init_global_template_inner(scope);
-    maybe_object_template_slot =
+
-      scope.get_slot::<SlotContextifyGlobalTemplate>();
+    if mode == ContextInitMode::UseSnapshot {
+      let contextify_global_template_slot = SlotContextifyGlobalTemplate(
+        v8::Global::new(scope, global_object_template),
+      );
+      scope.set_slot(contextify_global_template_slot);
     }
+    global_object_template
+  } else {
     let object_template_slot = maybe_object_template_slot
       .expect("ContextifyGlobalTemplate slot should be already populated.")
       .clone();
     v8::Local::new(scope, object_template_slot.0)
+  }
 }
 
-extern "C" fn c_noop(_: *const v8::FunctionCallbackInfo) {}
-
 // Using thread_local! to get around compiler bug.
 //
 // See NOTE in ext/node/global.rs#L12
@@ -235,11 +264,11 @@ thread_local! {
   pub static INDEXED_DESCRIPTOR_MAP_FN: v8::IndexedPropertyGetterCallback<'static> = indexed_property_descriptor.map_fn_to();
 }
 
-fn init_global_template_inner(scope: &mut v8::HandleScope) {
+pub fn init_global_template_inner<'a>(
-  let global_func_template =
+  scope: &mut v8::HandleScope<'a, ()>,
-    v8::FunctionTemplate::builder_raw(c_noop).build(scope);
+) -> v8::Local<'a, v8::ObjectTemplate> {
-  let global_object_template = global_func_template.instance_template(scope);
+  let global_object_template = v8::ObjectTemplate::new(scope);
-  global_object_template.set_internal_field_count(2);
+  global_object_template.set_internal_field_count(3);
 
   let named_property_handler_config = {
     let mut config = v8::NamedPropertyHandlerConfiguration::new()
@@ -279,10 +308,8 @@ fn init_global_template_inner(scope: &mut v8::HandleScope) {
     .set_named_property_handler(named_property_handler_config);
   global_object_template
     .set_indexed_property_handler(indexed_property_handler_config);
-  let contextify_global_template_slot = SlotContextifyGlobalTemplate(
+
-    v8::Global::new(scope, global_object_template),
+  global_object_template
-  );
-  scope.set_slot(contextify_global_template_slot);
 }
 
 fn property_getter<'s>(
@@ -291,7 +318,9 @@ fn property_getter<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   mut ret: v8::ReturnValue,
 ) -> v8::Intercepted {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return v8::Intercepted::No;
+  };
 
   let sandbox = ctx.sandbox(scope);
 
@@ -325,7 +354,9 @@ fn property_setter<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   mut rv: v8::ReturnValue,
 ) -> v8::Intercepted {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return v8::Intercepted::No;
+  };
 
   let (attributes, is_declared_on_global_proxy) = match ctx
     .global_proxy(scope)
@@ -416,7 +447,9 @@ fn property_deleter<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   mut rv: v8::ReturnValue,
 ) -> v8::Intercepted {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return v8::Intercepted::No;
+  };
 
   let context = ctx.context(scope);
   let sandbox = ctx.sandbox(scope);
@@ -434,7 +467,9 @@ fn property_enumerator<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   mut rv: v8::ReturnValue,
 ) {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return;
+  };
 
   let context = ctx.context(scope);
   let sandbox = ctx.sandbox(scope);
@@ -455,7 +490,9 @@ fn property_definer<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   _: v8::ReturnValue,
 ) -> v8::Intercepted {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return v8::Intercepted::No;
+  };
 
   let context = ctx.context(scope);
   let (attributes, is_declared) = match ctx
@@ -533,7 +570,9 @@ fn property_descriptor<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   mut rv: v8::ReturnValue,
 ) -> v8::Intercepted {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return v8::Intercepted::No;
+  };
 
   let context = ctx.context(scope);
   let sandbox = ctx.sandbox(scope);
@@ -585,7 +624,9 @@ fn indexed_property_deleter<'s>(
   args: v8::PropertyCallbackArguments<'s>,
   mut rv: v8::ReturnValue,
 ) -> v8::Intercepted {
-  let ctx = ContextifyContext::get(scope, args.this()).unwrap();
+  let Some(ctx) = ContextifyContext::get(scope, args.this()) else {
+    return v8::Intercepted::No;
+  };
 
   let context = ctx.context(scope);
   let sandbox = ctx.sandbox(scope);

runtime/snapshot.rs

@@ -285,7 +285,15 @@ pub fn create_runtime_snapshot(
         let isolate = rt.v8_isolate();
         let scope = &mut v8::HandleScope::new(isolate);
 
-        let ctx = v8::Context::new(scope);
+        let tmpl = deno_node::init_global_template(
+          scope,
+          deno_node::ContextInitMode::ForSnapshot,
+        );
+        let ctx = deno_node::create_v8_context(
+          scope,
+          tmpl,
+          deno_node::ContextInitMode::ForSnapshot,
+        );
         assert_eq!(scope.add_context(ctx), deno_node::VM_CONTEXT_INDEX);
       })),
       skip_op_registration: false,

tests/unit_node/vm_test.ts

@@ -148,3 +148,34 @@ reject().catch(() => {})
     script.runInNewContext();
   },
 });
+
+// https://github.com/denoland/deno/issues/22441
+Deno.test({
+  name: "vm runInNewContext module loader",
+  fn() {
+    const code = "import('node:process')";
+    const script = new Script(code);
+    script.runInNewContext();
+  },
+});
+
+// https://github.com/denoland/deno/issues/23913
+Deno.test({
+  name: "vm memory leak crash",
+  fn() {
+    const script = new Script("returnValue = 2+2");
+
+    for (let i = 0; i < 1000; i++) {
+      script.runInNewContext({}, { timeout: 10000 });
+    }
+  },
+});
+
+// https://github.com/denoland/deno/issues/23852
+Deno.test({
+  name: "vm runInThisContext global.foo",
+  fn() {
+    const result = runInThisContext(`global.foo = 1`);
+    assertEquals(result, 1);
+  },
+});