fix(repl): don't terminate on unhandled error events (#15548)

cli/tests/integration/repl_tests.rs

@@ -844,3 +844,20 @@ fn pty_tab_handler() {
     assert_not_contains!(output, "atob");
   });
 }
+
+#[test]
+fn repl_report_error() {
+  let (out, err) = util::run_and_collect_output(
+    true,
+    "repl",
+    Some(vec![
+      r#"console.log(1); reportError(new Error("foo")); console.log(2);"#,
+    ]),
+    Some(vec![("NO_COLOR".to_owned(), "1".to_owned())]),
+    false,
+  );
+
+  // TODO(nayeemrmn): The REPL should report event errors and rejections.
+  assert_contains!(out, "1\n2\nundefined\n");
+  assert!(err.is_empty());
+}

core/01_core.js

@@ -314,7 +314,6 @@
       error,
     ) => ops.op_abort_wasm_streaming(rid, error),
     destructureError: (error) => ops.op_destructure_error(error),
-    terminate: (exception) => ops.op_terminate(exception),
     opNames: () => ops.op_op_names(),
     eventLoopHasMoreWork: () => ops.op_event_loop_has_more_work(),
     setPromiseRejectCallback: (fn) => ops.op_set_promise_reject_callback(fn),

core/inspector.rs

@@ -150,7 +150,7 @@ impl JsRuntimeInspector {
   pub fn new(
     isolate: &mut v8::OwnedIsolate,
     context: v8::Global<v8::Context>,
-  ) -> Box<Self> {
+  ) -> Rc<RefCell<Self>> {
     let scope = &mut v8::HandleScope::new(isolate);
 
     let (new_session_tx, new_session_rx) =
@@ -162,7 +162,7 @@ impl JsRuntimeInspector {
     let waker = InspectorWaker::new(scope.thread_safe_handle());
 
     // Create JsRuntimeInspector instance.
-    let mut self_ = Box::new(Self {
+    let self__ = Rc::new(RefCell::new(Self {
       v8_inspector_client,
       v8_inspector: Default::default(),
       sessions: RefCell::new(SessionContainer::temporary_placeholder()),
@@ -170,7 +170,8 @@ impl JsRuntimeInspector {
       flags: Default::default(),
       waker,
       deregister_tx: None,
-    });
+    }));
+    let mut self_ = self__.borrow_mut();
     self_.v8_inspector = Rc::new(RefCell::new(
       v8::inspector::V8Inspector::create(scope, &mut *self_).into(),
     ));
@@ -192,8 +193,9 @@ impl JsRuntimeInspector {
     // Poll the session handler so we will get notified whenever there is
     // new incoming debugger activity.
     let _ = self_.poll_sessions(None).unwrap();
+    drop(self_);
 
-    self_
+    self__
   }
 
   pub fn has_active_sessions(&self) -> bool {

core/ops_builtin_v8.rs

@@ -43,7 +43,7 @@ pub(crate) fn init_builtins_v8() -> Vec<OpDecl> {
     op_set_wasm_streaming_callback::decl(),
     op_abort_wasm_streaming::decl(),
     op_destructure_error::decl(),
-    op_terminate::decl(),
+    op_dispatch_exception::decl(),
     op_op_names::decl(),
     op_apply_source_map::decl(),
     op_set_format_exception_callback::decl(),
@@ -718,13 +718,27 @@ fn op_destructure_error(
   JsError::from_v8_exception(scope, error.v8_value)
 }
 
+/// Effectively throw an uncatchable error. This will terminate runtime
+/// execution before any more JS code can run, except in the REPL where it
+/// should just output the error to the console.
 #[op(v8)]
-fn op_terminate(scope: &mut v8::HandleScope, exception: serde_v8::Value) {
+fn op_dispatch_exception(
+  scope: &mut v8::HandleScope,
+  exception: serde_v8::Value,
+) {
   let state_rc = JsRuntime::state(scope);
   let mut state = state_rc.borrow_mut();
-  state.explicit_terminate_exception =
-    Some(v8::Global::new(scope, exception.v8_value));
-  scope.terminate_execution();
+  state
+    .dispatched_exceptions
+    .push_front(v8::Global::new(scope, exception.v8_value));
+  // Only terminate execution if there are no inspector sessions.
+  match state.inspector().try_borrow() {
+    Ok(inspector) if !inspector.has_active_sessions() => {
+      scope.terminate_execution();
+    }
+    // If the inspector is borrowed at this time, assume an inspector is active.
+    _ => {}
+  }
 }
 
 #[op(v8)]

core/runtime.rs

@@ -36,6 +36,7 @@ use std::any::Any;
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::collections::HashSet;
+use std::collections::VecDeque;
 use std::ffi::c_void;
 use std::mem::forget;
 use std::option::Option;
@@ -80,9 +81,6 @@ pub struct JsRuntime {
   // This is an Option<OwnedIsolate> instead of just OwnedIsolate to workaround
   // a safety issue with SnapshotCreator. See JsRuntime::drop.
   v8_isolate: Option<v8::OwnedIsolate>,
-  // This is an Option<Box<JsRuntimeInspector> instead of just Box<JsRuntimeInspector>
-  // to workaround a safety issue. See JsRuntime::drop.
-  inspector: Option<Box<JsRuntimeInspector>>,
   snapshot_creator: Option<v8::SnapshotCreator>,
   has_snapshotted: bool,
   built_from_snapshot: bool,
@@ -182,19 +180,18 @@ pub(crate) struct JsRuntimeState {
   pub(crate) op_ctxs: Box<[OpCtx]>,
   pub(crate) shared_array_buffer_store: Option<SharedArrayBufferStore>,
   pub(crate) compiled_wasm_module_store: Option<CompiledWasmModuleStore>,
-  /// The error that was passed to an explicit `Deno.core.terminate` call.
+  /// The error that was passed to an `op_dispatch_exception` call.
   /// It will be retrieved by `exception_to_err_result` and used as an error
   /// instead of any other exceptions.
-  pub(crate) explicit_terminate_exception: Option<v8::Global<v8::Value>>,
+  // TODO(nayeemrmn): This is polled in `exception_to_err_result()` which is
+  // flimsy. Try to poll it similarly to `pending_promise_exceptions`.
+  pub(crate) dispatched_exceptions: VecDeque<v8::Global<v8::Value>>,
+  inspector: Option<Rc<RefCell<JsRuntimeInspector>>>,
   waker: AtomicWaker,
 }
 
 impl Drop for JsRuntime {
   fn drop(&mut self) {
-    // The Isolate object must outlive the Inspector object, but this is
-    // currently not enforced by the type system.
-    self.inspector.take();
-
     if let Some(creator) = self.snapshot_creator.take() {
       // TODO(ry): in rusty_v8, `SnapShotCreator::get_owned_isolate()` returns
       // a `struct OwnedIsolate` which is not actually owned, hence the need
@@ -421,7 +418,8 @@ impl JsRuntime {
       op_state: op_state.clone(),
       op_ctxs,
       have_unpolled_ops: false,
-      explicit_terminate_exception: None,
+      dispatched_exceptions: Default::default(),
+      inspector: Some(inspector),
       waker: AtomicWaker::new(),
     })));
 
@@ -434,7 +432,6 @@ impl JsRuntime {
 
     let mut js_runtime = Self {
       v8_isolate: Some(isolate),
-      inspector: Some(inspector),
       snapshot_creator: maybe_snapshot_creator,
       has_snapshotted: false,
       built_from_snapshot: has_startup_snapshot,
@@ -466,8 +463,8 @@ impl JsRuntime {
     self.v8_isolate.as_mut().unwrap()
   }
 
-  pub fn inspector(&mut self) -> &mut Box<JsRuntimeInspector> {
-    self.inspector.as_mut().unwrap()
+  pub fn inspector(&mut self) -> Rc<RefCell<JsRuntimeInspector>> {
+    Self::state(self.v8_isolate()).borrow().inspector()
   }
 
   pub fn global_realm(&mut self) -> JsRealm {
@@ -740,7 +737,7 @@ impl JsRuntime {
 
     state.borrow_mut().global_realm.take();
 
-    self.inspector.take();
+    state.borrow_mut().inspector.take();
 
     // Overwrite existing ModuleMap to drop v8::Global handles
     self
@@ -941,8 +938,7 @@ impl JsRuntime {
     wait_for_inspector: bool,
   ) -> Poll<Result<(), Error>> {
     // We always poll the inspector first
-    let _ = self.inspector().poll_unpin(cx);
-
+    let _ = self.inspector().borrow_mut().poll_unpin(cx);
     let state_rc = Self::state(self.v8_isolate());
     let module_map_rc = Self::module_map(self.v8_isolate());
     {
@@ -1003,11 +999,8 @@ impl JsRuntime {
     self.evaluate_pending_module();
 
     let pending_state = Self::event_loop_pending_state(self.v8_isolate());
-    let inspector_has_active_sessions = self
-      .inspector
-      .as_ref()
-      .map(|i| i.has_active_sessions())
-      .unwrap_or(false);
+    let inspector_has_active_sessions =
+      self.inspector().borrow_mut().has_active_sessions();
 
     if !pending_state.is_pending() && !maybe_scheduling {
       if wait_for_inspector && inspector_has_active_sessions {
@@ -1144,6 +1137,10 @@ where
 }
 
 impl JsRuntimeState {
+  pub(crate) fn inspector(&self) -> Rc<RefCell<JsRuntimeInspector>> {
+    self.inspector.as_ref().unwrap().clone()
+  }
+
   /// Called by `bindings::host_import_module_dynamically_callback`
   /// after initiating new dynamic import load.
   pub fn notify_new_dynamic_import(&mut self) {
@@ -1167,17 +1164,17 @@ pub(crate) fn exception_to_err_result<'s, T>(
   scope.cancel_terminate_execution();
   let mut exception = exception;
   {
-    // If the termination is the result of a `Deno.core.terminate` call, we want
+    // If termination is the result of a `op_dispatch_exception` call, we want
     // to use the exception that was passed to it rather than the exception that
     // was passed to this function.
     let mut state = state_rc.borrow_mut();
     exception = state
-      .explicit_terminate_exception
-      .take()
+      .dispatched_exceptions
+      .pop_back()
       .map(|exception| v8::Local::new(scope, exception))
       .unwrap_or_else(|| {
         // Maybe make a new exception object.
-        if exception.is_null_or_undefined() {
+        if was_terminating_execution && exception.is_null_or_undefined() {
           let message = v8::String::new(scope, "execution terminated").unwrap();
           v8::Exception::error(scope, message)
         } else {
@@ -1385,10 +1382,11 @@ impl JsRuntime {
     // Update status after evaluating.
     status = module.get_status();
 
-    let explicit_terminate_exception =
-      state_rc.borrow_mut().explicit_terminate_exception.take();
-    if let Some(exception) = explicit_terminate_exception {
-      let exception = v8::Local::new(tc_scope, exception);
+    let has_dispatched_exception =
+      !state_rc.borrow_mut().dispatched_exceptions.is_empty();
+    if has_dispatched_exception {
+      // This will be overrided in `exception_to_err_result()`.
+      let exception = v8::undefined(tc_scope).into();
       let pending_mod_evaluate = {
         let mut state = state_rc.borrow_mut();
         state.pending_mod_evaluate.take().unwrap()
@@ -2702,7 +2700,7 @@ pub mod tests {
           Deno.core.setPromiseRejectCallback(() => {
             return false;
           });
-          a = 1 + 2; 
+          a = 1 + 2;
       "#,
         )
         .unwrap();

ext/web/02_event.js

@@ -8,6 +8,7 @@
 
 ((window) => {
   const core = window.Deno.core;
+  const ops = core.ops;
   const webidl = window.__bootstrap.webidl;
   const { DOMException } = window.__bootstrap.domException;
   const consoleInternal = window.__bootstrap.console;
@@ -1451,7 +1452,7 @@
     });
     // Avoid recursing `reportException()` via error handlers more than once.
     if (reportExceptionStackedCalls > 1 || window.dispatchEvent(event)) {
-      core.terminate(error);
+      ops.op_dispatch_exception(error);
     }
     reportExceptionStackedCalls--;
   }

runtime/inspector_server.rs

@@ -69,7 +69,8 @@ impl InspectorServer {
     js_runtime: &mut JsRuntime,
     should_break_on_first_statement: bool,
   ) {
-    let inspector = js_runtime.inspector();
+    let inspector_rc = js_runtime.inspector();
+    let mut inspector = inspector_rc.borrow_mut();
     let session_sender = inspector.get_session_sender();
     let deregister_rx = inspector.add_deregister_handler();
     let info = InspectorInfo::new(

runtime/worker.rs

@@ -294,6 +294,7 @@ impl MainWorker {
       self
         .js_runtime
         .inspector()
+        .borrow_mut()
         .wait_for_session_and_break_on_next_statement()
     }
   }
@@ -301,8 +302,7 @@ impl MainWorker {
   /// Create new inspector session. This function panics if Worker
   /// was not configured to create inspector.
   pub async fn create_inspector_session(&mut self) -> LocalInspectorSession {
-    let inspector = self.js_runtime.inspector();
-    inspector.create_local_session()
+    self.js_runtime.inspector().borrow().create_local_session()
   }
 
   pub fn poll_event_loop(