Repository navigation
Conversation
Node assigns `this.encoding = normalizeEncoding(encoding)` in the
StringDecoder constructor, so the instance has an own, writable,
enumerable, configurable data property. Bun exposed `encoding` only via
a prototype accessor, so `Object.hasOwn(d, 'encoding')`,
`Object.getOwnPropertyNames(d)`, `{ ...d }` and `JSON.stringify(d)`
all silently lost the field.
Set `encoding` via putDirect in finishCreation (and in the
util.inherits call-as-function path, which previously used DontEnum)
and drop the prototype accessor.
|
Status: the diff is ready for review. The branch is current with main (367d939). The last push (236a5de) removes one code comment in Reproduce: bun -e 'const {StringDecoder}=require("node:string_decoder");const d=new StringDecoder("utf16le");console.log(Object.hasOwn(d,"encoding"),JSON.stringify(d))'
# before: false {}
# after: true {"encoding":"utf16le"}
# node: true {"encoding":"utf16le"}Verified on 236a5de:
CI (build 117649): 179 jobs passed and 2 failed. No failure comes from this diff.
#42575 rewrites the same |
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 2 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 2 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 39 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. Walkthrough
ChangesStringDecoder encoding property
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The compatibility change is covered across the affected construction and serialization behaviors, with no actionable current-head risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:51 AM PT - Sep 18th, 2026
❌ @robobun, your commit 236a5de has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34609That installs a local version of the PR into your bun-34609 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix.
What was reviewed:
m_encodingis set in the constructor beforefinishCreationruns, so theputDirectreads a valid value.convertEnumerationToJS<BufferEncodingType>isjsStringWithCacheon a static short string — no exception-check needed infinishCreation.- Internal readers of
decoder.encoding(streams/readable.ts:526,node/crypto.ts:390) see the same value as before; decoding itself uses the C++m_encodingfield, unaffected. - Test covers descriptor shape, spread/JSON.stringify/hasOwn, prototype absence, and the
util.inheritscall-as-function path.
Extended reasoning...
Overview
Moves StringDecoder#encoding from a read-only CustomAccessor on the prototype to an enumerable own data property set via putDirect in JSStringDecoder::finishCreation, and drops DontEnum from the call-as-function path. The prototype accessor and its getter are deleted. One new test in string-decoder.test.js asserts the full property descriptor, getOwnPropertyNames, spread, JSON.stringify, and prototype absence for both construction paths.
Security risks
None. This only changes the property-descriptor shape of a string field derived from a validated enum; no untrusted input, no buffer arithmetic, no allocation sizing.
Level of scrutiny
Low-to-moderate. It's a JSC bindings change, so I checked initialization order (m_encoding is assigned in the ctor before finishCreation), whether convertEnumerationToJS can throw (it's jsStringWithCache on a NeverDestroyed<String>, so no), and whether removing the prototype accessor affects internal consumers. src/js/internal/streams/readable.ts and src/js/node/crypto.ts both read decoder.encoding, and the own data property returns the identical string. The decode path (write/end/text) reads m_encoding directly, not the JS property, so user overwrites of the now-writable field don't affect decoding — matching Node, where encoding is stored in the native binding separately from this.encoding.
Other factors
The new test is precise (exact descriptor, both construction paths) and the PR reports all 96 existing string-decoder tests plus Node's parallel test-string-decoder*.js still pass. Dead code (jsStringDecoder_encoding getter and its declaration) is removed in the same commit. No outstanding review comments.
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix.
What was reviewed:
finishCreationnowputDirectsencodingwith default attributes;convertEnumerationToJSis a non-throwing static-string lookup andputDirecton a fresh object can't throw, so no exception scope is needed there.m_encodingis set in the constructor beforefinishCreationruns, so the value is correct; internal decode still readsm_encoding, matching Node where overwritingthis.encodingdoesn't affect decoding.- Removed prototype accessor and its getter are fully dead;
encodingPublicName()was already in use on the util.inherits path. - Test covers both construction paths and asserts the exact descriptor, spread,
JSON.stringify, own-names, and absence from the prototype.
Extended reasoning...
Overview
Moves StringDecoder#encoding from a non-enumerable CustomAccessor on the prototype to an enumerable own data property on each instance, matching Node's this.encoding = normalizeEncoding(encoding) assignment. Three edits in JSStringDecoder.cpp: add a putDirect in finishCreation, drop DontEnum from the util.inherits path's existing putDirect, and delete the now-unused prototype accessor + its HashTableValue entry. One new test in string-decoder.test.js.
Security risks
None. This only changes property descriptor attributes on a Node compat object; no untrusted input handling, allocation sizing, or lifetime changes.
Level of scrutiny
Low. The change is mechanical: m_encoding is initialized in the JSStringDecoder constructor before finishCreation is called (verified in the header), convertEnumerationToJS<BufferEncodingType> is a static-string table lookup via jsStringWithCache that cannot throw, and putDirect on a freshly-allocated cell with a known identifier does not throw — so the absence of a throw scope in finishCreation is correct. The util.inherits path only flips DontEnum → 0 on a call that was already there. The removed getter has no remaining references. Decoding behavior still keys off the C++ m_encoding field, which mirrors Node (where overwriting the JS encoding property does not change the native decoder state).
Other factors
The added test is strong: it asserts the full own-property descriptor, getOwnPropertyNames, spread, JSON.stringify, and prototype absence for both the direct constructor and the util.inherits call-as-function path, so both modified code sites are exercised. PR description reports all 96 existing string-decoder tests and Node's test-string-decoder* parallel tests still pass. No prior reviewer comments to address.
### Problem
- `class Sub extends StringDecoder {}; new Sub("utf8")` returns the
class `Sub` itself. `s.write` is `undefined`, and `Sub` gains an own
`encoding` property. `Reflect.construct(StringDecoder, ["utf8"],
Object)` returns `Object`. Node returns an instance.
- `JSStringDecoderConstructor` registers one native function for
[[Call]] and [[Construct]] (`src/jsc/bindings/JSStringDecoder.h:118`).
JSC passes new.target in the `this` slot of a native constructor, so the
branch for `StringDecoder.call(receiver, encoding)`
(`JSStringDecoder.cpp:592`) takes `Sub` as the receiver.
- Found by an audit. No issue reports it.
### Fix
- Register separate `call` and `construct` functions that share one
`createDecoder` body. For a [[Construct]] it takes the structure from
new.target with `InternalFunction::createSubclassStructure`, as `JSHash`
and the generated classes do. `call` keeps the receiver branch
unchanged.
- The Node.js compatibility page no longer lists this gap. Not in this
PR: `JSMockFunction.cpp:459` has the same cause, and #34575 and #41398
carry it.
- `new StringDecoder()` costs the same as before (ThinLTO A/B in the
notes).
- Verified: `test/js/node/string_decoder/string-decoder.test.js` (143
pass, released bun fails the 45 new cases), Node's
`test-string-decoder*.js`, `express-body-parser-test.test.ts`.
Self-reviewed: 4 concerns raised, 4 addressed.
### Background
- [[Call]] is `f()`. [[Construct]] is `new f()` or `Reflect.construct`.
A JSC `InternalFunction` takes one native function for each.
- new.target is the constructor that `new` was applied to: `Sub` for
`new Sub()`. The new object takes its prototype from it.
- A JSC `Structure` holds the prototype of an object.
`createSubclassStructure` returns the base structure with the prototype
of new.target.
- The receiver branch serves iconv-lite 0.4 (under express body-parser),
which runs `StringDecoder.call(this, enc)` on a plain object. It stores
the native decoder on that object.
<details><summary>Notes</summary>
**Repro**
```js
const { StringDecoder } = require("string_decoder");
class Sub extends StringDecoder {}
const s = new Sub("utf8");
console.log(s === Sub, typeof s, s instanceof Sub, Reflect.ownKeys(Sub));
// bun 1.4.3: true function false [ "length", "name", "prototype", "encoding" ]
// node v26.3.0: false object true [ 'length', 'name', 'prototype' ]
// this branch: false object true [ "length", "name", "prototype" ]
```
**Sibling sites** (every place that registers one function for both
[[Call]] and [[Construct]])
- `src/jsc/bindings/JSMockFunction.cpp:459`: `jsMockFunctionCall` reads
`thisValue()`, so `new mockFn()` sees new.target as `this`. Same root
cause. Excluded here because #34575 and #41398 already change that
function. This PR does not fix `mock()` or `spyOn()`.
- These never read `this` or new.target, so the confusion cannot happen
there: the seven sink constructors from
`src/codegen/generate-jssink.ts:59` (`js_construct` ignores the frame),
`JSDOMConstructorNotConstructable` (throws on both paths), the
`ObjectTemplate` `DummyCallback` in the V8 shim,
`jsNodeVmWasmDisallowed` (`NodeVM.cpp:1046`, throws on both paths),
`vm.createContext` (`NodeVM.cpp:1714`), and `SlowBuffer`.
- #42528 makes other native constructors honor new.target. It does not
cover `StringDecoder`.
**Cost of `new StringDecoder()`**
A first version had a parse helper and called `JSStringDecoder::create`
from both `call` and `construct`. With two call sites,
`allocateCell<JSStringDecoder>` went out of line and `new
StringDecoder()` got slower. One shared `createDecoder` keeps a single
call site for the allocation, and `construct` compiles to 9 instructions
that end in `jmp createDecoder`.
ThinLTO release build, only this translation unit replaced, one pinned
core, 300 reps of 200000 constructions, p50 in ns over 5 alternating
rounds:
| variant | `new StringDecoder()` | `new StringDecoder("utf8")` |
| --- | --- | --- |
| main | 11.6 to 12.3 | 19.5 to 21.1 |
| first version | 14.9 to 16.1 | 19.8 to 20.9 |
| this PR | 11.4 to 12.6 | 18.0 to 19.3 |
**Behavior details**
- Order matches Node: the `prototype` of new.target is read before the
encoding is parsed (probed with a Proxy new.target and an encoding
object with a `toString`).
- `call` is unchanged, including the `thisValue != constructor` check,
so `StringDecoder.call(StringDecoder, enc)` still returns a fresh
decoder. The two `putDirect` lines in the receiver branch are textually
untouched, so #34609 and other work on that branch should merge without
a conflict.
- A new.target whose `prototype` is not an object gets
`StringDecoder.prototype`, as for every JSC built-in class. Node gives
`Object.prototype` there, because its `StringDecoder` is an ordinary
function.
- Cross-realm: a class defined in a `node:vm` context or a ShadowRealm
can extend `StringDecoder`. The function realm of new.target is not
always a `Zig::GlobalObject`, so the code goes through
`defaultGlobalObject()`. A revoked Proxy as new.target throws a
TypeError.
- Remaining difference, not changed here: Node's `end(buf)` and `text()`
call `this.write()`, so a subclass that overrides `write` sees those
calls. Bun's native `end` and `text` call the native write directly. A
plain instance with a patched `write` shows the same difference today.
- Not changed here: `lastNeed`, `lastTotal` and `lastChar` throw on a
`StringDecoder.call(this)` receiver, because the prototype accessors
have a class check. Instances of a subclass are real `JSStringDecoder`
cells, so the accessors work on them.
**Suites run with the debug (ASAN) build**
- `test/js/node/string_decoder/string-decoder.test.js`: 143 pass. The
whole decode suite now also runs against `class SubStringDecoder extends
StringDecoder`. Released bun 1.4.3: 98 pass, 45 fail.
- The same file and a script of every throw path under
`BUN_JSC_validateExceptionChecks=1`: clean.
- `test/js/node/test/parallel/`: `test-string-decoder.js`,
`test-string-decoder-end.js`, `test-string-decoder-fuzz.js`,
`test-crypto-hash.js`, `test-fs-read-stream-encoding.js`,
`test-readline-keys.js`, `test-stream-decoder-objectmode.js`,
`test-stream-push-strings.js`,
`test-stream-readable-setEncoding-existing-buffers.js`,
`test-stream-readable-setEncoding-null.js`,
`test-stream2-decode-partial.js`, `test-stream2-set-encoding.js`.
- `test/js/third_party/body-parser/express-body-parser-test.test.ts`,
and iconv-lite 0.4.24 (`StringDecoder.call(this, enc)` with
`InternalDecoder.prototype = StringDecoder.prototype`) by hand.
**Self-review**
The review asked for four changes. All are in this PR: the stale docs
line, the sibling-sites list, the slower `new StringDecoder()` of the
first version, and multi-line comments in `src/`.
</details>
<!-- robobun:evidence:begin -->
---
**[human-review]** gate passed · iteration 0 · 4 files touched
<details><summary>fails on main (without fix)</summary>
```console
ASAN without fix: BUILD FAILED (no junit output)
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/string_decoder/string-decoder.test.js
ninja: Entering directory `/workspace/bun/build/debug'
[1/164] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o
[2/164] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited
[3/164] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
- ResolveMessage (15 fields)
- BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
- Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
- ResourceUsage (8 fields)
- Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
- CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
- FileSystemRouter (5 fields)
- FrameworkFileSystemRouter (2 fields)
- MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes
... (truncated)
release without fix: 45 FAILED
bun test v1.4.3-canary.1 (6a92015)
test/js/node/string_decoder/string-decoder.test.js:
(pass) require('string_decoder') [0.10ms]
(pass) Bun.inspect(StringDecoder) [0.09ms]
(pass) FakeStringDecoderCall > StringDecoder-utf8 [1.88ms]
(pass) FakeStringDecoderCall > StringDecoder-ucs-2 [0.94ms]
(pass) FakeStringDecoderCall > StringDecoder-utf16le [0.04ms]
(pass) FakeStringDecoderCall > StringDecoder-utf8-additional [0.11ms]
(pass) FakeStringDecoderCall > StringDecoder-utf16le-additional [0.06ms]
(pass) FakeStringDecoderCall > StringDecoder.end > base64 testbuf [0.18ms]
(pass) FakeStringDecoderCall > StringDecoder.end > base64url testbuf [0.10ms]
(pass) FakeStringDecoderCall > StringDecoder.end > hex testbuf [0.11ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf8 testbuf [0.06ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf16le testbuf [0.04ms]
(pass) FakeStringDecoderCall > StringDecoder.end > ucs2 testbuf [0.05ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf8 partial "�a" [0.03ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf8 partial "��"
(pass) FakeStringDecoderCall > StringDecoder.end > utf8 partial "��"
(pass) FakeS
... (truncated)
```
</details>
<details><summary>passes on PR (with fix)</summary>
```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/string_decoder/string-decoder.test.js
bun test v1.4.3 (6a92015)
test/js/node/string_decoder/string-decoder.test.js:
(pass) require('string_decoder') [6.27ms]
(pass) Bun.inspect(StringDecoder) [5.39ms]
(pass) FakeStringDecoderCall > StringDecoder-utf8 [340.80ms]
(pass) FakeStringDecoderCall > StringDecoder-ucs-2 [239.90ms]
(pass) FakeStringDecoderCall > StringDecoder-utf16le [4.82ms]
(pass) FakeStringDecoderCall > StringDecoder-utf8-additional [9.54ms]
(pass) FakeStringDecoderCall > StringDecoder-utf16le-additional [8.09ms]
(pass) FakeStringDecoderCall > StringDecoder.end > base64 testbuf [25.18ms]
(pass) FakeStringDecoderCall > StringDecoder.end > base64url testbuf [17.47ms]
(pass) FakeStringDecoderCall > StringDecoder.end > hex testbuf [13.70ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf8 testbuf [15.44ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf16le testbuf [11.48ms]
(pass) FakeStringDecoderCall > StringDecoder.end > ucs2 testbuf [12.16ms]
(pass) FakeStringDecoderCall > StringDecoder.end > utf
... (truncated)
release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 706ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/125] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o
[2/125] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited
[3/125] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
- ResolveMessage (15 fields)
- BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
- Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
- ResourceUsage (8 fields)
- Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
- CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
- FileSystemRouter (5 fields)
- FrameworkFileSystemRouter (2 fields)
- MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
- Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api
... (truncated)
```
</details>
<details><summary>diff hotspot</summary>
```
docs/runtime/nodejs-compat.mdx | 2 +-
src/jsc/bindings/JSStringDecoder.cpp | 45 ++++++--
src/jsc/bindings/JSStringDecoder.h | 5 +-
test/js/node/string_decoder/string-decoder.test.js | 114 ++++++++++++++++++++-
4 files changed, 151 insertions(+), 15 deletions(-)
```
</details>
**gate history** · 1 passed · 0 rejected · iteration 0
<details><summary>evidence per changed file</summary>
```
file reads edits tests
docs/runtime/nodejs-compat.mdx 1 1 18
src/jsc/bindings/JSStringDecoder.cpp 5 12 18
src/jsc/bindings/JSStringDecoder.h 1 1 18
test/js/node/string_decoder/string-decoder.test.js 2 5 18
```
</details>
<!-- robobun:evidence:end -->
…2582) ### Problem - `class Sub extends StringDecoder {}; new Sub("utf8")` returns the class `Sub` itself. `s.write` is `undefined`, and `Sub` gains an own `encoding` property. `Reflect.construct(StringDecoder, ["utf8"], Object)` returns `Object`. Node returns an instance. - `JSStringDecoderConstructor` registers one native function for [[Call]] and [[Construct]] (`src/jsc/bindings/JSStringDecoder.h:118`). JSC passes new.target in the `this` slot of a native constructor, so the branch for `StringDecoder.call(receiver, encoding)` (`JSStringDecoder.cpp:592`) takes `Sub` as the receiver. - Found by an audit. No issue reports it. ### Fix - Register separate `call` and `construct` functions that share one `createDecoder` body. For a [[Construct]] it takes the structure from new.target with `InternalFunction::createSubclassStructure`, as `JSHash` and the generated classes do. `call` keeps the receiver branch unchanged. - The Node.js compatibility page no longer lists this gap. Not in this PR: `JSMockFunction.cpp:459` has the same cause, and oven-sh#34575 and oven-sh#41398 carry it. - `new StringDecoder()` costs the same as before (ThinLTO A/B in the notes). - Verified: `test/js/node/string_decoder/string-decoder.test.js` (143 pass, released bun fails the 45 new cases), Node's `test-string-decoder*.js`, `express-body-parser-test.test.ts`. Self-reviewed: 4 concerns raised, 4 addressed. ### Background - [[Call]] is `f()`. [[Construct]] is `new f()` or `Reflect.construct`. A JSC `InternalFunction` takes one native function for each. - new.target is the constructor that `new` was applied to: `Sub` for `new Sub()`. The new object takes its prototype from it. - A JSC `Structure` holds the prototype of an object. `createSubclassStructure` returns the base structure with the prototype of new.target. - The receiver branch serves iconv-lite 0.4 (under express body-parser), which runs `StringDecoder.call(this, enc)` on a plain object. It stores the native decoder on that object. <details><summary>Notes</summary> **Repro** ```js const { StringDecoder } = require("string_decoder"); class Sub extends StringDecoder {} const s = new Sub("utf8"); console.log(s === Sub, typeof s, s instanceof Sub, Reflect.ownKeys(Sub)); // bun 1.4.3: true function false [ "length", "name", "prototype", "encoding" ] // node v26.3.0: false object true [ 'length', 'name', 'prototype' ] // this branch: false object true [ "length", "name", "prototype" ] ``` **Sibling sites** (every place that registers one function for both [[Call]] and [[Construct]]) - `src/jsc/bindings/JSMockFunction.cpp:459`: `jsMockFunctionCall` reads `thisValue()`, so `new mockFn()` sees new.target as `this`. Same root cause. Excluded here because oven-sh#34575 and oven-sh#41398 already change that function. This PR does not fix `mock()` or `spyOn()`. - These never read `this` or new.target, so the confusion cannot happen there: the seven sink constructors from `src/codegen/generate-jssink.ts:59` (`js_construct` ignores the frame), `JSDOMConstructorNotConstructable` (throws on both paths), the `ObjectTemplate` `DummyCallback` in the V8 shim, `jsNodeVmWasmDisallowed` (`NodeVM.cpp:1046`, throws on both paths), `vm.createContext` (`NodeVM.cpp:1714`), and `SlowBuffer`. - oven-sh#42528 makes other native constructors honor new.target. It does not cover `StringDecoder`. **Cost of `new StringDecoder()`** A first version had a parse helper and called `JSStringDecoder::create` from both `call` and `construct`. With two call sites, `allocateCell<JSStringDecoder>` went out of line and `new StringDecoder()` got slower. One shared `createDecoder` keeps a single call site for the allocation, and `construct` compiles to 9 instructions that end in `jmp createDecoder`. ThinLTO release build, only this translation unit replaced, one pinned core, 300 reps of 200000 constructions, p50 in ns over 5 alternating rounds: | variant | `new StringDecoder()` | `new StringDecoder("utf8")` | | --- | --- | --- | | main | 11.6 to 12.3 | 19.5 to 21.1 | | first version | 14.9 to 16.1 | 19.8 to 20.9 | | this PR | 11.4 to 12.6 | 18.0 to 19.3 | **Behavior details** - Order matches Node: the `prototype` of new.target is read before the encoding is parsed (probed with a Proxy new.target and an encoding object with a `toString`). - `call` is unchanged, including the `thisValue != constructor` check, so `StringDecoder.call(StringDecoder, enc)` still returns a fresh decoder. The two `putDirect` lines in the receiver branch are textually untouched, so oven-sh#34609 and other work on that branch should merge without a conflict. - A new.target whose `prototype` is not an object gets `StringDecoder.prototype`, as for every JSC built-in class. Node gives `Object.prototype` there, because its `StringDecoder` is an ordinary function. - Cross-realm: a class defined in a `node:vm` context or a ShadowRealm can extend `StringDecoder`. The function realm of new.target is not always a `Zig::GlobalObject`, so the code goes through `defaultGlobalObject()`. A revoked Proxy as new.target throws a TypeError. - Remaining difference, not changed here: Node's `end(buf)` and `text()` call `this.write()`, so a subclass that overrides `write` sees those calls. Bun's native `end` and `text` call the native write directly. A plain instance with a patched `write` shows the same difference today. - Not changed here: `lastNeed`, `lastTotal` and `lastChar` throw on a `StringDecoder.call(this)` receiver, because the prototype accessors have a class check. Instances of a subclass are real `JSStringDecoder` cells, so the accessors work on them. **Suites run with the debug (ASAN) build** - `test/js/node/string_decoder/string-decoder.test.js`: 143 pass. The whole decode suite now also runs against `class SubStringDecoder extends StringDecoder`. Released bun 1.4.3: 98 pass, 45 fail. - The same file and a script of every throw path under `BUN_JSC_validateExceptionChecks=1`: clean. - `test/js/node/test/parallel/`: `test-string-decoder.js`, `test-string-decoder-end.js`, `test-string-decoder-fuzz.js`, `test-crypto-hash.js`, `test-fs-read-stream-encoding.js`, `test-readline-keys.js`, `test-stream-decoder-objectmode.js`, `test-stream-push-strings.js`, `test-stream-readable-setEncoding-existing-buffers.js`, `test-stream-readable-setEncoding-null.js`, `test-stream2-decode-partial.js`, `test-stream2-set-encoding.js`. - `test/js/third_party/body-parser/express-body-parser-test.test.ts`, and iconv-lite 0.4.24 (`StringDecoder.call(this, enc)` with `InternalDecoder.prototype = StringDecoder.prototype`) by hand. **Self-review** The review asked for four changes. All are in this PR: the stale docs line, the sibling-sites list, the slower `new StringDecoder()` of the first version, and multi-line comments in `src/`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/string_decoder/string-decoder.test.js ninja: Entering directory `/workspace/bun/build/debug' [1/164] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o [2/164] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/164] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystemRouter (5 fields) - FrameworkFileSystemRouter (2 fields) - MatchedRoute (8 fields) Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes ... (truncated) release without fix: 45 FAILED bun test v1.4.3-canary.1 (6a92015) test/js/node/string_decoder/string-decoder.test.js: (pass) require('string_decoder') [0.10ms] (pass) Bun.inspect(StringDecoder) [0.09ms] (pass) FakeStringDecoderCall > StringDecoder-utf8 [1.88ms] (pass) FakeStringDecoderCall > StringDecoder-ucs-2 [0.94ms] (pass) FakeStringDecoderCall > StringDecoder-utf16le [0.04ms] (pass) FakeStringDecoderCall > StringDecoder-utf8-additional [0.11ms] (pass) FakeStringDecoderCall > StringDecoder-utf16le-additional [0.06ms] (pass) FakeStringDecoderCall > StringDecoder.end > base64 testbuf [0.18ms] (pass) FakeStringDecoderCall > StringDecoder.end > base64url testbuf [0.10ms] (pass) FakeStringDecoderCall > StringDecoder.end > hex testbuf [0.11ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf8 testbuf [0.06ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf16le testbuf [0.04ms] (pass) FakeStringDecoderCall > StringDecoder.end > ucs2 testbuf [0.05ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf8 partial "�a" [0.03ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf8 partial "��" (pass) FakeStringDecoderCall > StringDecoder.end > utf8 partial "��" (pass) FakeS ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/string_decoder/string-decoder.test.js bun test v1.4.3 (6a92015) test/js/node/string_decoder/string-decoder.test.js: (pass) require('string_decoder') [6.27ms] (pass) Bun.inspect(StringDecoder) [5.39ms] (pass) FakeStringDecoderCall > StringDecoder-utf8 [340.80ms] (pass) FakeStringDecoderCall > StringDecoder-ucs-2 [239.90ms] (pass) FakeStringDecoderCall > StringDecoder-utf16le [4.82ms] (pass) FakeStringDecoderCall > StringDecoder-utf8-additional [9.54ms] (pass) FakeStringDecoderCall > StringDecoder-utf16le-additional [8.09ms] (pass) FakeStringDecoderCall > StringDecoder.end > base64 testbuf [25.18ms] (pass) FakeStringDecoderCall > StringDecoder.end > base64url testbuf [17.47ms] (pass) FakeStringDecoderCall > StringDecoder.end > hex testbuf [13.70ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf8 testbuf [15.44ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf16le testbuf [11.48ms] (pass) FakeStringDecoderCall > StringDecoder.end > ucs2 testbuf [12.16ms] (pass) FakeStringDecoderCall > StringDecoder.end > utf ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 706ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/125] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o [2/125] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [3/125] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystemRouter (5 fields) - FrameworkFileSystemRouter (2 fields) - MatchedRoute (8 fields) Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts - Glob (5 fields) Found 1 classes from /workspace/bun/src/runtime/api ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` docs/runtime/nodejs-compat.mdx | 2 +- src/jsc/bindings/JSStringDecoder.cpp | 45 ++++++-- src/jsc/bindings/JSStringDecoder.h | 5 +- test/js/node/string_decoder/string-decoder.test.js | 114 ++++++++++++++++++++- 4 files changed, 151 insertions(+), 15 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests docs/runtime/nodejs-compat.mdx 1 1 18 src/jsc/bindings/JSStringDecoder.cpp 5 12 18 src/jsc/bindings/JSStringDecoder.h 1 1 18 test/js/node/string_decoder/string-decoder.test.js 2 5 18 ``` </details> <!-- robobun:evidence:end -->
…string-decoder-encoding-own-property
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, self-contained node:string_decoder compat fix that matches Node's this.encoding = normalizeEncoding(encoding) own-property shape.
What was reviewed:
- The new
putDirectinJSStringDecoder::finishCreation:convertEnumerationToJS<BufferEncodingType>returns a VM common string without entering JS, so no exception scope is needed andm_encodingis already set by the constructor initializer. - Removal of the prototype
encodingaccessor: Node's prototype has noencoding, and the two in-tree readers (internal/streams/readable.tssetEncoding,node/crypto.tsdecipher output) read it off instances, so they still see the own property. - The
util.inheritscall path now uses enumerable attributes while the privatedecodeslot stays DontEnum; the test covers real instance, foreign-this, and subclass, and asserts reassignment does not alterm_encoding.
Extended reasoning...
Overview
The PR touches one C++ binding (/home/claude/bun/src/jsc/bindings/JSStringDecoder.cpp) and one test file. It removes the prototype-level encoding custom getter and its HashTableValue entry, adds a putDirect of encoding (attributes 0 = writable/enumerable/configurable) in JSStringDecoder::finishCreation, and flips the util.inherits foreign-this path from DontEnum to 0 for encoding. Net change to native code is about a dozen lines. The test adds a single it block asserting the full own-property descriptor, getOwnPropertyNames, spread, JSON.stringify, absence on the prototype, and the same shape on the util.inherits and subclass paths, plus that reassigning encoding does not change decoding.
Security risks
None identified. The change only affects the JS-visible property shape of StringDecoder instances. convertEnumerationToJS<BufferEncodingType> (in JSBufferEncodingType.cpp) switches over the enum and returns cached common strings; it cannot throw or run user code, so the putDirect in finishCreation on a freshly allocated object is safe without a throw scope. No user-controlled data flows into the new code beyond the already-validated m_encoding enum.
Level of scrutiny
Low-to-moderate. The pattern (own data property set in finishCreation) is standard JSC binding practice, and the only behavioral risk was a consumer relying on the prototype accessor or on the previous non-enumerable attribute. I checked the in-tree consumers: src/js/internal/streams/readable.ts:526 and src/js/node/crypto.ts:448 both read decoder.encoding from an instance, which continues to work. The decode private-name slot on the util.inherits path remains DontEnum and is symbol-keyed regardless, so it does not leak into getOwnPropertyNames/spread. The four candidate issues ruled out by the hunt (pre-existing prototype enumerability of lastChar/lastNeed/lastTotal, one extra structure transition per instance, accessor-override on a util.inherits prototype, and undefined instead of TypeError on a non-instance receiver) are all either pre-existing or consistent with Node's own behavior, where encoding is likewise a plain own data property.
Other factors
No CODEOWNERS entry covers the changed files (only *.d.ts is owned). The earlier automated review on this PR was followed by a merge from main and a test-only commit extending coverage to a subclass instance and an assignment to encoding, which is consistent with feedback having been addressed. I was unable to execute the test in this environment (command execution for bun test was denied), so I reasoned about the fail/pass claim from the code: on main, Object.hasOwn(d, "encoding") is false because only the prototype accessor exists, so the new test necessarily fails there; with the change, getOwnPropertyNames yields exactly ["encoding"] since no other string-keyed own properties are installed on the instance or the empty subclass. The existing 96 string-decoder tests exercise the decoding paths, which this PR does not touch.
The test in string-decoder.test.js states why `encoding` is an own property.
Problem
StringDecoderinstance has no ownencodingproperty.Object.hasOwn(d, "encoding")isfalse.{ ...d }andJSON.stringify(d)give{}. Node gives{"encoding":"utf16le"}. A read ofd.encodingworks on both.encodingwas a read-onlyCustomAccessorin the prototype table ofsrc/jsc/bindings/JSStringDecoder.cpp. Node's constructor runsthis.encoding = normalizeEncoding(encoding), which makes an enumerable own data property.StringDecoder.call(obj, enc)did set an ownencodingonobj, but withDontEnum, so spread dropped it there too.Fix
JSStringDecoder::finishCreationputsencodingon the instance with attributes0(writable, enumerable, configurable). Node's descriptor is the same.encodingon the prototype. The call path dropsDontEnum.lastChar,lastNeedandlastTotaldo not change.m_encoding, never the property. An assignment tod.encodingchanges only the property, as in Node.test/js/node/string_decoder/string-decoder.test.js(144 pass with the fix, the new test fails without it). Node's threetest-string-decoder*.jsfiles pass.Background
JSStringDecoderis the native class behindnode:string_decoder. It keeps the encoding in the C++ fieldm_encoding.finishCreationis the JavaScriptCore hook that runs once, right after the object is allocated.CustomAccessorin a prototype table is a native getter on the prototype. Spread,JSON.stringifyandObject.hasOwnread own properties only, so they do not see it.util.inheritscode.StringDecoder.call(obj, enc)stores a hidden native decoder onobjand setsobj.encoding.Notes
Repro:
The test asserts the full own-property descriptor,
getOwnPropertyNames, spread,JSON.stringify, and the absence ofencodingon the prototype. It covers four cases:new StringDecoder, theutil.inheritscall path, a subclass instance, and an assignment tod.encodingfollowed by awrite().Compared with Node v26.3.0 on the merge with main (367d939): the descriptor and
JSON.stringifyoutput are equal for every encoding name, for a subclass, forStringDecoder.call(obj, enc), afterd.encoding = "hex", and afterdelete d.encoding.The internal readers of
decoder.encodingaresrc/js/internal/streams/readable.ts(setEncoding) andsrc/js/node/crypto.ts(getDecoder). Both read the same value as before.test-stream2-set-encoding.js,test-stream-readable-setEncoding-null.js,test-stream-readable-setEncoding-existing-buffers.js,test-crypto-cipheriv-decipheriv.jsandtest-crypto-encoding-validation-error.jspass.Overlap with #42575: that PR rewrites the same
putDirectline inJSStringDecoderConstructor::callto usedefineOwnProperty, and it keepsDontEnumonencoding. The second of the two PRs to merge needs a rebase, andencodingmust stay enumerable there. The test in this PR fails if it does not.Not in this PR:
StringDecoder.call(Object.freeze({}), "utf8")still adds the property to the frozen object (Node throws). #42575 covers that.