Repository navigation
Conversation
…lone surrogates encode16_impl pre-sized a JSC Uint8Array via simdutf's utf16->utf8 length, which counts an unpaired surrogate as 2 bytes. The U+FFFD replacement is 3 bytes, so the buffer was undersized, the in-place convert failed, and the code fell through to a second allocation via to_utf8_alloc_with_type, orphaning the first Uint8Array until GC. Match the original Zig path: stack-encode inputs up to 512 code units (2048 byte buffer / 4), and for larger inputs go straight to to_utf8_alloc_with_type wrapped as an external Uint8Array. Also restores the 512-unit fast-path threshold that the port had lowered to 64.
|
Updated 2:54 AM PT - Jun 16th, 2026
❌ @robobun, your commit fc46a5c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32383That installs a local version of the PR into your bun-32383 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The only cross-module caller was encode16_impl in TextEncoder.rs, removed in the previous commit. The remaining call site is in the same strings_impl module.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesTextEncoder UTF-16 Encoding Fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/TextEncoder.rs (1)
96-99:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConvert the
Vecinto a boxed slice before handing it toArrayBuffer::from_bytes.
ArrayBuffer::from_bytestakes ownership by reconstructing aBox<[u8]>from the slice, butbytes.leak()leaks aVec<u8>whose capacity is not guaranteed to equal its length.to_utf8_alloc_with_type()returns aVec, so this can deallocate with the wrong layout when JSC later frees theUint8Arraybacking store.Proposed fix
- let bytes = strings::to_utf8_alloc_with_type(slice); - ArrayBuffer::from_bytes(bytes.leak(), JSType::Uint8Array) + let bytes = strings::to_utf8_alloc_with_type(slice).into_boxed_slice(); + ArrayBuffer::from_bytes(Box::leak(bytes), JSType::Uint8Array) .to_js_unchecked(global_this) .unwrap_or(JSValue::ZERO)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/TextEncoder.rs` around lines 96 - 99, The current code calls bytes.leak() on a Vec<u8> returned from to_utf8_alloc_with_type(), but this creates a memory safety issue because leak() creates a slice whose capacity may not equal its length. When ArrayBuffer::from_bytes reconstructs a Box<[u8]> and JSC later deallocates it, it uses the wrong layout, causing corruption. Fix this by converting the Vec to a Box<[u8]> before passing it to ArrayBuffer::from_bytes. Replace bytes.leak() with bytes.into_boxed_slice() in the ArrayBuffer::from_bytes call to ensure the capacity equals the length and JSC deallocates with the correct layout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/encoding/text-encoder.test.js`:
- Around line 715-729: Reorder the assertions in this test to follow the
guideline of asserting the subprocess exit code last. Currently, the expect
statement checking stderr and exitCode appears at line 715 before the output
validation. Move the exitCode assertion to the end of the test, after all the
allocation delta and byte-length checks (results.large.delta, results.mid.delta,
etc.). You may keep the stderr assertion early as an output diagnostic, but
defer the exitCode check until after all the parsed results have been validated.
---
Outside diff comments:
In `@src/runtime/webcore/TextEncoder.rs`:
- Around line 96-99: The current code calls bytes.leak() on a Vec<u8> returned
from to_utf8_alloc_with_type(), but this creates a memory safety issue because
leak() creates a slice whose capacity may not equal its length. When
ArrayBuffer::from_bytes reconstructs a Box<[u8]> and JSC later deallocates it,
it uses the wrong layout, causing corruption. Fix this by converting the Vec to
a Box<[u8]> before passing it to ArrayBuffer::from_bytes. Replace bytes.leak()
with bytes.into_boxed_slice() in the ArrayBuffer::from_bytes call to ensure the
capacity equals the length and JSC deallocates with the correct layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d26289c-af80-478f-9202-212532945092
📒 Files selected for processing (3)
src/bun_core/lib.rssrc/runtime/webcore/TextEncoder.rstest/js/web/encoding/text-encoder.test.js
|
Re: the CodeRabbit "critical" finding on
The The exitCode-ordering nit is applied in fc46a5c. |
There was a problem hiding this comment.
LGTM — restores the Zig-parity single-allocation path for large UTF-16 encode; both prior nits addressed.
Extended reasoning...
Overview
This PR fixes a performance regression in TextEncoder.encode() for large UTF-16 strings containing lone surrogates. The Rust port had introduced an optimistic pre-sized JSC Uint8Array path in encode16_impl that used simdutf's UTF-16→UTF-8 length estimate; that estimate undercounts unpaired surrogates (2 bytes vs the 3-byte U+FFFD actually emitted), so the buffer came up short and the code fell through to a second allocation. The fix removes that path entirely and goes straight to the pre-existing to_utf8_alloc_with_type(...).leak() → ArrayBuffer::from_bytes fallback (which was already there as the last-resort branch), and restores the stack-buffer threshold from 64 to 512 code units to match the original Zig implementation. Net: −13 lines in TextEncoder.rs, a one-token visibility narrowing in lib.rs, and a new heap-stats regression test.
Security risks
None identified. No new unsafe blocks; the Vec::leak() → ArrayBuffer::from_bytes → MarkedArrayBuffer_deallocator (→ mi_free) ownership-transfer pattern was already present on this exact line before the PR (it was the fallback) and is used identically in streams.rs, ReadableStream.rs, and Blob.rs. The author's reply on the thread correctly explains why mi_free handles the capacity-vs-length case without needing the original layout. Output bytes were already correct before this change — this is purely an allocation-count fix.
Level of scrutiny
Moderate — TextEncoder is a hot web-standard path, but this is a simplification that reverts to the known-good Zig strategy rather than introducing new logic. The removed code was the buggy optimization; what remains was already exercised as the fallback. The 2 KB stack buffer matches the prior Zig constant. Both review nits (visibility of copy_utf16_into_utf8_with_utf8_len, and exitCode assertion ordering in the test) have been addressed in follow-up commits.
Other factors
- The new subprocess test asserts both correctness (output byte length) and the allocation-count invariant (≥20, <30
Uint8Arrays for 20 calls — the regressed behavior produced 40), covering both the >512-unit and the 200–512-unit threshold band. - The musl CI failures are LTO bitcode data-layout linker errors ("regular-lto-flag-stub.bc is ''") — a toolchain/build-infra issue unrelated to this Rust change.
- The full
text-encoder.test.jssuite (43 tests) passes per the PR description.
|
CI on build 62768: the only failing lane is
Both are pre-existing on other branches:
|
|
Heads up: #32784 fixes That is the same under-count this PR works around at Concretely:
Flagging so a maintainer can decide which shape to land rather than both. |
Six `node:buffer` behaviors that disagree with Node, found by comparing Bun and Node output over the same inputs. All are exercised by new or strengthened tests in `test/js/node/buffer.test.js`; every expected value was verified against `node v26.3.0`. ### `Buffer.byteLength` under-counts unpaired surrogates ```js const s = "\ud800a".repeat(17); Buffer.byteLength(s, "utf8"); // bun: 51 node: 68 Buffer.from(s, "utf8").length; // bun: 68 node: 68 ``` `Bun__encoding__byteLengthUTF16AsUTF8` (which carried a `// TODO(@190n) handle unpaired surrogates`) went through `element_length_utf16_into_utf8`, which called simdutf's non-validating `utf8_length_from_utf16le`. That function charges every code unit in `0xD800..0xDFFF` two bytes, half of an assumed 4-byte pair. The encoder replaces each unpaired surrogate with U+FFFD (three bytes), so `byteLength` disagreed with `Buffer.from(s).length`. That is an under-allocation footgun for the common `Buffer.allocUnsafe(Buffer.byteLength(s))` + `buf.write(s)` pattern (the write is bounds-checked, so the result is truncation, not a memory error). simdutf already ships the correct function, `utf8_length_from_utf16le_with_replacement` (SIMD-accelerated; `.count` is documented to be valid even when the error field is `SURROGATE`). It just was not exposed through the FFI shim. This adds the binding and points `element_length_utf16_into_utf8` at it. The new value is always greater than or equal to the old one and exactly matches the encoder's output, so every existing caller that used it as an allocation size or an expected-written length is either unchanged (valid UTF-16) or corrected (lone surrogates). As a side effect, `TextEncoder.encode` no longer falls back to a second allocation for lone-surrogate input. ### `lastIndexOf` with `utf16le` does a raw byte search ```js // "a" in utf16le is the byte pair [0x61, 0x00]; it occurs here only at byte offset 1 Buffer.from([0x00, 0x61, 0x00, 0x00]).lastIndexOf("a", undefined, "utf16le"); // bun: 1 node: -1 ``` Node searches in `uint16_t` units, so a UCS2 match can only start on an even byte offset. Bun's forward `indexOf`/`includes` already do this via `indexOf16`, but in `indexOfString`/`indexOfBuffer` the `last` branch was taken before the `ucs2` branch, so backward searches always fell through to the byte-level `std::find_end`. This adds a `lastIndexOf16` mirroring `indexOf16` and moves the `ucs2` branch outward. ### Empty haystack short-circuits before the empty-needle logic ```js Buffer.alloc(0).indexOf(Buffer.alloc(0)); // bun: -1 node: 0 ``` `computeIndexOfRange` already implements Node's semantics here (an empty needle is found at the clamped byteOffset; a non-empty needle in an empty haystack is -1), but `if (byteLength == 0) return -1;` in the dispatcher and three per-type re-checks ran first. They are removed, so the shared helper is the single source of truth. The byteOffset coercion (which can run user `valueOf`) is also no longer skipped for an empty buffer, matching Node. ### `fill()` silently truncates a fractional offset ```js Buffer.alloc(4, "x").fill("a", 1.5); // bun: <Buffer 78 61 61 61> // node: throws ERR_OUT_OF_RANGE 'The value of "offset" is out of range. It must be an integer. Received 1.5' ``` `offset` and `end` went through `validateNumber` (which has no integer check) and were then truncated with `toLength`. Node routes both through `validateOffset`, which is `validateInteger`. `Bun::V::validateInteger` already exists and is used the same way by `compare()` in the same file. ### `swap16`/`swap32`/`swap64` throw the wrong `code` ```js try { Buffer.alloc(3).swap16(); } catch (e) { e.code } // bun: "ERR_OUT_OF_RANGE" node: "ERR_INVALID_BUFFER_SIZE" ``` The message was already correct, but `throwNodeRangeError` hardcodes `ERR_OUT_OF_RANGE`. Node uses `ERR_INVALID_BUFFER_SIZE` (a `RangeError`), which did not exist in `ErrorCode.ts`; it is added there. ### `writeBig[U]Int64LE/BE` with a negative offset throws the wrong code ```js Buffer.alloc(16).writeBigInt64LE(1n, -1); // bun: ERR_BUFFER_OUT_OF_BOUNDS: "offset" is outside of buffer bounds // node: ERR_OUT_OF_RANGE: The value of "offset" is out of range. It must be >= 0 and <= 8. Received -1 ``` Node's `boundsError` puts a negative offset in the same `>= 0 and <= max` bucket as a too-large one; `ERR_BUFFER_OUT_OF_BOUNDS` is reserved for a buffer that is too short to hold 8 bytes at all. `validateOffsetBigInt64`'s two negative branches now issue the same `OUT_OF_RANGE` the too-large path already used, and the integer-ness check moves ahead of the sign check so that `-1.5` reports `an integer`, also matching Node. During review this grew into a full reordering of `validateOffsetBigInt64` to match Node's `validateNumber` then `boundsError` sequence exactly: the offset's type is checked first, then its integer-ness (using `Math.floor(value) !== value` semantics, so `NaN` and fractions report `an integer` while `+-Infinity` do not), then the too-short buffer, then the range. This makes `Buffer.alloc(7).writeBigInt64LE(1n, "x")` a `TypeError` `ERR_INVALID_ARG_TYPE` and `Buffer.alloc(7).writeBigInt64LE(1n, 1.5)` an `ERR_OUT_OF_RANGE`, as in Node, instead of `ERR_BUFFER_OUT_OF_BOUNDS` for both. Every combination is in the test block and verified against Node v26.3.0. ### Verification On the released Bun the 8 new or strengthened test blocks all fail (16 failures across `buffer.test.js`'s two parameterized suites; the other 501 tests are unchanged). With this change: - `bun bd test test/js/node/buffer.test.js`: 516 pass, 0 fail - Node's upstream `test-buffer-{indexof,fill,swap,bytelength,bigint64}.js` all still pass - `bun bd test test/js/web/encoding/`: 2433 pass, 0 fail (`element_length_utf16_into_utf8` also feeds `TextEncoder`) - `bun bd test test/js/node/buffer-utf16.test.ts test/js/node/buffer-indexOf-detach.test.ts test/js/node/buffer-copy-fill-detach.test.ts test/js/node/buffer-compare-bounds.test.ts test/js/node/buffer-concat.test.ts`: 64 pass, 0 fail ### Related #32383 works around the same `element_length_utf16_into_utf8` under-count at `TextEncoder`'s call site (it caused a double `Uint8Array` allocation per `encode()` for large lone-surrogate input). The root-cause fix here resolves that too: verified with 20 `encode()` calls on a 100,000-unit string containing a lone surrogate going from 40 live `Uint8Array`s to 20, with no change to `TextEncoder.rs`.
Reproduction
Cause
encode16_implinsrc/runtime/webcore/TextEncoder.rspre-sized a JSCUint8Arrayviasimdutf::length::utf8::from::utf16::le(). That function counts an unpaired surrogate as 2 output bytes (surrogate-pair cost / 2), but theU+FFFDreplacement we actually emit is 3 bytes. When the input contains a lone surrogate the buffer is undersized,simdutf__convert_utf16le_to_utf8_with_errorsfails, the scalar fallback runs out of space before consuming the whole input, theresult.read != slice.len()check fails, and the code falls through toto_utf8_alloc_with_type(slice).leak()wrapped in a second externalUint8Array. The first JSCUint8Array(potentially MB-sized) is orphaned until GC.The Rust port had also lowered the stack-buffer fast-path threshold from 512 code units (Zig:
2048 / 4) to 64 (192 / 3), so strings in the 65 to 512 range that previously hit the stack path now hit this slow path too.Output bytes were always correct; the regression is purely the doubled
Uint8Arrayallocation count and the associated GC pressure.Fix
Match the Zig
TextEncoder__encode16: encode inputs up to 512 code units into a 2048-byte stack buffer and copy into a single JSCUint8Array; for anything larger, allocate once viato_utf8_alloc_with_typeand wrap the result as an externalUint8Array. No pre-sized JSC buffer, no fallback allocation.Verification
New test in
test/js/web/encoding/text-encoder.test.jsspawns a subprocess that encodes a 200,001-unit string and a 401-unit string (both containing\uD800) 20 times each, and assertsheapStats().objectTypeCounts.Uint8Arraygrows by at least 20 and less than 30 (never 40). Also asserts the output byte lengths are correct (units + 3 + units).