From 2929ddabaab016520eb47411fcd8eb543fb256dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giovanny=20Guti=C3=A9rrez?= Date: Tue, 6 Sep 2022 07:35:04 -0500 Subject: [PATCH] fix(core): Register external references for imports to the SnapshotCreator (#15621) Several functions used for handling of dynamic imports and "import.meta" object were not registered as external references and caused V8 to crash during snapshotting. These functions are now registered as external refs and aborts are no longer happening. --- core/bindings.rs | 119 +++++++++++++++++++++++++++++------------------ core/modules.rs | 83 +++++++++++++++++++++++++++++++++ core/runtime.rs | 6 +-- 3 files changed, 159 insertions(+), 49 deletions(-) diff --git a/core/bindings.rs b/core/bindings.rs index eea16b13ae..7f4a0ebd96 100644 --- a/core/bindings.rs +++ b/core/bindings.rs @@ -19,9 +19,20 @@ pub fn external_references( ops: &[OpCtx], snapshot_loaded: bool, ) -> v8::ExternalReferences { - let mut references = vec![v8::ExternalReference { - function: call_console.map_fn_to(), - }]; + let mut references = vec![ + v8::ExternalReference { + function: call_console.map_fn_to(), + }, + v8::ExternalReference { + function: import_meta_resolve.map_fn_to(), + }, + v8::ExternalReference { + function: catch_dynamic_import_promise_error.map_fn_to(), + }, + v8::ExternalReference { + function: empty_fn.map_fn_to(), + }, + ]; for ctx in ops { let ctx_ptr = ctx as *const OpCtx as _; @@ -292,47 +303,11 @@ pub extern "C" fn host_import_module_dynamically_callback( // ones rethrown from this scope, so they include the call stack of the // dynamic import site. Error objects without any stack frames are assumed to // be module resolution errors, other exception values are left as they are. - let map_err = |scope: &mut v8::HandleScope, - args: v8::FunctionCallbackArguments, - _rv: v8::ReturnValue| { - let arg = args.get(0); - if is_instance_of_error(scope, arg) { - let e: crate::error::NativeJsError = - serde_v8::from_v8(scope, arg).unwrap(); - let name = e.name.unwrap_or_else(|| "Error".to_string()); - let message = v8::Exception::create_message(scope, arg); - if message.get_stack_trace(scope).unwrap().get_frame_count() == 0 { - let arg: v8::Local = arg.try_into().unwrap(); - let message_key = v8::String::new(scope, "message").unwrap(); - let message = arg.get(scope, message_key.into()).unwrap(); - let exception = match name.as_str() { - "RangeError" => { - v8::Exception::range_error(scope, message.try_into().unwrap()) - } - "TypeError" => { - v8::Exception::type_error(scope, message.try_into().unwrap()) - } - "SyntaxError" => { - v8::Exception::syntax_error(scope, message.try_into().unwrap()) - } - "ReferenceError" => { - v8::Exception::reference_error(scope, message.try_into().unwrap()) - } - _ => v8::Exception::error(scope, message.try_into().unwrap()), - }; - let code_key = v8::String::new(scope, "code").unwrap(); - let code_value = - v8::String::new(scope, "ERR_MODULE_NOT_FOUND").unwrap(); - let exception_obj = exception.to_object(scope).unwrap(); - exception_obj.set(scope, code_key.into(), code_value.into()); - scope.throw_exception(exception); - return; - } - } - scope.throw_exception(arg); - }; - let map_err = v8::FunctionTemplate::new(scope, map_err); - let map_err = map_err.get_function(scope).unwrap(); + let builder = v8::FunctionBuilder::new(catch_dynamic_import_promise_error); + + let map_err = + v8::FunctionBuilder::::build(builder, scope).unwrap(); + let promise = promise.catch(scope, map_err).unwrap(); &*promise as *const _ as *mut _ @@ -403,6 +378,62 @@ fn import_meta_resolve( }; } +fn empty_fn( + _scope: &mut v8::HandleScope, + _args: v8::FunctionCallbackArguments, + _rv: v8::ReturnValue, +) { + //Do Nothing +} + +//It creates a reference to an empty function which can be mantained after the snapshots +pub fn create_empty_fn<'s>( + scope: &mut v8::HandleScope<'s>, +) -> Option> { + let empty_fn = v8::FunctionTemplate::new(scope, empty_fn); + empty_fn.get_function(scope) +} + +fn catch_dynamic_import_promise_error( + scope: &mut v8::HandleScope, + args: v8::FunctionCallbackArguments, + _rv: v8::ReturnValue, +) { + let arg = args.get(0); + if is_instance_of_error(scope, arg) { + let e: crate::error::NativeJsError = serde_v8::from_v8(scope, arg).unwrap(); + let name = e.name.unwrap_or_else(|| "Error".to_string()); + let message = v8::Exception::create_message(scope, arg); + if message.get_stack_trace(scope).unwrap().get_frame_count() == 0 { + let arg: v8::Local = arg.try_into().unwrap(); + let message_key = v8::String::new(scope, "message").unwrap(); + let message = arg.get(scope, message_key.into()).unwrap(); + let exception = match name.as_str() { + "RangeError" => { + v8::Exception::range_error(scope, message.try_into().unwrap()) + } + "TypeError" => { + v8::Exception::type_error(scope, message.try_into().unwrap()) + } + "SyntaxError" => { + v8::Exception::syntax_error(scope, message.try_into().unwrap()) + } + "ReferenceError" => { + v8::Exception::reference_error(scope, message.try_into().unwrap()) + } + _ => v8::Exception::error(scope, message.try_into().unwrap()), + }; + let code_key = v8::String::new(scope, "code").unwrap(); + let code_value = v8::String::new(scope, "ERR_MODULE_NOT_FOUND").unwrap(); + let exception_obj = exception.to_object(scope).unwrap(); + exception_obj.set(scope, code_key.into(), code_value.into()); + scope.throw_exception(exception); + return; + } + } + scope.throw_exception(arg); +} + pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) { use v8::PromiseRejectEvent::*; diff --git a/core/modules.rs b/core/modules.rs index 545ad54d84..65b3852d91 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -1145,6 +1145,7 @@ mod tests { use crate::Extension; use crate::JsRuntime; use crate::RuntimeOptions; + use crate::Snapshot; use deno_ops::op; use futures::future::FutureExt; use parking_lot::Mutex; @@ -2305,4 +2306,86 @@ if (import.meta.url != 'file:///main_with_code.js') throw Error(); let _ = runtime.mod_evaluate(side_id); futures::executor::block_on(runtime.run_event_loop(false)).unwrap(); } + + #[test] + fn dynamic_imports_snapshot() { + //TODO: Once the issue with the ModuleNamespaceEntryGetter is fixed, we can maintain a reference to the module + // and use it when loading the snapshot + let snapshot = { + const MAIN_WITH_CODE_SRC: &str = r#" + await import("./b.js"); + "#; + + let loader = MockLoader::new(); + let mut runtime = JsRuntime::new(RuntimeOptions { + module_loader: Some(loader), + will_snapshot: true, + ..Default::default() + }); + // In default resolution code should be empty. + // Instead we explicitly pass in our own code. + // The behavior should be very similar to /a.js. + let spec = resolve_url("file:///main_with_code.js").unwrap(); + let main_id_fut = runtime + .load_main_module(&spec, Some(MAIN_WITH_CODE_SRC.to_owned())) + .boxed_local(); + let main_id = futures::executor::block_on(main_id_fut).unwrap(); + + let _ = runtime.mod_evaluate(main_id); + futures::executor::block_on(runtime.run_event_loop(false)).unwrap(); + runtime.snapshot() + }; + + let snapshot = Snapshot::JustCreated(snapshot); + let mut runtime2 = JsRuntime::new(RuntimeOptions { + startup_snapshot: Some(snapshot), + ..Default::default() + }); + + //Evaluate the snapshot with an empty function + runtime2.execute_script("check.js", "true").unwrap(); + } + + #[test] + fn import_meta_snapshot() { + let snapshot = { + const MAIN_WITH_CODE_SRC: &str = r#" + if (import.meta.url != 'file:///main_with_code.js') throw Error(); + globalThis.meta = import.meta; + globalThis.url = import.meta.url; + "#; + + let loader = MockLoader::new(); + let mut runtime = JsRuntime::new(RuntimeOptions { + module_loader: Some(loader), + will_snapshot: true, + ..Default::default() + }); + // In default resolution code should be empty. + // Instead we explicitly pass in our own code. + // The behavior should be very similar to /a.js. + let spec = resolve_url("file:///main_with_code.js").unwrap(); + let main_id_fut = runtime + .load_main_module(&spec, Some(MAIN_WITH_CODE_SRC.to_owned())) + .boxed_local(); + let main_id = futures::executor::block_on(main_id_fut).unwrap(); + + let _ = runtime.mod_evaluate(main_id); + futures::executor::block_on(runtime.run_event_loop(false)).unwrap(); + runtime.snapshot() + }; + + let snapshot = Snapshot::JustCreated(snapshot); + let mut runtime2 = JsRuntime::new(RuntimeOptions { + startup_snapshot: Some(snapshot), + ..Default::default() + }); + + runtime2 + .execute_script( + "check.js", + "if (globalThis.url !== 'file:///main_with_code.js') throw Error('x')", + ) + .unwrap(); + } } diff --git a/core/runtime.rs b/core/runtime.rs index 42496b2712..4aad2bd765 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -1283,11 +1283,7 @@ impl JsRuntime { ); let promise = v8::Local::::try_from(value) .expect("Expected to get promise as module evaluation result"); - let empty_fn = |_scope: &mut v8::HandleScope, - _args: v8::FunctionCallbackArguments, - _rv: v8::ReturnValue| {}; - let empty_fn = v8::FunctionTemplate::new(tc_scope, empty_fn); - let empty_fn = empty_fn.get_function(tc_scope).unwrap(); + let empty_fn = bindings::create_empty_fn(tc_scope).unwrap(); promise.catch(tc_scope, empty_fn); let mut state = state_rc.borrow_mut(); let promise_global = v8::Global::new(tc_scope, promise);