Never hand one StringImpl to two threads' atom tables (object URLs, BroadcastChannel, --compile workers)

REVIEW.md

@@ -25,7 +25,7 @@ Several situational sections live in `.claude/docs/landing-prs.md` — read the
 - **Never let a pointer or slice outlive the memory it points into** (unsafe Rust, C++, FFI). Rejected shapes: slices of stack buffers; pointers into growable containers held across any call that can append; network/parser callback buffers stored without cloning (they are reused); `.data()` of a dead temporary; slices of small-string-optimized values; buffers handed to layers that store them past the call. If background threads reference stack state, every exit path must join them first.
 - **Root or copy every JSValue held beyond the current call.** WriteBarrier members declared in `.classes.ts` and visited in visitChildrenImpl (same change); Strong/protect only for justified self-keepalive; MarkedArgumentBuffer for values accumulated across slow calls — never raw JSValues in malloc'd memory or std containers. A Strong ref does not prevent ArrayBuffer detach; pin() is not a GC root; hasPendingActivity uses a counter; zero-copy toSlice-style helpers return borrowed views — consume synchronously or clone. Prove GC-safety with a stress test (thousands of iterations + `Bun.gc(true)`). Don't add Strong refs or ensureStillAlive you can't justify — and don't silently delete existing ones.
 - **Anything that can run user JS can synchronously free your state** — toString/valueOf, getters, Proxy traps, event emits, close(). Do all coercions first while holding no raw pointers; read mutable state (byteLength, typed-array vectors) once, after all observable side effects; re-validate liveness guards after every callback; copy-and-null stored one-shot callbacks before calling them; bracket entry points that can reach synchronous teardown with ref()/defer deref(); register state and listeners BEFORE the call that can trigger them; null member fields before calling close on a local copy.
-- **Know the thread affinity of every line you touch.** JS-heap operations run only on the JS thread — marshal raw data and enqueue a task. Atomics for every shared counter (even metrics); a mutex only counts if EVERY accessor takes it; benign same-value races are still UB. Copy or `toThreadSafe` strings before another thread touches them; default to seq_cst and comment any weakened ordering. Cross-thread lifetime needs refcounts — a "finalized" boolean cannot prevent UAF. Never invoke callbacks while holding a non-recursive lock. Never back per-VM state with globals or thread-locals — workers share them.
+- **Know the thread affinity of every line you touch.** JS-heap operations run only on the JS thread — marshal raw data and enqueue a task. Atomics for every shared counter (even metrics); a mutex only counts if EVERY accessor takes it; benign same-value races are still UB. `thread_isolated_copy()` a string before one other thread touches it, `make_thread_shareable()` before several VMs can reach it; default to seq_cst and comment any weakened ordering. Cross-thread lifetime needs refcounts — a "finalized" boolean cannot prevent UAF. Never invoke callbacks while holding a non-recursive lock. Never back per-VM state with globals or thread-locals — workers share them.
 - **Reference counts provably balanced on every terminal path** — success, error, cancellation, finalize. Map each ref to a named owner; take a ref only after a fallible enqueue succeeds; never use saturating arithmetic on counts; never add a ref just to silence ASAN — find the actual imbalance. The released ref may be the last one mid-callback; dropping the final reference while holding the object's own lock is UB.
 
 ## Correctness: the bug class, not the bug

src/CLAUDE.md

@@ -115,16 +115,16 @@ source is a bare `&[u8]`/`EncodedSlice` view.
 `Utf8Bytes<'a>` is `Borrowed(&'a [u8]) | Owned(Vec<u8>) | Shared(String)`
 (`Shared` holds an 8-bit all-ASCII WTF-backed `String` and reads its buffer);
 it derefs to `[u8]`; `is_owned()` ⇔ the bytes were transcoded/copied.
-`Utf8WithString` (`String::into_utf8_with_string[_thread_safe]()`) keeps the
+`Utf8WithString` (`String::into_utf8_with_string[_thread_isolated]()`) keeps the
 UTF-8 bytes _and_ the source `String` so the value can go back to JS without
 re-encoding; `Utf8WithString::js_only(string)` wraps an output-only string.
-`PathLike<'a>` / `StringOrBuffer<'a>` arms: `String`/`ThreadsafeString`
+`PathLike<'a>` / `StringOrBuffer<'a>` arms: `String`/`ThreadIsolatedString`
 (`Utf8WithString` from a JS string), `Utf8(Utf8Bytes<'a>)` (transcoded JS
 string, or Rust-side bytes: `PathLike::borrowed(bytes)` lends `&'a [u8]` to a
 synchronous call, `PathLike::owned(vec)` when the value must own them),
-`Buffer`. Anything
-parsed from JS, stored, or sent to another thread (`to_thread_safe`,
-`ThreadSafe<T>`, the fs `args::*<'static>` async path) is `'static`.
+`Buffer`. Anything parsed from JS, stored, or sent to another thread
+(`make_thread_isolated` / `ThreadIsolated<T>` for the fs `args::*<'static>`
+async path, `PathLike::thread_isolated_copy` for a `Blob` store) is `'static`.
 
 `EncodedSlice<'a>` is the `{ptr, len}` + encoding-bits (Latin-1/UTF-8/UTF-16)
 borrowed view handed to C++. Constructors name the encoding of the bytes:
@@ -384,13 +384,24 @@ non-transferring path UAFs at GC.
 
 ### Cross-thread string hazards
 
-`AtomString`s live in a per-thread table. Never deref one from another thread —
-it trips `wasRemoved` in `AtomStringImpl::remove()`. If a `bun_core::String`
-may be dropped from a non-JS thread (HTTP worker, threadpool, dying VM), build
-it via `String::clone_utf8` (a plain `WTFStringImpl` with an atomic refcount),
-not from an interned/atomized JS string. See the comment in
-`src/runtime/webcore/fetch/FetchTasklet.rs` near `Response::init` for the
-canonical example of this bug class and its fix.
+`StringImpl` refcounts are atomic; two things are per-thread: using a string as
+a property key (`Identifier::fromString`) atomizes a non-atom impl _in place_
+into the current thread's atom table, and the last `deref()` of an atom removes
+it from the _current_ thread's table (`RELEASE_ASSERT(wasRemoved)`). The lazily
+computed hash/flags word is also unsynchronized. Rules:
+
+- Handing a value to one other thread (work pool, HTTP thread):
+  `String::thread_isolated_copy()`, `ThreadIsolated<T>`, or own bytes
+  (`Box<[u8]>`, `clone_utf8` on arrival).
+- Letting several VMs reach one impl (process-global registry, one
+  `SerializedScriptValue` with many receivers): `String::make_thread_shareable()`
+  (C++ `Bun::makeThreadShareable` / `threadShareableCopy` /
+  `toCrossThreadShareable`) once — pre-hashed, never atomized in place, so each
+  receiver's atom table takes its own copy — then hand out plain `clone()`s.
+  Static strings already qualify.
+
+Worked examples: `ObjectURLRegistry`, `StandaloneModuleGraph::File`, the
+structured-clone object fast paths.
 
 ## Common Patterns
 

src/bun_alloc/lib.rs

@@ -806,6 +806,8 @@ impl WTFStringImplStruct {
     // These details must stay in sync with WTFStringImpl.h in WebKit!
     // ---------------------------------------------------------------------
     pub(crate) const S_HASH_FLAG_8BIT_BUFFER: u32 = 1 << 2;
+    pub(crate) const S_HASH_FLAG_STRING_KIND_IS_ATOM: u32 = 1 << 4;
+    pub(crate) const S_HASH_FLAG_STRING_KIND_IS_SYMBOL: u32 = 1 << 5;
     /// The bottom bit in the ref count indicates a static (immortal) string.
     pub(crate) const S_REF_COUNT_FLAG_IS_STATIC_STRING: u32 = 0x1;
     /// This allows us to ref / deref without disturbing the static string flag.
@@ -926,8 +928,20 @@ impl WTFStringImplStruct {
         }
     }
     #[inline]
-    pub fn is_thread_safe(&self) -> bool {
-        WTFStringImpl__isThreadSafe(self)
+    pub fn is_atom(&self) -> bool {
+        (self.m_hash_and_flags.get() & Self::S_HASH_FLAG_STRING_KIND_IS_ATOM) != 0
+    }
+    #[inline]
+    pub fn is_symbol(&self) -> bool {
+        (self.m_hash_and_flags.get() & Self::S_HASH_FLAG_STRING_KIND_IS_SYMBOL) != 0
+    }
+    #[inline]
+    pub fn is_thread_isolated(&self) -> bool {
+        WTFStringImpl__isThreadIsolated(self)
+    }
+    #[inline]
+    pub fn is_thread_shareable(&self) -> bool {
+        WTFStringImpl__isThreadShareable(self)
     }
     /// Compute the hash() if necessary
     #[inline]
@@ -946,7 +960,8 @@ unsafe extern "C" {
     // `destroy` path crosses FFI. `*const` + `unsafe`: it frees the
     // allocation backing the pointer.
     pub fn Bun__WTFStringImpl__destroy(this: *const WTFStringImplStruct);
-    safe fn WTFStringImpl__isThreadSafe(this: &WTFStringImplStruct) -> bool;
+    safe fn WTFStringImpl__isThreadIsolated(this: &WTFStringImplStruct) -> bool;
+    safe fn WTFStringImpl__isThreadShareable(this: &WTFStringImplStruct) -> bool;
     safe fn Bun__WTFStringImpl__ensureHash(this: &WTFStringImplStruct);
 }
 

src/bun_core/string/mod.rs

@@ -103,7 +103,8 @@ unsafe extern "C" {
     // `assert_ffi_layout!` above). C++ reads/writes only the `tag`/`value`
     // fields in place; the type encodes the sole pointer-validity precondition,
     // so `safe fn` discharges the link-time proof here.
-    safe fn BunString__toThreadSafe(this: &mut String);
+    safe fn BunString__threadIsolatedCopy(this: &String) -> String;
+    safe fn BunString__makeThreadShareable(this: &mut String);
     fn BunString__createAtom(bytes: *const u8, len: usize) -> String;
     fn BunString__tryCreateAtom(bytes: *const u8, len: usize) -> String;
     fn BunString__createStaticExternal(bytes: *const u8, len: usize, isLatin1: bool) -> String;
@@ -507,28 +508,39 @@ impl String {
             core::ptr::null_mut()
         }
     }
-    pub fn to_thread_safe(&mut self) {
+    /// An isolated copy of a WTF-backed impl (+1, `clone()` for other tags),
+    /// for handing the value to one other thread; not for sharing one impl
+    /// between VMs.
+    pub fn thread_isolated_copy(&self) -> String {
+        let copy = if self.tag == Tag::WTFStringImpl {
+            BunString__threadIsolatedCopy(self)
+        } else {
+            self.clone()
+        };
+        debug_assert!(copy.is_thread_isolated());
+        copy
+    }
+    /// Make a WTF-backed impl safe to hand to any number of threads/VMs:
+    /// isolated if it was an atom/symbol/substring, pre-hashed, and never
+    /// atomized in place; hand it out with `clone()`.
+    pub fn make_thread_shareable(&mut self) {
         if self.tag == Tag::WTFStringImpl {
-            BunString__toThreadSafe(self)
+            BunString__makeThreadShareable(self)
         }
-        debug_assert!(self.is_thread_safe());
+        debug_assert!(self.is_thread_shareable());
     }
-    /// True iff this `String` may be sent to / shared with another thread
-    /// without racing the WTF `StringImpl`'s non-atomic refcount: every tag
-    /// except `WTFStringImpl` is inert (raw slice / static / dead), and a
-    /// WTF-backed string is safe iff its impl reports `isThreadSafe()`.
-    ///
-    /// Call sites that move a `String` across a thread boundary must ensure
-    /// this holds (typically by calling [`to_thread_safe`] first); see the
-    /// `Send`/`Sync` SAFETY comment for the full contract.
+    /// What [`thread_isolated_copy`] yields: no thread-affine state (not an
+    /// atom, symbol or substring), so the value may be handed to one other
+    /// thread. Non-WTF tags are inert and always qualify.
     #[inline]
-    pub(crate) fn is_thread_safe(&self) -> bool {
-        if self.tag == Tag::WTFStringImpl {
-            // SAFETY: WTF tag guarantees `value.wtf` is a valid live impl.
-            self.as_wtf().is_thread_safe()
-        } else {
-            true
-        }
+    pub(crate) fn is_thread_isolated(&self) -> bool {
+        self.tag != Tag::WTFStringImpl || self.as_wtf().is_thread_isolated()
+    }
+    /// What [`make_thread_shareable`] yields: isolated, pre-hashed and never
+    /// atomized in place, so any number of threads may hold it.
+    #[inline]
+    pub(crate) fn is_thread_shareable(&self) -> bool {
+        self.tag != Tag::WTFStringImpl || self.as_wtf().is_thread_shareable()
     }
 
     #[inline]
@@ -908,23 +920,13 @@ impl String {
         Utf8WithString { utf8, string: self }
     }
 
-    /// Like [`into_utf8_with_string`] but guarantees the resulting buffer is
-    /// safe to send to another thread.
+    /// [`into_utf8_with_string`] then [`Utf8WithString::make_thread_isolated`].
     ///
     /// [`into_utf8_with_string`]: Self::into_utf8_with_string
-    pub fn into_utf8_with_string_thread_safe(self) -> Utf8WithString {
+    #[inline]
+    pub fn into_utf8_with_string_thread_isolated(self) -> Utf8WithString {
         let mut out = self.into_utf8_with_string();
-        if out.string.tag == Tag::WTFStringImpl {
-            if out.utf8.is_none() {
-                // 8-bit all-ASCII: a thread-safe `string` keeps backing the bytes.
-                if out.string.is_thread_safe() {
-                    return out;
-                }
-                out.utf8 = Some(out.slice().to_vec());
-            }
-            // Transcoded or copied; drop the WTF backing to release memory.
-            out.string = String::EMPTY;
-        }
+        out.make_thread_isolated();
         out
     }
 
@@ -973,23 +975,21 @@ impl Default for String {
 }
 // SAFETY: `String` is a tag + raw ptr to a `WTF::StringImpl` (or a borrowed
 // `EncodedSlice` slice / static / dead sentinel). All non-WTF tags are trivially
-// `Send + Sync` (no interior mutability, no refcount). The WTF tag is the
-// hazard: `WTF::StringImpl`'s refcount is non-atomic unless the impl was
-// created thread-safe, so sending/sharing a non-thread-safe impl across
-// threads and then `ref_()`/`deref()`ing it is a data race.
+// `Send + Sync` (no interior mutability, no refcount). A `WTF::StringImpl`'s
+// refcount is atomic; the hazards are per-thread atom tables and the lazily
+// computed hash/flags (see "Cross-thread string hazards" in `src/CLAUDE.md`).
 //
 // We keep the blanket impls to match the C++ `BunString`
 // FFI contract (the type must round-trip by value through `extern "C"` and sit
 // in `Send + Sync` containers), and instead enforce the invariant at the
-// boundary: any code that moves a `String` to another thread MUST first call
-// [`String::to_thread_safe`] (or otherwise guarantee [`String::is_thread_safe`]
-// returns `true`). [`String::debug_assert_thread_safe`] is the debug-build
-// checkpoint for that hand-off; `to_thread_safe()` itself asserts its own
-// postcondition. A `ThreadSafeString` newtype split would make this static,
-// but is deferred until the FFI surface can be reshaped.
+// boundary: code that moves a `String` to one other thread calls
+// [`String::thread_isolated_copy`], code that shares one impl between
+// threads/VMs calls [`String::make_thread_shareable`] (asserting
+// [`String::is_thread_isolated`] / [`String::is_thread_shareable`]).
 unsafe impl Send for String {}
-// SAFETY: same contract as the `Send` impl above — sharing requires
-// `is_thread_safe()`; non-WTF tags are inert and trivially `Sync`.
+// SAFETY: same contract as the `Send` impl above — a hand-off requires
+// `is_thread_isolated()`, sharing `is_thread_shareable()`; non-WTF tags are
+// inert and trivially `Sync`.
 unsafe impl Sync for String {}
 
 impl Drop for String {
@@ -1560,9 +1560,41 @@ impl Utf8WithString {
         (self.utf8, self.string)
     }
 
-    /// If `string` is WTF-backed, migrate it to a thread-safe impl.
-    pub fn to_thread_safe(&mut self) {
-        self.string.to_thread_safe();
+    /// For handing the value to one work-pool job that is dropped back on
+    /// the JS thread: keep a WTF-backed `string` only when it backs the bytes
+    /// and is not an atom/symbol, otherwise own the bytes and drop it.
+    pub fn make_thread_isolated(&mut self) {
+        if self.string.tag != Tag::WTFStringImpl {
+            return;
+        }
+        if self.utf8.is_none() {
+            let wtf = self.string.as_wtf();
+            // The job only reads the bytes and the deref happens on the JS thread, so a plain/substring impl may stay.
+            if !wtf.is_atom() && !wtf.is_symbol() {
+                return;
+            }
+            self.utf8 = Some(self.slice().to_vec());
+        }
+        self.string = String::EMPTY;
+    }
+
+    /// For a holder that may be dropped on any thread (a `Blob` store): the
+    /// bytes end up owned by this value alone — the transcoded `utf8` if
+    /// there is one (the WTF backing is dropped), else a
+    /// [`String::thread_isolated_copy`] that is never handed to JS.
+    pub fn thread_isolated_copy(self) -> Self {
+        if self.string.tag != Tag::WTFStringImpl {
+            return self;
+        }
+        let string = if self.utf8.is_some() {
+            String::EMPTY
+        } else {
+            self.string.thread_isolated_copy()
+        };
+        Self {
+            utf8: self.utf8,
+            string,
+        }
     }
 }
 

src/bundler/lib.rs

@@ -364,7 +364,7 @@ bun_dispatch::link_interface! {
         fn is_blob_url(specifier: &[u8]) -> bool;
         fn resolve_blob(specifier: &[u8]) -> Option<options::OpaqueBlob>;
         fn blob_loader(blob: options::OpaqueBlob) -> Option<bun_ast::Loader>;
-        fn blob_file_name(blob: options::OpaqueBlob) -> Option<&'static [u8]>;
+        fn blob_store_path(blob: options::OpaqueBlob) -> Option<&'static [u8]>;
         fn blob_needs_read_file(blob: options::OpaqueBlob) -> bool;
         fn blob_shared_view(blob: options::OpaqueBlob) -> &'static [u8];
         fn blob_deinit(blob: options::OpaqueBlob);

src/bundler/options.rs

@@ -520,7 +520,7 @@ pub fn get_loader_and_virtual_source<'a>(
 
             // "file:" loader makes no sense for blobs
             // so let's default to tsx.
-            if let Some(filename) = jsc_vm.blob_file_name(blob) {
+            if let Some(filename) = jsc_vm.blob_store_path(blob) {
                 let current_path = Fs::Path::init(filename);
 
                 // Only treat it as a file if is a Bun.file()

src/bunfig/arguments.rs

@@ -141,9 +141,8 @@ pub fn load_config(
     // If running as a standalone executable with autoloadBunfig disabled, skip config loading
     // unless an explicit config path was provided via --config
     if user_config_path_.is_none() {
-        if let Some(graph) = StandaloneModuleGraph::get() {
-            // SAFETY: `get()` returns a non-null process-global pointer when Some.
-            if unsafe { (*graph).flags }.contains(
+        if let Some(graph) = StandaloneModuleGraph::get_ref() {
+            if graph.flags.contains(
                 bun_standalone_graph::StandaloneModuleGraph::Flags::DISABLE_AUTOLOAD_BUNFIG,
             ) {
                 return Ok(());