Conversation
Upstream 318399@main (https://bugs.webkit.org/show_bug.cgi?id=320820) removed StringRecursionChecker, so converting an array that contains itself with Array.prototype.join, Array.prototype.toString, Array.prototype.toLocaleString or ToString / ToPrimitive now recurses until the stack check throws a RangeError. V8 and SpiderMonkey detect the cycle and convert the nested occurrence to the empty string, so `a = [1]; a[1] = a; a.join()` is "1," in Node.js and in every browser, and it was "1," in every JavaScriptCore before that change. Bring the detection back for the array conversions only, under USE(BUN_JSC_ADDITIONS): arrayProtoFuncJoin, arrayProtoFuncToLocaleString, JSArray::fastToString (the Array.prototype.toString fast path and the array case of JSObject::toString / toPrimitive), and the DFG / FTL ArrayJoin operation, which did not exist when the checker was removed and would otherwise make a compiled join site return "1-1," where the interpreter returns "1-". The checker only does the cycle bookkeeping; stack overflow stays with the checks upstream added. Error.prototype.toString and RegExp.prototype.toString are not tracked, matching V8 and upstream. JSTests/stress/string-conversion-recursion.js now describes this behavior: the array cases through every entry point, array-likes, acyclic reuse of an array, cleanup after a conversion throws, Error / RegExp cycles and deep acyclic nesting still overflowing, and the cyclic results agreeing between the interpreter and the optimizing tiers.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it deliberately diverges from upstream JSC and the spec on hot-path array conversion semantics, and adds per-VM state that must stay consistent across all four entry points (interpreter, fastToString, and the DFG/FTL join intrinsic), a human look would still be worthwhile.
Checked the RAII register/unregister logic in StringRecursionChecker — the first-object fast path and hash-set path pair correctly, and JSC's return-based exception unwinding runs the destructors.
Checked arrayJoinWithStringSeparator: the checker sits after the JSOnlyStringsAndInt32sJoiner fast path, which cannot recurse (bails on non-string/non-int32 elements), so no cycle escapes there.
Checked that the raw JSObject* in the VM fields are always live on the C++ stack while registered, so conservative scanning keeps them rooted and JSC's non-moving GC keeps the pointers stable.
Extended reasoning...
Overview
This PR restores cycle detection for array-to-string conversions (join, toString, toLocaleString, and the ToString/ToPrimitive fast path) in Bun's JavaScriptCore fork, after upstream WebKit removed it in 318399@main. It adds a new StringRecursionChecker RAII helper (header-only, guarded by USE(BUN_JSC_ADDITIONS)), two fields on VM (stringRecursionCheckFirstObject and stringRecursionCheckVisitedObjects), and inserts the checker at four entry points: arrayProtoFuncJoin, arrayProtoFuncToLocaleString, JSArray::fastToString, and the DFG/FTL arrayJoinWithStringSeparator operation. The stress test is rewritten to assert V8-compatible cycle behavior instead of upstream's stack-overflow behavior.
Security risks
None identified. The change is purely additive under a fork-specific compile guard, stores raw pointers to objects that are already conservatively rooted on the C++ stack for the lifetime of the checker, and does not touch auth, crypto, permissions, or memory-layout-sensitive code. The hash set is per-VM (single-threaded) so there are no concurrency concerns.
Level of scrutiny
High. This modifies extremely hot runtime paths (Array.prototype.join / toString are among the most-called builtins) across the interpreter, the JSObject::toString fast path, and the DFG/FTL JIT operation, and it must keep those tiers observably identical. It is also a maintained, intentional divergence from both the ECMAScript spec and upstream WebKit — a design decision about which engine's behavior Bun tracks. A maintainer should confirm that this divergence is desired and that the four insertion points cover every route into fastArrayJoin going forward.
Other factors
The implementation itself looks correct: the first-object / hash-set bookkeeping mirrors the pre-removal upstream code, the destructor's branch order matches the constructor's, and the [[unlikely]] recursive branch keeps the common path cheap. The DFG placement after JSOnlyStringsAndInt32sJoiner::tryJoin is safe because that fast path bails on any object element and therefore cannot recurse. The PR description documents thorough local verification (debug + ASan, tier-consistency test, cross-check against Node.js v26). Test coverage in the rewritten stress file is comprehensive — direct/mutual cycles, user-code cycles, array-like receivers, throw-unwinding cleanup, deep acyclic overflow, and JIT warm-up. Given the scope, hot-path placement, and the long-term maintenance implication of diverging from upstream, deferring to a human reviewer rather than auto-approving.
Preview Builds
|
|
Closing: superseded by #559, which restores the same cycle guard and is merged. |
2 participants
Problem
3722912ff800(Upgrade to upstream WebKit 3722912ff800 #383), converting an array that contains itself (directly or through other arrays) throwsRangeError: Maximum call stack size exceeded: witha = [1]; a[1] = a,a.join(),String(a),`${a}`anda.toLocaleString()all throw. Node.js (V8), SpiderMonkey and every JavaScriptCore before that sync return"1,": the nested occurrence of the array being converted becomes the empty string. Bun 1.3.14 returned"1,", the 1.4 builds throw (found by a minifier equivalence corpus, 2 of 600 programs; it also breaksjoin()on graph shaped data and logging helpers that relied on the old behavior).arrayProtoFuncJoin,arrayProtoFuncToLocaleStringandJSArray::fastToStringconsulted, because the specification has no cycle rule, and replaced it with a stack check infastToString. The conversion now recurses natively (fastToString->fastArrayJoin->JSStringJoiner::append->JSObject::toString->fastToString) untilisSafeToRecurseSoft()fails, which is why the error carries no JS frames.ArrayJoinintrinsic (operationArrayJoininDFGOperations.cpp), which callsfastArrayJoindirectly. Restoring only the three old sites would make a compiledjoincall site return"1-1,"where the interpreter returns"1-"(verified with a build that has everything but the DFG hunk).Fix
StringRecursionChecker.h(new, underUSE(BUN_JSC_ADDITIONS)) is the cycle bookkeeping of the removed class and nothing else: the constructor registers the receiver inVM::stringRecursionCheckFirstObject(the non nested case, two stores) orVM::stringRecursionCheckVisitedObjects(nested conversions) and reports whether it was already registered, the destructor unregisters it. The stack overflow half of the old class is not restored: upstream'sisSafeToRecurseSoft()check infastToString, plus the stack checks on the call paths into the other entry points, already cover it, so a deep acyclic nesting still throwsRangeErrorexactly as upstream.arrayProtoFuncJoinandarrayProtoFuncToLocaleString(where the removed checks were, afterToObject),JSArray::fastToString(theArray.prototype.toStringfast path and the array case ofJSObject::toString/toPrimitive, which coversString(), template literals and+), andarrayJoinWithStringSeparator, shared byoperationArrayJoin/operationArrayJoinGeneric, which is what a DFG or FTL compiledjoincall runs.CycleProtectedArrayJoinpushes the receiver on a per isolate join stack); every expected value in the test was checked against Node.js v26, which passes the test as is.Error.prototype.toStringandRegExp.prototype.toString, which the removed class also tracked, stay untracked: V8 does not track them, and tracking them caused the cross conversion oddities upstream cited (an element whosetoStringcallsError.prototype.toString.call(array)used to make the join""; it is now"Error", as in V8).toString, or the stack overflow of a deep nesting) unregisters every array it registered, on the first object path and the hash set path alike; the test checks both.JSTests/stress/string-conversion-recursion.jsis upstream's test from the removal rewritten to the behavior this fork keeps: the self containing array through every entry point, fan out, mutual cycles converted from either side, cycles closed through usertoString/toLocaleString/ a replacedjoin, array-like receivers on the generic paths, acyclic reuse of an array, unregistration after throwing conversions,Error/RegExpcycles and a 100000 deep acyclic nesting still throwingRangeError(the shallowest overflowing depth measured is about 1100 arrays in the debug ASan build and 4800 in release), and atestLoopCountwarm up on a contiguous acyclic array followed by the cyclic one, whose compiled tier results have to equal the interpreter's.--useConcurrentJIT=0; on the unmodifiedautobuild-f0f60fd2releasejscit fails at its first array assertion with theRangeErrorabove; with only theDFGOperations.cpphunk removed it fails in the tier section withexpected "1-" but got "1-1,".JSTests/stressfiles aboutjoin/toString/toLocaleStringof arrays, errors and regexps, andwasm/v8/regress/regress-769846.js, pass on the same build, except four that need ICU data the local shell does not have (details below). The modified translation units also compile clean (-fsyntax-only,-Wall -Wextra) with the flags of thef0f60fd2release and debug ASan prebuilts. The companion Bun PR pins this PR's preview build and runs the same test as ajsc-stressfixture against the real engine and ICU.Background
Array.prototype.toStringcallsjoin;ToString(array)(whatString(), template literals and+do) goes throughJSObject::toString/toPrimitive, which for an array with an untouched prototype chain callsJSArray::fastToStringdirectly; andjoinhas had a DFG/FTL intrinsic since the same sync. Those are the four places a conversion can start, and all of them convert elements throughJSStringJoiner::append, which for an object element lands back in one of them, so a cycle always re-enters one of the four with the array still registered.USE(BUN_JSC_ADDITIONS)marks the fork's own changes so they are recognizable when upstream is merged; the hunks here are purely additive next to upstream's code for the same reason. The twoVMfields are the ones the removed class used; aVMbelongs to one thread, which is what makes two plain fields a correct "conversions in progress" stack.string-conversion-recursion.jsasserted theRangeErrorfor arrays, so it had to change. It is the only test inJSTeststhat depended on the removal:regress-191731.jsandwasm/v8/regress/regress-769846.jswere made indifferent to it by the same upstream commit and pass on this branch.Local shell and ICU
The local build links the
libicudata.afrom the prebuilt tarball, whose items are zstd compressed and decompressed by a hook that Bun provides and thejscshell does not, soNumber.prototype.toLocaleStringandIntlthrow in that shell. Affected, and unrelated to this change:array-toLocaleString.js,array-tolocalestring-empty-separator.js,array-tolocalestring-options.jsandmissing-exception-check-in-array-prototype-fastJoin.jsfail there before reaching the changed code and pass on the unmodified prebuilt; the last one passes locally onceNumber.prototype.toLocaleStringis stubbed, the other three compare locale output. The new test's own numbertoLocaleStringcalls were stubbed the same way (Number.prototype.toLocaleString = function() { return String(this); }prepended) for the local run; thejsc-stressfixture in the Bun PR runs it unstubbed. The other 25 related files (array-join-*.js,array-prototype-join-*.js,array-tolocalestring-*.js,array-toString-non-callable-join.js,empty-string-join.js,immutable-butterfly-to-string-cache-should-not-happen-for-generic-join.js,regress-191731.js,regexp-prototype-tostring.js,nativeerror-prototype-tostring.js,errors-with-simple-names-or-messages-should-not-crash-toString.js,object-to-string-and-to-this.js,bigint-to-string-in-array.js,json-fast-stringifier-on-cyclic-structure.js) pass as is.