From 68d83ff649e085538166c162bd5a035bf97b9b78 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 04:50:36 +0000 Subject: [PATCH 1/4] bundler: touch outstanding plugin requests through their pointer, field by field While an onResolve/onLoad request is out with the plugins' thread it is still linked in the bundle thread's outstanding list, which that thread writes through (push of the next request, unlink of an answered one, Load::deferred from on_notify_defer_mini). Every place the plugins' thread touched a request held the whole struct as &mut for the duration (run_task arms, run_on_js_thread, the onResolveAsync/onLoadAsync/addError/ onDefer thunks, on_*_async across the post-back), and the list reborrowed the neighbours whole to write their links. Pass the request as *mut everywhere it may be outstanding and access only the owning side's fields through it; OutstandingNode projects the link as a raw pointer. Removes Load::was_file, which was never set. Adds a source lint over this population of functions. --- src/bundler/Graph.rs | 58 +- src/bundler/bundle_v2.rs | 157 +++-- src/runtime/api/JSBundler.rs | 314 ++++----- src/runtime/dispatch.rs | 23 +- ...bundler-plugin-outstanding-request.test.ts | 626 ++++++++++++++++++ 5 files changed, 940 insertions(+), 238 deletions(-) create mode 100644 test/internal/source-lints/bundler-plugin-outstanding-request.test.ts diff --git a/src/bundler/Graph.rs b/src/bundler/Graph.rs index aa13706b082..d96e8ea87aa 100644 --- a/src/bundler/Graph.rs +++ b/src/bundler/Graph.rs @@ -274,8 +274,16 @@ impl Default for OutstandingLink { } } } +/// A linked node is shared with the plugins' thread by field (see the docs on +/// `api::JSBundler::Resolve` / `Load`): the list owns the link and nothing +/// else, so it reaches the link through this raw projection and never forms a +/// reference to the whole node. pub trait OutstandingNode: Sized { - fn link(&mut self) -> &mut OutstandingLink; + /// `&raw mut (*this).outstanding`. + /// + /// # Safety + /// `this` points at a live node. + unsafe fn link_raw(this: *mut Self) -> *mut OutstandingLink; } /// A bundle pass's outstanding plugin requests; single-threaded (bundle thread). pub struct OutstandingList { @@ -290,40 +298,43 @@ impl Default for OutstandingList { } impl OutstandingList { pub(crate) fn push(&mut self, node: *mut T) { - // SAFETY: `node` is arena-live and unlinked; bundle thread. + // SAFETY: `node` is arena-live and unlinked; the current head is + // linked ⇒ arena-live (and possibly being answered by the plugins' + // thread right now, hence only its link is touched); bundle thread. unsafe { - let l = (*node).link(); - debug_assert!(!l.linked); - l.linked = true; - l.prev = core::ptr::null_mut(); - l.next = self.head; + let l = T::link_raw(node); + debug_assert!(!(*l).linked); + (*l).linked = true; + (*l).prev = core::ptr::null_mut(); + (*l).next = self.head; if !self.head.is_null() { - (*self.head).link().prev = node; + (*T::link_raw(self.head)).prev = node; } } self.head = node; } /// No-op if `node` is not linked (already answered / never dispatched). - pub(crate) fn unlink(&mut self, node: &mut T) { - let node_ptr: *mut T = node; - let l = node.link(); - if !l.linked { - return; - } - l.linked = false; - let (prev, next) = (l.prev, l.next); - l.prev = core::ptr::null_mut(); - l.next = core::ptr::null_mut(); - // SAFETY: neighbours are linked ⇒ arena-live; bundle thread. + pub(crate) fn unlink(&mut self, node: *mut T) { + // SAFETY: `node` is arena-live; its neighbours are linked ⇒ arena-live + // (and still out with the plugins' thread, hence only their links are + // touched); bundle thread. unsafe { + let l = T::link_raw(node); + if !(*l).linked { + return; + } + (*l).linked = false; + let (prev, next) = ((*l).prev, (*l).next); + (*l).prev = core::ptr::null_mut(); + (*l).next = core::ptr::null_mut(); if prev.is_null() { - debug_assert!(core::ptr::eq(self.head, node_ptr)); + debug_assert!(core::ptr::eq(self.head, node)); self.head = next; } else { - (*prev).link().next = next; + (*T::link_raw(prev)).next = next; } if !next.is_null() { - (*next).link().prev = prev; + (*T::link_raw(next)).prev = prev; } } } @@ -332,8 +343,7 @@ impl OutstandingList { if head.is_null() { return None; } - // SAFETY: linked ⇒ arena-live. - self.unlink(unsafe { &mut *head }); + self.unlink(head); Some(head) } } diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 0af0e0ce265..4eaaaa0ede7 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -1082,6 +1082,16 @@ pub mod bv2_impl { /// Both `js_task` and `task` /// are the real lower-tier `bun_event_loop` types, so `dispatch()` / /// `run_on_js_thread()` are implemented inherently (no T6 hook). + /// + /// From `dispatch` until the answer reaches `BundleV2::on_resolve`, + /// the request is shared between two threads by field: the plugins' + /// thread reads `bv2` / `import_record` and writes `value` (and + /// `task`, to post the answer), while the bundle thread owns + /// `outstanding` and writes it whenever a neighbouring request is + /// pushed or unlinked. During that window both sides go through the + /// raw pointer field by field; a `&Resolve` / `&mut Resolve` would + /// claim the other side's fields too. (Under bake the two "threads" + /// are one loop; the code is shared, so it keeps the same shape.) pub struct Resolve { pub bv2: *mut BundleV2<'static>, pub import_record: MiniImportRecord, @@ -1143,25 +1153,28 @@ pub mod bv2_impl { bv2.enqueue_on_js_loop_for_plugins(task); } } - pub fn run_on_js_thread(&mut self) { - let kind = self.import_record.kind; - // reshaped for borrowck — capture the erased self - // pointer before borrowing fields immutably for the FFI call. - let self_ptr = std::ptr::from_mut::(self).cast::(); - // SAFETY: `bv2` is a valid backref set by `init`; the plugin - // storage is disjoint from `self`, so the `&mut JSBundlerPlugin` - // returned by `plugins_mut()` does not alias the - // `&self.import_record.*` borrows below. - unsafe { &mut *self.bv2 } - .plugins_mut() - .expect("plugins") - .match_on_resolve( - &self.import_record.specifier, - &self.import_record.namespace, - &self.import_record.source_file, - self_ptr, - kind, + /// Plugins' thread. `this` itself is the context C++ hands back + /// to the `JSBundlerPlugin__*` answer thunks. + /// + /// # Safety + /// `this` is the request `dispatch` posted, not yet answered. + pub unsafe fn run_on_js_thread(this: *mut Self) { + // SAFETY: fn contract; of the request only this thread's + // fields are touched (type docs). `bv2` is the backref set + // by `init`, and the plugin storage is disjoint from the + // request, so the `&mut JSBundlerPlugin` does not alias + // `record`. + unsafe { + let record = &(*this).import_record; + let bv2 = &mut *(*this).bv2; + bv2.plugins_mut().expect("plugins").match_on_resolve( + &record.specifier, + &record.namespace, + &record.source_file, + this.cast::(), + record.kind, ); + } } } @@ -1189,6 +1202,15 @@ pub mod bv2_impl { } /// Task driving an onLoad plugin invocation for one source file. + /// + /// Shared by field while outstanding, exactly as [`Resolve`]: the + /// plugins' thread reads `bv2` / `path` / `namespace` / + /// `default_loader` / `parse_task` and writes `value`, `called_defer` + /// and `task`; the bundle thread owns `outstanding` and `deferred` + /// (the latter written by `on_notify_defer_mini` while the plugin's + /// callback is still suspended in `.defer()`). Until the answer + /// reaches `BundleV2::on_load`, neither side forms a `&Load` / + /// `&mut Load`. pub struct Load { pub bv2: *mut BundleV2<'static>, pub(crate) source_index: bun_ast::Index, @@ -1197,8 +1219,6 @@ pub mod bv2_impl { pub(crate) namespace: Box<[u8]>, pub value: LoadValue, pub parse_task: bun_ptr::BackRef, - /// Faster path: skip the extra threadpool dispatch when the file is not found. - pub was_file: bool, /// Defer may only be called once. pub called_defer: bool, /// `.defer()`ed and not yet drained: its scan-counter unit sits in @@ -1225,7 +1245,6 @@ pub mod bv2_impl { value: LoadValue::Pending, path: parse.path.text.to_vec().into_boxed_slice(), namespace: parse.path.namespace.to_vec().into_boxed_slice(), - was_file: false, called_defer: false, deferred: false, task: bun_event_loop::AnyTaskWithExtraContext::AnyTaskWithExtraContext::default(), @@ -1278,26 +1297,30 @@ pub mod bv2_impl { bv2.enqueue_on_js_loop_for_plugins(concurrent_task); } } - pub fn run_on_js_thread(&mut self) { - let is_server_side = self.bake_graph() != crate::bake_types::Graph::Client; - let default_loader = self.default_loader; - // reshaped for borrowck — capture the erased self - // pointer before borrowing fields immutably for the FFI call. - let self_ptr = std::ptr::from_mut::(self).cast::(); - // SAFETY: `bv2` is a valid backref set by `init`; the plugin - // storage is disjoint from `self`, so the `&mut JSBundlerPlugin` - // returned by `plugins_mut()` does not alias the - // `&self.path` / `&self.namespace` borrows below. - unsafe { &mut *self.bv2 } - .plugins_mut() - .expect("plugins") - .match_on_load( - &self.path, - &self.namespace, - self_ptr, - default_loader, + /// Plugins' thread; see `Resolve::run_on_js_thread`. + /// + /// # Safety + /// `this` is the request `dispatch` posted, not yet answered. + pub unsafe fn run_on_js_thread(this: *mut Self) { + // SAFETY: fn contract; of the request only this thread's + // fields are touched (type docs), and the `ParseTask` behind + // `parse_task` is not scheduled until the answer. `bv2` is + // the backref set by `init`, and the plugin storage is + // disjoint from the request, so the `&mut JSBundlerPlugin` + // does not alias the `path` / `namespace` slices. + unsafe { + let parse_task = (*this).parse_task; + let is_server_side = parse_task.known_target.bake_graph() + != crate::bake_types::Graph::Client; + let bv2 = &mut *(*this).bv2; + bv2.plugins_mut().expect("plugins").match_on_load( + &(*this).path, + &(*this).namespace, + this.cast::(), + (*this).default_loader, is_server_side, ); + } } } impl bun_event_loop::Taskable for Load { @@ -1306,13 +1329,16 @@ pub mod bv2_impl { unsafe fn release_unrun(_: *mut Self) {} } impl crate::Graph::OutstandingNode for Load { - fn link(&mut self) -> &mut crate::Graph::OutstandingLink { - &mut self.outstanding + unsafe fn link_raw(this: *mut Self) -> *mut crate::Graph::OutstandingLink { + // SAFETY: `this` is live (trait contract); a raw field + // projection borrows nothing. + unsafe { &raw mut (*this).outstanding } } } impl crate::Graph::OutstandingNode for Resolve { - fn link(&mut self) -> &mut crate::Graph::OutstandingLink { - &mut self.outstanding + unsafe fn link_raw(this: *mut Self) -> *mut crate::Graph::OutstandingLink { + // SAFETY: as for `Load`. + unsafe { &raw mut (*this).outstanding } } } } @@ -4316,7 +4342,14 @@ pub mod bv2_impl { Ok(()) } - pub fn on_load_async(&mut self, load: &mut jsc_api::JSBundler::Load) { + /// Plugins' thread: `load.value` has been filled in; hand the request + /// back. Nothing touches `*load` after the post, and `load` (the pointer + /// the bundle thread already holds in `outstanding_loads`) is what is + /// posted, not a reborrow: see the `Load` docs. + /// + /// # Safety + /// `load` is an outstanding request of this pass, answered by nobody else. + pub unsafe fn on_load_async(&mut self, load: *mut jsc_api::JSBundler::Load) { // Dispatch to the loop that *owns* `BundleV2`. // For `Bun.build` this is a Mini loop running on the bundler thread, so // `on_load` must land there — not on the JS plugin loop — or it will @@ -4324,7 +4357,7 @@ pub mod bv2_impl { match self.any_loop_mut() { bun_event_loop::AnyEventLoop::Js { .. } => { let ct = bun_event_loop::ConcurrentTask::ConcurrentTask::from_callback( - std::ptr::from_mut(load), + load, on_load_from_js_loop_raw, ); let poster = self @@ -4340,11 +4373,12 @@ pub mod bv2_impl { } } bun_event_loop::AnyEventLoop::Mini(mini) => { - // SAFETY: `load` is a valid &mut for the duration of the enqueue; - // the mini loop dispatches `on_load_mini` on the bundler thread. + // SAFETY: `load` is arena-live until answered (fn contract) + // and `task` is its intrusive node; the mini loop dispatches + // `on_load_mini` on the bundler thread. unsafe { mini.enqueue_task_concurrent_with_extra_ctx::>( - std::ptr::from_mut(load), + load, on_load_mini, core::mem::offset_of!(jsc_api::JSBundler::Load, task), ); @@ -4353,12 +4387,16 @@ pub mod bv2_impl { } } - pub fn on_resolve_async(&mut self, resolve: &mut jsc_api::JSBundler::Resolve) { + /// Plugins' thread; as [`Self::on_load_async`], for `Resolve`. + /// + /// # Safety + /// `resolve` is an outstanding request of this pass, answered by nobody else. + pub unsafe fn on_resolve_async(&mut self, resolve: *mut jsc_api::JSBundler::Resolve) { // See `on_load_async` — must dispatch on the bundler's own loop. match self.any_loop_mut() { bun_event_loop::AnyEventLoop::Js { .. } => { let ct = bun_event_loop::ConcurrentTask::ConcurrentTask::from_callback( - std::ptr::from_mut(resolve), + resolve, on_resolve_from_js_loop_raw, ); let poster = self @@ -4374,11 +4412,10 @@ pub mod bv2_impl { } } bun_event_loop::AnyEventLoop::Mini(mini) => { - // SAFETY: `resolve` is a valid &mut for the duration of the enqueue; - // the mini loop dispatches `on_resolve_mini` on the bundler thread. + // SAFETY: as in `on_load_async`. unsafe { mini.enqueue_task_concurrent_with_extra_ctx::>( - std::ptr::from_mut(resolve), + resolve, on_resolve_mini, core::mem::offset_of!(jsc_api::JSBundler::Resolve, task), ); @@ -6944,8 +6981,18 @@ pub mod bv2_impl { self.decrement_scan_counter(); } - pub fn on_notify_defer_mini(load: &mut jsc_api::JSBundler::Load, this: &mut BundleV2) { - load.deferred = true; + /// Bundle thread. `load` is still out with the plugin (its onLoad + /// callback is awaiting `.defer()`), so only this thread's `deferred` + /// field is touched: see the `Load` docs. + /// + /// # Safety + /// `load` is an outstanding request of this pass. + pub unsafe fn on_notify_defer_mini( + load: *mut jsc_api::JSBundler::Load, + this: &mut BundleV2, + ) { + // SAFETY: fn contract; `deferred` belongs to this thread. + unsafe { (*load).deferred = true }; this.on_notify_defer(); } diff --git a/src/runtime/api/JSBundler.rs b/src/runtime/api/JSBundler.rs index 24817615296..a14f927db55 100644 --- a/src/runtime/api/JSBundler.rs +++ b/src/runtime/api/JSBundler.rs @@ -1412,13 +1412,11 @@ pub mod js_bundler { /// `…__addError`, `on_notify_defer_raw`) stay safe at the call site. `bv2` /// is the back-reference set in `Resolve::init`/`Load::init`; the /// `BundleV2` heap allocation outlives every plugin callback (owner- - /// creates-child, single-JS-thread). The `BundleV2` storage is heap- - /// disjoint from `Resolve`/`Load`, so the returned `&mut` does not alias - /// the caller's `&mut Resolve`/`&mut Load`. + /// creates-child, single-JS-thread). #[inline] fn bv2_mut<'a>(bv2: *mut BundleV2<'static>) -> &'a mut BundleV2<'static> { // SAFETY: see fn doc — live backref (owner-creates-child), single - // JS-thread, disjoint heap from the `Resolve`/`Load` callers borrow. + // JS-thread. unsafe { &mut *bv2 } } @@ -1428,18 +1426,24 @@ pub mod js_bundler { /// (`JSBundlerPlugin__onResolveAsync`, `on_defer`, /// `JSBundlerPlugin__onLoadAsync`) stay safe at the call site. `plugins` /// is `Some` whenever the plugin chain is dispatched (asserted by - /// `enqueue_on_js_loop_for_plugins`). The `Plugin` storage is heap- - /// disjoint from `Resolve`/`Load`, so the returned `&mut` does not alias - /// the caller's `&mut Resolve`/`&mut Load`. + /// `enqueue_on_js_loop_for_plugins`). #[inline] fn bv2_plugin<'a>(bv2: *mut BundleV2<'static>) -> &'a mut Plugin { - // SAFETY: see fn doc — `plugins.is_some()`, disjoint heap. + // SAFETY: see fn doc — `plugins.is_some()`. unsafe { &mut *bv2_mut(bv2).plugins.unwrap().as_ptr() } } + // The four answer thunks below (`onResolveAsync`, `onDefer`, `onLoadAsync`, + // `addError`) run on the plugins' thread while the request they are + // handed is still linked in the bundle thread's outstanding list, which + // that thread writes through as other requests come and go. So they + // touch the request field by field through the pointer and never form a + // `&Resolve` / `&mut Resolve` (`&Load` / `&mut Load`) over it: see the + // docs on those types in `bun_bundler::bundle_v2`. + /// # Safety - /// `resolve` must be the live `*mut Resolve` previously handed to C++ via - /// `Resolve::dispatch`; sole owner on the JS thread for the call duration. + /// `resolve` must be the outstanding `*mut Resolve` C++ received from + /// `Resolve::run_on_js_thread`, not yet answered. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__onResolveAsync( resolve: *mut Resolve, @@ -1448,103 +1452,107 @@ pub mod js_bundler { namespace_value: JSValue, external_value: JSValue, ) { - // SAFETY: called from C++ with valid Resolve pointer - let resolve = unsafe { &mut *resolve }; - if path_value.is_empty_or_undefined_or_null() - || namespace_value.is_empty_or_undefined_or_null() - { - resolve.value = ResolveValue::NoMatch; - } else { - let global = bv2_plugin(resolve.bv2).global_object(); - // `to_slice_clone` already heap-allocates; `into_vec` moves that - // buffer out instead of allocating a second copy. - let path = path_value - .to_slice_clone(global) - .expect("Unexpected: path is not a string") - .into_vec() - .into_boxed_slice(); - let namespace = namespace_value - .to_slice_clone(global) - .expect("Unexpected: namespace is not a string") - .into_vec() - .into_boxed_slice(); - resolve.value = ResolveValue::Success(ResolveSuccess { - path, - namespace, - external: external_value.to_boolean(), - }); - } + // SAFETY: fn contract; `value` is this thread's field of the request + // (see above), and posting the answer is the last thing done with it. + unsafe { + let bv2 = (*resolve).bv2; + if path_value.is_empty_or_undefined_or_null() + || namespace_value.is_empty_or_undefined_or_null() + { + (*resolve).value = ResolveValue::NoMatch; + } else { + let global = bv2_plugin(bv2).global_object(); + // `to_slice_clone` already heap-allocates; `into_vec` moves that + // buffer out instead of allocating a second copy. + let path = path_value + .to_slice_clone(global) + .expect("Unexpected: path is not a string") + .into_vec() + .into_boxed_slice(); + let namespace = namespace_value + .to_slice_clone(global) + .expect("Unexpected: namespace is not a string") + .into_vec() + .into_boxed_slice(); + (*resolve).value = ResolveValue::Success(ResolveSuccess { + path, + namespace, + external: external_value.to_boolean(), + }); + } - bv2_mut(resolve.bv2).on_resolve_async(resolve); + bv2_mut(bv2).on_resolve_async(resolve); + } } bun_output::declare_scope!(BUNDLER_DEFERRED, hidden); - /// JSC-aware plumbing for `Load` (upstream owns `init`/`dispatch`/ - /// `run_on_js_thread`/`bake_graph`). Only `on_defer` lives here because it - /// returns a `JSValue` and throws on the `JSGlobalObject`. - trait LoadJsExt { - fn on_defer(&mut self, global_object: &JSGlobalObject) -> JsResult; - } - - impl LoadJsExt for Load { - fn on_defer(&mut self, global_object: &JSGlobalObject) -> JsResult { - if self.called_defer { + /// `args.defer()` inside an onLoad callback: tell the bundle thread this + /// load is parked and hand JS the promise it will resolve later. + /// + /// # Safety + /// `load` must be the outstanding `*mut Load` C++ received from + /// `Load::run_on_js_thread`, not yet answered. + unsafe fn on_defer(load: *mut Load, global_object: &JSGlobalObject) -> JsResult { + // SAFETY: fn contract; `called_defer` is this thread's field of the + // request (see above), `bv2` / `parse_task` are backrefs that outlive + // it, and the notify post is the last thing done with the request: + // from then on the bundle thread may write `deferred`. + unsafe { + if (*load).called_defer { return Err(global_object.throw(format_args!( "Can't call .defer() more than once within an onLoad plugin" ))); } - self.called_defer = true; + (*load).called_defer = true; bun_output::scoped_log!( BUNDLER_DEFERRED, "JSBundlerPlugin__onDefer(0x{:x}, {})", - std::ptr::from_ref(self) as usize, - bstr::BStr::new(&self.path) + load.addr(), + bstr::BStr::new(&(*load).path) ); + let plugin = bv2_plugin((*load).bv2); + let parse_task = (*load).parse_task; + let ctx = parse_task.ctx.expect("ParseTask.ctx unset"); + // Notify the *bundler thread* about the deferral. This will // decrement the pending item counter and increment the deferred // counter. Must land on `parse_task.ctx.loop()` (the loop running // BundleV2), which is distinct from `js_loop_for_plugins()` (the // plugin host's JS loop) when `Bun.build` runs the bundler on its - // own Mini event loop. - // SAFETY: parse_task.ctx and bv2 are valid backrefs; `r#loop()` - // points at a live `AnyEventLoop` owned by the bundle thread / - // runtime for the duration of the bundle. - unsafe { - let ctx = (*self.parse_task).ctx.expect("ParseTask.ctx unset"); - // SAFETY: write provenance from `ParseTask::init`; bundle outlives plugin. - let any_loop = ctx - .assume_mut() - .r#loop() - .expect("BundleV2.linker.loop must be set before plugins run"); - match &mut *any_loop.as_ptr() { - bun_event_loop::AnyEventLoop::Js { .. } => { - let ct = - ConcurrentTask::from_callback(ctx.as_mut_ptr(), on_notify_defer_raw); - let poster = (*ctx.as_mut_ptr()) - .js_poster - .as_ref() - .expect("JS-owned bundle has a poster"); - if let bun_event_loop::Posted::Refused(ct) = poster.post(ct) { - // Owning JS VM torn down mid-bundle: the notify never runs. - bun_event_loop::ConcurrentTask::ConcurrentTask::release_refused(ct); - } - } - bun_event_loop::AnyEventLoop::Mini(mini) => { - // `mini.enqueueTaskConcurrentWithExtraCtx( - // Load, BundleV2, this, BundleV2.onNotifyDeferMini, .task)` - mini.enqueue_task_concurrent_with_extra_ctx::>( - std::ptr::from_mut::(self), - on_notify_defer_mini_wrap, - core::mem::offset_of!(Load, task), - ); + // own Mini event loop. `r#loop()` points at a live `AnyEventLoop` + // owned by the bundle thread / runtime for the duration of the + // bundle; write provenance from `ParseTask::init`. + let any_loop = ctx + .assume_mut() + .r#loop() + .expect("BundleV2.linker.loop must be set before plugins run"); + match &mut *any_loop.as_ptr() { + bun_event_loop::AnyEventLoop::Js { .. } => { + let ct = ConcurrentTask::from_callback(ctx.as_mut_ptr(), on_notify_defer_raw); + let poster = (*ctx.as_mut_ptr()) + .js_poster + .as_ref() + .expect("JS-owned bundle has a poster"); + if let bun_event_loop::Posted::Refused(ct) = poster.post(ct) { + // Owning JS VM torn down mid-bundle: the notify never runs. + bun_event_loop::ConcurrentTask::ConcurrentTask::release_refused(ct); } } - - Ok(bv2_plugin(self.bv2).append_defer_promise()) + bun_event_loop::AnyEventLoop::Mini(mini) => { + // `mini.enqueueTaskConcurrentWithExtraCtx( + // Load, BundleV2, this, BundleV2.onNotifyDeferMini, .task)` + mini.enqueue_task_concurrent_with_extra_ctx::>( + load, + on_notify_defer_mini_wrap, + core::mem::offset_of!(Load, task), + ); + } } + + Ok(plugin.append_defer_promise()) } } @@ -1554,75 +1562,70 @@ pub mod js_bundler { } fn on_notify_defer_mini_wrap(load: *mut Load, ctx: *mut BundleV2<'static>) { - // SAFETY: callback contract — `load` was passed as the `Context` arg to - // `enqueue_task_concurrent_with_extra_ctx`; `ctx` is the bundle-thread - // `BundleV2` backref the mini loop's tick supplies as `ParentContext`. - BundleV2::on_notify_defer_mini(unsafe { &mut *load }, unsafe { &mut *ctx }); + // SAFETY: callback contract — `load` is the still-outstanding request + // `on_defer` passed as the `Context` arg to + // `enqueue_task_concurrent_with_extra_ctx` (handed on as the pointer); + // `ctx` is the bundle-thread `BundleV2` backref the mini loop's tick + // supplies as `ParentContext`. + unsafe { BundleV2::on_notify_defer_mini(load, &mut *ctx) }; } /// # Safety - /// `load` must be the live `*mut Load` previously handed to C++ via - /// `Load::dispatch`, and `global` must be the plugin's owning - /// `JSGlobalObject`; both valid and exclusively accessed on the JS thread. + /// `load`: see [`on_defer`]. `global` must be the plugin's owning + /// `JSGlobalObject`, valid for the call. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__onDefer( load: *mut Load, global: *mut JSGlobalObject, ) -> JSValue { - // SAFETY: called from C++ with valid pointers - unsafe { jsc::to_js_host_call(&*global, || (&mut *load).on_defer(&*global)) } + // SAFETY: fn contract, passed through. + unsafe { jsc::to_js_host_call(&*global, || on_defer(load, &*global)) } } + /// # Safety + /// `this` must be the outstanding `*mut Load` C++ received from + /// `Load::run_on_js_thread`, not yet answered. #[unsafe(no_mangle)] - extern "C" fn JSBundlerPlugin__onLoadAsync( - this: &mut Load, + unsafe extern "C" fn JSBundlerPlugin__onLoadAsync( + this: *mut Load, _unused: *mut c_void, source_code_value: JSValue, loader_as_int: JSValue, ) { jsc::mark_binding(); - if source_code_value.is_empty_or_undefined_or_null() - || loader_as_int.is_empty_or_undefined_or_null() - { - this.value = LoadValue::NoMatch; - - if this.was_file { - // Faster path: skip the extra threadpool dispatch - // SAFETY: bv2 backref is valid; pool/worker_pool are live for bundle. - unsafe { - (*(*(*this.bv2).graph.pool.as_ptr()).worker_pool).schedule( - bun_threading::thread_pool::Batch::from(core::ptr::addr_of_mut!( - (*this.parse_task.as_ptr()).task - )), - ); - } - this.value = LoadValue::Consumed; - return; - } - } else { - let loader = api::Loader::from_raw(loader_as_int.as_int32() as u8); - let global = bv2_plugin(this.bv2).global_object(); - let source_code = match crate::node::StringOrBuffer::from_js_to_owned_slice( - global, - source_code_value, - ) { - Ok(s) => s, - Err(err) => { - match err { - JsError::OutOfMemory => bun_core::out_of_memory(), - JsError::Thrown => {} - JsError::Terminated => {} + // SAFETY: fn contract; `value` is this thread's field of the request + // (see above), and posting the answer is the last thing done with it. + unsafe { + let bv2 = (*this).bv2; + if source_code_value.is_empty_or_undefined_or_null() + || loader_as_int.is_empty_or_undefined_or_null() + { + (*this).value = LoadValue::NoMatch; + } else { + let loader = api::Loader::from_raw(loader_as_int.as_int32() as u8); + let global = bv2_plugin(bv2).global_object(); + let source_code = match crate::node::StringOrBuffer::from_js_to_owned_slice( + global, + source_code_value, + ) { + Ok(s) => s, + Err(err) => { + match err { + JsError::OutOfMemory => bun_core::out_of_memory(), + JsError::Thrown => {} + JsError::Terminated => {} + } + panic!("Unexpected: source_code is not a string"); } - panic!("Unexpected: source_code is not a string"); - } - }; - this.value = LoadValue::Success(LoadSuccess { - loader: bun_ast::Loader::from_api(loader), - source_code: source_code.into(), - }); - } + }; + (*this).value = LoadValue::Success(LoadSuccess { + loader: bun_ast::Loader::from_api(loader), + source_code: source_code.into(), + }); + } - bv2_mut(this.bv2).on_load_async(this); + bv2_mut(bv2).on_load_async(this); + } } /// Opaque FFI handle for the C++ `JSBundlerPlugin`. The opaque type and @@ -1844,9 +1847,9 @@ pub mod js_bundler { /// # Safety /// `plugin` must be a live `JSBundlerPlugin` opaque handle. `ctx` must be - /// the live `*mut Resolve` (when `which == 0`) or `*mut Load` (when - /// `which == 1`) previously handed to C++ via `dispatch`; sole owner on - /// the JS thread. + /// the outstanding `*mut Resolve` (when `which == 0`) or `*mut Load` (when + /// `which == 1`) C++ received from the request's `run_on_js_thread`, not + /// yet answered. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__addError( ctx: *mut c_void, @@ -1854,24 +1857,33 @@ pub mod js_bundler { exception: JSValue, which: JSValue, ) { - // SAFETY: plugin is valid opaque FFI handle; ctx is *mut Resolve or *mut Load + // SAFETY: plugin is valid opaque FFI handle let plugin = unsafe { &mut *plugin }; match which.as_int32() { 0 => { - // SAFETY: C++ caller passes the live `*mut Resolve` it received from - // `Resolve::dispatch` as `ctx` when `which == 0`; sole owner on the JS thread. - let resolve = unsafe { bun_ptr::callback_ctx::(ctx) }; - let msg = plugin_msg_from_js(plugin, &resolve.import_record.source_file, exception); - resolve.value = ResolveValue::Err(msg); - bv2_mut(resolve.bv2).on_resolve_async(resolve); + let resolve = ctx.cast::(); + // SAFETY: fn contract (`which` says which type `ctx` is); + // `value` is this thread's field of the request (see the note + // above `JSBundlerPlugin__onResolveAsync`), and posting the + // answer is the last thing done with it. + unsafe { + let msg = plugin_msg_from_js( + plugin, + &(*resolve).import_record.source_file, + exception, + ); + (*resolve).value = ResolveValue::Err(msg); + bv2_mut((*resolve).bv2).on_resolve_async(resolve); + } } 1 => { - // SAFETY: C++ caller passes the live `*mut Load` it received from - // `Load::dispatch` as `ctx` when `which == 1`; sole owner on the JS thread. - let load = unsafe { bun_ptr::callback_ctx::(ctx) }; - let msg = plugin_msg_from_js(plugin, &load.path, exception); - load.value = LoadValue::Err(msg); - bv2_mut(load.bv2).on_load_async(load); + let load = ctx.cast::(); + // SAFETY: as for `Resolve`. + unsafe { + let msg = plugin_msg_from_js(plugin, &(*load).path, exception); + (*load).value = LoadValue::Err(msg); + bv2_mut((*load).bv2).on_load_async(load); + } } _ => panic!("invalid error type"), } diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index df0996b0438..9b475b4462c 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -222,16 +222,23 @@ pub(crate) fn run_task( }); } task_tag::BundleV2PluginResolve => { - // SAFETY: tag identifies pointee — a live `Resolve` owned by the - // plugin dispatch chain. - unsafe { &mut *cast_ptr!(bun_bundler::bundle_v2::api::JSBundler::Resolve) } - .run_on_js_thread(); + // SAFETY: tag identifies pointee — the `Resolve` its pass posted and + // is waiting on. Passed as the raw pointer: the bundle thread may + // be writing the request's list link meanwhile (see the `Resolve` + // docs), so no `&mut` to it is formed here. + unsafe { + bun_bundler::bundle_v2::api::JSBundler::Resolve::run_on_js_thread(cast_ptr!( + bun_bundler::bundle_v2::api::JSBundler::Resolve + )) + }; } task_tag::BundleV2PluginLoad => { - // SAFETY: tag identifies pointee — a live `Load` owned by the plugin - // dispatch chain. - unsafe { &mut *cast_ptr!(bun_bundler::bundle_v2::api::JSBundler::Load) } - .run_on_js_thread(); + // SAFETY: as `BundleV2PluginResolve`. + unsafe { + bun_bundler::bundle_v2::api::JSBundler::Load::run_on_js_thread(cast_ptr!( + bun_bundler::bundle_v2::api::JSBundler::Load + )) + }; } task_tag::JSBundleCompletionTask => { if let Err(err) = diff --git a/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts b/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts new file mode 100644 index 00000000000..afc6c3f106b --- /dev/null +++ b/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts @@ -0,0 +1,626 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import path from "path"; + +// An outstanding bundler plugin request is never borrowed whole. +// +// `api::JSBundler::Resolve` / `Load` (src/bundler/bundle_v2.rs) are the +// onResolve / onLoad requests a bundle pass hands to the thread running the +// plugins. From `dispatch` until the answer comes back, one request is used +// from two threads at once, by field: the plugins' thread reads the fields +// set before dispatch and writes the answer (`value`, `called_defer`, the +// intrusive `task` node), while the request stays linked in the bundle +// thread's `Graph::outstanding_*` list, and that thread writes the link +// (`OutstandingList::push` of the next request writes the head's `prev`, +// `unlink` of an answered one writes its neighbours') and `Load::deferred` +// (`on_notify_defer_mini`) whenever it likes. For `Bun.build` this really is +// concurrent; the two sides only ever touch disjoint fields, so it is fine as +// long as each side goes through the raw pointer field by field. A `&Load` / +// `&mut Load` (or `&Resolve`) formed on either side covers the other side's +// fields as well: as a function argument it is protected for the duration of +// the call, and the other thread's write is then a foreign write to a +// protected tag, which Miri rejects under Tree Borrows (the model +// `bun run rust:miri` uses) and under Stacked Borrows. This tree used to do +// that in every place the plugins' thread touched a request (`run_task`'s two +// arms formed `&mut *ptr`, `run_on_js_thread` / `on_defer` took `&mut self`, +// `onLoadAsync` took `&mut Load` straight from C++, `addError` went through +// `callback_ctx`, `on_*_async` took `&mut` and held it across the post), and +// in the two places the bundle thread touched a request that was still out +// (`OutstandingNode::link(&mut self)` on the neighbours, `on_notify_defer_mini`). +// +// What this lint holds in place, for the population listed in ENTRIES below +// (every function that touches a request while it may be outstanding; the +// answer-side consumers `on_load` / `on_resolve` and the cancellation path own +// the request again and are deliberately not in it): +// +// - the request arrives as a raw pointer: a `*mut` parameter, never a +// `self` receiver or a reference parameter; +// - the body does not turn it into a reference: no `&mut *req` / `&*req` / +// `&(*req)` of the whole request (`&(*req).field` is fine, that is the +// point), no `ptr::from_mut(req)`, no method call on `(*req)` (which +// auto-borrows the whole request; rustc's `dangerous_implicit_autorefs` +// only catches the variant that goes through an overloaded `Deref`, such +// as a `BackRef` field, which is why those are copied out first), no +// `callback_ctx`; +// - where the request is handed on (to C++ as the context cookie, to the +// post-back, to the notify), the body passes the pointer it received, not +// a reborrow, so the list, the plugins' thread and the answer all hold the +// one pointer; +// - `OutstandingNode` reaches a node's link through a raw projection, and +// `OutstandingList` never reborrows a node. +// +// The pass itself (`bv2`) is a different object and not covered: how the +// plugin hops and the answer thunks reach it is bundler-plugin-hops-raw-pass / +// event-loop-post-shared territory. `dispatch()` (the posting side, where the +// request is not yet outstanding) is bundler-plugin-dispatch-raw-request's. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); + +type Found = { file: string; name: string; line: number; complaints: string[] }; + +type Entry = { + file: string; + /** `fn ` to audit (every definition with that name in the file). */ + fn: string; + /** How many definitions the file is expected to contain. */ + count: number; + /** + * The function belongs to the request type, so a `self` receiver would be + * the request. (For `BundleV2` methods `self` is the pass, which is fine.) + */ + selfIsRequest?: boolean; + /** Body identifiers that hold the request besides the `*mut` parameters. */ + bodyNames?: string[]; + /** + * Each is given the request's parameter name and must match the body: the + * places the request is handed on, which must receive that very pointer. + */ + handsOn?: ((name: string) => RegExp)[]; +}; + +const REQUEST_TYPE = /\b(?:Resolve|Load|Self)\b/; + +const ENTRIES: Entry[] = [ + // Plugins' thread: the hop that calls into the plugin, passing the request + // as the context cookie C++ later hands back to the answer thunks. + { + file: "src/bundler/bundle_v2.rs", + fn: "run_on_js_thread", + count: 2, + selfIsRequest: true, + handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)], + }, + // Plugins' thread: the answer is posted back through the same pointer. + { + file: "src/bundler/bundle_v2.rs", + fn: "on_load_async", + count: 1, + handsOn: [ + n => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), + n => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), + ], + }, + { + file: "src/bundler/bundle_v2.rs", + fn: "on_resolve_async", + count: 1, + handsOn: [ + n => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), + n => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), + ], + }, + // Bundle thread, while the load's callback is parked in `.defer()`. + { file: "src/bundler/bundle_v2.rs", fn: "on_notify_defer_mini", count: 1 }, + // The C++-facing answer thunks and what they call. + { + file: "src/runtime/api/JSBundler.rs", + fn: "JSBundlerPlugin__onResolveAsync", + count: 1, + handsOn: [n => new RegExp(String.raw`\bon_resolve_async\(\s*${n}\s*\)`)], + }, + { + file: "src/runtime/api/JSBundler.rs", + fn: "JSBundlerPlugin__onLoadAsync", + count: 1, + handsOn: [n => new RegExp(String.raw`\bon_load_async\(\s*${n}\s*\)`)], + }, + { + file: "src/runtime/api/JSBundler.rs", + fn: "JSBundlerPlugin__onDefer", + count: 1, + handsOn: [n => new RegExp(String.raw`\bon_defer\(\s*${n}\s*,`)], + }, + { + file: "src/runtime/api/JSBundler.rs", + fn: "on_defer", + count: 1, + selfIsRequest: true, + handsOn: [n => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`)], + }, + // `addError` gets the request as `*mut c_void` plus a discriminator and + // casts it itself; the casts are the handles the body must not reborrow. + { + file: "src/runtime/api/JSBundler.rs", + fn: "JSBundlerPlugin__addError", + count: 1, + bodyNames: ["ctx", "resolve", "load"], + }, + { file: "src/runtime/api/JSBundler.rs", fn: "on_notify_defer_mini_wrap", count: 1 }, +]; + +function lineOf(text: string, index: number): number { + return text.slice(0, index).split("\n").length; +} + +/** + * Comments go first (full-line ones, then trailing ones, keeping newlines so + * line numbers hold), then string literals, so neither prose about the banned + * shapes nor a `{` inside a format string confuses the scans below. Nothing + * audited here puts `//` or a newline inside a string literal. + */ +function stripCommentsAndStrings(text: string): string { + return text + .replace(/^[ \t]*\/\/.*$/gm, "") + .replace(/[ \t]\/\/.*$/gm, "") + .replace(/"(?:[^"\\\n]|\\.)*"/g, '""'); +} + +/** The text from the `open` character at `start` to its balanced close, inclusive. */ +function balanced(text: string, start: number, open: string, close: string): string | null { + let depth = 0; + for (let i = start; i < text.length; i++) { + const c = text[i]; + if (c === open) depth++; + else if (c === close && --depth === 0) return text.slice(start, i + 1); + } + return null; +} + +/** Every `fn (...) ... { ... }` in (already stripped) `text`. */ +function fnDefinitions(text: string, name: string): { line: number; params: string; body: string | null }[] { + const out: { line: number; params: string; body: string | null }[] = []; + for (const m of text.matchAll(new RegExp(String.raw`\bfn\s+${name}\s*(?:<[^>]*>)?\s*\(`, "g"))) { + const parenAt = m.index + m[0].length - 1; + const paramList = balanced(text, parenAt, "(", ")"); + if (paramList === null) continue; + const afterParams = parenAt + paramList.length; + // A trait declaration (`fn f(&mut self) -> T;`) has no body; still + // audited for its signature. + const restOfLine = text.slice(afterParams, text.indexOf("\n", afterParams)); + let body: string | null = null; + if (!/^[^{]*;/.test(restOfLine)) { + const braceAt = text.indexOf("{", afterParams); + body = braceAt < 0 ? null : balanced(text, braceAt, "{", "}"); + } + out.push({ line: lineOf(text, m.index), params: paramList.slice(1, -1), body }); + } + return out; +} + +/** Top-level split of a parameter list on commas (generics may nest commas). */ +function splitParams(params: string): string[] { + const out: string[] = []; + let depth = 0; + let current = ""; + for (const c of params) { + if (c === "<" || c === "(" || c === "[") depth++; + else if (c === ">" || c === ")" || c === "]") depth--; + if (c === "," && depth === 0) { + out.push(current); + current = ""; + } else current += c; + } + out.push(current); + return out.map(p => p.trim()).filter(Boolean); +} + +/** The ways a body can turn the request called `name` back into a reference, or borrow it whole. */ +function referenceForming(name: string): { what: string; pattern: RegExp }[] { + const n = String.raw`\b${name}\b`; + return [ + { what: `reborrows \`${name}\``, pattern: new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${n}`) }, + { + what: `borrows \`${name}\` whole`, + pattern: new RegExp(String.raw`&\s*(?:mut\s+)?\(\s*\*\s*${n}\s*\)(?!\s*\.)`), + }, + { + what: `makes a reference to \`${name}\``, + pattern: new RegExp(String.raw`\bfrom_(?:mut|ref)(?:::<[^>]*>)?\(\s*${n}\s*\)|\bNonNull::from\(\s*${n}\s*\)`), + }, + { + what: `calls a method on \`*${name}\` (auto-borrows the whole request)`, + pattern: new RegExp(String.raw`\(\s*\*\s*${n}\s*\)\s*\.\s*\w+\s*(?:::<[^>]*>)?\s*\(`), + }, + ]; +} + +function auditFn(entry: Entry, text: string): Found[] { + return fnDefinitions(text, entry.fn).map(({ line, params, body }) => { + const complaints: string[] = []; + const names = [...(entry.bodyNames ?? [])]; + for (const param of splitParams(params)) { + if (/^(?:&\s*(?:mut\s+)?)?(?:mut\s+)?self\b/.test(param) || /^self\s*:/.test(param)) { + if (entry.selfIsRequest) complaints.push(`takes the request as a self receiver: \`${param}\``); + continue; + } + const colon = param.indexOf(":"); + if (colon < 0) continue; + const pname = param.slice(0, colon).trim(); + const ptype = param.slice(colon + 1).trim(); + if (!REQUEST_TYPE.test(ptype)) continue; + if (!/^\*\s*mut\b/.test(ptype)) complaints.push(`takes the request as \`${param}\`, not \`*mut\``); + names.push(pname); + } + if (entry.selfIsRequest && names.length === 0 && !complaints.length) { + complaints.push("takes no request parameter"); + } + if (body === null) { + if (!entry.selfIsRequest || !complaints.length) complaints.push("could not find the body"); + return { file: entry.file, name: entry.fn, line, complaints }; + } + if (entry.selfIsRequest && /\bself\b/.test(body)) complaints.push("body uses `self`"); + if (/\bcallback_ctx\b/.test(body)) complaints.push("body goes through `callback_ctx` (a `&mut` to the request)"); + for (const name of names) { + for (const { what, pattern } of referenceForming(name)) { + if (pattern.test(body)) complaints.push(`body ${what}`); + } + } + if (entry.handsOn && !entry.bodyNames) { + // The parameter that is the request (one per function in this population). + const [name] = names; + if (name !== undefined) { + for (const make of entry.handsOn) { + const pattern = make(name); + if (!pattern.test(body)) complaints.push(`body does not hand \`${name}\` itself on (expected ${pattern})`); + } + } + } + return { file: entry.file, name: entry.fn, line, complaints }; + }); +} + +/** + * src/runtime/dispatch.rs: the `run_task` arms for the two hop tags. The arm + * has to call `run_on_js_thread` with the queued pointer itself (`cast_ptr!`), + * not through `cast!` / `&mut *`, which mint a `&mut` over the request. + */ +function auditDispatchArms(text: string): Found[] { + const out: Found[] = []; + for (const m of text.matchAll(/\btask_tag::(BundleV2Plugin(?:Resolve|Load))\b\s*=>\s*\{/g)) { + const body = balanced(text, m.index + m[0].length - 1, "{", "}"); + if (body === null || !/\brun_on_js_thread\b/.test(body)) continue; // the release arm, or unreadable + const complaints: string[] = []; + if (/\bcast!\s*\(/.test(body)) complaints.push("arm uses `cast!` (forms `&mut` to the request)"); + if (/&\s*(?:mut\s+)?\*/.test(body)) complaints.push("arm reborrows the request"); + if (!/\brun_on_js_thread\(\s*cast_ptr!\s*\(/.test(body)) { + complaints.push("arm does not pass the queued pointer (`cast_ptr!`) to `run_on_js_thread`"); + } + out.push({ file: "src/runtime/dispatch.rs", name: `${m[1]} arm`, line: lineOf(text, m.index), complaints }); + } + return out; +} + +/** + * src/bundler/Graph.rs: the list only ever owns the link inside a node, and + * its neighbours are out with the plugins' thread while it writes them. + */ +function auditOutstandingList(text: string): Found[] { + const out: Found[] = []; + const trait = /\btrait\s+OutstandingNode\b[^{]*\{/.exec(text); + if (trait === null) { + out.push({ file: "src/bundler/Graph.rs", name: "trait OutstandingNode", line: 0, complaints: ["not found"] }); + } else { + const body = balanced(text, trait.index + trait[0].length - 1, "{", "}") ?? ""; + const complaints: string[] = []; + if (/&/.test(body)) + complaints.push( + "trait hands out a reference (receiver or return); the link must be reached as `*mut` from a `*mut Self`", + ); + out.push({ + file: "src/bundler/Graph.rs", + name: "trait OutstandingNode", + line: lineOf(text, trait.index), + complaints, + }); + } + const impl = /\bimpl\s*<[^>]*>\s*OutstandingList\s*<[^>]*>\s*\{/.exec(text); + if (impl === null) { + out.push({ file: "src/bundler/Graph.rs", name: "impl OutstandingList", line: 0, complaints: ["not found"] }); + } else { + const body = balanced(text, impl.index + impl[0].length - 1, "{", "}") ?? ""; + const complaints: string[] = []; + if (/&\s*(?:mut\s+)?\*/.test(body)) complaints.push("impl reborrows a node"); + if (/\(\s*\*[^()]*\)\s*\.\s*\w+\s*\(/.test(body)) + complaints.push("impl calls a method on a dereferenced node (auto-borrows it whole)"); + for (const { line, params } of fnDefinitions(body, String.raw`(?:push|unlink)`)) { + for (const param of splitParams(params)) { + if (/\bself\b/.test(param)) continue; + if (!/:\s*\*\s*mut\b/.test(param)) { + complaints.push(`line ${lineOf(text, impl.index) + line - 1}: node parameter \`${param}\` is not \`*mut\``); + } + } + } + out.push({ + file: "src/bundler/Graph.rs", + name: "impl OutstandingList", + line: lineOf(text, impl.index), + complaints, + }); + } + return out; +} + +const FILES = [...new Set([...ENTRIES.map(e => e.file), "src/runtime/dispatch.rs", "src/bundler/Graph.rs"])]; +const sources = new Map(); +for (const f of FILES) sources.set(f, stripCommentsAndStrings(await file(path.join(root, f)).text())); + +const found: Found[] = []; +for (const entry of ENTRIES) found.push(...auditFn(entry, sources.get(entry.file)!)); +found.push(...auditDispatchArms(sources.get("src/runtime/dispatch.rs")!)); +found.push(...auditOutstandingList(sources.get("src/bundler/Graph.rs")!)); + +const offenders = found + .filter(f => f.complaints.length) + .sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line) + .flatMap(f => f.complaints.map(c => `${f.file}:${f.line} (${f.name}): ${c}`)); + +test("the audited functions are still where this lint looks for them", () => { + // A mismatch means a function in this population was added, removed, moved + // or renamed: update ENTRIES (and the header) rather than the checks. + const counts: Record = {}; + for (const f of found) counts[`${f.file} ${f.name}`] = (counts[`${f.file} ${f.name}`] ?? 0) + 1; + const expected: Record = { + "src/runtime/dispatch.rs BundleV2PluginResolve arm": 1, + "src/runtime/dispatch.rs BundleV2PluginLoad arm": 1, + "src/bundler/Graph.rs trait OutstandingNode": 1, + "src/bundler/Graph.rs impl OutstandingList": 1, + }; + for (const e of ENTRIES) expected[`${e.file} ${e.fn}`] = e.count; + expect(counts).toEqual(expected); +}); + +test("the audit recognizes the shapes it claims to", () => { + const complaintsOf = (entry: Omit, snippet: string) => + auditFn({ file: "x.rs", count: 1, ...entry }, stripCommentsAndStrings(snippet)).flatMap(f => f.complaints); + const post = [ + (n: string) => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), + (n: string) => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), + ]; + + // Conforming shapes, as in the tree now. + expect( + complaintsOf( + { fn: "run_on_js_thread", selfIsRequest: true, handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)] }, + ` + pub unsafe fn run_on_js_thread(this: *mut Self) { + // SAFETY: prose mentioning &mut *this does not count. + unsafe { + let record = &(*this).import_record; + let bv2 = &mut *(*this).bv2; + bv2.plugins_mut().expect("plugins").match_on_resolve( + &record.specifier, this.cast::(), record.kind, + ); + } + }`, + ), + ).toEqual([]); + expect( + complaintsOf( + { fn: "on_load_async", handsOn: post }, + ` + pub unsafe fn on_load_async(&self, load: *mut jsc_api::JSBundler::Load) { + match self.any_loop() { + Js { .. } => { let ct = from_callback(load, on_load_from_js_loop_raw); } + Mini(mini) => unsafe { + mini.enqueue_task_concurrent_with_extra_ctx::>( + load, on_load_mini, core::mem::offset_of!(jsc_api::JSBundler::Load, task), + ); + }, + } + }`, + ), + ).toEqual([]); + expect( + complaintsOf( + { fn: "on_defer", selfIsRequest: true }, + ` + unsafe fn on_defer(load: *mut Load, global_object: &JSGlobalObject) -> JsResult { + unsafe { + (*load).called_defer = true; + log!("JSBundlerPlugin__onDefer(0x{:x}, {})", load.addr(), bstr::BStr::new(&(*load).path)); + let parse_task = (*load).parse_task; + let ctx = parse_task.ctx.expect("ParseTask.ctx unset"); + let value = core::mem::take(&mut (*load).value); + (*load).value.consume(); + Ok(plugin.append_defer_promise()) + } + }`, + ), + ).toEqual([]); + expect( + complaintsOf( + { fn: "JSBundlerPlugin__addError", bodyNames: ["ctx", "resolve", "load"] }, + ` + unsafe extern "C" fn JSBundlerPlugin__addError(ctx: *mut c_void, plugin: *mut Plugin, which: JSValue) { + let plugin = unsafe { &mut *plugin }; + match which.as_int32() { + 0 => { let resolve = ctx.cast::(); unsafe { (*resolve).value = v; bv2((*resolve).bv2).on_resolve_async(resolve); } } + 1 => { let load = ctx.cast::(); unsafe { (*load).value = v; bv2((*load).bv2).on_load_async(load); } } + _ => panic!("invalid error type"), + } + }`, + ), + ).toEqual([]); + // A request parameter next to a non-request one that may be reborrowed. + expect( + complaintsOf( + { fn: "on_notify_defer_mini_wrap" }, + "fn on_notify_defer_mini_wrap(load: *mut Load, ctx: *mut BundleV2<'static>) { unsafe { BundleV2::on_notify_defer_mini(load, &mut *ctx) }; }", + ), + ).toEqual([]); + + // The shapes this lint was written against. + expect( + complaintsOf( + { fn: "run_on_js_thread", selfIsRequest: true, handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)] }, + ` + pub fn run_on_js_thread(&mut self) { + let self_ptr = std::ptr::from_mut::(self).cast::(); + unsafe { &mut *self.bv2 }.plugins_mut().expect("plugins").match_on_load(&self.path, self_ptr); + }`, + ), + ).toEqual(["takes the request as a self receiver: `&mut self`", "body uses `self`"]); + expect( + complaintsOf( + { fn: "on_load_async", handsOn: post }, + ` + pub fn on_load_async(&mut self, load: &mut jsc_api::JSBundler::Load) { + match self.any_loop_mut() { + Js { .. } => { let ct = from_callback(std::ptr::from_mut(load), on_load_from_js_loop_raw); } + Mini(mini) => unsafe { + mini.enqueue_task_concurrent_with_extra_ctx::>( + std::ptr::from_mut(load), on_load_mini, core::mem::offset_of!(jsc_api::JSBundler::Load, task), + ); + }, + } + }`, + ), + ).toEqual([ + "takes the request as `load: &mut jsc_api::JSBundler::Load`, not `*mut`", + "body makes a reference to `load`", + `body does not hand \`load\` itself on (expected ${post[0]("load")})`, + `body does not hand \`load\` itself on (expected ${post[1]("load")})`, + ]); + expect( + complaintsOf( + { fn: "on_notify_defer_mini" }, + "pub fn on_notify_defer_mini(load: &mut jsc_api::JSBundler::Load, this: &mut BundleV2) { load.deferred = true; }", + ), + ).toEqual(["takes the request as `load: &mut jsc_api::JSBundler::Load`, not `*mut`"]); + expect( + complaintsOf( + { fn: "JSBundlerPlugin__onLoadAsync" }, + 'extern "C" fn JSBundlerPlugin__onLoadAsync(this: &mut Load, _unused: *mut c_void) { this.value = LoadValue::NoMatch; }', + ), + ).toEqual(["takes the request as `this: &mut Load`, not `*mut`"]); + expect( + complaintsOf( + { fn: "JSBundlerPlugin__onResolveAsync" }, + 'unsafe extern "C" fn JSBundlerPlugin__onResolveAsync(resolve: *mut Resolve, _unused: *mut c_void) { let resolve = unsafe { &mut *resolve }; }', + ), + ).toEqual(["body reborrows `resolve`"]); + expect( + complaintsOf( + { fn: "JSBundlerPlugin__onDefer" }, + 'unsafe extern "C" fn JSBundlerPlugin__onDefer(load: *mut Load, global: *mut JSGlobalObject) -> JSValue { unsafe { to_js_host_call(&*global, || (&mut *load).on_defer(&*global)) } }', + ), + ).toEqual(["body reborrows `load`"]); + // The trait-method form: a bodiless declaration plus the impl. + expect( + complaintsOf( + { fn: "on_defer", selfIsRequest: true }, + ` + trait LoadJsExt { + fn on_defer(&mut self, global_object: &JSGlobalObject) -> JsResult; + } + impl LoadJsExt for Load { + fn on_defer(&mut self, global_object: &JSGlobalObject) -> JsResult { + self.called_defer = true; + mini.enqueue_task_concurrent_with_extra_ctx::>(std::ptr::from_mut::(self), cb, off); + } + }`, + ), + ).toEqual([ + "takes the request as a self receiver: `&mut self`", + "takes the request as a self receiver: `&mut self`", + "body uses `self`", + ]); + expect( + complaintsOf( + { fn: "JSBundlerPlugin__addError", bodyNames: ["ctx", "resolve", "load"] }, + 'unsafe extern "C" fn JSBundlerPlugin__addError(ctx: *mut c_void) { let load = unsafe { bun_ptr::callback_ctx::(ctx) }; load.value = v; }', + ), + ).toEqual(["body goes through `callback_ctx` (a `&mut` to the request)"]); + expect( + complaintsOf( + { fn: "on_notify_defer_mini_wrap" }, + "fn on_notify_defer_mini_wrap(load: *mut Load, ctx: *mut BundleV2<'static>) { BundleV2::on_notify_defer_mini(unsafe { &mut *load }, unsafe { &mut *ctx }); }", + ), + ).toEqual(["body reborrows `load`"]); + // Raw parameter, but the body borrows the request whole again. + expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { let l = &mut (*load); }")).toEqual([ + "body borrows `load` whole", + ]); + expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { (*load).bake_graph(); }")).toEqual([ + "body calls a method on `*load` (auto-borrows the whole request)", + ]); + expect( + complaintsOf( + { fn: "f" }, + "unsafe fn f(load: *mut Load) { (*load).value.consume(); x(&(*load).path, &mut (*load).value); }", + ), + ).toEqual([]); + + // The match arms. + const arms = (snippet: string) => auditDispatchArms(stripCommentsAndStrings(snippet)).flatMap(f => f.complaints); + expect( + arms(` + task_tag::BundleV2PluginLoad => { + // SAFETY: as the Resolve arm. + unsafe { Load::run_on_js_thread(cast_ptr!(bun_bundler::bundle_v2::api::JSBundler::Load)) }; + } + task_tag::BundleV2PluginLoad => release!(bun_bundler::bundle_v2::api::JSBundler::Load),`), + ).toEqual([]); + expect( + arms(` + task_tag::BundleV2PluginLoad => { + unsafe { &mut *cast_ptr!(bun_bundler::bundle_v2::api::JSBundler::Load) }.run_on_js_thread(); + }`), + ).toEqual(["arm reborrows the request", "arm does not pass the queued pointer (`cast_ptr!`) to `run_on_js_thread`"]); + expect(arms("task_tag::BundleV2PluginResolve => { cast!(Resolve).run_on_js_thread(); }")).toEqual([ + "arm uses `cast!` (forms `&mut` to the request)", + "arm does not pass the queued pointer (`cast_ptr!`) to `run_on_js_thread`", + ]); + + // The list. + const list = (snippet: string) => auditOutstandingList(stripCommentsAndStrings(snippet)).flatMap(f => f.complaints); + expect( + list(` + pub trait OutstandingNode: Sized { + unsafe fn link_raw(this: *mut Self) -> *mut OutstandingLink; + } + impl OutstandingList { + pub(crate) fn push(&mut self, node: *mut T) { + unsafe { + let l = T::link_raw(node); + (*l).next = self.head; + if !self.head.is_null() { (*T::link_raw(self.head)).prev = node; } + } + } + pub(crate) fn unlink(&mut self, node: *mut T) { unsafe { (*T::link_raw(node)).linked = false; } } + pub(crate) fn pop(&mut self) -> Option<*mut T> { let head = self.head; self.unlink(head); Some(head) } + }`), + ).toEqual([]); + expect( + list(` + pub trait OutstandingNode: Sized { + fn link(&mut self) -> &mut OutstandingLink; + } + impl OutstandingList { + pub(crate) fn push(&mut self, node: *mut T) { + unsafe { let l = (*node).link(); l.next = self.head; (*self.head).link().prev = node; } + } + pub(crate) fn unlink(&mut self, node: &mut T) { let l = node.link(); } + pub(crate) fn pop(&mut self) -> Option<*mut T> { self.unlink(unsafe { &mut *head }); Some(head) } + }`), + ).toEqual([ + "trait hands out a reference (receiver or return); the link must be reached as `*mut` from a `*mut Self`", + "impl reborrows a node", + "impl calls a method on a dereferenced node (auto-borrows it whole)", + "line 9: node parameter `node: &mut T` is not `*mut`", + ]); +}); + +test("an outstanding plugin request is only ever touched through its raw pointer, field by field", () => { + expect(offenders).toEqual([]); +}); From 3cc7518ddd6a0b0ff7d90b40ebb0e0ce81746a3d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:51:53 +0000 Subject: [PATCH 2/4] bundler: copy the plugin call's strings out of the request first; walk the outstanding list raw A plugin that answers without suspending posts the answer from inside match_on_load / match_on_resolve, after which the pass side may consume the request and free its buffers, so the &[u8] arguments into the request those functions took were protected across exactly that window. The hops now build the BunStrings first and the match functions take them by value. OutstandingList gets a raw for_each so drain_deferred_tasks no longer walks the links itself; the type docs name both deferred writers. The lint now also covers drain_deferred_tasks and the two OutstandingNode impls, follows rebindings of the request pointer, rejects the as_mut / read / write / NonNull and untyped-parameter spellings, and requires the match_on_* call in the hops to take no borrows. --- src/bundler/Graph.rs | 35 +- src/bundler/bundle_v2.rs | 155 +++--- ...bundler-plugin-outstanding-request.test.ts | 450 ++++++++++++------ 3 files changed, 433 insertions(+), 207 deletions(-) diff --git a/src/bundler/Graph.rs b/src/bundler/Graph.rs index d96e8ea87aa..4135c38b14a 100644 --- a/src/bundler/Graph.rs +++ b/src/bundler/Graph.rs @@ -234,14 +234,12 @@ impl<'a> Graph<'a> { self.pending_items += self.deferred_pending; self.deferred_pending = 0; // Their units are back in `pending_items`. - let mut load = self.outstanding_loads.head; - while !load.is_null() { - // SAFETY: linked ⇒ arena-live; bundle thread. - unsafe { - (*load).deferred = false; - load = (*load).outstanding.next; - } - } + self.outstanding_loads.for_each(|load| { + // SAFETY: linked ⇒ arena-live. The load is still out with the + // plugin (its callback is parked in `.defer()`); `deferred` is + // this side's field (see `Load`), so nothing else is touched. + unsafe { (*load).deferred = false }; + }); transpiler.drain_defer_task.init(); transpiler.drain_defer_task.schedule(); @@ -262,7 +260,7 @@ use bun_ast::SideEffects; /// Intrusive doubly-linked membership in an [`OutstandingList`]. pub struct OutstandingLink { prev: *mut T, - pub(crate) next: *mut T, + next: *mut T, linked: bool, } impl Default for OutstandingLink { @@ -274,10 +272,10 @@ impl Default for OutstandingLink { } } } -/// A linked node is shared with the plugins' thread by field (see the docs on -/// `api::JSBundler::Resolve` / `Load`): the list owns the link and nothing -/// else, so it reaches the link through this raw projection and never forms a -/// reference to the whole node. +/// A linked node is shared with the side running the plugins, by field (see +/// the docs on `api::JSBundler::Resolve` / `Load`): the list owns the link and +/// nothing else, so it reaches the link through this raw projection and never +/// forms a reference to the whole node. pub trait OutstandingNode: Sized { /// `&raw mut (*this).outstanding`. /// @@ -346,4 +344,15 @@ impl OutstandingList { self.unlink(head); Some(head) } + /// Every linked node, each still out with the plugins: `f` may only touch + /// the fields this side owns, and must not link or unlink. + pub(crate) fn for_each(&self, mut f: impl FnMut(*mut T)) { + let mut node = self.head; + while !node.is_null() { + // SAFETY: linked ⇒ arena-live; only the link is read; bundle thread. + let next = unsafe { (*T::link_raw(node)).next }; + f(node); + node = next; + } + } } diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index 4eaaaa0ede7..a0a423005a5 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -817,52 +817,70 @@ pub mod bv2_impl { ) } + /// The namespace as onLoad plugins see it: `""` is `file`. + pub(crate) fn load_namespace(namespace: &[u8]) -> BunString { + if namespace.is_empty() { + BunString::static_(b"file") + } else { + BunString::clone_utf8(namespace) + } + } + + /// As [`Self::load_namespace`] for onResolve, which also folds a + /// spelled-out `file` into the static. + pub(crate) fn resolve_namespace(namespace: &[u8]) -> BunString { + if namespace.is_empty() || namespace == b"file" { + BunString::static_(b"file") + } else { + BunString::clone_utf8(namespace) + } + } + + /// Runs the onLoad chain for the request `context` points at. + /// + /// The strings are copies taken by the caller, not borrows of + /// the request: a callback that does not actually suspend (or no + /// matching callback at all; `runOnLoadPlugins` unwraps settled + /// promises without awaiting) answers from inside this call, and + /// from then on the bundle thread may consume the request and + /// free its buffers while this call is still unwinding. A `&[u8]` + /// argument into the request would be protected for exactly that + /// window. pub(crate) fn match_on_load( &mut self, - path: &[u8], - namespace: &[u8], + mut path: BunString, + mut namespace: BunString, context: *mut core::ffi::c_void, default_loader: Loader, is_server_side: bool, ) { let _tracer = bun_core::perf::trace("JSBundler.matchOnLoad"); - let mut namespace_string = if namespace.is_empty() { - BunString::static_(b"file") - } else { - BunString::clone_utf8(namespace) - }; - let mut path_string = BunString::clone_utf8(path); JSBundlerPlugin__matchOnLoad( self, - &mut namespace_string, - &mut path_string, + &mut namespace, + &mut path, context, default_loader as u8, is_server_side, ); } + /// Runs the onResolve chain; owned strings for the same reason as + /// [`Self::match_on_load`]. pub(crate) fn match_on_resolve( &mut self, - path: &[u8], - namespace: &[u8], - importer: &[u8], + mut path: BunString, + mut namespace: BunString, + mut importer: BunString, context: *mut core::ffi::c_void, import_record_kind: ImportKind, ) { let _tracer = bun_core::perf::trace("JSBundler.matchOnResolve"); - let mut namespace_string = if namespace.is_empty() || namespace == b"file" { - BunString::static_(b"file") - } else { - BunString::clone_utf8(namespace) - }; - let mut path_string = BunString::clone_utf8(path); - let mut importer_string = BunString::clone_utf8(importer); JSBundlerPlugin__matchOnResolve( self, - &mut namespace_string, - &mut path_string, - &mut importer_string, + &mut namespace, + &mut path, + &mut importer, context, import_record_kind as u8, ); @@ -1083,15 +1101,21 @@ pub mod bv2_impl { /// are the real lower-tier `bun_event_loop` types, so `dispatch()` / /// `run_on_js_thread()` are implemented inherently (no T6 hook). /// - /// From `dispatch` until the answer reaches `BundleV2::on_resolve`, - /// the request is shared between two threads by field: the plugins' - /// thread reads `bv2` / `import_record` and writes `value` (and - /// `task`, to post the answer), while the bundle thread owns - /// `outstanding` and writes it whenever a neighbouring request is - /// pushed or unlinked. During that window both sides go through the - /// raw pointer field by field; a `&Resolve` / `&mut Resolve` would - /// claim the other side's fields too. (Under bake the two "threads" - /// are one loop; the code is shared, so it keeps the same shape.) + /// From `dispatch` until `BundleV2::on_resolve` consumes the answer, + /// the request is shared by field between the side running the + /// plugins and the side running the pass (two threads under + /// `Bun.build`, one loop under bake; the code is the same): the + /// plugin side reads the fields set by `init` and writes `value` + /// (plus `task` when it posts), while the pass side owns + /// `outstanding` (`Graph::OutstandingList`) and writes it whenever a + /// neighbouring request is pushed or unlinked. So during that window + /// each side goes through the raw pointer, field by field; a + /// `&Resolve` / `&mut Resolve` would claim the other side's fields + /// too. The answer may be consumed as soon as it is posted, and a + /// plugin that answers synchronously posts it from inside + /// `run_on_js_thread`'s call into the plugin, so nothing borrowed + /// from the request may be live across that call or after the post + /// (`Plugin::match_on_resolve`). pub struct Resolve { pub bv2: *mut BundleV2<'static>, pub import_record: MiniImportRecord, @@ -1159,20 +1183,28 @@ pub mod bv2_impl { /// # Safety /// `this` is the request `dispatch` posted, not yet answered. pub unsafe fn run_on_js_thread(this: *mut Self) { - // SAFETY: fn contract; of the request only this thread's - // fields are touched (type docs). `bv2` is the backref set - // by `init`, and the plugin storage is disjoint from the - // request, so the `&mut JSBundlerPlugin` does not alias - // `record`. + // SAFETY: fn contract; of the request only this side's fields + // are read (type docs), and every borrow of them ends before + // the call into the plugin, which may answer (and so have + // the request consumed) before it returns. `bv2` is the + // backref set by `init`. unsafe { - let record = &(*this).import_record; + let (path, namespace, importer, kind) = { + let record = &(*this).import_record; + ( + BunString::clone_utf8(&record.specifier), + Plugin::resolve_namespace(&record.namespace), + BunString::clone_utf8(&record.source_file), + record.kind, + ) + }; let bv2 = &mut *(*this).bv2; bv2.plugins_mut().expect("plugins").match_on_resolve( - &record.specifier, - &record.namespace, - &record.source_file, + path, + namespace, + importer, this.cast::(), - record.kind, + kind, ); } } @@ -1204,13 +1236,14 @@ pub mod bv2_impl { /// Task driving an onLoad plugin invocation for one source file. /// /// Shared by field while outstanding, exactly as [`Resolve`]: the - /// plugins' thread reads `bv2` / `path` / `namespace` / - /// `default_loader` / `parse_task` and writes `value`, `called_defer` - /// and `task`; the bundle thread owns `outstanding` and `deferred` - /// (the latter written by `on_notify_defer_mini` while the plugin's - /// callback is still suspended in `.defer()`). Until the answer - /// reaches `BundleV2::on_load`, neither side forms a `&Load` / - /// `&mut Load`. + /// plugin side reads the fields set by `init` and writes `value`, + /// `called_defer` and `task`; the pass side owns `outstanding` and + /// `deferred`, the latter written while the onLoad callback is + /// parked in `.defer()`, by `Graph::drain_deferred_tasks` and (when + /// the pass runs on its own mini loop) `BundleV2::on_notify_defer_mini`. + /// Until `BundleV2::on_load` consumes the answer, neither side forms + /// a `&Load` / `&mut Load`, and nothing borrowed from the request is + /// live across the call into the plugin or after a post. pub struct Load { pub bv2: *mut BundleV2<'static>, pub(crate) source_index: bun_ast::Index, @@ -1302,22 +1335,23 @@ pub mod bv2_impl { /// # Safety /// `this` is the request `dispatch` posted, not yet answered. pub unsafe fn run_on_js_thread(this: *mut Self) { - // SAFETY: fn contract; of the request only this thread's - // fields are touched (type docs), and the `ParseTask` behind - // `parse_task` is not scheduled until the answer. `bv2` is - // the backref set by `init`, and the plugin storage is - // disjoint from the request, so the `&mut JSBundlerPlugin` - // does not alias the `path` / `namespace` slices. + // SAFETY: as `Resolve::run_on_js_thread`; additionally the + // `ParseTask` behind `parse_task` is not scheduled until the + // answer is consumed, which cannot happen before the call + // into the plugin below. unsafe { + let path = BunString::clone_utf8(&(*this).path); + let namespace = Plugin::load_namespace(&(*this).namespace); + let default_loader = (*this).default_loader; let parse_task = (*this).parse_task; let is_server_side = parse_task.known_target.bake_graph() != crate::bake_types::Graph::Client; let bv2 = &mut *(*this).bv2; bv2.plugins_mut().expect("plugins").match_on_load( - &(*this).path, - &(*this).namespace, + path, + namespace, this.cast::(), - (*this).default_loader, + default_loader, is_server_side, ); } @@ -2097,8 +2131,9 @@ pub mod bv2_impl { // reshaped for borrowck — `&self.graph` and // `self` go to the same call. Take a raw ptr so the two `&mut` don't // overlap from rustc's view. - // SAFETY: `drain_deferred_tasks` only touches `self.graph.deferred_*` - // fields and the `BundleV2` callback surface; no aliasing UB. + // SAFETY: `this` is the live `self`. `drain_deferred_tasks` + // touches the graph's counters, the outstanding loads (through + // their own pointers) and `drain_defer_task`. if unsafe { (*this).graph.drain_deferred_tasks(&mut *this) } { return false; } diff --git a/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts b/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts index afc6c3f106b..f27eebabf60 100644 --- a/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts +++ b/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts @@ -2,55 +2,69 @@ import { file } from "bun"; import { expect, test } from "bun:test"; import path from "path"; -// An outstanding bundler plugin request is never borrowed whole. +// An outstanding bundler plugin request is only ever touched through its raw +// pointer, one field at a time. // // `api::JSBundler::Resolve` / `Load` (src/bundler/bundle_v2.rs) are the -// onResolve / onLoad requests a bundle pass hands to the thread running the -// plugins. From `dispatch` until the answer comes back, one request is used -// from two threads at once, by field: the plugins' thread reads the fields -// set before dispatch and writes the answer (`value`, `called_defer`, the -// intrusive `task` node), while the request stays linked in the bundle -// thread's `Graph::outstanding_*` list, and that thread writes the link +// onResolve / onLoad requests a bundle pass hands to whatever runs the plugins +// (another thread under `Bun.build`, the same loop under bake; the code is +// shared). From `dispatch` until the answer is consumed, one request is in use +// on both sides at once, by field: the plugin side reads the fields set before +// dispatch and writes the answer (`value`, `called_defer`, the intrusive +// `task` node), while the request stays linked in the pass's +// `Graph::outstanding_*` list, and the pass side writes the link // (`OutstandingList::push` of the next request writes the head's `prev`, // `unlink` of an answered one writes its neighbours') and `Load::deferred` -// (`on_notify_defer_mini`) whenever it likes. For `Bun.build` this really is -// concurrent; the two sides only ever touch disjoint fields, so it is fine as -// long as each side goes through the raw pointer field by field. A `&Load` / -// `&mut Load` (or `&Resolve`) formed on either side covers the other side's -// fields as well: as a function argument it is protected for the duration of -// the call, and the other thread's write is then a foreign write to a -// protected tag, which Miri rejects under Tree Borrows (the model -// `bun run rust:miri` uses) and under Stacked Borrows. This tree used to do -// that in every place the plugins' thread touched a request (`run_task`'s two -// arms formed `&mut *ptr`, `run_on_js_thread` / `on_defer` took `&mut self`, -// `onLoadAsync` took `&mut Load` straight from C++, `addError` went through -// `callback_ctx`, `on_*_async` took `&mut` and held it across the post), and -// in the two places the bundle thread touched a request that was still out -// (`OutstandingNode::link(&mut self)` on the neighbours, `on_notify_defer_mini`). +// (`Graph::drain_deferred_tasks` on every linked load; `on_notify_defer_mini` +// when the pass runs on its own mini loop) whenever it likes. Under `Bun.build` +// this really is concurrent, and it is fine only because each side goes +// through the raw pointer and touches its own fields. A `&Load` / `&mut Load` +// (or `&Resolve`) formed on either side covers the other side's fields as +// well: as a function argument it is protected for the duration of the call, +// and the other side's write is then a foreign write to a protected tag, which +// Miri rejects under Tree Borrows (the model `bun run rust:miri` uses) and +// under Stacked Borrows. Timing adds one more rule: the answer may be consumed +// (and the request's buffers freed) the moment it is posted, and a plugin that +// answers synchronously posts it from inside `run_on_js_thread`'s call into the +// plugin, so nothing borrowed from the request may be an argument of that call +// or be touched after a post. This tree used to break all of this: `run_task`'s +// two arms formed `&mut *ptr`, `run_on_js_thread` / `on_defer` took `&mut self` +// and passed `&self.path` into the plugin call, `onLoadAsync` took `&mut Load` +// straight from C++, `addError` went through `callback_ctx`, `on_*_async` took +// `&mut` and held it across the post, `OutstandingNode::link(&mut self)` +// reborrowed the neighbours, and `on_notify_defer_mini` took `&mut Load`. // -// What this lint holds in place, for the population listed in ENTRIES below -// (every function that touches a request while it may be outstanding; the -// answer-side consumers `on_load` / `on_resolve` and the cancellation path own -// the request again and are deliberately not in it): +// Checked, for the population in ENTRIES plus the three structural audits +// below (the answer-side consumers `on_load` / `on_resolve` and the +// cancellation path own the request again and are deliberately not in it): // -// - the request arrives as a raw pointer: a `*mut` parameter, never a -// `self` receiver or a reference parameter; -// - the body does not turn it into a reference: no `&mut *req` / `&*req` / -// `&(*req)` of the whole request (`&(*req).field` is fine, that is the -// point), no `ptr::from_mut(req)`, no method call on `(*req)` (which -// auto-borrows the whole request; rustc's `dangerous_implicit_autorefs` -// only catches the variant that goes through an overloaded `Deref`, such -// as a `BackRef` field, which is why those are copied out first), no -// `callback_ctx`; -// - where the request is handed on (to C++ as the context cookie, to the -// post-back, to the notify), the body passes the pointer it received, not -// a reborrow, so the list, the plugins' thread and the answer all hold the -// one pointer; -// - `OutstandingNode` reaches a node's link through a raw projection, and -// `OutstandingList` never reborrows a node. +// 1. The request arrives as a `*mut Resolve` / `*mut Load` / `*mut Self` +// parameter: not a `self` receiver, not a reference, and not an untyped +// pointer either (an entry with no recognized request parameter fails, +// unless it names the handles itself via `bodyNames`, as `addError`, which +// gets a `*mut c_void` plus a discriminator, and `drain_deferred_tasks`, +// which gets its loads from the list, do). +// 2. The body does not get at the whole request through that pointer or any +// local rebound from it (`let x = req;`, `let x = req.cast::();`): +// no `&mut *req` / `&*req` / `&(*req)` (`&(*req).field` is the point and is +// fine), no `ptr::from_mut(req)` / `NonNull`, no `req.as_mut()` / +// `as_ref()`, no whole-struct `req.read()` / `write()` / `replace()` / +// `*req = ..`, no method call on `(*req)` (which auto-borrows the whole +// request; rustc's `dangerous_implicit_autorefs` only catches the variant +// that goes through an overloaded `Deref`, such as a `BackRef` field, which +// is why those are copied out first), no `callback_ctx`. +// 3. Where the request is handed on (as the context cookie, to the post-back, +// to the notify) the body passes the pointer it received, and the call +// that enters the plugin (`match_on_*`) takes no `&` arguments at all. +// 4. `OutstandingNode::link_raw` is a raw projection (both impls, and the +// trait hands out no references), and `OutstandingList` never reborrows a +// node; `run_task`'s two arms pass the queued pointer on. // -// The pass itself (`bv2`) is a different object and not covered: how the -// plugin hops and the answer thunks reach it is bundler-plugin-hops-raw-pass / +// Enforcement boundary: these are per-function text checks. A helper that +// takes the pointer and forms the reference inside is outside them, as is any +// spelling not listed in referenceForming(); if you add one, add it there and +// to the self-test. The pass itself (`bv2`) is a different object and not +// covered: how the hops and thunks reach it is bundler-plugin-hops-raw-pass / // event-loop-post-shared territory. `dispatch()` (the posting side, where the // request is not yet outstanding) is bundler-plugin-dispatch-raw-request's. @@ -66,51 +80,45 @@ type Entry = { count: number; /** * The function belongs to the request type, so a `self` receiver would be - * the request. (For `BundleV2` methods `self` is the pass, which is fine.) + * the request. (For `BundleV2` / `Graph` methods `self` is something else.) */ selfIsRequest?: boolean; - /** Body identifiers that hold the request besides the `*mut` parameters. */ + /** Body identifiers holding the request, for functions that do not get it as a typed parameter. */ bodyNames?: string[]; /** - * Each is given the request's parameter name and must match the body: the + * Each is given the request parameter's name and must match the body: the * places the request is handed on, which must receive that very pointer. */ handsOn?: ((name: string) => RegExp)[]; + /** A call the body must make, and whose argument list must contain no `&`. */ + callWithoutBorrows?: RegExp; }; const REQUEST_TYPE = /\b(?:Resolve|Load|Self)\b/; +const POST = [ + (n: string) => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), + (n: string) => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), +]; + const ENTRIES: Entry[] = [ - // Plugins' thread: the hop that calls into the plugin, passing the request - // as the context cookie C++ later hands back to the answer thunks. + // Plugin side: the hop that calls into the plugin, passing the request as + // the context cookie C++ later hands back to the answer thunks. Its call + // into the plugin may answer before it returns. { file: "src/bundler/bundle_v2.rs", fn: "run_on_js_thread", count: 2, selfIsRequest: true, handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)], + callWithoutBorrows: /\bmatch_on_(?:load|resolve)\b/, }, - // Plugins' thread: the answer is posted back through the same pointer. - { - file: "src/bundler/bundle_v2.rs", - fn: "on_load_async", - count: 1, - handsOn: [ - n => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), - n => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), - ], - }, - { - file: "src/bundler/bundle_v2.rs", - fn: "on_resolve_async", - count: 1, - handsOn: [ - n => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), - n => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), - ], - }, - // Bundle thread, while the load's callback is parked in `.defer()`. + // Plugin side: the answer is posted back through the same pointer. + { file: "src/bundler/bundle_v2.rs", fn: "on_load_async", count: 1, handsOn: POST }, + { file: "src/bundler/bundle_v2.rs", fn: "on_resolve_async", count: 1, handsOn: POST }, + // Pass side, while the loads' callbacks are parked in `.defer()`. { file: "src/bundler/bundle_v2.rs", fn: "on_notify_defer_mini", count: 1 }, + { file: "src/bundler/Graph.rs", fn: "drain_deferred_tasks", count: 1, bodyNames: ["load"] }, // The C++-facing answer thunks and what they call. { file: "src/runtime/api/JSBundler.rs", @@ -130,21 +138,10 @@ const ENTRIES: Entry[] = [ count: 1, handsOn: [n => new RegExp(String.raw`\bon_defer\(\s*${n}\s*,`)], }, - { - file: "src/runtime/api/JSBundler.rs", - fn: "on_defer", - count: 1, - selfIsRequest: true, - handsOn: [n => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`)], - }, - // `addError` gets the request as `*mut c_void` plus a discriminator and - // casts it itself; the casts are the handles the body must not reborrow. - { - file: "src/runtime/api/JSBundler.rs", - fn: "JSBundlerPlugin__addError", - count: 1, - bodyNames: ["ctx", "resolve", "load"], - }, + { file: "src/runtime/api/JSBundler.rs", fn: "on_defer", count: 1, selfIsRequest: true, handsOn: [POST[1]] }, + // `ctx` is the cookie; the `let resolve = ctx.cast::()` / + // `let load = ..` locals are picked up as rebindings of it. + { file: "src/runtime/api/JSBundler.rs", fn: "JSBundlerPlugin__addError", count: 1, bodyNames: ["ctx"] }, { file: "src/runtime/api/JSBundler.rs", fn: "on_notify_defer_mini_wrap", count: 1 }, ]; @@ -184,8 +181,8 @@ function fnDefinitions(text: string, name: string): { line: number; params: stri const paramList = balanced(text, parenAt, "(", ")"); if (paramList === null) continue; const afterParams = parenAt + paramList.length; - // A trait declaration (`fn f(&mut self) -> T;`) has no body; still - // audited for its signature. + // A trait declaration (`fn f(&mut self) -> T;`) has no body; its + // signature is still audited. const restOfLine = text.slice(afterParams, text.indexOf("\n", afterParams)); let body: string | null = null; if (!/^[^{]*;/.test(restOfLine)) { @@ -214,9 +211,25 @@ function splitParams(params: string): string[] { return out.map(p => p.trim()).filter(Boolean); } -/** The ways a body can turn the request called `name` back into a reference, or borrow it whole. */ +/** + * `names`, plus every local the body binds straight from one of them: + * `let x = req;`, `let x: T = req;`, `let x = req.cast::();`, + * `let x = req as *mut Load;`. Iterates so a rebinding of a rebinding counts. + */ +function withRebindings(body: string, names: readonly string[]): string[] { + const all = [...names]; + for (let i = 0; i < all.length; i++) { + const source = String.raw`\b${all[i]}\b(?:\s*\.\s*cast(?:::<[^>]*>)?\(\s*\)|\s+as\s+[^;]+)?`; + const rebind = new RegExp(String.raw`\blet\s+(?:mut\s+)?(\w+)(?:\s*:[^=;]+)?\s*=\s*${source}\s*;`, "g"); + for (const m of body.matchAll(rebind)) if (!all.includes(m[1])) all.push(m[1]); + } + return all; +} + +/** The ways a body can get at the whole request held in `name`. */ function referenceForming(name: string): { what: string; pattern: RegExp }[] { const n = String.raw`\b${name}\b`; + const turbofish = String.raw`(?:::<[^>]*>)?`; return [ { what: `reborrows \`${name}\``, pattern: new RegExp(String.raw`&\s*(?:mut\s+)?\*\s*${n}`) }, { @@ -224,12 +237,20 @@ function referenceForming(name: string): { what: string; pattern: RegExp }[] { pattern: new RegExp(String.raw`&\s*(?:mut\s+)?\(\s*\*\s*${n}\s*\)(?!\s*\.)`), }, { - what: `makes a reference to \`${name}\``, - pattern: new RegExp(String.raw`\bfrom_(?:mut|ref)(?:::<[^>]*>)?\(\s*${n}\s*\)|\bNonNull::from\(\s*${n}\s*\)`), + what: `makes a reference (or NonNull) to \`${name}\``, + pattern: new RegExp( + String.raw`\bfrom_(?:mut|ref)${turbofish}\(\s*${n}\s*\)|\bNonNull\s*::\s*(?:from|new|new_unchecked)\(\s*${n}\s*\)|${n}\s*\.\s*as_(?:ref|mut)\s*\(`, + ), + }, + { + what: `reads or writes \`${name}\` as a whole`, + pattern: new RegExp( + String.raw`${n}\s*\.\s*(?:read|write|replace|swap|drop_in_place)(?:_\w+)?${turbofish}\s*\(|\b(?:read|write|replace|swap|drop_in_place)(?:_\w+)?${turbofish}\(\s*${n}\s*[,)]|\*\s*${n}\s*=[^=]`, + ), }, { what: `calls a method on \`*${name}\` (auto-borrows the whole request)`, - pattern: new RegExp(String.raw`\(\s*\*\s*${n}\s*\)\s*\.\s*\w+\s*(?:::<[^>]*>)?\s*\(`), + pattern: new RegExp(String.raw`\(\s*\*\s*${n}\s*\)\s*\.\s*\w+\s*${turbofish}\s*\(`), }, ]; } @@ -237,7 +258,7 @@ function referenceForming(name: string): { what: string; pattern: RegExp }[] { function auditFn(entry: Entry, text: string): Found[] { return fnDefinitions(text, entry.fn).map(({ line, params, body }) => { const complaints: string[] = []; - const names = [...(entry.bodyNames ?? [])]; + const paramNames: string[] = []; for (const param of splitParams(params)) { if (/^(?:&\s*(?:mut\s+)?)?(?:mut\s+)?self\b/.test(param) || /^self\s*:/.test(param)) { if (entry.selfIsRequest) complaints.push(`takes the request as a self receiver: \`${param}\``); @@ -249,30 +270,36 @@ function auditFn(entry: Entry, text: string): Found[] { const ptype = param.slice(colon + 1).trim(); if (!REQUEST_TYPE.test(ptype)) continue; if (!/^\*\s*mut\b/.test(ptype)) complaints.push(`takes the request as \`${param}\`, not \`*mut\``); - names.push(pname); + paramNames.push(pname); } - if (entry.selfIsRequest && names.length === 0 && !complaints.length) { - complaints.push("takes no request parameter"); + const seeds = [...paramNames, ...(entry.bodyNames ?? [])]; + if (seeds.length === 0 && complaints.length === 0) { + complaints.push("has no `*mut Resolve` / `*mut Load` / `*mut Self` parameter (and no `bodyNames`)"); } if (body === null) { - if (!entry.selfIsRequest || !complaints.length) complaints.push("could not find the body"); + if (complaints.length === 0) complaints.push("could not find the body"); return { file: entry.file, name: entry.fn, line, complaints }; } if (entry.selfIsRequest && /\bself\b/.test(body)) complaints.push("body uses `self`"); if (/\bcallback_ctx\b/.test(body)) complaints.push("body goes through `callback_ctx` (a `&mut` to the request)"); - for (const name of names) { + for (const name of withRebindings(body, seeds)) { for (const { what, pattern } of referenceForming(name)) { if (pattern.test(body)) complaints.push(`body ${what}`); } } - if (entry.handsOn && !entry.bodyNames) { - // The parameter that is the request (one per function in this population). - const [name] = names; - if (name !== undefined) { - for (const make of entry.handsOn) { - const pattern = make(name); - if (!pattern.test(body)) complaints.push(`body does not hand \`${name}\` itself on (expected ${pattern})`); - } + const [request] = paramNames; + if (entry.handsOn && request !== undefined) { + for (const make of entry.handsOn) { + const pattern = make(request); + if (!pattern.test(body)) complaints.push(`body does not hand \`${request}\` itself on (expected ${pattern})`); + } + } + if (entry.callWithoutBorrows) { + const call = entry.callWithoutBorrows.exec(body); + const args = call === null ? null : balanced(body, body.indexOf("(", call.index + call[0].length), "(", ")"); + if (args === null) complaints.push(`body does not call ${entry.callWithoutBorrows}`); + else if (args.includes("&")) { + complaints.push(`body passes a borrow into \`${call![0]}\`, which may answer the request before it returns`); } } return { file: entry.file, name: entry.fn, line, complaints }; @@ -302,7 +329,7 @@ function auditDispatchArms(text: string): Found[] { /** * src/bundler/Graph.rs: the list only ever owns the link inside a node, and - * its neighbours are out with the plugins' thread while it writes them. + * the nodes it touches are out with the plugins while it does. */ function auditOutstandingList(text: string): Found[] { const out: Found[] = []; @@ -312,10 +339,11 @@ function auditOutstandingList(text: string): Found[] { } else { const body = balanced(text, trait.index + trait[0].length - 1, "{", "}") ?? ""; const complaints: string[] = []; - if (/&/.test(body)) + if (/&/.test(body)) { complaints.push( "trait hands out a reference (receiver or return); the link must be reached as `*mut` from a `*mut Self`", ); + } out.push({ file: "src/bundler/Graph.rs", name: "trait OutstandingNode", @@ -330,8 +358,9 @@ function auditOutstandingList(text: string): Found[] { const body = balanced(text, impl.index + impl[0].length - 1, "{", "}") ?? ""; const complaints: string[] = []; if (/&\s*(?:mut\s+)?\*/.test(body)) complaints.push("impl reborrows a node"); - if (/\(\s*\*[^()]*\)\s*\.\s*\w+\s*\(/.test(body)) + if (/\(\s*\*[^()]*\)\s*\.\s*\w+\s*\(/.test(body)) { complaints.push("impl calls a method on a dereferenced node (auto-borrows it whole)"); + } for (const { line, params } of fnDefinitions(body, String.raw`(?:push|unlink)`)) { for (const param of splitParams(params)) { if (/\bself\b/.test(param)) continue; @@ -350,7 +379,30 @@ function auditOutstandingList(text: string): Found[] { return out; } -const FILES = [...new Set([...ENTRIES.map(e => e.file), "src/runtime/dispatch.rs", "src/bundler/Graph.rs"])]; +/** + * src/bundler/bundle_v2.rs: the two `OutstandingNode` impls. `link_raw` has to + * be a raw projection (`&raw mut (*this).outstanding`); going through + * `&mut *this` or `&mut (*this).outstanding` on the way would reintroduce the + * reference the trait exists to avoid. + */ +function auditOutstandingNodeImpls(text: string): Found[] { + const out: Found[] = []; + for (const m of text.matchAll(/\bimpl\s+(?:[\w:]*::)?OutstandingNode\s+for\s+(\w+)\s*\{/g)) { + const body = balanced(text, m.index + m[0].length - 1, "{", "}"); + const complaints: string[] = []; + if (body === null) complaints.push("could not find the body"); + else if (/&(?!\s*raw\b)/.test(body)) complaints.push("impl forms a reference instead of a raw projection"); + out.push({ + file: "src/bundler/bundle_v2.rs", + name: `impl OutstandingNode for ${m[1]}`, + line: lineOf(text, m.index), + complaints, + }); + } + return out; +} + +const FILES = [...new Set([...ENTRIES.map(e => e.file), "src/runtime/dispatch.rs"])]; const sources = new Map(); for (const f of FILES) sources.set(f, stripCommentsAndStrings(await file(path.join(root, f)).text())); @@ -358,6 +410,7 @@ const found: Found[] = []; for (const entry of ENTRIES) found.push(...auditFn(entry, sources.get(entry.file)!)); found.push(...auditDispatchArms(sources.get("src/runtime/dispatch.rs")!)); found.push(...auditOutstandingList(sources.get("src/bundler/Graph.rs")!)); +found.push(...auditOutstandingNodeImpls(sources.get("src/bundler/bundle_v2.rs")!)); const offenders = found .filter(f => f.complaints.length) @@ -365,7 +418,7 @@ const offenders = found .flatMap(f => f.complaints.map(c => `${f.file}:${f.line} (${f.name}): ${c}`)); test("the audited functions are still where this lint looks for them", () => { - // A mismatch means a function in this population was added, removed, moved + // A mismatch means something in this population was added, removed, moved // or renamed: update ENTRIES (and the header) rather than the checks. const counts: Record = {}; for (const f of found) counts[`${f.file} ${f.name}`] = (counts[`${f.file} ${f.name}`] ?? 0) + 1; @@ -374,6 +427,8 @@ test("the audited functions are still where this lint looks for them", () => { "src/runtime/dispatch.rs BundleV2PluginLoad arm": 1, "src/bundler/Graph.rs trait OutstandingNode": 1, "src/bundler/Graph.rs impl OutstandingList": 1, + "src/bundler/bundle_v2.rs impl OutstandingNode for Load": 1, + "src/bundler/bundle_v2.rs impl OutstandingNode for Resolve": 1, }; for (const e of ENTRIES) expected[`${e.file} ${e.fn}`] = e.count; expect(counts).toEqual(expected); @@ -382,31 +437,34 @@ test("the audited functions are still where this lint looks for them", () => { test("the audit recognizes the shapes it claims to", () => { const complaintsOf = (entry: Omit, snippet: string) => auditFn({ file: "x.rs", count: 1, ...entry }, stripCommentsAndStrings(snippet)).flatMap(f => f.complaints); - const post = [ - (n: string) => new RegExp(String.raw`\bfrom_callback\(\s*${n}\s*,`), - (n: string) => new RegExp(String.raw`\benqueue_task_concurrent_with_extra_ctx(?:::<[^(]*>)?\(\s*${n}\s*,`), - ]; + const hop: Omit = { + fn: "run_on_js_thread", + selfIsRequest: true, + handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)], + callWithoutBorrows: /\bmatch_on_(?:load|resolve)\b/, + }; // Conforming shapes, as in the tree now. expect( complaintsOf( - { fn: "run_on_js_thread", selfIsRequest: true, handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)] }, + hop, ` pub unsafe fn run_on_js_thread(this: *mut Self) { // SAFETY: prose mentioning &mut *this does not count. unsafe { - let record = &(*this).import_record; + let (path, namespace, kind) = { + let record = &(*this).import_record; + (BunString::clone_utf8(&record.specifier), Plugin::resolve_namespace(&record.namespace), record.kind) + }; let bv2 = &mut *(*this).bv2; - bv2.plugins_mut().expect("plugins").match_on_resolve( - &record.specifier, this.cast::(), record.kind, - ); + bv2.plugins_mut().expect("plugins").match_on_resolve(path, namespace, this.cast::(), kind); } }`, ), ).toEqual([]); expect( complaintsOf( - { fn: "on_load_async", handsOn: post }, + { fn: "on_load_async", handsOn: POST }, ` pub unsafe fn on_load_async(&self, load: *mut jsc_api::JSBundler::Load) { match self.any_loop() { @@ -439,7 +497,7 @@ test("the audit recognizes the shapes it claims to", () => { ).toEqual([]); expect( complaintsOf( - { fn: "JSBundlerPlugin__addError", bodyNames: ["ctx", "resolve", "load"] }, + { fn: "JSBundlerPlugin__addError", bodyNames: ["ctx"] }, ` unsafe extern "C" fn JSBundlerPlugin__addError(ctx: *mut c_void, plugin: *mut Plugin, which: JSValue) { let plugin = unsafe { &mut *plugin }; @@ -451,6 +509,19 @@ test("the audit recognizes the shapes it claims to", () => { }`, ), ).toEqual([]); + expect( + complaintsOf( + { fn: "drain_deferred_tasks", bodyNames: ["load"] }, + ` + pub(crate) fn drain_deferred_tasks(&mut self, transpiler: &mut BundleV2) -> bool { + self.outstanding_loads.for_each(|load| { + unsafe { (*load).deferred = false }; + }); + transpiler.drain_defer_task.schedule(); + true + }`, + ), + ).toEqual([]); // A request parameter next to a non-request one that may be reborrowed. expect( complaintsOf( @@ -458,21 +529,42 @@ test("the audit recognizes the shapes it claims to", () => { "fn on_notify_defer_mini_wrap(load: *mut Load, ctx: *mut BundleV2<'static>) { unsafe { BundleV2::on_notify_defer_mini(load, &mut *ctx) }; }", ), ).toEqual([]); + // Field-scoped access of every kind is fine. + expect( + complaintsOf( + { fn: "f" }, + "unsafe fn f(load: *mut Load) { (*load).value.consume(); x(&(*load).path, &mut (*load).value); (*load).deferred = true; let p = &raw mut (*load).outstanding; }", + ), + ).toEqual([]); // The shapes this lint was written against. expect( complaintsOf( - { fn: "run_on_js_thread", selfIsRequest: true, handsOn: [n => new RegExp(String.raw`\b${n}\s*\.\s*cast\b`)] }, + hop, ` pub fn run_on_js_thread(&mut self) { let self_ptr = std::ptr::from_mut::(self).cast::(); - unsafe { &mut *self.bv2 }.plugins_mut().expect("plugins").match_on_load(&self.path, self_ptr); + unsafe { &mut *self.bv2 }.plugins_mut().expect("plugins").match_on_load(&self.path, &self.namespace, self_ptr); }`, ), - ).toEqual(["takes the request as a self receiver: `&mut self`", "body uses `self`"]); + ).toEqual([ + "takes the request as a self receiver: `&mut self`", + "body uses `self`", + "body passes a borrow into `match_on_load`, which may answer the request before it returns", + ]); + // Raw parameter, but a borrow of the request is still an argument of the plugin call. + expect( + complaintsOf( + hop, + "pub unsafe fn run_on_js_thread(this: *mut Self) { unsafe { plugin.match_on_load(&(*this).path, &(*this).namespace, this.cast::(), loader) } }", + ), + ).toEqual(["body passes a borrow into `match_on_load`, which may answer the request before it returns"]); + expect( + complaintsOf(hop, "pub unsafe fn run_on_js_thread(this: *mut Self) { let _ = this.cast::(); }"), + ).toEqual(["body does not call /\\bmatch_on_(?:load|resolve)\\b/"]); expect( complaintsOf( - { fn: "on_load_async", handsOn: post }, + { fn: "on_load_async", handsOn: POST }, ` pub fn on_load_async(&mut self, load: &mut jsc_api::JSBundler::Load) { match self.any_loop_mut() { @@ -487,9 +579,9 @@ test("the audit recognizes the shapes it claims to", () => { ), ).toEqual([ "takes the request as `load: &mut jsc_api::JSBundler::Load`, not `*mut`", - "body makes a reference to `load`", - `body does not hand \`load\` itself on (expected ${post[0]("load")})`, - `body does not hand \`load\` itself on (expected ${post[1]("load")})`, + "body makes a reference (or NonNull) to `load`", + `body does not hand \`load\` itself on (expected ${POST[0]("load")})`, + `body does not hand \`load\` itself on (expected ${POST[1]("load")})`, ]); expect( complaintsOf( @@ -497,6 +589,21 @@ test("the audit recognizes the shapes it claims to", () => { "pub fn on_notify_defer_mini(load: &mut jsc_api::JSBundler::Load, this: &mut BundleV2) { load.deferred = true; }", ), ).toEqual(["takes the request as `load: &mut jsc_api::JSBundler::Load`, not `*mut`"]); + expect( + complaintsOf( + { fn: "drain_deferred_tasks", bodyNames: ["load"] }, + ` + pub(crate) fn drain_deferred_tasks(&mut self, transpiler: &mut BundleV2) -> bool { + let mut load = self.outstanding_loads.head; + while !load.is_null() { + let l = unsafe { &mut *load }; + l.deferred = false; + load = l.outstanding.next; + } + true + }`, + ), + ).toEqual(["body reborrows `load`"]); expect( complaintsOf( { fn: "JSBundlerPlugin__onLoadAsync" }, @@ -537,7 +644,7 @@ test("the audit recognizes the shapes it claims to", () => { ]); expect( complaintsOf( - { fn: "JSBundlerPlugin__addError", bodyNames: ["ctx", "resolve", "load"] }, + { fn: "JSBundlerPlugin__addError", bodyNames: ["ctx"] }, 'unsafe extern "C" fn JSBundlerPlugin__addError(ctx: *mut c_void) { let load = unsafe { bun_ptr::callback_ctx::(ctx) }; load.value = v; }', ), ).toEqual(["body goes through `callback_ctx` (a `&mut` to the request)"]); @@ -547,19 +654,66 @@ test("the audit recognizes the shapes it claims to", () => { "fn on_notify_defer_mini_wrap(load: *mut Load, ctx: *mut BundleV2<'static>) { BundleV2::on_notify_defer_mini(unsafe { &mut *load }, unsafe { &mut *ctx }); }", ), ).toEqual(["body reborrows `load`"]); - // Raw parameter, but the body borrows the request whole again. + + // Evasions of the raw parameter: retyping it, rebinding it, or getting at + // the whole request through pointer methods. + expect( + complaintsOf( + { fn: "JSBundlerPlugin__onLoadAsync" }, + 'unsafe extern "C" fn JSBundlerPlugin__onLoadAsync(this: *mut c_void, _unused: *mut c_void) { let this = unsafe { &mut *this.cast::() }; this.value = v; }', + ), + ).toEqual(["has no `*mut Resolve` / `*mut Load` / `*mut Self` parameter (and no `bodyNames`)"]); + expect( + complaintsOf( + { fn: "on_notify_defer_mini" }, + "pub fn on_notify_defer_mini(&mut self, this: &mut BundleV2) { self.deferred = true; }", + ), + ).toEqual(["has no `*mut Resolve` / `*mut Load` / `*mut Self` parameter (and no `bodyNames`)"]); + expect( + complaintsOf( + { fn: "f" }, + "unsafe fn f(this: *mut Load) { let l = unsafe { this.as_mut() }.unwrap(); l.value = v; }", + ), + ).toEqual(["body makes a reference (or NonNull) to `this`"]); + expect( + complaintsOf({ fn: "f" }, "unsafe fn f(this: *mut Self) { let me = unsafe { this.as_ref() }.unwrap(); }"), + ).toEqual(["body makes a reference (or NonNull) to `this`"]); + expect( + complaintsOf({ fn: "f" }, "unsafe fn f(this: *mut Load) { let load = this; let l = unsafe { &mut *load }; }"), + ).toEqual(["body reborrows `load`"]); + expect( + complaintsOf({ fn: "f" }, "unsafe fn f(ctx: *mut c_void) { let load = ctx.cast::(); let l = &mut *load; }"), + ).toEqual(["has no `*mut Resolve` / `*mut Load` / `*mut Self` parameter (and no `bodyNames`)"]); + expect( + complaintsOf( + { fn: "f", bodyNames: ["ctx"] }, + "unsafe fn f(ctx: *mut c_void) { let load = ctx.cast::(); let again = load; let l = &mut *again; }", + ), + ).toEqual(["body reborrows `again`"]); + expect( + complaintsOf( + { fn: "f" }, + "unsafe fn f(load: *mut Load) { let p = load as *mut Load; let r = NonNull::new(p).unwrap(); }", + ), + ).toEqual(["body makes a reference (or NonNull) to `p`"]); + expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { load.write(Load::default()); }")).toEqual([ + "body reads or writes `load` as a whole", + ]); + expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { let l = load.read(); }")).toEqual([ + "body reads or writes `load` as a whole", + ]); + expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { core::ptr::drop_in_place(load); }")).toEqual([ + "body reads or writes `load` as a whole", + ]); + expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { *load = Load::default(); }")).toEqual([ + "body reads or writes `load` as a whole", + ]); expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { let l = &mut (*load); }")).toEqual([ "body borrows `load` whole", ]); expect(complaintsOf({ fn: "f" }, "unsafe fn f(load: *mut Load) { (*load).bake_graph(); }")).toEqual([ "body calls a method on `*load` (auto-borrows the whole request)", ]); - expect( - complaintsOf( - { fn: "f" }, - "unsafe fn f(load: *mut Load) { (*load).value.consume(); x(&(*load).path, &mut (*load).value); }", - ), - ).toEqual([]); // The match arms. const arms = (snippet: string) => auditDispatchArms(stripCommentsAndStrings(snippet)).flatMap(f => f.complaints); @@ -599,6 +753,7 @@ test("the audit recognizes the shapes it claims to", () => { } pub(crate) fn unlink(&mut self, node: *mut T) { unsafe { (*T::link_raw(node)).linked = false; } } pub(crate) fn pop(&mut self) -> Option<*mut T> { let head = self.head; self.unlink(head); Some(head) } + pub(crate) fn for_each(&self, mut f: impl FnMut(*mut T)) { let next = unsafe { (*T::link_raw(self.head)).next }; f(self.head); } }`), ).toEqual([]); expect( @@ -619,8 +774,35 @@ test("the audit recognizes the shapes it claims to", () => { "impl calls a method on a dereferenced node (auto-borrows it whole)", "line 9: node parameter `node: &mut T` is not `*mut`", ]); + + // The node impls. + const impls = (snippet: string) => + auditOutstandingNodeImpls(stripCommentsAndStrings(snippet)).flatMap(f => f.complaints); + expect( + impls(` + impl crate::Graph::OutstandingNode for Load { + unsafe fn link_raw(this: *mut Self) -> *mut crate::Graph::OutstandingLink { + // SAFETY: prose about &mut *this does not count. + unsafe { &raw mut (*this).outstanding } + } + }`), + ).toEqual([]); + expect( + impls(` + impl OutstandingNode for Load { + unsafe fn link_raw(this: *mut Self) -> *mut OutstandingLink { + let node: &mut Self = unsafe { &mut *this }; + &mut node.outstanding + } + }`), + ).toEqual(["impl forms a reference instead of a raw projection"]); + expect( + impls( + "impl OutstandingNode for Load { unsafe fn link_raw(this: *mut Self) -> *mut Link { unsafe { &mut (*this).outstanding } } }", + ), + ).toEqual(["impl forms a reference instead of a raw projection"]); }); -test("an outstanding plugin request is only ever touched through its raw pointer, field by field", () => { +test("outstanding plugin requests are reached only as raw pointers, field by field, in every audited function", () => { expect(offenders).toEqual([]); }); From 895a4fb3bb4423b62007eba0827d1a6f816b3ad5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:18:11 +0000 Subject: [PATCH 3/4] bundler: state the outstanding-request contract once, on the types; read lint sources in beforeAll --- src/bundler/Graph.rs | 28 ++---- src/bundler/bundle_v2.rs | 97 +++++++------------ src/runtime/api/JSBundler.rs | 65 ++++--------- ...bundler-plugin-outstanding-request.test.ts | 30 +++--- 4 files changed, 81 insertions(+), 139 deletions(-) diff --git a/src/bundler/Graph.rs b/src/bundler/Graph.rs index 4135c38b14a..e9c3614bfb3 100644 --- a/src/bundler/Graph.rs +++ b/src/bundler/Graph.rs @@ -235,9 +235,7 @@ impl<'a> Graph<'a> { self.deferred_pending = 0; // Their units are back in `pending_items`. self.outstanding_loads.for_each(|load| { - // SAFETY: linked ⇒ arena-live. The load is still out with the - // plugin (its callback is parked in `.defer()`); `deferred` is - // this side's field (see `Load`), so nothing else is touched. + // SAFETY: linked ⇒ arena-live; `deferred` is this side's field (see `Load`). unsafe { (*load).deferred = false }; }); @@ -272,18 +270,15 @@ impl Default for OutstandingLink { } } } -/// A linked node is shared with the side running the plugins, by field (see -/// the docs on `api::JSBundler::Resolve` / `Load`): the list owns the link and -/// nothing else, so it reaches the link through this raw projection and never -/// forms a reference to the whole node. +/// Raw, not `&mut self`: the list owns only the link inside a node it shares +/// with the plugin side (see `api::JSBundler::Resolve`). pub trait OutstandingNode: Sized { - /// `&raw mut (*this).outstanding`. - /// /// # Safety /// `this` points at a live node. unsafe fn link_raw(this: *mut Self) -> *mut OutstandingLink; } -/// A bundle pass's outstanding plugin requests; single-threaded (bundle thread). +/// A bundle pass's outstanding plugin requests; pass side only. Every linked +/// node is out with the plugins, so the list touches nothing but links. pub struct OutstandingList { head: *mut T, } @@ -296,9 +291,7 @@ impl Default for OutstandingList { } impl OutstandingList { pub(crate) fn push(&mut self, node: *mut T) { - // SAFETY: `node` is arena-live and unlinked; the current head is - // linked ⇒ arena-live (and possibly being answered by the plugins' - // thread right now, hence only its link is touched); bundle thread. + // SAFETY: `node` is arena-live and unlinked; the head is linked ⇒ arena-live. unsafe { let l = T::link_raw(node); debug_assert!(!(*l).linked); @@ -313,9 +306,7 @@ impl OutstandingList { } /// No-op if `node` is not linked (already answered / never dispatched). pub(crate) fn unlink(&mut self, node: *mut T) { - // SAFETY: `node` is arena-live; its neighbours are linked ⇒ arena-live - // (and still out with the plugins' thread, hence only their links are - // touched); bundle thread. + // SAFETY: `node` is arena-live; its neighbours are linked ⇒ arena-live. unsafe { let l = T::link_raw(node); if !(*l).linked { @@ -344,12 +335,11 @@ impl OutstandingList { self.unlink(head); Some(head) } - /// Every linked node, each still out with the plugins: `f` may only touch - /// the fields this side owns, and must not link or unlink. + /// `f` may only touch the pass side's fields of each node, and must not link or unlink. pub(crate) fn for_each(&self, mut f: impl FnMut(*mut T)) { let mut node = self.head; while !node.is_null() { - // SAFETY: linked ⇒ arena-live; only the link is read; bundle thread. + // SAFETY: linked ⇒ arena-live. let next = unsafe { (*T::link_raw(node)).next }; f(node); node = next; diff --git a/src/bundler/bundle_v2.rs b/src/bundler/bundle_v2.rs index a0a423005a5..d6eec65535e 100644 --- a/src/bundler/bundle_v2.rs +++ b/src/bundler/bundle_v2.rs @@ -826,8 +826,7 @@ pub mod bv2_impl { } } - /// As [`Self::load_namespace`] for onResolve, which also folds a - /// spelled-out `file` into the static. + /// The namespace as onResolve plugins see it: `""` and `file` are `file`. pub(crate) fn resolve_namespace(namespace: &[u8]) -> BunString { if namespace.is_empty() || namespace == b"file" { BunString::static_(b"file") @@ -836,16 +835,9 @@ pub mod bv2_impl { } } - /// Runs the onLoad chain for the request `context` points at. - /// - /// The strings are copies taken by the caller, not borrows of - /// the request: a callback that does not actually suspend (or no - /// matching callback at all; `runOnLoadPlugins` unwraps settled - /// promises without awaiting) answers from inside this call, and - /// from then on the bundle thread may consume the request and - /// free its buffers while this call is still unwinding. A `&[u8]` - /// argument into the request would be protected for exactly that - /// window. + /// Owned strings, not borrows of the request: a plugin that answers + /// without suspending does so from inside this call, after which + /// the request's buffers may be freed before the call returns. pub(crate) fn match_on_load( &mut self, mut path: BunString, @@ -865,8 +857,7 @@ pub mod bv2_impl { ); } - /// Runs the onResolve chain; owned strings for the same reason as - /// [`Self::match_on_load`]. + /// Owned strings as in [`Self::match_on_load`]. pub(crate) fn match_on_resolve( &mut self, mut path: BunString, @@ -1101,21 +1092,16 @@ pub mod bv2_impl { /// are the real lower-tier `bun_event_loop` types, so `dispatch()` / /// `run_on_js_thread()` are implemented inherently (no T6 hook). /// - /// From `dispatch` until `BundleV2::on_resolve` consumes the answer, - /// the request is shared by field between the side running the - /// plugins and the side running the pass (two threads under - /// `Bun.build`, one loop under bake; the code is the same): the - /// plugin side reads the fields set by `init` and writes `value` - /// (plus `task` when it posts), while the pass side owns - /// `outstanding` (`Graph::OutstandingList`) and writes it whenever a - /// neighbouring request is pushed or unlinked. So during that window - /// each side goes through the raw pointer, field by field; a - /// `&Resolve` / `&mut Resolve` would claim the other side's fields - /// too. The answer may be consumed as soon as it is posted, and a - /// plugin that answers synchronously posts it from inside - /// `run_on_js_thread`'s call into the plugin, so nothing borrowed - /// from the request may be live across that call or after the post - /// (`Plugin::match_on_resolve`). + /// Shared by field while outstanding (from `dispatch` until + /// `BundleV2::on_resolve`), between the side running the plugins and + /// the side running the pass (two threads under `Bun.build`): the + /// plugin side reads what `init` set and writes `value` and `task`; + /// the pass side owns `outstanding`, which it writes as neighbouring + /// requests come and go. So each side uses the raw pointer field by + /// field, a `&Resolve` / `&mut Resolve` would cover the other side's + /// fields too, and since the answer may be consumed as soon as it is + /// posted (even from inside the plugin call), nothing borrowed from + /// the request is live across that call or after the post. pub struct Resolve { pub bv2: *mut BundleV2<'static>, pub import_record: MiniImportRecord, @@ -1177,17 +1163,15 @@ pub mod bv2_impl { bv2.enqueue_on_js_loop_for_plugins(task); } } - /// Plugins' thread. `this` itself is the context C++ hands back - /// to the `JSBundlerPlugin__*` answer thunks. + /// Plugin side; `this` is also the context C++ hands back to the + /// `JSBundlerPlugin__*` thunks. /// /// # Safety /// `this` is the request `dispatch` posted, not yet answered. pub unsafe fn run_on_js_thread(this: *mut Self) { - // SAFETY: fn contract; of the request only this side's fields - // are read (type docs), and every borrow of them ends before - // the call into the plugin, which may answer (and so have - // the request consumed) before it returns. `bv2` is the - // backref set by `init`. + // SAFETY: fn contract; the type docs' rules, with every borrow + // of the request ending before `match_on_resolve`. `bv2` is + // `init`'s backref. unsafe { let (path, namespace, importer, kind) = { let record = &(*this).import_record; @@ -1235,15 +1219,9 @@ pub mod bv2_impl { /// Task driving an onLoad plugin invocation for one source file. /// - /// Shared by field while outstanding, exactly as [`Resolve`]: the - /// plugin side reads the fields set by `init` and writes `value`, - /// `called_defer` and `task`; the pass side owns `outstanding` and - /// `deferred`, the latter written while the onLoad callback is - /// parked in `.defer()`, by `Graph::drain_deferred_tasks` and (when - /// the pass runs on its own mini loop) `BundleV2::on_notify_defer_mini`. - /// Until `BundleV2::on_load` consumes the answer, neither side forms - /// a `&Load` / `&mut Load`, and nothing borrowed from the request is - /// live across the call into the plugin or after a post. + /// Shared by field while outstanding, under [`Resolve`]'s rules; here + /// the plugin side also writes `called_defer`, and the pass side also + /// owns `deferred` (`Graph::drain_deferred_tasks`, `on_notify_defer_mini`). pub struct Load { pub bv2: *mut BundleV2<'static>, pub(crate) source_index: bun_ast::Index, @@ -1330,15 +1308,13 @@ pub mod bv2_impl { bv2.enqueue_on_js_loop_for_plugins(concurrent_task); } } - /// Plugins' thread; see `Resolve::run_on_js_thread`. + /// As `Resolve::run_on_js_thread`. /// /// # Safety /// `this` is the request `dispatch` posted, not yet answered. pub unsafe fn run_on_js_thread(this: *mut Self) { - // SAFETY: as `Resolve::run_on_js_thread`; additionally the - // `ParseTask` behind `parse_task` is not scheduled until the - // answer is consumed, which cannot happen before the call - // into the plugin below. + // SAFETY: as `Resolve::run_on_js_thread`; the `ParseTask` is + // not scheduled until the answer, i.e. not before `match_on_load`. unsafe { let path = BunString::clone_utf8(&(*this).path); let namespace = Plugin::load_namespace(&(*this).namespace); @@ -4377,13 +4353,11 @@ pub mod bv2_impl { Ok(()) } - /// Plugins' thread: `load.value` has been filled in; hand the request - /// back. Nothing touches `*load` after the post, and `load` (the pointer - /// the bundle thread already holds in `outstanding_loads`) is what is - /// posted, not a reborrow: see the `Load` docs. + /// Plugin side: post the answered request back; the post is the last + /// thing done with it (see `Load`). /// /// # Safety - /// `load` is an outstanding request of this pass, answered by nobody else. + /// `load` is outstanding on this pass and answered by nobody else. pub unsafe fn on_load_async(&mut self, load: *mut jsc_api::JSBundler::Load) { // Dispatch to the loop that *owns* `BundleV2`. // For `Bun.build` this is a Mini loop running on the bundler thread, so @@ -4422,10 +4396,10 @@ pub mod bv2_impl { } } - /// Plugins' thread; as [`Self::on_load_async`], for `Resolve`. + /// As [`Self::on_load_async`]. /// /// # Safety - /// `resolve` is an outstanding request of this pass, answered by nobody else. + /// `resolve` is outstanding on this pass and answered by nobody else. pub unsafe fn on_resolve_async(&mut self, resolve: *mut jsc_api::JSBundler::Resolve) { // See `on_load_async` — must dispatch on the bundler's own loop. match self.any_loop_mut() { @@ -7016,17 +6990,16 @@ pub mod bv2_impl { self.decrement_scan_counter(); } - /// Bundle thread. `load` is still out with the plugin (its onLoad - /// callback is awaiting `.defer()`), so only this thread's `deferred` - /// field is touched: see the `Load` docs. + /// Pass side; `load` is still out with the plugin, so only `deferred` is + /// touched (see `Load`). /// /// # Safety - /// `load` is an outstanding request of this pass. + /// `load` is outstanding on this pass. pub unsafe fn on_notify_defer_mini( load: *mut jsc_api::JSBundler::Load, this: &mut BundleV2, ) { - // SAFETY: fn contract; `deferred` belongs to this thread. + // SAFETY: fn contract; `deferred` is this side's field. unsafe { (*load).deferred = true }; this.on_notify_defer(); } diff --git a/src/runtime/api/JSBundler.rs b/src/runtime/api/JSBundler.rs index a14f927db55..d9456c29c49 100644 --- a/src/runtime/api/JSBundler.rs +++ b/src/runtime/api/JSBundler.rs @@ -1433,17 +1433,11 @@ pub mod js_bundler { unsafe { &mut *bv2_mut(bv2).plugins.unwrap().as_ptr() } } - // The four answer thunks below (`onResolveAsync`, `onDefer`, `onLoadAsync`, - // `addError`) run on the plugins' thread while the request they are - // handed is still linked in the bundle thread's outstanding list, which - // that thread writes through as other requests come and go. So they - // touch the request field by field through the pointer and never form a - // `&Resolve` / `&mut Resolve` (`&Load` / `&mut Load`) over it: see the - // docs on those types in `bun_bundler::bundle_v2`. + // The thunks below get a request that is still outstanding, so they follow + // the field-by-field rules on `Resolve` / `Load` (bun_bundler::bundle_v2). /// # Safety - /// `resolve` must be the outstanding `*mut Resolve` C++ received from - /// `Resolve::run_on_js_thread`, not yet answered. + /// `resolve` is the outstanding request C++ got from `Resolve::run_on_js_thread`. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__onResolveAsync( resolve: *mut Resolve, @@ -1452,8 +1446,7 @@ pub mod js_bundler { namespace_value: JSValue, external_value: JSValue, ) { - // SAFETY: fn contract; `value` is this thread's field of the request - // (see above), and posting the answer is the last thing done with it. + // SAFETY: fn contract; `value` is this side's field; the post is the last touch. unsafe { let bv2 = (*resolve).bv2; if path_value.is_empty_or_undefined_or_null() @@ -1487,17 +1480,14 @@ pub mod js_bundler { bun_output::declare_scope!(BUNDLER_DEFERRED, hidden); - /// `args.defer()` inside an onLoad callback: tell the bundle thread this - /// load is parked and hand JS the promise it will resolve later. + /// `args.defer()`: park the load on the pass side and hand JS the promise. /// /// # Safety - /// `load` must be the outstanding `*mut Load` C++ received from - /// `Load::run_on_js_thread`, not yet answered. + /// `load` is the outstanding request C++ got from `Load::run_on_js_thread`. unsafe fn on_defer(load: *mut Load, global_object: &JSGlobalObject) -> JsResult { - // SAFETY: fn contract; `called_defer` is this thread's field of the - // request (see above), `bv2` / `parse_task` are backrefs that outlive - // it, and the notify post is the last thing done with the request: - // from then on the bundle thread may write `deferred`. + // SAFETY: fn contract; `called_defer` is this side's field, the backrefs + // (`bv2`, `parse_task`, its `ctx` and that pass's loop) outlive the + // request, and the notify post is the last touch. unsafe { if (*load).called_defer { return Err(global_object.throw(format_args!( @@ -1517,14 +1507,8 @@ pub mod js_bundler { let parse_task = (*load).parse_task; let ctx = parse_task.ctx.expect("ParseTask.ctx unset"); - // Notify the *bundler thread* about the deferral. This will - // decrement the pending item counter and increment the deferred - // counter. Must land on `parse_task.ctx.loop()` (the loop running - // BundleV2), which is distinct from `js_loop_for_plugins()` (the - // plugin host's JS loop) when `Bun.build` runs the bundler on its - // own Mini event loop. `r#loop()` points at a live `AnyEventLoop` - // owned by the bundle thread / runtime for the duration of the - // bundle; write provenance from `ParseTask::init`. + // The notify must land on the loop running the BundleV2, which under + // `Bun.build` is its own mini loop, not the plugins' JS loop. let any_loop = ctx .assume_mut() .r#loop() @@ -1562,17 +1546,13 @@ pub mod js_bundler { } fn on_notify_defer_mini_wrap(load: *mut Load, ctx: *mut BundleV2<'static>) { - // SAFETY: callback contract — `load` is the still-outstanding request - // `on_defer` passed as the `Context` arg to - // `enqueue_task_concurrent_with_extra_ctx` (handed on as the pointer); - // `ctx` is the bundle-thread `BundleV2` backref the mini loop's tick - // supplies as `ParentContext`. + // SAFETY: callback contract — `load` is the outstanding request `on_defer` + // enqueued; `ctx` is the `BundleV2` the mini loop's tick passes as the parent. unsafe { BundleV2::on_notify_defer_mini(load, &mut *ctx) }; } /// # Safety - /// `load`: see [`on_defer`]. `global` must be the plugin's owning - /// `JSGlobalObject`, valid for the call. + /// `load` as for [`on_defer`]; `global` is the plugin's live `JSGlobalObject`. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__onDefer( load: *mut Load, @@ -1583,8 +1563,7 @@ pub mod js_bundler { } /// # Safety - /// `this` must be the outstanding `*mut Load` C++ received from - /// `Load::run_on_js_thread`, not yet answered. + /// `this` is the outstanding request C++ got from `Load::run_on_js_thread`. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__onLoadAsync( this: *mut Load, @@ -1593,8 +1572,7 @@ pub mod js_bundler { loader_as_int: JSValue, ) { jsc::mark_binding(); - // SAFETY: fn contract; `value` is this thread's field of the request - // (see above), and posting the answer is the last thing done with it. + // SAFETY: fn contract; `value` is this side's field; the post is the last touch. unsafe { let bv2 = (*this).bv2; if source_code_value.is_empty_or_undefined_or_null() @@ -1846,10 +1824,8 @@ pub mod js_bundler { } /// # Safety - /// `plugin` must be a live `JSBundlerPlugin` opaque handle. `ctx` must be - /// the outstanding `*mut Resolve` (when `which == 0`) or `*mut Load` (when - /// `which == 1`) C++ received from the request's `run_on_js_thread`, not - /// yet answered. + /// `plugin` is a live `JSBundlerPlugin` handle; `ctx` is the outstanding `*mut Resolve` + /// (`which == 0`) or `*mut Load` (`which == 1`) C++ got from its `run_on_js_thread`. #[unsafe(no_mangle)] unsafe extern "C" fn JSBundlerPlugin__addError( ctx: *mut c_void, @@ -1862,10 +1838,7 @@ pub mod js_bundler { match which.as_int32() { 0 => { let resolve = ctx.cast::(); - // SAFETY: fn contract (`which` says which type `ctx` is); - // `value` is this thread's field of the request (see the note - // above `JSBundlerPlugin__onResolveAsync`), and posting the - // answer is the last thing done with it. + // SAFETY: fn contract; `value` is this side's field; the post is the last touch. unsafe { let msg = plugin_msg_from_js( plugin, diff --git a/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts b/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts index f27eebabf60..01bdaefa870 100644 --- a/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts +++ b/test/internal/source-lints/bundler-plugin-outstanding-request.test.ts @@ -1,5 +1,5 @@ import { file } from "bun"; -import { expect, test } from "bun:test"; +import { beforeAll, expect, test } from "bun:test"; import path from "path"; // An outstanding bundler plugin request is only ever touched through its raw @@ -403,19 +403,25 @@ function auditOutstandingNodeImpls(text: string): Found[] { } const FILES = [...new Set([...ENTRIES.map(e => e.file), "src/runtime/dispatch.rs"])]; -const sources = new Map(); -for (const f of FILES) sources.set(f, stripCommentsAndStrings(await file(path.join(root, f)).text())); -const found: Found[] = []; -for (const entry of ENTRIES) found.push(...auditFn(entry, sources.get(entry.file)!)); -found.push(...auditDispatchArms(sources.get("src/runtime/dispatch.rs")!)); -found.push(...auditOutstandingList(sources.get("src/bundler/Graph.rs")!)); -found.push(...auditOutstandingNodeImpls(sources.get("src/bundler/bundle_v2.rs")!)); +// Read inside the suite rather than at module scope, so a moved or renamed +// source file shows up as a failing test instead of a file that failed to load. +let found: Found[] = []; +let offenders: string[] = []; +beforeAll(async () => { + const sources = new Map(); + for (const f of FILES) sources.set(f, stripCommentsAndStrings(await file(path.join(root, f)).text())); -const offenders = found - .filter(f => f.complaints.length) - .sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line) - .flatMap(f => f.complaints.map(c => `${f.file}:${f.line} (${f.name}): ${c}`)); + found = ENTRIES.flatMap(entry => auditFn(entry, sources.get(entry.file)!)); + found.push(...auditDispatchArms(sources.get("src/runtime/dispatch.rs")!)); + found.push(...auditOutstandingList(sources.get("src/bundler/Graph.rs")!)); + found.push(...auditOutstandingNodeImpls(sources.get("src/bundler/bundle_v2.rs")!)); + + offenders = found + .filter(f => f.complaints.length) + .sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line) + .flatMap(f => f.complaints.map(c => `${f.file}:${f.line} (${f.name}): ${c}`)); +}); test("the audited functions are still where this lint looks for them", () => { // A mismatch means something in this population was added, removed, moved From c7960c246a29d11e594fce72afa9e39a272152ef Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:11:53 +0000 Subject: [PATCH 4/4] ci: retrigger