refactor(core): always attach inspector to isolate (#11042)

This commit changes "deno_core::JsRuntime" to always create
"deno_core::JsRuntimeInspector" instance.

cli/main.rs

@@ -91,8 +91,6 @@ fn create_web_worker_callback(
       PrettyJsError::create(source_mapped_error)
     });
 
-    let attach_inspector = program_state.maybe_inspector_server.is_some()
-      || program_state.coverage_dir.is_some();
     let maybe_inspector_server = program_state.maybe_inspector_server.clone();
 
     let module_loader = CliModuleLoader::new_for_worker(
@@ -117,7 +115,6 @@ fn create_web_worker_callback(
       create_web_worker_cb,
       js_error_create_fn: Some(js_error_create_fn),
       use_deno_namespace: args.use_deno_namespace,
-      attach_inspector,
       maybe_inspector_server,
       runtime_version: version::deno(),
       ts_version: version::TYPESCRIPT.to_string(),
@@ -173,9 +170,6 @@ pub fn create_main_worker(
     PrettyJsError::create(source_mapped_error)
   });
 
-  let attach_inspector = program_state.maybe_inspector_server.is_some()
-    || program_state.flags.repl
-    || program_state.coverage_dir.is_some();
   let maybe_inspector_server = program_state.maybe_inspector_server.clone();
   let should_break_on_first_statement =
     program_state.flags.inspect_brk.is_some();
@@ -195,7 +189,6 @@ pub fn create_main_worker(
     seed: program_state.flags.seed,
     js_error_create_fn: Some(js_error_create_fn),
     create_web_worker_cb,
-    attach_inspector,
     maybe_inspector_server,
     should_break_on_first_statement,
     module_loader,

cli/standalone.rs

@@ -236,7 +236,6 @@ pub async fn run(
     seed: metadata.seed,
     js_error_create_fn: None,
     create_web_worker_cb,
-    attach_inspector: false,
     maybe_inspector_server: None,
     should_break_on_first_statement: false,
     module_loader,

core/runtime.rs

@@ -75,8 +75,10 @@ struct IsolateAllocations {
 /// and an optional zero copy buffer, each async Op is tied to a Promise in JavaScript.
 pub struct JsRuntime {
   // This is an Option<OwnedIsolate> instead of just OwnedIsolate to workaround
-  // an safety issue with SnapshotCreator. See JsRuntime::drop.
+  // 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,
@@ -205,9 +207,6 @@ pub struct RuntimeOptions {
   /// V8 platform instance to use. Used when Deno initializes V8
   /// (which it only does once), otherwise it's silenty dropped.
   pub v8_platform: Option<v8::UniquePtr<v8::Platform>>,
-
-  /// Create a V8 inspector and attach to the runtime.
-  pub attach_inspector: bool,
 }
 
 impl JsRuntime {
@@ -268,13 +267,8 @@ impl JsRuntime {
       (isolate, None)
     };
 
-    let maybe_inspector = if options.attach_inspector {
-      let inspector =
-        JsRuntimeInspector::new(&mut isolate, global_context.clone());
-      Some(inspector)
-    } else {
-      None
-    };
+    let inspector =
+      JsRuntimeInspector::new(&mut isolate, global_context.clone());
 
     let loader = options
       .module_loader
@@ -316,7 +310,7 @@ impl JsRuntime {
 
     let mut js_runtime = Self {
       v8_isolate: Some(isolate),
-      inspector: maybe_inspector,
+      inspector: Some(inspector),
       snapshot_creator: maybe_snapshot_creator,
       has_snapshotted: false,
       allocations: IsolateAllocations::default(),
@@ -347,8 +341,8 @@ impl JsRuntime {
     self.v8_isolate.as_mut().unwrap()
   }
 
-  pub fn inspector(&mut self) -> Option<&mut Box<JsRuntimeInspector>> {
-    self.inspector.as_mut()
+  pub fn inspector(&mut self) -> &mut Box<JsRuntimeInspector> {
+    self.inspector.as_mut().unwrap()
   }
 
   pub fn handle_scope(&mut self) -> v8::HandleScope {
@@ -511,6 +505,8 @@ impl JsRuntime {
     // TODO(piscisaureus): The rusty_v8 type system should enforce this.
     state.borrow_mut().global_context.take();
 
+    self.inspector.take();
+
     // Overwrite existing ModuleMap to drop v8::Global handles
     self
       .v8_isolate()
@@ -609,8 +605,8 @@ impl JsRuntime {
     cx: &mut Context,
     wait_for_inspector: bool,
   ) -> Poll<Result<(), AnyError>> {
-    // We always poll the inspector if it exists.
-    let _ = self.inspector().map(|i| i.poll_unpin(cx));
+    // We always poll the inspector first
+    let _ = self.inspector().poll_unpin(cx);
 
     let state_rc = Self::state(self.v8_isolate());
     let module_map_rc = Self::module_map(self.v8_isolate());

runtime/examples/hello_runtime.rs

@@ -32,7 +32,6 @@ async fn main() -> Result<(), AnyError> {
     seed: None,
     js_error_create_fn: None,
     create_web_worker_cb,
-    attach_inspector: false,
     maybe_inspector_server: None,
     should_break_on_first_statement: false,
     module_loader,

runtime/web_worker.rs

@@ -218,7 +218,6 @@ pub struct WebWorkerOptions {
   pub create_web_worker_cb: Arc<ops::worker_host::CreateWebWorkerCb>,
   pub js_error_create_fn: Option<Rc<JsErrorCreateFn>>,
   pub use_deno_namespace: bool,
-  pub attach_inspector: bool,
   pub maybe_inspector_server: Option<Arc<InspectorServer>>,
   pub apply_source_maps: bool,
   /// Sets `Deno.version.deno` in JS runtime.
@@ -314,17 +313,15 @@ impl WebWorker {
       startup_snapshot: Some(js::deno_isolate_init()),
       js_error_create_fn: options.js_error_create_fn.clone(),
       get_error_class_fn: options.get_error_class_fn,
-      attach_inspector: options.attach_inspector,
       extensions,
       ..Default::default()
     });
 
-    if let Some(inspector) = js_runtime.inspector() {
-      if let Some(server) = options.maybe_inspector_server.clone() {
-        let session_sender = inspector.get_session_sender();
-        let deregister_rx = inspector.add_deregister_handler();
-        server.register_inspector(session_sender, deregister_rx);
-      }
+    if let Some(server) = options.maybe_inspector_server.clone() {
+      let inspector = js_runtime.inspector();
+      let session_sender = inspector.get_session_sender();
+      let deregister_rx = inspector.add_deregister_handler();
+      server.register_inspector(session_sender, deregister_rx);
     }
 
     let (internal_handle, external_handle) = {
@@ -550,7 +547,6 @@ mod tests {
       create_web_worker_cb,
       js_error_create_fn: None,
       use_deno_namespace: false,
-      attach_inspector: false,
       maybe_inspector_server: None,
       runtime_version: "x".to_string(),
       ts_version: "x".to_string(),

runtime/worker.rs

@@ -58,7 +58,6 @@ pub struct WorkerOptions {
   // of WebWorker
   pub create_web_worker_cb: Arc<ops::worker_host::CreateWebWorkerCb>,
   pub js_error_create_fn: Option<Rc<JsErrorCreateFn>>,
-  pub attach_inspector: bool,
   pub maybe_inspector_server: Option<Arc<InspectorServer>>,
   pub should_break_on_first_statement: bool,
   /// Sets `Deno.version.deno` in JS runtime.
@@ -141,24 +140,19 @@ impl MainWorker {
       js_error_create_fn: options.js_error_create_fn.clone(),
       get_error_class_fn: options.get_error_class_fn,
       extensions,
-      attach_inspector: options.attach_inspector,
       ..Default::default()
     });
 
-    let mut should_break_on_first_statement = false;
-
-    if let Some(inspector) = js_runtime.inspector() {
-      if let Some(server) = options.maybe_inspector_server.clone() {
-        let session_sender = inspector.get_session_sender();
-        let deregister_rx = inspector.add_deregister_handler();
-        server.register_inspector(session_sender, deregister_rx);
-      }
-      should_break_on_first_statement = options.should_break_on_first_statement;
+    if let Some(server) = options.maybe_inspector_server.clone() {
+      let inspector = js_runtime.inspector();
+      let session_sender = inspector.get_session_sender();
+      let deregister_rx = inspector.add_deregister_handler();
+      server.register_inspector(session_sender, deregister_rx);
     }
 
     Self {
       js_runtime,
-      should_break_on_first_statement,
+      should_break_on_first_statement: options.should_break_on_first_statement,
     }
   }
 
@@ -231,7 +225,6 @@ impl MainWorker {
       self
         .js_runtime
         .inspector()
-        .unwrap()
         .wait_for_session_and_break_on_next_statement()
     }
   }
@@ -239,7 +232,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().unwrap();
+    let inspector = self.js_runtime.inspector();
     inspector.create_local_session()
   }
 
@@ -295,7 +288,6 @@ mod tests {
       seed: None,
       js_error_create_fn: None,
       create_web_worker_cb: Arc::new(|_| unreachable!()),
-      attach_inspector: false,
       maybe_inspector_server: None,
       should_break_on_first_statement: false,
       module_loader: Rc::new(deno_core::FsModuleLoader),