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
2024-11-21 20:38:55 +00:00 · 2024-06-12 22:32:54 +05:30 · 2024-06-12 22:32:54 +05:30 · bf9e6c4df9
commit bf9e6c4df9
parent 7dcbc3b674
8 changed files with 214 additions and 46 deletions
--- a/.github/workflows/ci.generate.ts
+++ b/.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: [
--- a/.github/workflows/ci.yml
+++ b/.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'') }}'
-          restore-keys: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
+          key: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
+          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
--- a/ext/node/global.rs
+++ b/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
--- a/ext/node/lib.rs
+++ b/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 {
--- a/ext/node/ops/vm.rs
+++ b/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,
 }
--- a/ext/node/ops/vm_internal.rs
+++ b/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>(
-  scope: &mut v8::HandleScope<'a>,
+#[derive(PartialEq)]
+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,24 +219,30 @@ fn create_v8_context<'a>(
 #[derive(Debug, Clone)]
 struct SlotContextifyGlobalTemplate(v8::Global<v8::ObjectTemplate>);

-fn init_global_template<'a>(
-  scope: &mut v8::HandleScope<'a>,
+pub fn init_global_template<'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);
-    maybe_object_template_slot =
-      scope.get_slot::<SlotContextifyGlobalTemplate>();
-  }
-  let object_template_slot = maybe_object_template_slot
-    .expect("ContextifyGlobalTemplate slot should be already populated.")
-    .clone();
-  v8::Local::new(scope, object_template_slot.0)
-}
+    let global_object_template = init_global_template_inner(scope);

-extern "C" fn c_noop(_: *const v8::FunctionCallbackInfo) {}
+    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)
+  }
+}

 // Using thread_local! to get around compiler bug.
 //
@ -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) {
-  let global_func_template =
-    v8::FunctionTemplate::builder_raw(c_noop).build(scope);
-  let global_object_template = global_func_template.instance_template(scope);
-  global_object_template.set_internal_field_count(2);
+pub fn init_global_template_inner<'a>(
+  scope: &mut v8::HandleScope<'a, ()>,
+) -> v8::Local<'a, v8::ObjectTemplate> {
+  let global_object_template = v8::ObjectTemplate::new(scope);
+  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),
-  );
-  scope.set_slot(contextify_global_template_slot);
+
+  global_object_template
 }

 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);
--- a/runtime/snapshot.rs
+++ b/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,
--- a/tests/unit_node/vm_test.ts
+++ b/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);
+  },
+});