refactor(core): Move optional callbacks from JsRuntimeState to `Con…

…textState` (denoland#15599)

The `JsRuntimeState` struct stores a number of JS callbacks that are
used either in the event loop or when interacting with V8. Some of
these callback fields are vectors of callbacks, and therefore could
plausibly store at least one callback per realm. However, some of
those fields are `Option<v8::Global<v8::Function>>`, which would make
the callbacks set by a realm override the one that might have been set
by a different realm.

As it turns out, all of the current such optional callbacks
(`js_promise_reject_cb`, `js_format_exception_cb` and
`js_wasm_streaming_cb`) are only used from inside a realm, and
therefore this change makes it so such callbacks can only be set from
inside a realm, and will only affect that realm.

core/bindings.rs

@@ -7,6 +7,7 @@ use crate::modules::validate_import_assertions;
 use crate::modules::ImportAssertionsKind;
 use crate::modules::ModuleMap;
 use crate::ops::OpCtx;
+use crate::JsRealm;
 use crate::JsRuntime;
 use log::debug;
 use std::option::Option;
@@ -408,15 +409,15 @@ pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) {
  // SAFETY: `CallbackScope` can be safely constructed from `&PromiseRejectMessage`
  let scope = &mut unsafe { v8::CallbackScope::new(&message) };
 
- let state_rc = JsRuntime::state(scope);
- let mut state = state_rc.borrow_mut();
+ let realm_state_rc = JsRealm::state_from_scope(scope);
 
- if let Some(js_promise_reject_cb) = state.js_promise_reject_cb.clone() {
+ if let Some(js_promise_reject_cb) =
+ realm_state_rc.borrow().js_promise_reject_cb.clone()
+ {
  let tc_scope = &mut v8::TryCatch::new(scope);
  let undefined: v8::Local<v8::Value> = v8::undefined(tc_scope).into();
  let type_ = v8::Integer::new(tc_scope, message.get_event() as i32);
  let promise = message.get_promise();
- drop(state); // Drop borrow, callbacks can call back into runtime.
 
  let reason = match message.get_event() {
  PromiseRejectWithNoHandler
@@ -439,6 +440,7 @@ pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) {
  };
 
  if has_unhandled_rejection_handler {
+ let state_rc = JsRuntime::state(tc_scope);
  let mut state = state_rc.borrow_mut();
  if let Some(pending_mod_evaluate) = state.pending_mod_evaluate.as_mut() {
  if !pending_mod_evaluate.has_evaluated {
@@ -449,6 +451,8 @@ pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) {
  }
  }
  } else {
+ let state_rc = JsRuntime::state(scope);
+ let mut state = state_rc.borrow_mut();
  let promise = message.get_promise();
  let promise_global = v8::Global::new(scope, promise);
  match message.get_event() {
@@ -467,7 +471,7 @@ pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) {
  // Should not warn. See #1272
  }
  }
- }
+ };
 }
 
 /// This binding should be used if there's a custom console implementation

core/error.rs

@@ -218,10 +218,10 @@ impl JsError {
  let msg = v8::Exception::create_message(scope, exception);
 
  let mut exception_message = None;
- let state_rc = JsRuntime::state(scope);
+ let realm_state_rc = JsRealm::state_from_scope(scope);
 
  let js_format_exception_cb =
- state_rc.borrow().js_format_exception_cb.clone();
+ realm_state_rc.borrow().js_format_exception_cb.clone();
  if let Some(format_exception_cb) = js_format_exception_cb {
  let format_exception_cb = format_exception_cb.open(scope);
  let this = v8::undefined(scope).into();
@@ -286,6 +286,7 @@ impl JsError {
  let mut source_line = None;
  let mut source_line_frame_index = None;
  {
+ let state_rc = JsRuntime::state(scope);
  let state = &mut *state_rc.borrow_mut();
 
  // When the stack frame array is empty, but the source location given by

core/ops_builtin_v8.rs

@@ -103,8 +103,8 @@ fn op_set_promise_reject_callback<'a>(
  cb: serde_v8::Value,
 ) -> Result<Option<serde_v8::Value<'a>>, Error> {
  let cb = to_v8_fn(scope, cb)?;
- let state_rc = JsRuntime::state(scope);
- let old = state_rc.borrow_mut().js_promise_reject_cb.replace(cb);
+ let realm_state_rc = JsRealm::state_from_scope(scope);
+ let old = realm_state_rc.borrow_mut().js_promise_reject_cb.replace(cb);
  let old = old.map(|v| v8::Local::new(scope, v));
  Ok(old.map(|v| from_v8(scope, v.into()).unwrap()))
 }
@@ -641,22 +641,28 @@ fn op_set_wasm_streaming_callback(
  cb: serde_v8::Value,
 ) -> Result<(), Error> {
  let cb = to_v8_fn(scope, cb)?;
- let state_rc = JsRuntime::state(scope);
- let mut state = state_rc.borrow_mut();
+ let realm_state_rc = JsRealm::state_from_scope(scope);
+ let mut realm_state = realm_state_rc.borrow_mut();
  // The callback to pass to the v8 API has to be a unit type, so it can't
  // borrow or move any local variables. Therefore, we're storing the JS
  // callback in a JsRuntimeState slot.
- if state.js_wasm_streaming_cb.is_some() {
+ if realm_state.js_wasm_streaming_cb.is_some() {
  return Err(type_error("op_set_wasm_streaming_callback already called"));
  }
- state.js_wasm_streaming_cb = Some(cb);
+ realm_state.js_wasm_streaming_cb = Some(cb);
 
  scope.set_wasm_streaming_callback(|scope, arg, wasm_streaming| {
  let (cb_handle, streaming_rid) = {
+ let realm_state_rc = JsRealm::state_from_scope(scope);
+ let cb_handle = realm_state_rc
+ .borrow()
+ .js_wasm_streaming_cb
+ .as_ref()
+ .unwrap()
+ .clone();
  let state_rc = JsRuntime::state(scope);
- let state = state_rc.borrow();
- let cb_handle = state.js_wasm_streaming_cb.as_ref().unwrap().clone();
- let streaming_rid = state
+ let streaming_rid = state_rc
+ .borrow()
  .op_state
  .borrow_mut()
  .resource_table
@@ -775,8 +781,11 @@ fn op_set_format_exception_callback<'a>(
  cb: serde_v8::Value<'a>,
 ) -> Result<Option<serde_v8::Value<'a>>, Error> {
  let cb = to_v8_fn(scope, cb)?;
- let state_rc = JsRuntime::state(scope);
- let old = state_rc.borrow_mut().js_format_exception_cb.replace(cb);
+ let realm_state_rc = JsRealm::state_from_scope(scope);
+ let old = realm_state_rc
+ .borrow_mut()
+ .js_format_exception_cb
+ .replace(cb);
  let old = old.map(|v| v8::Local::new(scope, v));
  Ok(old.map(|v| from_v8(scope, v.into()).unwrap()))
 }

core/runtime.rs

@@ -151,8 +151,9 @@ pub type CompiledWasmModuleStore = CrossIsolateStore<v8::CompiledWasmModule>;
 pub(crate) struct ContextState {
  js_recv_cb: Option<v8::Global<v8::Function>>,
  pub(crate) js_build_custom_error_cb: Option<v8::Global<v8::Function>>,
- // TODO(andreubotella): Move the rest of Option<Global<Function>> fields from
- // JsRuntimeState to this struct.
+ pub(crate) js_promise_reject_cb: Option<v8::Global<v8::Function>>,
+ pub(crate) js_format_exception_cb: Option<v8::Global<v8::Function>>,
+ pub(crate) js_wasm_streaming_cb: Option<v8::Global<v8::Function>>,
  pub(crate) unrefed_ops: HashSet<i32>,
 }
 
@@ -163,10 +164,7 @@ pub(crate) struct JsRuntimeState {
  known_realms: Vec<v8::Weak<v8::Context>>,
  pub(crate) js_macrotask_cbs: Vec<v8::Global<v8::Function>>,
  pub(crate) js_nexttick_cbs: Vec<v8::Global<v8::Function>>,
- pub(crate) js_promise_reject_cb: Option<v8::Global<v8::Function>>,
- pub(crate) js_format_exception_cb: Option<v8::Global<v8::Function>>,
  pub(crate) has_tick_scheduled: bool,
- pub(crate) js_wasm_streaming_cb: Option<v8::Global<v8::Function>>,
  pub(crate) pending_promise_exceptions:
  HashMap<v8::Global<v8::Promise>, v8::Global<v8::Value>>,
  pub(crate) pending_dyn_mod_evaluate: Vec<DynImportModEvaluate>,
@@ -414,10 +412,7 @@ impl JsRuntime {
  dyn_module_evaluate_idle_counter: 0,
  js_macrotask_cbs: vec![],
  js_nexttick_cbs: vec![],
- js_promise_reject_cb: None,
- js_format_exception_cb: None,
  has_tick_scheduled: false,
- js_wasm_streaming_cb: None,
  source_map_getter: options.source_map_getter,
  source_map_cache: Default::default(),
  pending_ops: FuturesUnordered::new(),
@@ -775,9 +770,6 @@ impl JsRuntime {
  // Free up additional global handles before creating the snapshot
  state.js_macrotask_cbs.clear();
  state.js_nexttick_cbs.clear();
- state.js_wasm_streaming_cb = None;
- state.js_format_exception_cb = None;
- state.js_promise_reject_cb = None;
  }
 
  let snapshot_creator = self.snapshot_creator.as_mut().unwrap();
@@ -3461,6 +3453,54 @@ assertEquals(1, notify_return_value);
  assert_eq!(2, PROMISE_REJECT.load(Ordering::Relaxed));
  }
 
+ #[tokio::test]
+ async fn test_set_promise_reject_callback_realms() {
+ let mut runtime = JsRuntime::new(RuntimeOptions::default());
+ let global_realm = runtime.global_realm();
+ let realm1 = runtime.create_realm().unwrap();
+ let realm2 = runtime.create_realm().unwrap();
+
+ let realm_expectations = &[
+ (&global_realm, "global_realm", 42),
+ (&realm1, "realm1", 140),
+ (&realm2, "realm2", 720),
+ ];
+
+ // Set up promise reject callbacks.
+ for (realm, realm_name, number) in realm_expectations {
+ realm
+ .execute_script(
+ runtime.v8_isolate(),
+ "",
+ &format!(
+ r#"
+ globalThis.rejectValue = undefined;
+ Deno.core.setPromiseRejectCallback((_type, _promise, reason) => {{
+ globalThis.rejectValue = `{realm_name}/${{reason}}`;
+ }});
+ Deno.core.opAsync("op_void_async").then(() => Promise.reject({number}));
+ "#,
+ realm_name=realm_name,
+ number=number
+ ),
+ )
+ .unwrap();
+ }
+
+ runtime.run_event_loop(false).await.unwrap();
+
+ for (realm, realm_name, number) in realm_expectations {
+ let reject_value = realm
+ .execute_script(runtime.v8_isolate(), "", "globalThis.rejectValue")
+ .unwrap();
+ let scope = &mut realm.handle_scope(runtime.v8_isolate());
+ let reject_value = v8::Local::new(scope, reject_value);
+ assert!(reject_value.is_string());
+ let reject_value_string = reject_value.to_rust_string_lossy(scope);
+ assert_eq!(reject_value_string, format!("{}/{}", realm_name, number));
+ }
+ }
+
  #[tokio::test]
  async fn test_set_promise_reject_callback_top_level_await() {
  static PROMISE_REJECT: AtomicUsize = AtomicUsize::new(0);