fix(core): keep op contexts alive until the isolate is disposed

@@ -103,6 +103,53 @@ v8_static_strings! { pub struct PluginHostProxy {   tx: mpsc::Sender<PluginHostRequest>,   pub(crate) plugin_info: Arc<Mutex<Vec<PluginInfo>>>,+  // Must be declared after `tx`: fields are dropped in declaration order, and+  // the plugin host's run loop only ends once `tx` has been dropped.+  _thread: PluginHostThread,+}++/// How long dropping a [`PluginHostProxy`] waits for the plugin host to tear+/// down its worker. Teardown normally takes milliseconds; the bound only+/// guarantees that `deno lint` can never hang on it.+const PLUGIN_HOST_TEARDOWN_TIMEOUT: std::time::Duration =+  std::time::Duration::from_secs(10);++/// The thread running the [`PluginHost`].+///+/// On drop, waits until the plugin host has torn down its worker, and with it+/// the V8 isolate. Without this the thread was detached, so the process could+/// exit while V8 was still disposing of the isolate on that thread.+#[derive(Debug)]+struct PluginHostThread {+  join_handle: Option<std::thread::JoinHandle<Result<(), AnyError>>>,+  /// Disconnected once the plugin host future, and the worker it owns, have+  /// been dropped. (In a `Mutex` only because `PluginHostProxy` is `Sync`.)+  worker_dropped_rx: Mutex<std::sync::mpsc::Receiver<()>>,+}++impl Drop for PluginHostThread {+  fn drop(&mut self) {+    match self+      .worker_dropped_rx+      .get_mut()+      .recv_timeout(PLUGIN_HOST_TEARDOWN_TIMEOUT)+    {+      Ok(()) | Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {}+      Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {+        log::debug!("Timed out waiting for the lint PluginHost to shut down");+        return;+      }+    }+    // The worker is gone; what remains is shutting down the thread's tokio+    // runtime, which waits for any blocking task a plugin left running. Join+    // only if that has already finished so a stuck task can't hang us.+    if let Some(join_handle) = self.join_handle.take()+      && join_handle.is_finished()+      && let Ok(Err(err)) = join_handle.join()+    {+      log::debug!("Lint PluginHost thread failed: {err:#}");+    }+  } }  impl PluginHostProxy {@@ -229,13 +276,18 @@ impl PluginHost {   fn create(logger: PluginLogger) -> Result<PluginHostProxy, AnyError> {     let (tx_req, rx_req) = mpsc::channel(10); +    let (worker_dropped_tx, worker_dropped_rx) = std::sync::mpsc::channel();+     let logger_ = logger.clone();-    let _join_handle: std::thread::JoinHandle<Result<(), AnyError>> =+    let join_handle: std::thread::JoinHandle<Result<(), AnyError>> =       std::thread::spawn(move || {         let logger = logger_;         log::debug!("Lint PluginHost thread spawned");         let start = std::time::Instant::now();         let fut = async move {+          // Declared first so that it is dropped last, after the runner (and+          // its worker) on every path out of this future.+          let _worker_dropped_tx = worker_dropped_tx;           let runner =             create_plugin_runner_inner(logger.clone(), rx_req).await?;           log::debug!("Lint PlugibnHost running loop");@@ -253,6 +305,10 @@ impl PluginHost {     let proxy = PluginHostProxy {       tx: tx_req,       plugin_info: Arc::new(Mutex::new(vec![])),+      _thread: PluginHostThread {+        join_handle: Some(join_handle),+        worker_dropped_rx: Mutex::new(worker_dropped_rx),+      },     };      Ok(proxy)

@@ -116,8 +116,10 @@ pub struct OpCtx {   /// The fast-call overload passed to `FunctionTemplate::build_fast`. Stored   /// here (rather than synthesized on the stack) because V8 150.x keeps the raw   /// `CFunction` pointers directly inside `FunctionTemplateInfo`, so the slice-  /// must outlive every template built from this op. `OpCtx` lives until isolate-  /// disposal, which satisfies that.+  /// must outlive every template built from this op, including any concurrent+  /// TurboFan job that inlines the fast call. The runtime keeps its `OpCtxs`+  /// alive until after the isolate has been disposed (see+  /// `JsRealmInner::destroy`), which satisfies that.   ///   /// The single element is also the op's `CFunctionInfo` source (see   /// [`OpCtx::fast_fn_info`]).

@@ -1029,8 +1029,10 @@ pub(crate) fn op_ctx_template<'s, 'i>(     // SAFETY: V8 150.x stores the raw `CFunction` pointers from `overloads`     // directly in the `FunctionTemplateInfo`, so `build_fast` requires a     // `'static` slice. The backing storage lives in `op_ctx`, which outlives-    // every `FunctionTemplate` created from it (both are torn down at isolate-    // disposal), so extending the borrow to `'static` is sound.+    // every `FunctionTemplate` created from it (the runtime frees the op+    // contexts only after disposing of the isolate, which also joins any+    // concurrent compilation job reading them), so extending the borrow to+    // `'static` is sound.     let overloads: &'static [v8::fast_api::CFunction] = unsafe {       std::mem::transmute::<&[_], &'static [_]>(overloads.as_slice())     };

@@ -100,8 +100,12 @@ pub struct ContextState {   pub(crate) activity_traces: RuntimeActivityTraces,   pub(crate) pending_ops: Rc<OpDriverImpl>,   // We don't explicitly re-read this prop but need the slice to live alongside-  // the context-  pub(crate) op_ctxs: OpCtxs,+  // the context. It is reference counted because it must also outlive the+  // isolate: V8 keeps raw pointers to each op's fast-call `CFunction`+  // overloads (stored in `OpCtx`) and concurrent TurboFan jobs may read them+  // until the isolate is disposed. `JsRealmInner::destroy` hands a reference+  // back to the runtime, which drops it only after disposing of the isolate.+  pub(crate) op_ctxs: Rc<OpCtxs>,   pub(crate) op_method_decls: Vec<OpMethodDecl>,   pub(crate) methods_ctx_offset: usize,   /// Snapshots built against V8 14.9+ bake the *slow* version of each op@@ -214,7 +218,7 @@ impl ContextState {       wasm_instance_fn: Default::default(),       wasm_instances_map: Default::default(),       activity_traces: Default::default(),-      op_ctxs,+      op_ctxs: Rc::new(op_ctxs),       op_method_decls,       methods_ctx_offset,       fast_ops_upgraded: Cell::new(false),@@ -313,8 +317,17 @@ impl JsRealmInner {     self.function_templates.clone()   } -  pub fn destroy(self) {+  /// Tears down the realm's context state.+  ///+  /// Returns the realm's op contexts, which the caller must keep alive until+  /// the isolate has been disposed: function templates built with fast-call+  /// overloads point into them, and V8 may still read those pointers from+  /// concurrent compilation jobs until `Isolate::Dispose` cancels and joins+  /// them.+  #[must_use = "the op contexts must outlive the isolate"]+  pub fn destroy(self) -> Rc<OpCtxs> {     let state = self.state();+    let op_ctxs = state.op_ctxs.clone();     let raw_ptr = self.state().isolate.unwrap();     // SAFETY: We know the isolate outlives the realm     let mut isolate = unsafe { v8::Isolate::from_raw_isolate_ptr(raw_ptr) };@@ -388,6 +401,8 @@ impl JsRealmInner {     // Expect that this context is dead (we only check this in debug mode)     // TODO(mmastrac): This check fails for some tests, will need to fix this     // debug_assert_eq!(Rc::strong_count(&self.context), 1, "Realm was still alive when we wanted to destroy it. Not dropped?");++    op_ctxs   } } 

@@ -120,6 +120,7 @@ use crate::modules::SideModuleKind; use crate::modules::ValidateImportAttributesCb; use crate::modules::recursive_load::RecursiveModuleLoad; use crate::modules::script_origin;+use crate::ops::OpCtxs; use crate::ops_metrics::OpMetricsFactoryFn; use crate::ops_metrics::dispatch_metrics_async; use crate::runtime::ContextState;@@ -218,6 +219,11 @@ pub(crate) struct InnerIsolateState {   source_count: usize,   addl_refs_count: usize,   main_realm: ManuallyDrop<JsRealm>,+  /// The main realm's op contexts, handed back by `JsRealmInner::destroy`.+  /// Function templates with fast-call overloads hold raw pointers into+  /// them, which concurrent TurboFan jobs may read until the isolate is+  /// disposed, so they are dropped only after `v8_isolate`.+  retired_op_ctxs: Option<Rc<OpCtxs>>,   pub(crate) state: ManuallyDropRc<JsRuntimeState>,   v8_isolate: ManuallyDrop<v8::OwnedIsolate>, }@@ -260,9 +266,12 @@ impl InnerIsolateState {     // release the Global<Context> stored in loop.data. If we clear     // op_state first, the Box<UvLoop> is freed and destroy() would     // dereference a dangling pointer (use-after-free).-    unsafe {-      ManuallyDrop::take(&mut self.main_realm).0.destroy();-    }+    //+    // The op contexts it returns are kept until the isolate has been disposed+    // (see `retired_op_ctxs`).+    // SAFETY: `main_realm` is taken exactly once, here, and never used again.+    let main_realm = unsafe { ManuallyDrop::take(&mut self.main_realm) };+    self.retired_op_ctxs = Some(main_realm.0.destroy());      // Now safe to clear op_state (drops Box<UvLoop> etc.) since     // destroy() has already released the Global from loop.data.@@ -280,6 +289,10 @@ impl InnerIsolateState {    pub fn prepare_for_snapshot(mut self) -> v8::OwnedIsolate {     self.cleanup();+    // Templates built for a snapshot have no fast-call overloads (see+    // `op_ctx_template`), so nothing in the isolate points into the op+    // contexts. Drop them here rather than leaking them with `self`.+    drop(self.retired_op_ctxs.take());      // SAFETY: We're copying out of self and then immediately forgetting self     unsafe {@@ -314,6 +327,10 @@ impl Drop for InnerIsolateState {         ManuallyDrop::drop(&mut self.v8_isolate);       }     }+    // Only now that the isolate is disposed (and with it every concurrent+    // compilation job that could read a fast-call `CFunction`) is it safe to+    // free the op contexts.+    drop(self.retired_op_ctxs.take());   } } @@ -1237,6 +1254,7 @@ impl JsRuntime {         source_count,         addl_refs_count,         main_realm: ManuallyDrop::new(main_realm),+        retired_op_ctxs: None,         state: ManuallyDropRc(ManuallyDrop::new(state_rc)),         v8_isolate: ManuallyDrop::new(isolate),       },

@@ -2192,3 +2192,58 @@ fn foreground_tasks_delivered_without_tokio_handle() {     runtime.resolve_value(promise).await.unwrap();   }); }++/// Function templates built with fast-call overloads keep raw pointers to the+/// `CFunction`s stored in the op contexts, and a concurrent TurboFan job that+/// inlines a fast call reads them until the isolate is disposed. The op+/// contexts must therefore still be alive while the isolate is being dropped,+/// and be released afterwards.+#[test]+fn op_ctxs_outlive_isolate() {+  struct OpCtxsProbe {+    op_ctxs: std::rc::Weak<crate::ops::OpCtxs>,+    alive_at_isolate_drop: Rc<std::cell::Cell<Option<bool>>>,+  }++  impl Drop for OpCtxsProbe {+    fn drop(&mut self) {+      self+        .alive_at_isolate_drop+        .set(Some(self.op_ctxs.strong_count() > 0));+    }+  }++  let mut runtime = JsRuntime::new(Default::default());+  runtime+    .execute_script(+      "a.js",+      "for (let i = 0; i < 10_000; i++) Deno.core.ops.op_add(i, 1);",+    )+    .unwrap();++  let op_ctxs = Rc::downgrade(&runtime.main_realm().0.context_state.op_ctxs);+  assert!(+    op_ctxs+      .upgrade()+      .unwrap()+      .iter()+      .any(|op_ctx| op_ctx.fast_fn_info().is_some())+  );++  // Isolate slots are dropped by `OwnedIsolate::drop`, i.e. only once the+  // runtime has finished its own cleanup and starts disposing the isolate.+  let alive_at_isolate_drop = Rc::new(std::cell::Cell::new(None));+  runtime.v8_isolate().set_slot(OpCtxsProbe {+    op_ctxs: op_ctxs.clone(),+    alive_at_isolate_drop: alive_at_isolate_drop.clone(),+  });++  drop(runtime);++  assert_eq!(+    alive_at_isolate_drop.get(),+    Some(true),+    "op contexts were freed before the isolate was disposed"+  );+  assert_eq!(op_ctxs.strong_count(), 0, "op contexts were leaked");+}

@@ -0,0 +1,10 @@+{+  // `deno lint` waits for the plugin host to shut down before exiting. Work a+  // plugin leaves pending must not keep it from exiting.+  "steps": [+    {+      "args": "lint a.ts",+      "output": "lint.out"+    }+  ]+}

@@ -0,0 +1 @@+const _a = "foo";

@@ -0,0 +1,5 @@+{+  "lint": {+    "plugins": ["./plugin.ts"]+  }+}

@@ -0,0 +1 @@+Checked 1 file

@@ -0,0 +1,15 @@+export default {+  name: "test-plugin",+  rules: {+    "my-rule": {+      create() {+        // The plugin host doesn't run its event loop after linting a file, so+        // this work is still pending when `deno lint` shuts the host down.+        setTimeout(() => {}, 1_000_000);+        setInterval(() => {}, 1_000);+        Deno.readTextFile(import.meta.filename!);+        return {};+      },+    },+  },+};