Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/bundler/bundle_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ impl<'a> BundleV2<'a> {
self.client_transpiler.as_deref()
}

/// Safe projection of the `plugins` backref (opaque C++ `BunPlugin`).
/// Safe projection of the `plugins` backref (opaque C++ `Bun::BundlerPlugin`).
/// Set once in `init` from `BakeOptions` / completion config; live for the
/// bundle pass.
#[inline]
Expand Down Expand Up @@ -694,12 +694,13 @@ pub mod bv2_impl {
use bun_core::String as BunString;
use bun_resolver::fs::PathResolverExt as _;

// `Plugin = opaque {}` — backed by C++ `BunPlugin`. The bundler calls
// `has_any_matches` / `match_on_load` / `match_on_resolve` directly
// (no JSC types needed — only `BunString` / raw context ptrs). The
// JSC-aware methods (`create`, `add_plugin`, `global_object`, …) are
// added by `bun_runtime` via the `PluginJscExt` extension trait so
// this crate stays free of `JSValue` / `JSGlobalObject`.
// `Plugin = opaque {}` — backed by C++ `Bun::BundlerPlugin` (heap,
// ThreadSafeRefCounted). The bundler calls `has_any_matches` /
// `match_on_load` / `match_on_resolve` directly (no JSC types
// needed — only `BunString` / raw context ptrs). The JSC-aware
// methods (`create`, `add_plugin`, `global_object`, …) are added by
// `bun_runtime` via the `PluginJscExt` extension trait so this
// crate stays free of `JSValue` / `JSGlobalObject`.
Comment thread
robobun marked this conversation as resolved.
bun_opaque::opaque_ffi! { pub struct Plugin; }
unsafe extern "C" {
// The three `safe fn`s below take only Rust references / by-value
Expand Down
192 changes: 100 additions & 92 deletions src/jsc/bindings/JSBundlerPlugin.cpp

Large diffs are not rendered by default.

50 changes: 34 additions & 16 deletions src/jsc/bindings/JSBundlerPlugin.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <JavaScriptCore/RegularExpression.h>
#include "napi_external.h"
#include <JavaScriptCore/Yarr.h>
#include <wtf/ThreadSafeRefCounted.h>
#include "WriteBarrierList.h"

typedef void (*JSBundlerPluginAddErrorCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue);
Expand All @@ -18,7 +19,11 @@ namespace Bun {

using namespace JSC;

class BundlerPlugin final {
class JSBundlerPlugin;

// Ref-counted separately from the JSBundlerPlugin GC cell so the bundle thread can
// read filter lists after a Worker's VM is torn down. Cell holds one ref, Rust another.
Comment thread
robobun marked this conversation as resolved.
class BundlerPlugin final : public ThreadSafeRefCounted<BundlerPlugin> {
public:
/// In native plugins, the regular expression could be called concurrently on multiple threads.
/// Therefore, we need a mutex to synchronize access.
Expand All @@ -41,7 +46,7 @@ class BundlerPlugin final {
{
}

bool match(JSC::VM& vm, const String& path);
bool match(const String& path);
};

class NamespaceList {
Expand Down Expand Up @@ -74,7 +79,9 @@ class BundlerPlugin final {

struct NativePluginCallback {
JSBundlerPluginNativeOnBeforeParseCallback callback;
Bun::NapiExternal* external;
// NapiExternal::value() captured at append time so the parse thread never
// reads the GC cell; onBeforeParseExternals keeps the cell alive for GC.
Comment thread
robobun marked this conversation as resolved.
void* externalValue;
Comment thread
robobun marked this conversation as resolved.
/// This refers to the string exported in the native plugin under
/// the symbol BUN_PLUGIN_NAME
///
Expand All @@ -95,8 +102,8 @@ class BundlerPlugin final {
PerNamespaceCallbackList fileCallbacks = {};
Vector<PerNamespaceCallbackList> namespaceCallbacks = {};

int call(JSC::VM& vm, BundlerPlugin* plugin, int* shouldContinue, void* bunContextPtr, const BunString* namespaceStr, const BunString* pathString, OnBeforeParseArguments* onBeforeParseArgs, OnBeforeParseResult* onBeforeParseResult);
void append(JSC::VM& vm, JSC::RegExp* filter, String& namespaceString, JSBundlerPluginNativeOnBeforeParseCallback callback, const char* name, NapiExternal* external);
int call(BundlerPlugin* plugin, int* shouldContinue, void* bunContextPtr, const BunString* namespaceStr, const BunString* pathString, OnBeforeParseArguments* onBeforeParseArgs, OnBeforeParseResult* onBeforeParseResult);
void append(JSC::VM& vm, JSC::RegExp* filter, String& namespaceString, JSBundlerPluginNativeOnBeforeParseCallback callback, const char* name, void* externalValue);

Vector<FilterRegExp>* group(const String& namespaceStr, unsigned& index)
{
Expand All @@ -118,33 +125,44 @@ class BundlerPlugin final {
};

public:
bool anyMatchesCrossThread(JSC::VM&, BunString* namespaceStr, BunString* path, bool isOnLoad);
void tombstone() { tombstoned = true; }

BundlerPlugin(void* config, BunPluginTarget target, JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
: addError(addError)
, onLoadAsync(onLoadAsync)
, onResolveAsync(onResolveAsync)
static Ref<BundlerPlugin> create(void* config, BunPluginTarget target, JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
{
this->target = target;
this->config = config;
return adoptRef(*new BundlerPlugin(config, target, addError, onLoadAsync, onResolveAsync));
}

bool anyMatchesCrossThread(BunString* namespaceStr, BunString* path, bool isOnLoad);
void tombstone() { tombstoned = true; }

JSBundlerPlugin* cell() const { return m_cell; }
void setCell(JSBundlerPlugin* cell) { m_cell = cell; }
Comment thread
coderabbitai[bot] marked this conversation as resolved.

NamespaceList onLoad = {};
NamespaceList onResolve = {};
NativePluginList onBeforeParse = {};
BunPluginTarget target { BunPluginTargetBrowser };

WriteBarrierList<JSC::JSPromise> deferredPromises = {};
// The raw `NapiExternal*` stored in `NativePluginCallback` is dereferenced
// off the JS thread; this list keeps those cells alive for GC.
// Roots each registered NapiExternal against normal GC while the build runs.
WriteBarrierList<NapiExternal> onBeforeParseExternals = {};

JSBundlerPluginAddErrorCallback addError;
JSBundlerPluginOnLoadAsyncCallback onLoadAsync;
JSBundlerPluginOnResolveAsyncCallback onResolveAsync;
void* config { nullptr };
bool tombstoned { false };

private:
BundlerPlugin(void* config, BunPluginTarget target, JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
: addError(addError)
, onLoadAsync(onLoadAsync)
, onResolveAsync(onResolveAsync)
{
this->target = target;
this->config = config;
}

// Back-pointer into the owning VM's GC heap. Only dereferenced on the JS thread.
JSBundlerPlugin* m_cell { nullptr };
};

} // namespace Zig
24 changes: 12 additions & 12 deletions src/runtime/api/JSBundler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1620,7 +1620,7 @@
bv2_mut(this.bv2).on_load_async(this);
}

/// Opaque FFI handle for the C++ `JSBundlerPlugin`. The opaque type and
/// Opaque FFI handle for the C++ `Bun::BundlerPlugin`. The opaque type and
/// `has_any_matches` (the one method `bun_bundler` needs) live in the
/// lower-tier crate; JSC-aware methods are added here via `PluginJscExt`.
pub use bun_bundler::bundle_v2::api::JSBundler::Plugin;
Expand All @@ -1635,7 +1635,7 @@
global: &JSGlobalObject,
target: jsc::BunPluginTarget,
) -> *mut Plugin;
safe fn JSBundlerPlugin__tombstone(plugin: &Plugin);
fn JSBundlerPlugin__destroy(plugin: *mut Plugin);
safe fn JSBundlerPlugin__runOnEndCallbacks(
plugin: &mut Plugin,
build_promise: JSValue,
Expand Down Expand Up @@ -1663,9 +1663,9 @@
) -> JSValue;
}

/// JSC-aware methods on the C++ `JSBundlerPlugin` opaque. The opaque type
/// itself is owned by `bun_bundler` (lower tier, no JSC dep), so these are
/// added as an extension trait rather than an inherent `impl`.
/// JSC-aware methods on the C++ `Bun::BundlerPlugin` opaque. The opaque
/// type itself is owned by `bun_bundler` (lower tier, no JSC dep), so these
/// are added as an extension trait rather than an inherent `impl`.
Comment thread
robobun marked this conversation as resolved.
pub trait PluginJscExt {
fn create(global: &JSGlobalObject, target: jsc::BunPluginTarget) -> *mut Plugin;
fn run_on_end_callbacks(
Expand All @@ -1675,8 +1675,8 @@
build_result: JSValue,
rejection: JsResult<JSValue>,
) -> JsResult<JSValue>;
/// `this` must be a live handle previously returned by `Plugin::create`;
/// non-null is checked via `Plugin::opaque_ref` (panics on null).
/// `this` must be the non-null +1 handle returned by `Plugin::create`
/// (debug-asserted); the callee releases it and may free the allocation.
Comment thread
robobun marked this conversation as resolved.
fn destroy(this: *mut Plugin);
fn global_object(&self) -> &JSGlobalObject;
fn append_defer_promise(&mut self) -> JSValue;
Expand All @@ -1701,9 +1701,7 @@
impl PluginJscExt for Plugin {
fn create(global: &JSGlobalObject, target: jsc::BunPluginTarget) -> *mut Plugin {
jsc::mark_binding();
let plugin = JSBundlerPlugin__create(global, target);
JSValue::from_cell(plugin).protect();
plugin
JSBundlerPlugin__create(global, target)
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

fn run_on_end_callbacks(
Expand Down Expand Up @@ -1739,8 +1737,10 @@

fn destroy(this: *mut Plugin) {
jsc::mark_binding();
JSBundlerPlugin__tombstone(Plugin::opaque_ref(this));
JSValue::from_cell(this).unprotect();
debug_assert!(!this.is_null());
// SAFETY: `this` is the +1 handle returned by `JSBundlerPlugin__create`;
// the callee releases it and may free the allocation.
unsafe { JSBundlerPlugin__destroy(this) };

Check failure on line 1743 in src/runtime/api/JSBundler.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

this public function might dereference a raw pointer but is not marked `unsafe`
}
Comment thread
robobun marked this conversation as resolved.

fn global_object(&self) -> &JSGlobalObject {
Expand Down
16 changes: 10 additions & 6 deletions src/runtime/api/js_bundle_completion_task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,11 @@ impl JSBundleCompletionTask {
///
/// Centralises the `Option<NonNull> → Option<&mut T>` deref so callers
/// (`to_js_error` / `on_complete_anytask`) stay safe. The plugin is a C++
/// `JSBundlerPlugin` opaque created by [`PluginJscExt::create`] and
/// `protect()`-ed for the task's lifetime; it is freed only via
/// `Plugin::destroy` in `deinit` *after* `take()` clears `self.plugins`.
/// While the field is `Some` the pointee is therefore live, pinned, and
/// disjoint from `*self` (separate C++-heap allocation).
/// `BundlerPlugin` heap allocation created by [`PluginJscExt::create`]
/// (which also `gcProtect`s the owning `JSBundlerPlugin` GC cell and takes
/// a +1 ref for this task); it is released only via `Plugin::destroy` in
/// `deinit` *after* `take()` clears `self.plugins`. While the field is
/// `Some` the pointee is therefore live, pinned, and disjoint from `*self`.
Comment thread
robobun marked this conversation as resolved.
#[inline]
fn plugins_mut(&mut self) -> Option<&mut Plugin> {
// SAFETY: see fn doc — C++-heap opaque, live while `self.plugins` is
Expand Down Expand Up @@ -544,12 +544,16 @@ impl JSBundleCompletionTask {
// SAFETY: `vm` is the live per-thread VM (`global_this.bun_vm_ptr()`).
this.poll_ref
.unref(unsafe { jsc::virtual_machine::VirtualMachine::event_loop_ctx(vm) });
if this.html_build_task.is_some() {
// The HTML-bundle path borrows `plugins` from `ServePluginsState::Loaded`;
// clear it before `deinit` (cancelled or not) so only the owner destroys it.
Comment thread
robobun marked this conversation as resolved.
this.plugins = None;
}
if this.cancelled {
return Ok(());
}

if let Some(html_build_task) = this.html_build_task {
this.plugins = None;
// SAFETY: `html_build_task` is a backref set by `HTMLBundle::Route` which
// bumped its own refcount before scheduling and stays alive until this returns.
// R-2: deref as shared — `on_complete` takes `&self`.
Expand Down
8 changes: 3 additions & 5 deletions src/runtime/bake/bake_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,7 @@ impl Drop for UserOptions {
if let Some(p) = self.bundler_options.plugin {
// `p` is the FFI handle returned by `Plugin::create` in
// `parse_plugin_array`; `PluginJscExt::destroy` is its paired
// (safe) destructor — it null-checks via `opaque_ref` and
// unprotect()s the JSCell / tombstones the C++ object.
// destructor (tombstones + gcUnprotects the cell + drops the ref).
Plugin::destroy(p.as_ptr());
}
}
Expand Down Expand Up @@ -321,8 +320,7 @@ impl SplitBundlerOptions {
Some(p) => p,
None => {
let p = Plugin::create(global, bun_jsc::BunPluginTarget::Bun);
let p = NonNull::new(p)
.expect("JSBundlerPlugin__create returns a non-null protected JSCell");
let p = NonNull::new(p).expect("JSBundlerPlugin__create returns non-null");
self.plugin = Some(p);
p
}
Expand Down Expand Up @@ -360,7 +358,7 @@ impl SplitBundlerOptions {
};

// `Plugin` is an `opaque_ffi!` ZST — `opaque_mut` is the safe
// deref. Handle held live in `self.plugin` (protected JSCell).
// deref. Handle held live in `self.plugin` (ref-counted C++ heap).
let plugin_result = Plugin::opaque_mut(plugin.as_ptr()).add_plugin(
function,
empty_object,
Expand Down
10 changes: 5 additions & 5 deletions src/runtime/bake/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ pub use bake_body::{PatternBuffer, UserOptions, print_warning};

/// All bake JSC references go through this re-export of `bun_jsc`.
pub mod jsc {
/// `jsc.API.JSBundler.Plugin` — the C++ `BunPlugin` FFI handle. The
/// canonical opaque struct lives in `bun_bundler::bundle_v2::api::JSBundler`
/// (T5) and is re-exported through `crate::api::js_bundler` so the
/// JSC-aware `PluginJscExt` methods are in scope; both paths name the same
/// nominal type.
/// `jsc.API.JSBundler.Plugin` — the C++ `Bun::BundlerPlugin` FFI handle.
/// The canonical opaque struct lives in
/// `bun_bundler::bundle_v2::api::JSBundler` (T5) and is re-exported through
/// `crate::api::js_bundler` so the JSC-aware `PluginJscExt` methods are in
/// scope; both paths name the same nominal type.
Comment thread
robobun marked this conversation as resolved.
pub(crate) use crate::api::js_bundler::Plugin;
pub(crate) use crate::jsc::*;
}
Expand Down
23 changes: 17 additions & 6 deletions src/runtime/server/server_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1045,8 +1045,12 @@ impl ServePlugins {
// below (Stacked Borrows), making the eventual `heap::take` in `deref_` UB.

let plugin = JSBundler::Plugin::create(global, bun_jsc::BunPluginTarget::Browser);
// SAFETY: `Plugin::create` returns a freshly-boxed `*mut Plugin` (single owner).
// `Plugin` is an `opaque_ffi!` ZST; boxing the handle lets the enum own it
// by value. `Box<ZST>` drop is a no-op, so release goes through
// `Plugin::destroy` (guard below / `handle_on_reject` / `Drop for ServePlugins`).
// SAFETY: `Plugin::create` returns a non-null +1 handle.
let plugin: Box<JSBundler::Plugin> = unsafe { bun_core::heap::take(plugin) };
Comment thread
robobun marked this conversation as resolved.
let plugin = scopeguard::guard(plugin, |p| JSBundler::Plugin::destroy(Box::into_raw(p)));
let mut bunstring_array: Vec<BunString> = Vec::with_capacity(plugin_list.len());
for raw_plugin in &plugin_list {
bunstring_array.push(BunString::init(&***raw_plugin));
Expand All @@ -1056,7 +1060,7 @@ impl ServePlugins {

self.state = ServePluginsState::Pending {
promise: jsc::JSPromiseStrong::init(global),
plugin,
plugin: scopeguard::ScopeGuard::into_inner(plugin),
html_bundle_routes: Vec::new(),
dev_server: None,
};
Expand Down Expand Up @@ -1172,7 +1176,7 @@ impl ServePlugins {
else {
unreachable!()
};
drop(plugin); // pending.plugin.deinit()
JSBundler::Plugin::destroy(Box::into_raw(plugin));
drop(promise); // Drop on JscStrong releases the slot.

for route in html_bundle_routes {
Expand Down Expand Up @@ -1224,10 +1228,17 @@ impl Drop for ServePluginsRef {

impl Drop for ServePlugins {
fn drop(&mut self) {
match &self.state {
match mem::replace(&mut self.state, ServePluginsState::Err) {
ServePluginsState::Unqueued(_) => {}
ServePluginsState::Pending { .. } => debug_assert!(false), // should have one ref while pending!
ServePluginsState::Loaded(_) => {} // Box<Plugin> drops
ServePluginsState::Pending { plugin, .. } => {
// Reachable only if the setup host call threw before the promise
// `.then()` took its +1 ref; release the handle we created.
Comment thread
robobun marked this conversation as resolved.
JSBundler::Plugin::destroy(Box::into_raw(plugin));
debug_assert!(false); // should have one ref while pending!
}
ServePluginsState::Loaded(plugin) => {
JSBundler::Plugin::destroy(Box::into_raw(plugin));
}
Comment thread
robobun marked this conversation as resolved.
ServePluginsState::Err => {}
}
}
Expand Down
Loading
Loading