Conversation
|
Status: diff is green; remaining CI failures are unrelated to Reproduction (fails on released bun, passes on this branch): The Remaining failures on #76080:
Depends on oven-sh/WebKit#312 (preview |
|
Updated 12:37 AM PT - Jul 20th, 2026
❌ @robobun, your commit a2d436f has 1 failures in 🧪 To try this PR locally: bunx bun-pr 34755That installs a local version of the PR into your bun-34755 --bun |
WalkthroughChangesConstructor-name recovery
WebKit version update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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 `@scripts/build/deps/webkit.ts`:
- Line 10: Update WEBKIT_VERSION to the merged WebKit main-branch SHA, replacing
the temporary autobuild-preview-pr-312-73c011c1 tag so dependency resolution and
prebuilt cache keys use the permanent revision.
🪄 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: 5cb494cd-43ae-4164-be5e-298652d060c9
📒 Files selected for processing (6)
scripts/build/deps/webkit.tssrc/js/internal/util/inspect.jssrc/jsc/bindings/UtilInspect.cppsrc/jsc/bindings/UtilInspect.htest/js/node/util/node-inspect-tests/parallel/util-format.test.jstest/js/node/util/node-inspect-tests/parallel/util-inspect.test.js
7a2ec00 to
ef36834
Compare
There was a problem hiding this comment.
The Bun-side glue here looks correct, but this needs a maintainer to land: it depends on oven-sh/WebKit#312 (still open) and pins WEBKIT_VERSION to a preview tag that must be swapped for the merged SHA. The substantive change — caching the constructor name on StructureRareData during changePrototypeTransition — lives in the WebKit PR, not this diff, and touches JSC structure-transition internals that warrant human review.
What was reviewed:
jsFunctionGetSourceConstructorName: no JS execution, no throw scope needed (structure/typeInfo reads only),FinalObjectTypegate matches the V8 behavior described.internalGetConstructorNamefallback ordering: new lookup slots between.constructor?.nameand the[object X]regex, soObject.create(null)/ non-final objects are unaffected.- Test changes un-TODO existing Node-parity assertions and add a focused test; the 30s timeout on
no assertion failures 2was discussed and resolved (pre-existing debug+ASAN slowness on main).
Extended reasoning...
Overview
This PR makes util.inspect(Object.setPrototypeOf(new Foo(), null)) print [Foo: null prototype] {} instead of [Object: null prototype] {}, matching Node.js. On the Bun side it adds a ~15-line C++ host function jsFunctionGetSourceConstructorName in UtilInspect.cpp that reads Structure::sourceConstructorName() and returns it as a JS string (or undefined for non-FinalObjectType objects, empty names, or "Object"), wires it into internalGetConstructorName in src/js/internal/util/inspect.js as a fallback before the Object.prototype.toString regex, un-TODOs four Node-parity assertions across two test files, and adds a dedicated regression test. It also bumps WEBKIT_VERSION to a preview tag for the unmerged oven-sh/WebKit#312.
Why this can't be auto-approved
The load-bearing change is not in this diff. Structure::sourceConstructorName() and the caching in changePrototypeTransition / toDictionaryTransition live in the WebKit fork PR, which touches JSC's structure-transition machinery and StructureRareData allocation. Per REVIEW.md's Dependencies & vendoring guidance, WebKit bumps and engine-internal changes need maintainer review — the perf claim (one VMInquiry lookup per setPrototypeOf, Object.prototype → null skipped so Object.create(null) allocates no rare data) and GC-safety claim (plain WTF::String, no GC root) can only be verified against that PR. Additionally, the PR is explicitly not landable as-is: WEBKIT_VERSION points at autobuild-preview-pr-312-0bab8a9a, and both the description and author state it must be updated to the merged main SHA before merge.
Security risks
None identified. The binding reads engine-internal metadata (structure type, cached string) with no user-JS execution, no coercions, and no external input parsing. util.inspect output formatting is not security-sensitive.
Level of scrutiny
High — not for the Bun-side diff (which is small and mechanical) but because it's coupled to a JSC-internals change in a separate repo and a WebKit version bump. The Bun-side host function is straightforward: getObject() null-checks, typeInfo().type() and sourceConstructorName() are non-throwing structure accessors so no ThrowScope/RETURN_IF_EXCEPTION is needed, and jsString(vm, WTF::move(name)) is the standard pattern. The JS-side insertion preserves existing fallback order.
Other factors
The prior inline nit about the 30s per-test timeout on no assertion failures 2 was answered with evidence that the block already times out on main under debug+ASAN (~10s vs 5s default) independent of this change; that thread is resolved. Test coverage is good: both entry points (util.inspect and util.format('%s', ...)), property adds after the prototype change, chained transitions, Symbol.toStringTag combination, and negative cases (Object.create(null), { __proto__: null }) are all asserted.
…nces
Object.setPrototypeOf(new Foo(), null) now inspects as
'[Foo: null prototype] {}' instead of '[Object: null prototype] {}',
matching Node.js.
Node recovers the name via V8's Map::constructor back-reference which
survives prototype transitions. JSC's changePrototypeTransition pins the
new Structure and clears previousID(), so neither the old prototype nor
its constructor is reachable from the object afterwards.
The JSC side (oven-sh/WebKit#312, preview autobuild-preview-pr-312-0bab8a9a)
caches the previous prototype's constructor name on StructureRareData
during changePrototypeTransition and carries it through
toDictionaryTransition. Structure::sourceConstructorName() walks
previousID() to find it. The cache is a plain WTF::String so it adds no
GC root.
On the Bun side, internalGetConstructorName consults a new
jsFunctionGetSourceConstructorName binding before falling back to
'[object X]' parsing. The binding only answers for FinalObjectType
instances; V8 reports the base type for Error/Array/etc. subclasses
after setPrototypeOf(null), which Object.prototype.toString already
covers.
The 'no assertion failures 2' test block (~1500 util.inspect assertions)
already exceeds the 5s default under debug+ASAN on main; gave it a 30s
ceiling so the file is runnable via bun bd.
ef36834 to
a2d436f
Compare
There was a problem hiding this comment.
I didn't find bugs in the Bun-side diff, but this needs a maintainer: the load-bearing change is in oven-sh/WebKit#312 (Structure::changePrototypeTransition / StructureRareData caching), which isn't reviewable here, and WEBKIT_VERSION is still pinned to a preview tag that must be swapped for the merged sha before landing.
Checked: jsFunctionGetSourceConstructorName has no throwing calls (structure reads + jsString on a WTF::String), so no exception scope needed; the FinalObjectType gate keeps built-in subclasses on the Object.prototype.toString path; internalGetConstructorName still tries val.constructor?.name first so the new lookup only fires on the null-prototype fallback.
Extended reasoning...
Overview
Adds a C++ binding jsFunctionGetSourceConstructorName that reads Structure::sourceConstructorName() (new JSC API from oven-sh/WebKit#312), wires it into internalGetConstructorName in src/js/internal/util/inspect.js as a fallback before the [object X] regex, un-TODOs matching Node compat assertions in util-inspect.test.js / util-format.test.js, adds a dedicated regression test, and pins WEBKIT_VERSION to autobuild-preview-pr-312-d4aabd18.
Security risks
None identified. The binding does pure in-memory reads on a Structure and returns a string or undefined; no user JS is executed, no allocation on untrusted sizes.
Level of scrutiny
High — for reasons outside this diff. The Bun-side changes are small and look correct, but the actual fix lives in the WebKit fork and modifies changePrototypeTransition / toDictionaryTransition to cache a constructor name on StructureRareData. That is core-engine hot-path code affecting every Object.setPrototypeOf call, and a prior iteration already tripped an ExceptionScope::assertNoException() SIGABRT in unrelated tests. A maintainer needs to review the WebKit PR itself and coordinate the merge order (WebKit first, then update WEBKIT_VERSION here to the permanent sha).
Other factors
- The PR description and author comments explicitly state this must not land while
WEBKIT_VERSIONpoints at the preview tag; approving now would contradict that. - The 30s timeout on
no assertion failures 2was discussed in a prior thread; the author showed the block already times out on main under debug+ASAN, so it's a pre-existing condition — reasonable to leave for a follow-up split, but a maintainer may still prefer that split happen here. - Test coverage on the Bun side is good: chained prototype transitions, property adds after the transition,
Symbol.toStringTagcombination, and the negative cases (Object.create(null),{ __proto__: null }) are all asserted.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-20, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Object.setPrototypeOf(new Foo(), null)now inspects as[Foo: null prototype] {}instead of[Object: null prototype] {}, matching Node.js.Cause
Node recovers the name via V8's
Map::constructorback-reference, which is set at allocation time and survives prototype transitions. JSC'sStructure::changePrototypeTransitionpins the newStructureand clearspreviousID(), so neither the old prototype nor its constructor is reachable from the object afterwards.calculatedClassName()falls back to"Object".Fix
JSC side (oven-sh/WebKit#312):
changePrototypeTransitionlooks up the previous prototype's ownconstructor(VMInquiry, no JS execution) before enteringDeferGCand caches the resulting name on the new structure'sStructureRareData.toDictionaryTransitioncarries an existing cached name forward.Structure::sourceConstructorName()walkspreviousID()to find the cached value so property-add transitions after the prototype change can still reach it. The cache is a plainWTF::String, so it adds no GC root; theObject.prototype→nullcommon path is skipped soObject.create(null)allocates no rare data.Bun side:
internalGetConstructorNameinutil.inspectconsults a newjsFunctionGetSourceConstructorNamebinding before falling back to[object X]parsing.Verification
The new assertions match Node.js v26.3.0 output for
setPrototypeOf(new Foo(), null), subsequent property adds, chained prototype changes (Foo→Bar.prototype→nullstill reportsFoo), and combinations withSymbol.toStringTag.Object.create(null)and{ __proto__: null }still report[Object: null prototype]. Also un-TODOs the matching assertions inutil-inspect.test.jsandutil-format.test.js.Depends on
This PR pins
WEBKIT_VERSIONto the preview build of oven-sh/WebKit#312 (autobuild-preview-pr-312-0bab8a9a). CI here will fail until that preview release is published; the WebKit preview build is currently queued on GitHub Actions. Once oven-sh/WebKit#312 merges,WEBKIT_VERSIONshould be updated to the merged main sha before this lands.no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js