Repository: denoland/deno_core
Pull request #81 (2023-07-23 16:00:57)

feat(realms): Support ESM extensions in realms
This PR adds support for ESM snapshots and `preserve_snapshotted_modules` to realms, as well as fixing a few bugs where `JsRuntime::init_extension_js` was using `JsRuntime` methods instead of `JsRealm` methods in ESM extension-related code.

It also adds realm-specific variants of a few ESM extensions tests.


mmastrac: LGTM -- we can resolve the conflict either by re-applying or using `#[allow(deprecated)]` for now. We should allow for direct source to be allowed in the `extension!` macro to avoid having to use the builder for this case.
andreubotella: > LGTM -- we can resolve the conflict either by re-applying or using `#[allow(deprecated)]` for now. We should allow for direct source to be allowed in the `extension!` macro to avoid having to use the builder for this case.

I was already fixing the conflicts and changing any other new tests to use the `Extension` struct rather than the builder.
mmastrac: Thanks! I filed #90 to allow us to use the `extension!` macro more easily for these tests.

Patch:
diff --git a/core/extensions.rs b/core/extensions.rs
index 501db19..4d4a60d 100644
--- a/core/extensions.rs
+++ b/core/extensions.rs
@@ -246,9 +246,9 @@ macro_rules! extension {
     $(, bounds = [ $( $bound:path : $bound_type:ident ),+ ] )?
     $(, ops_fn = $ops_symbol:ident $( < $ops_param:ident > )? )?
     $(, ops = [ $( $(#[$m:meta])* $( $op:ident )::+ $( < $( $op_param:ident ),* > )?  ),+ $(,)? ] )?
-    $(, esm_entry_point = $esm_entry_point:literal )?
-    $(, esm = [ $( dir $dir_esm:literal , )? $( $esm:literal $( with_specifier $esm_specifier:literal )? ),* $(,)? ] )?
-    $(, js = [ $( dir $dir_js:literal , )? $( $js:literal ),* $(,)? ] )?
+    $(, esm_entry_point = $esm_entry_point:expr )?
+    $(, esm = [ $( dir $dir_esm:expr , )? $( $esm:literal $( with_specifier $esm_specifier:expr )? ),* $(,)? ] )?
+    $(, js = [ $( dir $dir_js:expr , )? $( $js:expr ),* $(,)? ] )?
     $(, options = { $( $options_id:ident : $options_type:ty ),* $(,)? } )?
     $(, middleware = $middleware_fn:expr )?
     $(, state = $state_fn:expr )?
@@ -274,9 +274,20 @@ macro_rules! extension {
           // Computed at compile-time, may be modified at runtime with `Cow`:
           name: stringify!($name),
           deps: &[ $( $( stringify!($dep) ),* )? ],
-          js_files: std::borrow::Cow::Borrowed(&$crate::or!($($crate::include_js_files!( $name $( dir $dir_js , )? $( $js , )* ))?, [])),
-          esm_files: std::borrow::Cow::Borrowed(&$crate::or!($($crate::include_js_files!( $name $( dir $dir_esm , )? $( $esm $( with_specifier $esm_specifier )? , )* ))?, [])),
-          esm_entry_point: $crate::or!($(Some($esm_entry_point))?, None),
+          // Use intermediary `const`s here to disable user expressions which
+          // can't be evaluated at compile-time.
+          js_files: {
+            const V: std::borrow::Cow<'static, [$crate::ExtensionFileSource]> = std::borrow::Cow::Borrowed(&$crate::or!($($crate::include_js_files!( $name $( dir $dir_js , )? $( $js , )* ))?, []));
+            V
+          },
+          esm_files: {
+            const V: std::borrow::Cow<'static, [$crate::ExtensionFileSource]> = std::borrow::Cow::Borrowed(&$crate::or!($($crate::include_js_files!( $name $( dir $dir_esm , )? $( $esm $( with_specifier $esm_specifier )? , )* ))?, []));
+            V
+          },
+          esm_entry_point: {
+            const V: Option<&'static str> = $crate::or!($(Some($esm_entry_point))?, None);
+            V
+          },
           ops: std::borrow::Cow::Borrowed(&[$($(
             $( #[ $m ] )*
             $( $op )::+ $( :: < $($op_param),* > )? :: DECL
@@ -720,7 +731,7 @@ impl ExtensionBuilder {
 #[cfg(not(feature = "include_js_files_for_snapshotting"))]
 #[macro_export]
 macro_rules! include_js_files {
-  ($name:ident dir $dir:literal, $($file:literal $(with_specifier $esm_specifier:literal)?,)+) => {
+  ($name:ident dir $dir:expr, $($file:literal $(with_specifier $esm_specifier:expr)?,)+) => {
     [
       $($crate::ExtensionFileSource {
         specifier: $crate::or!($($esm_specifier)?, concat!("ext:", stringify!($name), "/", $file)),
@@ -731,7 +742,7 @@ macro_rules! include_js_files {
     ]
   };
 
-  ($name:ident $($file:literal $(with_specifier $esm_specifier:literal)?,)+) => {
+  ($name:ident $($file:literal $(with_specifier $esm_specifier:expr)?,)+) => {
     [
       $($crate::ExtensionFileSource {
         specifier: $crate::or!($($esm_specifier)?, concat!("ext:", stringify!($name), "/", $file)),
@@ -746,7 +757,7 @@ macro_rules! include_js_files {
 #[cfg(feature = "include_js_files_for_snapshotting")]
 #[macro_export]
 macro_rules! include_js_files {
-  ($name:ident dir $dir:literal, $($file:literal $(with_specifier $esm_specifier:literal)?,)+) => {
+  ($name:ident dir $dir:expr, $($file:literal $(with_specifier $esm_specifier:expr)?,)+) => {
     [
       $($crate::ExtensionFileSource {
         specifier: $crate::or!($($esm_specifier)?, concat!("ext:", stringify!($name), "/", $file)),
@@ -757,7 +768,7 @@ macro_rules! include_js_files {
     ]
   };
 
-  ($name:ident $($file:literal $(with_specifier $esm_specifier:literal)?,)+) => {
+  ($name:ident $($file:literal $(with_specifier $esm_specifier:expr)?,)+) => {
     [
       $($crate::ExtensionFileSource {
         specifier: $crate::or!($($esm_specifier)?, concat!("ext:", stringify!($name), "/", $file)),
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 2225756..828fcb1 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -211,6 +211,7 @@ pub struct JsRuntime {
   pub(crate) inner: InnerIsolateState,
   pub(crate) allocations: IsolateAllocations,
   extensions: Vec<Extension>,
+  preserve_snapshotted_modules: Option<&'static [&'static str]>,
   event_loop_middlewares: Vec<Box<EventLoopMiddlewareFn>>,
   global_template_middlewares: Vec<Box<GlobalTemplateMiddlewareFn>>,
   global_object_middlewares: Vec<Box<GlobalObjectMiddlewareFn>>,
@@ -381,8 +382,7 @@ pub struct RuntimeOptions {
   pub extensions: Vec<Extension>,
 
   /// If provided, the module map will be cleared and left only with the specifiers
-  /// in this list, with the new names provided. If not provided, the module map is
-  /// left intact.
+  /// in this list. If not provided, the module map is left intact.
   pub preserve_snapshotted_modules: Option<&'static [&'static str]>,
 
   /// V8 snapshot that should be loaded on startup.
@@ -728,6 +728,7 @@ impl JsRuntime {
       global_template_middlewares,
       global_object_middlewares,
       extensions: options.extensions,
+      preserve_snapshotted_modules: options.preserve_snapshotted_modules,
       is_main_runtime: options.is_main,
     };
 
@@ -737,7 +738,8 @@ impl JsRuntime {
       .init_extension_js(&realm, maybe_load_callback)
       .unwrap();
 
-    // If the user has requested that we rename modules
+    // If the embedder has requested to clear the module map resulting from
+    // extensions, possibly with exceptions.
     if let Some(preserve_snapshotted_modules) =
       options.preserve_snapshotted_modules
     {
@@ -848,7 +850,13 @@ impl JsRuntime {
         .module_loader
         .unwrap_or_else(|| Rc::new(NoopModuleLoader));
       let module_map_rc = Rc::new(RefCell::new(ModuleMap::new(loader)));
-      // TODO(andreubotella): Initialize the module map with snapshotted data.
+      if self.init_mode == InitMode::FromSnapshot {
+        let snapshotted_data =
+          snapshot_util::get_snapshotted_data(scope, context);
+        module_map_rc
+          .borrow_mut()
+          .update_with_snapshotted_data(scope, snapshotted_data);
+      }
       context.set_slot(scope, module_map_rc.clone());
 
       let realm = JsRealmInner::new(
@@ -865,7 +873,17 @@ impl JsRuntime {
 
     self.init_extension_js(&realm, None)?;
 
-    // TODO(andreubotella): Handle preserve_snapshotted_modules.
+    // If the embedder has requested to clear the module map resulting from
+    // extensions, possibly with exceptions.
+    if let Some(preserve_snapshotted_modules) =
+      self.preserve_snapshotted_modules
+    {
+      realm
+        .0
+        .module_map()
+        .borrow_mut()
+        .clear_module_map(preserve_snapshotted_modules);
+    }
 
     Ok(realm)
   }
@@ -918,8 +936,9 @@ impl JsRuntime {
         let maybe_esm_entry_point = extension.get_esm_entry_point();
 
         for file_source in extension.get_esm_sources() {
-          self
+          realm
             .load_side_module(
+              self.v8_isolate(),
               &ModuleSpecifier::parse(file_source.specifier)?,
               None,
             )
@@ -948,7 +967,7 @@ impl JsRuntime {
               panic!("{} not present in the module map", specifier)
             })
         };
-        let receiver = self.mod_evaluate(mod_id);
+        let receiver = realm.mod_evaluate(self.v8_isolate(), mod_id);
         self.run_event_loop(false).await?;
         receiver
           .await?


Test patch:
diff --git a/core/runtime/tests/jsrealm.rs b/core/runtime/tests/jsrealm.rs
index 6ca645d..315f586 100644
--- a/core/runtime/tests/jsrealm.rs
+++ b/core/runtime/tests/jsrealm.rs
@@ -2,6 +2,7 @@
 use crate as deno_core;
 use crate::modules::ModuleCode;
 use crate::modules::StaticModuleLoader;
+use crate::runtime::tests::snapshot::generic_es_snapshot_without_runtime_module_loader;
 use crate::*;
 use anyhow::Error;
 use deno_ops::op;
@@ -9,6 +10,7 @@ use futures::channel::oneshot;
 use futures::future::poll_fn;
 use futures::future::Future;
 use futures::FutureExt;
+use std::borrow::Cow;
 use std::cell::RefCell;
 use std::pin::Pin;
 use std::rc::Rc;
@@ -18,6 +20,8 @@ use std::task::Context;
 use std::task::Poll;
 use url::Url;
 
+use super::snapshot::generic_preserve_snapshotted_modules_test;
+
 #[tokio::test]
 async fn test_set_promise_reject_callback_realms() {
   let mut runtime = JsRuntime::new(RuntimeOptions::default());
@@ -799,3 +803,88 @@ async fn test_realm_concurrent_dynamic_imports() {
     v8::PromiseState::Fulfilled
   );
 }
+
+#[test]
+fn es_snapshot() {
+  let startup_data = {
+    let extension = Extension {
+      name: "module_snapshot",
+      esm_files: Cow::Borrowed(&[ExtensionFileSource {
+        specifier: "mod:test",
+        code: ExtensionFileSourceCode::IncludedInBinary(
+          "globalThis.TEST = 'foo'; export const TEST = 'bar';",
+        ),
+      }]),
+      esm_entry_point: Some("mod:test"),
+      ..Default::default()
+    };
+
+    let runtime = JsRuntimeForSnapshot::new(
+      RuntimeOptions {
+        extensions: vec![extension],
+        ..Default::default()
+      },
+      Default::default(),
+    );
+    runtime.snapshot()
+  };
+
+  let mut runtime = JsRuntime::new(RuntimeOptions {
+    module_loader: None,
+    startup_snapshot: Some(Snapshot::JustCreated(startup_data)),
+    ..Default::default()
+  });
+
+  let realm = runtime
+    .create_realm(CreateRealmOptions {
+      module_loader: Some(Rc::new(StaticModuleLoader::new([]))),
+    })
+    .unwrap();
+
+  // The module was evaluated ahead of time
+  {
+    let global_test = realm
+      .execute_script_static(runtime.v8_isolate(), "", "globalThis.TEST")
+      .unwrap();
+    let scope = &mut realm.handle_scope(runtime.v8_isolate());
+    let global_test = v8::Local::new(scope, global_test);
+    assert!(global_test.is_string());
+    assert_eq!(global_test.to_rust_string_lossy(scope).as_str(), "foo");
+  }
+
+  // The module can be imported
+  {
+    let test_export_promise = realm
+      .execute_script_static(
+        runtime.v8_isolate(),
+        "",
+        "import('mod:test').then(module => module.TEST)",
+      )
+      .unwrap();
+    let test_export =
+      futures::executor::block_on(runtime.resolve_value(test_export_promise))
+        .unwrap();
+
+    let scope = &mut realm.handle_scope(runtime.v8_isolate());
+    let test_export = v8::Local::new(scope, test_export);
+    assert!(test_export.is_string());
+    assert_eq!(test_export.to_rust_string_lossy(scope).as_str(), "bar");
+  }
+}
+
+#[test]
+fn es_snapshot_without_runtime_module_loader_in_realm() {
+  generic_es_snapshot_without_runtime_module_loader(true)
+}
+
+#[test]
+fn preserve_snapshotted_modules() {
+  generic_preserve_snapshotted_modules_test(true, true)
+}
+
+/// Test that `RuntimeOptions::preserve_snapshotted_modules` also works without
+/// a snapshot.
+#[test]
+fn non_snapshot_preserve_snapshotted_modules() {
+  generic_preserve_snapshotted_modules_test(false, true)
+}
diff --git a/core/runtime/tests/snapshot.rs b/core/runtime/tests/snapshot.rs
index cb49d91..399d7ff 100644
--- a/core/runtime/tests/snapshot.rs
+++ b/core/runtime/tests/snapshot.rs
@@ -296,8 +296,9 @@ fn es_snapshot() {
   }
 }
 
-#[test]
-fn es_snapshot_without_runtime_module_loader() {
+pub(crate) fn generic_es_snapshot_without_runtime_module_loader(
+  test_realms: bool,
+) {
   let startup_data = {
     let extension = Extension {
       name: "module_snapshot",
@@ -322,15 +323,31 @@ fn es_snapshot_without_runtime_module_loader() {
     runtime.snapshot()
   };
 
-  let mut runtime = JsRuntime::new(RuntimeOptions {
-    module_loader: None,
-    startup_snapshot: Some(Snapshot::JustCreated(startup_data)),
-    ..Default::default()
-  });
+  let mut runtime;
+  let realm;
+  if test_realms {
+    runtime = JsRuntime::new(RuntimeOptions {
+      module_loader: Some(Rc::new(StaticModuleLoader::new([]))),
+      startup_snapshot: Some(Snapshot::JustCreated(startup_data)),
+      ..Default::default()
+    });
+    realm = runtime
+      .create_realm(CreateRealmOptions {
+        module_loader: None,
+      })
+      .unwrap();
+  } else {
+    runtime = JsRuntime::new(RuntimeOptions {
+      module_loader: None,
+      startup_snapshot: Some(Snapshot::JustCreated(startup_data)),
+      ..Default::default()
+    });
+    realm = runtime.main_realm();
+  }
 
   // Make sure the module was evaluated.
   {
-    let scope = &mut runtime.handle_scope();
+    let scope = &mut realm.handle_scope(runtime.v8_isolate());
     let global_test: v8::Local<v8::String> =
       JsRuntime::eval(scope, "globalThis.TEST").unwrap();
     assert_eq!(
@@ -340,16 +357,24 @@ fn es_snapshot_without_runtime_module_loader() {
   }
 
   // We can still import a module that was registered manually
-  let dyn_import_promise = runtime
-    .execute_script_static("", "import('ext:module_snapshot/test.js')")
+  let dyn_import_promise = realm
+    .execute_script_static(
+      runtime.v8_isolate(),
+      "",
+      "import('ext:module_snapshot/test.js')",
+    )
     .unwrap();
   let dyn_import_result =
     futures::executor::block_on(runtime.resolve_value(dyn_import_promise));
   assert!(dyn_import_result.is_ok());
 
   // But not a new one
-  let dyn_import_promise = runtime
-    .execute_script_static("", "import('ext:module_snapshot/test2.js')")
+  let dyn_import_promise = realm
+    .execute_script_static(
+      runtime.v8_isolate(),
+      "",
+      "import('ext:module_snapshot/test2.js')",
+    )
     .unwrap();
   let dyn_import_result =
     futures::executor::block_on(runtime.resolve_value(dyn_import_promise));
@@ -361,97 +386,16 @@ fn es_snapshot_without_runtime_module_loader() {
 }
 
 #[test]
-fn preserve_snapshotted_modules() {
-  let startup_data = {
-    let extension = Extension {
-      name: "module_snapshot",
-      esm_files: Cow::Borrowed(&[
-        ExtensionFileSource {
-          specifier: "test:preserved",
-          code: ExtensionFileSourceCode::IncludedInBinary(
-            "export const TEST = 'foo';",
-          ),
-        },
-        ExtensionFileSource {
-          specifier: "test:not-preserved",
-          code: ExtensionFileSourceCode::IncludedInBinary(
-            "import 'test:preserved'; export const TEST = 'bar';",
-          ),
-        },
-      ]),
-      esm_entry_point: Some("test:not-preserved"),
-      ..Default::default()
-    };
-
-    let runtime = JsRuntimeForSnapshot::new(
-      RuntimeOptions {
-        extensions: vec![extension],
-        ..Default::default()
-      },
-      Default::default(),
-    );
-
-    runtime.snapshot()
-  };
-
-  let loader = Rc::new(LoggingModuleLoader::new(NoopModuleLoader));
-
-  let mut runtime = JsRuntime::new(RuntimeOptions {
-    module_loader: Some(loader.clone()),
-    startup_snapshot: Some(Snapshot::JustCreated(startup_data)),
-    preserve_snapshotted_modules: Some(&["test:preserved"]),
-    ..Default::default()
-  });
-
-  // We can't import "test:not-preserved"
-  {
-    assert_eq!(loader.log(), vec![]);
-    let dyn_import_promise = runtime
-      .execute_script_static("", "import('test:not-preserved')")
-      .unwrap();
-    let dyn_import_result =
-      futures::executor::block_on(runtime.resolve_value(dyn_import_promise));
-    assert!(dyn_import_result.is_err());
-    assert_eq!(
-      dyn_import_result.err().unwrap().to_string().as_str(),
-      "Uncaught TypeError: Module loading is not supported; attempted to load: \"test:not-preserved\" from \"(no referrer)\""
-    );
-    // Ensure that we tried to load `test:not-preserved`
-    assert_eq!(
-      loader.log(),
-      vec![Url::parse("test:not-preserved").unwrap()]
-    );
-  }
-
-  // But we can import "test:preserved"
-  {
-    let dyn_import_promise = runtime
-      .execute_script_static(
-        "",
-        "import('test:preserved').then(module => module.TEST)",
-      )
-      .unwrap();
-    let dyn_import_result =
-      futures::executor::block_on(runtime.resolve_value(dyn_import_promise))
-        .unwrap();
-    let scope = &mut runtime.handle_scope();
-    assert!(dyn_import_result.open(scope).is_string());
-    assert_eq!(
-      dyn_import_result
-        .open(scope)
-        .to_rust_string_lossy(scope)
-        .as_str(),
-      "foo"
-    );
-  }
+fn es_snapshot_without_runtime_module_loader() {
+  generic_es_snapshot_without_runtime_module_loader(false)
 }
 
-/// Test that `RuntimeOptions::preserve_snapshotted_modules` also works without
-/// a snapshot.
-#[test]
-fn non_snapshot_preserve_snapshotted_modules() {
+pub(crate) fn generic_preserve_snapshotted_modules_test(
+  test_snapshot: bool,
+  test_realms: bool,
+) {
   let extension = Extension {
-    name: "esm_extension",
+    name: "module_snapshot",
     esm_files: Cow::Borrowed(&[
       ExtensionFileSource {
         specifier: "test:preserved",
@@ -472,18 +416,50 @@ fn non_snapshot_preserve_snapshotted_modules() {
 
   let loader = Rc::new(LoggingModuleLoader::new(NoopModuleLoader));
 
-  let mut runtime = JsRuntime::new(RuntimeOptions {
-    module_loader: Some(loader.clone()),
-    extensions: vec![extension],
-    preserve_snapshotted_modules: Some(&["test:preserved"]),
-    ..Default::default()
-  });
+  let mut runtime = if test_snapshot {
+    let snapshot_runtime = JsRuntimeForSnapshot::new(
+      RuntimeOptions {
+        extensions: vec![extension],
+        ..Default::default()
+      },
+      Default::default(),
+    );
+    let startup_data = snapshot_runtime.snapshot();
+
+    JsRuntime::new(RuntimeOptions {
+      module_loader: Some(loader.clone()),
+      startup_snapshot: Some(Snapshot::JustCreated(startup_data)),
+      preserve_snapshotted_modules: Some(&["test:preserved"]),
+      ..Default::default()
+    })
+  } else {
+    JsRuntime::new(RuntimeOptions {
+      module_loader: Some(loader.clone()),
+      extensions: vec![extension],
+      preserve_snapshotted_modules: Some(&["test:preserved"]),
+      ..Default::default()
+    })
+  };
+
+  let realm = if test_realms {
+    runtime
+      .create_realm(CreateRealmOptions {
+        module_loader: Some(loader.clone()),
+      })
+      .unwrap()
+  } else {
+    runtime.main_realm()
+  };
 
   // We can't import "test:not-preserved"
   {
     assert_eq!(loader.log(), vec![]);
-    let dyn_import_promise = runtime
-      .execute_script_static("", "import('test:not-preserved')")
+    let dyn_import_promise = realm
+      .execute_script_static(
+        runtime.v8_isolate(),
+        "",
+        "import('test:not-preserved')",
+      )
       .unwrap();
     let dyn_import_result =
       futures::executor::block_on(runtime.resolve_value(dyn_import_promise));
@@ -501,8 +477,9 @@ fn non_snapshot_preserve_snapshotted_modules() {
 
   // But we can import "test:preserved"
   {
-    let dyn_import_promise = runtime
+    let dyn_import_promise = realm
       .execute_script_static(
+        runtime.v8_isolate(),
         "",
         "import('test:preserved').then(module => module.TEST)",
       )
@@ -510,7 +487,7 @@ fn non_snapshot_preserve_snapshotted_modules() {
     let dyn_import_result =
       futures::executor::block_on(runtime.resolve_value(dyn_import_promise))
         .unwrap();
-    let scope = &mut runtime.handle_scope();
+    let scope = &mut realm.handle_scope(runtime.v8_isolate());
     assert!(dyn_import_result.open(scope).is_string());
     assert_eq!(
       dyn_import_result
@@ -521,3 +498,15 @@ fn non_snapshot_preserve_snapshotted_modules() {
     );
   }
 }
+
+#[test]
+fn preserve_snapshotted_modules() {
+  generic_preserve_snapshotted_modules_test(true, false)
+}
+
+/// Test that `RuntimeOptions::preserve_snapshotted_modules` also works without
+/// a snapshot.
+#[test]
+fn non_snapshot_preserve_snapshotted_modules() {
+  generic_preserve_snapshotted_modules_test(false, false)
+}