From 175f973a323791b69d1d17191224aeecd620e754 Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Tue, 20 Dec 2022 11:09:16 -0800 Subject: [PATCH] fix(core): Have custom errors be created in the right realm (#17050) Although PR #16366 did not fully revert `deno_core`'s support for realms, since `JsRealm` still existed after that, it did remove the `ContextState` struct, originally introduced in #14734. This change made `js_build_custom_error_cb`, among other properties, a per-runtime callback, rather than per-realm, which cause a bug where errors thrown from an op would always be constructed in the main realm, rather than in the current one. This change adds back `ContextState` to fix this bug, adds back the `known_realms` field of `JsRuntimeState` (needed to be able to drop the callback when snapshotting), and also relands #14750, which adds the `js_realm_sync_ops` test for this bug that was removed in #16366. --- core/error.rs | 3 +- core/runtime.rs | 148 ++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 134 insertions(+), 17 deletions(-) diff --git a/core/error.rs b/core/error.rs index 2ee97c0acb..dc50b4d73b 100644 --- a/core/error.rs +++ b/core/error.rs @@ -1,6 +1,7 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. use crate::runtime::GetErrorClassFn; +use crate::runtime::JsRealm; use crate::runtime::JsRuntime; use crate::source_map::apply_source_map; use crate::source_map::get_source_line; @@ -98,7 +99,7 @@ pub fn to_v8_error<'a>( error: &Error, ) -> v8::Local<'a, v8::Value> { let tc_scope = &mut v8::TryCatch::new(scope); - let cb = JsRuntime::state(tc_scope) + let cb = JsRealm::state_from_scope(tc_scope) .borrow() .js_build_custom_error_cb .clone() diff --git a/core/runtime.rs b/core/runtime.rs index 498441a6d4..2e417bb9d3 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -148,16 +148,21 @@ pub type SharedArrayBufferStore = pub type CompiledWasmModuleStore = CrossIsolateStore; +#[derive(Default)] +pub(crate) struct ContextState { + pub(crate) js_build_custom_error_cb: Option>, +} + /// Internal state for JsRuntime which is stored in one of v8::Isolate's /// embedder slots. pub struct JsRuntimeState { global_realm: Option, + known_realms: Vec>, pub(crate) js_recv_cb: Option>, pub(crate) js_macrotask_cbs: Vec>, pub(crate) js_nexttick_cbs: Vec>, pub(crate) js_promise_reject_cb: Option>, pub(crate) js_format_exception_cb: Option>, - pub(crate) js_build_custom_error_cb: Option>, pub(crate) has_tick_scheduled: bool, pub(crate) js_wasm_streaming_cb: Option>, pub(crate) pending_promise_exceptions: @@ -388,7 +393,6 @@ impl JsRuntime { js_nexttick_cbs: vec![], js_promise_reject_cb: None, js_format_exception_cb: None, - js_build_custom_error_cb: None, has_tick_scheduled: false, js_wasm_streaming_cb: None, source_map_getter: options.source_map_getter, @@ -405,6 +409,7 @@ impl JsRuntime { inspector: None, op_ctxs: vec![].into_boxed_slice(), global_realm: None, + known_realms: Vec::with_capacity(1), })); let weak = Rc::downgrade(&state_rc); @@ -517,6 +522,10 @@ impl JsRuntime { (isolate, snapshot_options) }; + global_context + .open(&mut isolate) + .set_slot(&mut isolate, Rc::>::default()); + op_state.borrow_mut().put(isolate_ptr); let inspector = if options.inspector { Some(JsRuntimeInspector::new( @@ -533,9 +542,12 @@ impl JsRuntime { .unwrap_or_else(|| Rc::new(NoopModuleLoader)); { let mut state = state_rc.borrow_mut(); - state.global_realm = Some(JsRealm(global_context)); + state.global_realm = Some(JsRealm(global_context.clone())); state.op_ctxs = op_ctxs; state.inspector = inspector; + state + .known_realms + .push(v8::Weak::new(&mut isolate, global_context)); } isolate.set_data( Self::STATE_DATA_OFFSET, @@ -632,10 +644,19 @@ impl JsRuntime { &self.state.borrow().op_ctxs, self.snapshot_options, ); + context.set_slot(scope, Rc::>::default()); + + self + .state + .borrow_mut() + .known_realms + .push(v8::Weak::new(scope, context)); + JsRealm::new(v8::Global::new(scope, context)) }; self.init_extension_js(&realm)?; + self.init_realm_cbs(&realm); Ok(realm) } @@ -797,19 +818,36 @@ impl JsRuntime { /// Grabs a reference to core.js' opresolve & syncOpsCache() fn init_cbs(&mut self) { - let scope = &mut self.handle_scope(); - let recv_cb = - Self::eval::(scope, "Deno.core.opresolve").unwrap(); - let recv_cb = v8::Global::new(scope, recv_cb); - let build_custom_error_cb = - Self::eval::(scope, "Deno.core.buildCustomError") - .expect("Deno.core.buildCustomError is undefined in the realm"); - let build_custom_error_cb = v8::Global::new(scope, build_custom_error_cb); - // Put global handles in state - let state_rc = JsRuntime::state(scope); - let mut state = state_rc.borrow_mut(); - state.js_recv_cb.replace(recv_cb); + { + let scope = &mut self.handle_scope(); + let recv_cb = + Self::eval::(scope, "Deno.core.opresolve").unwrap(); + let recv_cb = v8::Global::new(scope, recv_cb); + // Put global handle in state + let state_rc = JsRuntime::state(scope); + let mut state = state_rc.borrow_mut(); + state.js_recv_cb.replace(recv_cb); + } + + // Also run init_realm_cbs for the main realm. + // TODO(@andreubotella): Merge this method back with `init_realm_cbs` when + // `js_recv_cb` is moved to ContextState. + let global_realm = self.global_realm(); + self.init_realm_cbs(&global_realm); + } + + fn init_realm_cbs(&mut self, realm: &JsRealm) { + let build_custom_error_cb = { + let scope = &mut realm.handle_scope(self.v8_isolate()); + let build_custom_error_cb = + Self::eval::(scope, "Deno.core.buildCustomError") + .expect("Deno.core.buildCustomError is undefined in the realm"); + v8::Global::new(scope, build_custom_error_cb) + }; + // Put global handle in the realm's ContextState + let state = realm.state(self.v8_isolate()); state + .borrow_mut() .js_build_custom_error_cb .replace(build_custom_error_cb); } @@ -873,14 +911,26 @@ impl JsRuntime { } // Drop other v8::Global handles before snapshotting { + for weak_context in &self.state.clone().borrow().known_realms { + let v8_isolate = self.v8_isolate(); + if let Some(context) = weak_context.to_global(v8_isolate) { + let realm = JsRealm::new(context.clone()); + let realm_state = realm.state(v8_isolate); + std::mem::take( + &mut realm_state.borrow_mut().js_build_custom_error_cb, + ); + context.open(v8_isolate).clear_all_slots(v8_isolate); + } + } + let mut state = self.state.borrow_mut(); std::mem::take(&mut state.js_recv_cb); std::mem::take(&mut state.js_promise_reject_cb); std::mem::take(&mut state.js_format_exception_cb); std::mem::take(&mut state.js_wasm_streaming_cb); - std::mem::take(&mut state.js_build_custom_error_cb); state.js_macrotask_cbs.clear(); state.js_nexttick_cbs.clear(); + state.known_realms.clear(); } let snapshot_creator = self.v8_isolate.take().unwrap(); @@ -2246,6 +2296,25 @@ impl JsRealm { &self.0 } + fn state(&self, isolate: &mut v8::Isolate) -> Rc> { + self + .context() + .open(isolate) + .get_slot::>>(isolate) + .unwrap() + .clone() + } + + pub(crate) fn state_from_scope( + scope: &mut v8::HandleScope, + ) -> Rc> { + let context = scope.get_current_context(); + context + .get_slot::>>(scope) + .unwrap() + .clone() + } + pub fn handle_scope<'s>( &self, isolate: &'s mut v8::Isolate, @@ -4187,6 +4256,53 @@ Deno.core.ops.op_async_serialize_object_with_numbers_as_keys({ assert_eq!(ret, serde_v8::to_v8(scope, "Test").unwrap()); } + #[test] + fn js_realm_sync_ops() { + // Test that returning a ZeroCopyBuf and throwing an exception from a sync + // op result in objects with prototypes from the right realm. Note that we + // don't test the result of returning structs, because they will be + // serialized to objects with null prototype. + + #[op] + fn op_test(fail: bool) -> Result { + if !fail { + Ok(ZeroCopyBuf::empty()) + } else { + Err(crate::error::type_error("Test")) + } + } + + let mut runtime = JsRuntime::new(RuntimeOptions { + extensions: vec![Extension::builder().ops(vec![op_test::decl()]).build()], + get_error_class_fn: Some(&|error| { + crate::error::get_custom_error_class(error).unwrap() + }), + ..Default::default() + }); + let new_realm = runtime.create_realm().unwrap(); + + // Test in both realms + for realm in [runtime.global_realm(), new_realm].into_iter() { + let ret = realm + .execute_script( + runtime.v8_isolate(), + "", + r#" + const buf = Deno.core.ops.op_test(false); + try { + Deno.core.ops.op_test(true); + } catch(e) { + err = e; + } + buf instanceof Uint8Array && buf.byteLength === 0 && + err instanceof TypeError && err.message === "Test" + "#, + ) + .unwrap(); + assert!(ret.open(runtime.v8_isolate()).is_true()); + } + } + #[test] fn test_array_by_copy() { // Verify that "array by copy" proposal is enabled (https://github.com/tc39/proposal-change-array-by-copy)