Conversation
SerializeJSONProperty looks up toJSON with GetV, which finds own properties
regardless of enumerability. FastStringifier's object case only checks
Object.prototype for toJSON and then skips DontEnum properties, so an own
non-enumerable toJSON was never called:
const o = { uri: "u", text: "t" };
Object.defineProperty(o, "toJSON", { value: () => ({ uri: o.uri }), enumerable: false });
JSON.stringify(o); // {"uri":"u","text":"t"}, expected {"uri":"u"}
An own enumerable toJSON happens to be handled already, because the fast path
bails out when it reaches the callable property value. The array case checks
the structure for toJSON; do the same for objects, in the property loop that
is already walking the structure.
|
The red Preview Build check is not this diff. It fails in Every Preview Build run since 2026-07-02 17:17Z fails the same way (the last green one was 01:39Z), so the workflows need their Happy to send that pinning as a separate PR if it's useful. |
WalkthroughUpdates JSON.stringify fast-path handling for toJSON on objects and arrays, and adds stress tests for non-enumerable and prototype-based toJSON cases across nested values, spacing, and replacer combinations. ChangesJSON.stringify toJSON handling
Sequence Diagram(s)Not applicable. Related issues: None specified 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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
🤖 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 `@Source/JavaScriptCore/runtime/JSONObject.cpp`:
- Around line 1426-1434: The own-toJSON key check is duplicated in both the
object and array serialization paths, so consolidate it into a small shared
helper in JSONObject.cpp. Create a helper such as one that inspects an entry and
records the provided failure message when the key matches
m_vm.propertyNames->toJSON, then use it at both the object-side
structure.forEachProperty path and the array-side equivalent so the matching
logic stays consistent and easier to update later.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28829ea2-6769-4213-a4fa-718e2b03db36
📒 Files selected for processing (2)
JSTests/stress/json-stringify-non-enumerable-to-json.jsSource/JavaScriptCore/runtime/JSONObject.cpp
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…type
FastStringifier's array case checks the global Array.prototype for toJSON and
scans the array's own properties, but never looks at the array's actual
prototype, so a swapped prototype carrying toJSON was ignored:
const proto = { __proto__: Array.prototype, toJSON() { return "x"; } };
const array = [1, 2, 3];
Object.setPrototypeOf(array, proto);
JSON.stringify(array); // [1,2,3], expected "x"
The object case already bails on a non-standard prototype. Do the same for
arrays, in the branch that already handles non-original array structures, so
plain arrays are unaffected.
robobun
changed the title
|
Two updates. c6a845a fixes a second hole in the same function, raised in review above: the The red Preview Build is the Actions pinning policy, not this diff. #269 pins every action in Both commits apply to WebKit/WebKit |
There was a problem hiding this comment.
Thanks for folding in the array-prototype guard — the fix and its test look right to me. Both changes are conservative bailouts that only widen when FastStringifier defers to the general path, but this is hot JSC runtime code with subtle structure/spec reasoning, so I'd like a JSC maintainer to sign off.
Extended reasoning...
Overview
Two targeted spec-conformance fixes in FastStringifier::append (Source/JavaScriptCore/runtime/JSONObject.cpp), plus two new stress tests. The object-path fix adds a toJSON key check inside the existing structure.forEachProperty loop, before the DontEnum skip, so an own non-enumerable toJSON triggers recordFailure and falls back to the general Stringifier. The array-path fix (added in c6a845a in response to my earlier comment) adds hasPolyProto() and storedPrototype() != arrayPrototype() guards inside the already-[[unlikely]] !isOriginalArrayStructure branch, mirroring the object case's prototype guard. Both changes only add bailouts — they never alter what the fast path emits, only when it gives up.
Security risks
None. This is a correctness fix for JSON.stringify output on edge-case inputs (own non-enumerable toJSON, arrays with swapped prototypes). No new attack surface, no memory-safety implications; recordFailure is the established, side-effect-free bailout mechanism used throughout this function.
Level of scrutiny
High. JSON.stringify is one of the most heavily exercised built-ins, and FastStringifier is on the hot path for essentially every serialization. The reasoning is subtle: it depends on UniquedStringImpl* pointer equality against propertyNames->toJSON, on forEachProperty iteration order relative to the DontEnum skip, on storedPrototype()'s mono-proto precondition, and on isOriginalArrayStructure implying Array.prototype. The author's analysis is thorough and the patterns exactly mirror existing code in the same function, but a JSC maintainer is better positioned than I am to confirm there are no structure-transition or cross-realm corner cases I'm missing.
Other factors
The PR is well-supported: two new stress tests covering nested/array-wrapped/gap/DynamicBuffer/non-callable/null-prototype/shadowing shapes, differential checks against the general stringifier via an identity replacer, verified against V8, all 55 existing *json*/*stringif* stress tests passing, and Release benchmarks showing no measurable regression. All prior review threads (CodeRabbit's dedup nit, my array-prototype note) are resolved. The engine diff is ~16 lines and applies cleanly to upstream WebKit. I'm deferring rather than approving purely because of where the change sits, not because I found anything wrong with it.
|
Closing: superseded. The conflict with main is that main already contains both fixes. They went upstream as 316941@main (bugs.webkit.org/318507, also covers a third case: objects with non-reified static properties, e.g. The two stress tests here exist on main under the same names, so there is nothing left in this branch to rebase. |
1 participant
FastStringifierdecides on its own whether a value can have atoJSON, and it gets two shapes wrong. In both,JSON.stringifysilently returns valid but wrong JSON: no throw, no crash.1. An own
toJSONthat is not enumerableSerializeJSONPropertystep 3 looks uptoJSONwithGetV, which finds own properties regardless of enumerability. TheObjectType/FinalObjectTypecase checksObject.prototypefortoJSON(mayHaveToJSON) but never checks the object itself, and its property loop skipsDontEnumentries first, so an own non-enumerabletoJSONis dropped. An own enumerabletoJSONis handled by accident: the loop reaches the callable property value andrecordFailure("callable object")sends the whole object down the slow path.Fix: look for
toJSONin the object property loop beforeDontEnumentries are skipped, mirroring what theArrayTypecase already does for own properties. It costs one pointer compare per property, and on a hit the object goes to the generalStringifier, which does theGetVlookup and the call.2. An array whose prototype was replaced
The
ArrayTypecase checks the globalArray.prototypefortoJSONand scans the array's own properties, but never looks at the array's actualstoredPrototype().Object.setPrototypeOfkeepsArrayTypeand the indexing type, so the array stays on the fast path with a prototype nobody checked. The object case bails on a non-standard prototype; arrays had no equivalent guard.Fix: bail when the stored prototype is not
Array.prototype, inside the branch that already handles non-original array structures, so plain arrays are untouched. (Found by a review comment on this PR, same bug class, so it is fixed here.)Verification
Built
jsc(JSCOnly,ENABLE_STATIC_JSC=ON,USE_BUN_JSC_ADDITIONS=ON, Debug) atc9ad5813fdwith and without the change.JSTests/stress/json-stringify-non-enumerable-to-json.jsandJSTests/stress/json-stringify-array-prototype-to-json.js(both new) fail before and pass after.JSTests/stresstest matching*json*/*stringif*passes (55 files,--useDollarVM=true). The one exclusion,missing-exception-check-in-json-stringifier-gap.js, fails the same way on an unmodified build in this environment (it is amemoryHogtest and trips ontoLocaleString).DynamicBufferpath, a non-callable owntoJSON, an enumerable owntoJSON, a null-prototype array, and an owntoJSONshadowing the prototype's.JSTests/stresstests matching*array-prototype*/*set-prototype*/*freeze*/*seal*/*proto-chain*pass. Three fail (array-prototype-flat-reentrant-mutation,missing-exception-check-in-array-prototype-fastJoin,object-freeze-with-arguments-no-oom-error), identically on an unmodified build in this environment.jscbuilt both ways (best of 5, in ms): 20k-row array 36.8 vs 36.7, 200-property object 6.6 vs 6.7, small nested object 47.8 vs 46.9, 20k-row array withspace: 218.3 vs 18.4.Both fixes match V8 for every shape probed: own non-enumerable
toJSON, nested, inside an array, inherited fromObject.prototype, inherited from a custom prototype, non-callable, enumerable, on a swapped array prototype, and shadowed by an own one.History
Neither bug is a recent regression, and neither is fork-specific. The object case of
FastStringifierhas skippedDontEnumproperties since it was introduced in e608d7d ("Speed up JSON.stringify by adding a separate fast case algorithm", 2022-07-10), and WebKit/WebKitmainhas the same code today, so this patch applies there unchanged.What changed for Bun is narrower than the bug. On WebKit
5488984d20e0(Bun 1.3.14) a frozen object escaped the fast path, because freezing transitioned it toNonArrayWithArrayStorageandcanPerformFastPropertyEnumeration()rejects structures with indexed properties:On
c9ad5813fdthe frozen object staysNonArray, reaches the fast path, and hits the hole. (canPerformFastPropertyEnumerationitself is unchanged between those two revisions.) The same object withoutObject.freezewas already serialized wrong on 1.3.14.