internal/primordials: Node.js parity, generated and engine-backed - #35567
internal/primordials: Node.js parity, generated and engine-backed#35567robobun wants to merge 25 commits into
Conversation
Bump WebKit to 6d9faf57f9 which adds 435 link-time constants named
after Node.js primordials (@ArrayPrototypePush, @ObjectDefineProperty,
@MapPrototypeGetSize, ...) captured at holder-creation time so builtin
JavaScript can call originals tamper-proof in JSC style:
$ArrayPrototypePush.$call(array, value);
$ObjectDefineProperty(obj, key, desc);
$MapPrototypeGetSize.$call(map);
@-name resolution emits moveLinkTimeConstant (constant-pool slot) and
.@call in a builtin compiles to a bare op_call, so there is no lookup
overhead versus an unpolluted direct call.
Add $-prefixed typedefs for all 435 names to builtins.d.ts and a
bun:internal-for-testing probe that exercises one primordial per holder
kind (eager prototype, LUT-backed constructor, LazyClassStructure,
LazyProperty, PropertyCallback) plus a getter and a symbol-keyed
method. The test pollutes prototypes and namespace objects and verifies
the primordials still return the original behavior, including the case
where a lazy holder (Map, Math) is first touched after tampering.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR adds generated JSC primordial bindings, hardened runtime wrappers, audit/test interfaces, extensive tampering and isolation tests, and changes the default WebKit prebuilt identifier. Link-time primordials
WebKit artifact selection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:04 PM PT - Aug 3rd, 2026
❌ @dylan-conway, your commit 75292f8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35567That installs a local version of the PR into your bun-35567 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Use the autobuild-preview-pr-341-6d9faf57 prerelease until the WebKit PR merges and a main-branch autobuild is available.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…tins Under bun -e the first user arg is process.argv[1], not [2], so the tamper block was never entered and the test was vacuous. Switch to a TAMPER env var and capture JSON.stringify/process.stdout.write/Map/ Uint8Array before tampering so the harness itself does not trip the poisoned builtins.
There was a problem hiding this comment.
Thanks for addressing the earlier test-harness feedback — the tamper gate now uses process.env.TAMPER, and the child script captures JSON.stringify/write/Map/Uint8Array before tampering and uses indexed iteration, so the "are tamper-proof" case is no longer vacuous. No new issues found this pass, but this is a WebKit bump (to a preview PR tag) whose actual behavior change lives in oven-sh/WebKit#341, so it still wants a human sign-off.
What was reviewed
- Revised child script: harness values are built pre-tamper; no remaining paths through tampered
Symbol.iterator/bind/JSON.stringify. primordials.run()body under tampering —.next(),.length,new DataView(u8.buffer),instanceof $Promiseall go through un-tampered slots.prebuiltUrl/prebuiltDestDirinwebkit.tsalready handleautobuild-*tags, so the preview version string is well-formed for both.- 30s Worker-test timeout flagged and ruled out (Worker startup under debug+ASAN routinely exceeds the default).
Extended reasoning...
Overview
Four files: a one-line WEBKIT_VERSION bump to autobuild-preview-pr-341-6d9faf57, 435 declare const $...: Function typedefs appended to src/js/builtins.d.ts, a ~40-line primordials probe in src/js/internal-for-testing.ts, and a new test/js/bun/util/primordials.test.ts. The behavior change (capturing 435 link-time-constant primordials in JSGlobalObject) is entirely in the WebKit fork PR — this repo only contains typedefs, a test probe, and the version pin.
Prior feedback resolved
My earlier inline comment identified that process.argv[2] under bun -e is always undefined (making the tamper test vacuous) and that once fixed the harness would trip over its own tampered builtins. Commit e56d298 addresses both: gating moved to process.env.TAMPER, stringify/write/map/u8 captured before the tamper block, Uint8Array filled by index (no array iterator), and the for-of/destructure replaced with an indexed for. I re-traced every statement after the tamper block against the tamper list and found no remaining leak.
Security risks
None introduced by this repo's diff — .d.ts declarations have no runtime effect, and internal-for-testing.ts is CI-gated. The security-relevant surface (whether primordial capture is truly race-free against user tampering across all lazy-holder kinds) is implemented in WebKit#341 and not reviewable here.
Level of scrutiny
High, because it is a WebKit dependency bump and the version string is a preview PR tag rather than a merged commit hash. Per the repo's own guidance a dependency bump — especially to a fork of the JS engine — needs a maintainer to confirm WebKit#341 is landing and to swap the preview tag for the real autobuild-<sha> before merge. The Bun-side changes themselves are mechanical.
Other factors
prebuiltUrl()already special-casesautobuild-prefixes andprebuiltDestDir()slices the prefix off, so the preview tag won't produce a malformed URL or cache key.- The PR description already states CI will fail until the WebKit autobuild completes; that plus the preview tag are both things a human needs to coordinate.
- comment-cop bot pings on
builtins.d.ts/internal-for-testing.tsappear to be false positives (the added comments are one-liners). - No
src/js/builtin has been migrated to the new intrinsics yet, so there is no user-visible behavior to regress in this PR itself.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 6: Update the WEBKIT_VERSION constant to reference the merged main commit
or stable tag instead of the transient autobuild-preview-pr-341-6d9faf57 value,
ensuring prebuilt fetches remain available after the PR lifecycle ends.
In `@test/js/bun/util/primordials.test.ts`:
- Line 130: Remove the explicit timeout option from the “Worker globals capture
independently” test declaration, leaving the test body and repository-level
timeout behavior unchanged.
- Around line 35-37: Extend the primordials tampering setup in the test to
poison getter-backed properties, capturing Object.defineProperty before any
tampering and using it to replace at least Map.prototype.size with a throwing
getter before primordials.run(). Also cover the exercised
TypedArray.prototype.length and DataView.prototype.byteLength getters, ensuring
the test fails if they are looked up late while preserving the existing tampered
subarray and Promise.resolve checks.
🪄 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: 12276c73-15ea-4e84-b3b1-5a2aded73fee
📒 Files selected for processing (4)
scripts/build/deps/webkit.tssrc/js/builtins.d.tssrc/js/internal-for-testing.tstest/js/bun/util/primordials.test.ts
There was a problem hiding this comment.
Beyond the inline nit: confirmed e56d298 fully addresses the earlier 🔴 — the child now gates on process.env.TAMPER, captures JSON.stringify/write/map/u8 before tampering, builds the Uint8Array via indexed writes, and uses an indexed for loop; I re-walked every post-tamper line (including .next().value, .length, new DataView(u8.buffer), Reflect.getPrototypeOf) and none go through a tampered builtin. Deferring on the merge decision itself since WEBKIT_VERSION points at a preview autobuild tag for an as-yet-unmerged oven-sh/WebKit PR.
Extended reasoning...
The prior blocking finding was addressed in e56d298 and the fix is complete on inspection. This run's only new finding is a nit (missing tamper-ran canary). The PR is otherwise type declarations + a test probe + a WebKit version bump; the substantive implementation lives in oven-sh/WebKit#341, and WEBKIT_VERSION currently targets a preview autobuild tag rather than a merged commit, so a maintainer should coordinate the landing order.
… canary Tamper Map.prototype.size, %TypedArray%.prototype.length, DataView.prototype.byteLength and RegExp.prototype.source with throwing getters so a late getter lookup in capturePrimordials would fail the test. Route the probe's own reads of subarray().length and u8.buffer through primordial getters so the probe does not trip the poison. Emit out.tampered = (Array.prototype.push !== originalPush) and assert it in the parent so a future regression in the TAMPER gate cannot pass vacuously.
There was a problem hiding this comment.
No issues found in the Bun-side changes, but this bumps WebKit/JSC and the substantive change (435 link-time constants, lazy-capture hooks in JSGlobalObject::init) lives in oven-sh/WebKit#341 — that plus the temporary autobuild-preview-pr-* pin needs a human sign-off.
Reviewed: the tamper-proof test now gates on process.env.TAMPER, captures stringify/defineProperty/write/map/u8 before poisoning, and asserts the tampered canary — the earlier vacuous-gate class is closed. Checked that the child harness's remaining userland calls (Reflect.getPrototypeOf, process.stdout.write, indexed for loop) don't hit anything the tamper block replaces. The primordials.run probe covers one holder per kind (eager, LUT, LazyClassStructure, LazyProperty, PropertyCallback, getter, symbol-keyed) and the getter-backed ones are now poisoned via captured defineProperty.
Extended reasoning...
Overview
Four files: a one-line WEBKIT_VERSION bump to autobuild-preview-pr-341-6d9faf57, ~435 generated declare const $Name: Function typedefs appended to src/js/builtins.d.ts, a primordials.{run,refs} probe in src/js/internal-for-testing.ts that exercises one link-time constant per JSC holder kind, and a new test/js/bun/util/primordials.test.ts that spawns child processes to verify capture, tamper-proofing (with a tampered canary asserting the poison block actually ran), lazy-holder-first-touch ordering, per-Worker capture, and PropertyCallback-holder overwrite-before-first-read.
Security risks
None introduced on the Bun side — the diff adds type declarations, a test-only probe gated behind bun:internal-for-testing, and tests. The security-relevant behavior (tamper-proof primordial capture in m_linkTimeConstants) is implemented entirely in the WebKit PR, which is not reviewable from this diff.
Level of scrutiny
High. WEBKIT_VERSION controls which JavaScriptCore build every Bun binary links against; any JSC bump is a critical-path change even when the Bun-side diff is small. The pin is also to a transient autobuild-preview-pr-* tag that the author has stated will be replaced with the merged main-branch commit once oven-sh/WebKit#341 lands — a human should confirm that hand-off before merge. The 435-name typedef list is stated to be generated from JSC_FOREACH_PRIMORDIAL_NAME; drift between it and the WebKit header would surface as a codegen/link failure, not silent misbehavior, so I did not audit it name-by-name.
Other factors
Both of my earlier findings on this PR (the process.argv[2] dead gate under bun -e, and the missing tamper canary) were addressed in e56d298 and e3f28fe; CodeRabbit's getter-poisoning and preview-pin threads are resolved. CI on 8ea828c is still building per robobun, and the PR body notes CI will fail until the WebKit autobuild completes — another reason for a human to gate the merge on the dependency landing.
|
Status at d7c3488 (build #81759): No test failures. primordials.test.ts passes on every lane that ran. All bot review threads resolved; two clean review summaries from the review bot ("No further findings", deferring to maintainer for the WebKit preview pin + Only red: Review follow-ups applied on top of e4b4873:
Before merge: swap |
- Expose JSC's primordial manifest through `bun:internal-for-testing` (primordials.audit()) so tests iterate all 435 entries instead of a hand-picked probe. - Generate src/js/primordials.d.ts from JSCPrimordials.h (src/codegen/generate-primordials-dts.ts) with real method/getter types, replacing the hand-written `Function`-typed block; a test pins the generated names to the engine's manifest. - After Bun installs its own Error.captureStackTrace, override the ErrorConstructor primordial slots so `$ErrorCaptureStackTrace` is what user code sees. - Rewrite test/js/bun/util/primordials.test.ts: identity of every entry against the live builtin (before and after use), direct and roundabout tampering of every key on every holder plus the holders and global bindings themselves, pollution planted before any lazy holder exists, behavior of tampered builtins through their primordials, option-off configuration (unavailable placeholders), and per-Worker capture.
|
Pushed e4b4873 on top of the WebKit rework in oven-sh/WebKit#341 (pin bumped to
Follow-up (not in this PR): migrate |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/codegen/primordials-module-epilogue.js`:
- Line 18: Fix all three lint violations in
src/codegen/primordials-module-epilogue.js: remove the unused ObjectCreate
destructuring entry; in the unsafe.prototype handling block, cache
unsafe.prototype locally and reuse it for the membership check, ReflectOwnKeys,
descriptor access, and else branch; in the items-processing block, cache
items.length once and reuse that local in both the if and while conditions.
Regenerate src/js/internal/primordials.js afterward.
In `@src/js/primordials.d.ts`:
- Around line 104-105: Update the nativeError() helper in
generate-primordials.ts to type the prototype holder as Error while retaining
ErrorConstructor for the constructor holder, following the part-aware pattern
used by typedArrayHolder(). Regenerate primordials.d.ts so all AggregateError
and other native error prototype declarations, plus their derived constructor
methods, use the corrected types; do not edit the generated file directly.
In `@test/js/bun/util/primordials.test.ts`:
- Around line 105-116: Update the native-error and typed-array loops that
populate holderFactories so each factory captures only name during prelude
construction and reads globalThis[name] when invoked, including resolving the
prototype from that deferred constructor. Preserve the existing Constructor and
Prototype holder keys and behavior.
- Line 734: Update the uncurryThis assertion in the primordials test to remove
the `? true : true` conditional and avoid passing `null`; invoke `p.uncurryThis`
with the appropriate Map prototype method and assert its actual behavior with a
falsifiable expectation.
🪄 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: 6f130050-aefd-44f3-ae8d-b84fcff0276a
📒 Files selected for processing (9)
scripts/build/deps/webkit.tssrc/codegen/generate-primordials-probe.jssrc/codegen/generate-primordials.tssrc/codegen/primordials-module-epilogue.jssrc/codegen/primordials-module-prologue.jssrc/js/internal-for-testing.tssrc/js/internal/primordials.jssrc/js/primordials.d.tstest/js/bun/util/primordials.test.ts
- generate-primordials.ts: native-error prototype holders are typed as Error (not ErrorConstructor) in the generated .d.ts; regenerated. - module epilogue: drop the unused ObjectCreate destructure; regenerated. - test: make the uncurryThis assertion falsifiable, and defer the holder factories' global reads to invocation so building the prelude does not create the lazy holders that later tests must poison first.
The Safe* classes use `constructor(i) { super(i); }` as Node's do rather than
the implicit default constructor, whose `super(...args)` spread is
spec-observable through the array iterator protocol.
| // vendored tests need more internals. Values are lazy so merely requiring | ||
| // bun:internal-for-testing does not evaluate the whole internal-module graph. | ||
| export const exposedInternals = { |
There was a problem hiding this comment.
🟡 The lazy-getter conversion for exposedInternals (with its comment "Values are lazy so merely requiring bun:internal-for-testing does not evaluate the whole internal-module graph") missed the sibling export const fs = require("node:fs/promises").$data; at line 340, which still eagerly pulls in internal/primordials (via internal/fs/glob → internal/shared). So the prelude's require("bun:internal-for-testing") still evaluates the primordials module before the "tampering planted before the module has ever loaded" test body tampers — that test's stated precondition ("First load of internal/primordials happens now, after the tampering") is false and it cannot catch a regression where the epilogue reads live prototypes at load time. Convert fs to a lazy getter like the exposedInternals entries, and/or have the load-timing test assert its own precondition (REVIEW.md § Tests reviewers reject: "assert that setup created the precondition"; § Correctness: "Grep for every sibling site sharing the pattern").
Extended reasoning...
What the issue is
This PR converted the eight exposedInternals entries in src/js/internal-for-testing.ts from eager require(...) values to lazy getters, and added a comment (lines 280-282) explaining why: "Values are lazy so merely requiring bun:internal-for-testing does not evaluate the whole internal-module graph." That change was made specifically so the new "primordials module load timing" test can tamper prototypes before internal/primordials first loads.
But the same file still has, at line 340 (unchanged by this PR):
export const fs = require("node:fs/promises").$data;which is an eager top-level require that transitively evaluates internal/primordials:
src/js/node/fs.promises.ts:5→const { Glob } = require("internal/fs/glob")(top-level)src/js/internal/fs/glob.ts:19→const { kEmptyObject } = require("internal/shared")(top-level)src/js/internal/shared.ts:1→const { SafeArrayIterator } = require("internal/primordials")(top-level)
So the comment added in this PR is false, and the invariant it was added to guarantee doesn't hold.
The specific test whose precondition breaks
test/js/bun/util/primordials.test.ts — the "primordials module load timing" describe block — spawns a child whose script is prelude + body. The prelude's very first line is:
const { primordials } = require("bun:internal-for-testing");Evaluating bun:internal-for-testing runs internal-for-testing.ts top-to-bottom, hits line 340, and — via the chain above — evaluates internal/primordials and caches it. The test body then does:
RegExp.prototype.exec = function evilExec() { return null; };
Map.prototype.get = function evilGet() { return "evil get"; };
Promise.prototype.then = function () { throw new Error("evil then"); };
// First load of internal/primordials happens now, after the tampering.
const p = primordials.object;That comment is wrong: primordials.object is a getter that calls require("internal/primordials"), but the module is already in the require cache from the prelude. p is the instance built from a pristine environment. If a future change made makeSafeFromPristine or the OriginalRegExpPrototype* captures accidentally read Map.prototype.get / RegExp.prototype.exec at load time instead of the $Name constants, this test would still pass — the module never sees the tampering it was written to expose it to.
Why this is the sibling-site pattern REVIEW.md calls out
REVIEW.md § Correctness: "Fix the whole class in the same PR ... Grep for every sibling site sharing the pattern." This PR grepped for eager require(...) in exposedInternals and converted eight of them, but the identical pattern at line 340 (and its rationale — the added comment applies to the whole file, not just exposedInternals) was left. export const Dequeue = require("internal/fifo") at line 226 is fine — internal/fifo has no transitive requires — so fs is the one remaining leak.
REVIEW.md § Tests reviewers reject: "assert that setup created the precondition." The load-timing test asserts its intended precondition only in a comment. If it also asserted it (e.g. by checking that internal/primordials isn't already in the builtin-module cache before tampering, or by having the prelude expose a "was primordials already loaded?" bit), a false precondition would fail the test instead of silently passing.
Step-by-step proof
- Child process starts;
bun -ereceivesprelude + body. - Prelude line 1 executes:
require("bun:internal-for-testing"). - Builtin bundler evaluates
internal-for-testing.tstop-to-bottom. Line 340:require("node:fs/promises")runs. fs.promises.tsline 5: top-levelrequire("internal/fs/glob")runs.glob.tsline 19: top-levelrequire("internal/shared")runs.shared.tsline 1: top-levelrequire("internal/primordials")runs. The module evaluates now, with a pristineRegExp.prototype.exec,Map.prototype.get,Promise.prototype.then.SafeMap,SafeSet,hardenRegExp'sOriginalRegExpPrototype*locals, andSafePromiseare all built from pristine constants — as intended, but not because the module is robust; because it loaded early.- Prelude returns. Test body runs
RegExp.prototype.exec = evilExec; Map.prototype.get = evilGet; .... const p = primordials.object→ getter →require("internal/primordials")→ cache hit from step 6.p.SafeMap,p.hardenRegExp,p.SafePromiseAllwere all built before step 7's tampering existed.- All assertions pass. But they would also pass if, hypothetically, the epilogue's
OriginalRegExpPrototypeExecwere changed to readRegExp.prototype.execinstead of$RegExpPrototypeExec— because at step 6 that was still the original.
Impact and fix
Nit — test-quality only, nothing user-facing breaks. The primordials module is correct today (it uses $Name constants), so the test passes for the right runtime reason; it just isn't the guard against future regressions its own comment says it is. The eager fs export is pre-existing code; only the false comment and the test that relies on it are new in this PR.
Fix: convert line 340 to a lazy getter matching the eight exposedInternals entries this PR already converted:
export const fs = {
get $data() { return require("node:fs/promises").$data; },
}.$data;or more simply, since fs is only consumed by tests, wrap it the same way as primordials.object:
let _fs;
export const fs = { get $data() { return _fs ??= require("node:fs/promises").$data; } };
// or just: export function fs() { return require("node:fs/promises").$data; }(whatever shape the existing test consumers of fs accept). Independently, the load-timing test should assert its precondition so a future eager-require reintroduction fails loudly rather than silently disarming the test.
|
|
||
| // ─────────────────────────────────────────────────────────────────────────────── | ||
| // Normalize the probe entries | ||
| // ─────────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| const entries = probeResult.entries; | ||
| for (const entry of entries) { | ||
| if (entry.kind !== "Value" || entry.valueType !== "object" || !entry.holder || !entry.key) continue; | ||
| // A constructor's `prototype` / a prototype's own object property that is itself a | ||
| // holder: reference the holder object directly rather than reading the property. | ||
| const target = | ||
| entry.key === "prototype" && entry.holder.endsWith("Constructor") | ||
| ? entry.holder.replace(/Constructor$/, "Prototype") | ||
| : entry.name in holders | ||
| ? entry.name |
There was a problem hiding this comment.
🟡 FunctionPrototype is the one XPrototype entry that escapes this normalization pass: per spec typeof Function.prototype === "function", so the probe records it as kind: "Method" (not kind: "Value", valueType: "object") and the guard on line 267 skips it. Directly visible in the checked-in .d.ts: declare const $FunctionPrototype: PrimordialMethod<FunctionConstructor, "prototype"> vs every peer ($ArrayPrototype: Array<any>, $MapPrototype: Map<any, any>, …). Runtime impact is nil today (static-Method emits identically to HolderSelf, the type resolves to Function, and Function.prototype is non-configurable), but it's the sibling-site inconsistency REVIEW.md flags — broaden the guard to also match entry.key === "prototype" && entry.holder.endsWith("Constructor") regardless of kind, then regenerate.
Extended reasoning...
What the issue is
The normalization loop at generate-primordials.ts:265-279 rewrites each constructor's prototype own-property entry from a property read into { kind: "HolderSelf", holder: "XPrototype" }, so $XPrototype is captured directly as the holder object rather than via XConstructor.prototype. The loop's guard is if (entry.kind !== "Value" || entry.valueType !== "object" || !entry.holder || !entry.key) continue;. That is correct for every constructor except Function, because %Function.prototype% is itself a callable (ECMA-262 §20.2.3: "is itself a built-in function object"), so typeof Function.prototype === "function".
The probe (generate-primordials-probe.js, copyOwnProperties) classifies own properties by typeof desc.value: "function" → kind: "Method". Scanning Function's own keys therefore records { name: "FunctionPrototype", holder: "FunctionConstructor", key: "prototype", kind: "Method", call: "static" }. The normalization guard sees kind === "Method" and continues, so this entry keeps holder: "FunctionConstructor" and is never promoted to HolderSelf on FunctionPrototype.
Observable in the checked-in artifacts
src/js/primordials.d.ts: declare const $FunctionPrototype: PrimordialMethod<FunctionConstructor, "prototype">; — every other $XPrototype is typed directly as its holder type ($ArrayPrototype: Array<any>, $ObjectPrototype: Object, $MapPrototype: Map<any, any>, $ArrayBufferPrototype: ArrayBuffer, …). The holders.FunctionPrototype.type = "Function" value at line 101 is not consulted for this declaration because entry.holder stayed "FunctionConstructor".
JSCPrimordialsTable.h (generated into WebKit): entries are grouped by entry.holder, so FunctionPrototype lands as V(FunctionPrototype, PROP("prototype"), Method) under JSC_FOREACH_PRIMORDIAL_FunctionConstructor — the only XPrototype entry captured via a property read on its constructor rather than V(FunctionPrototype, SELF, Self) under its own holder table.
src/js/internal/primordials.js: FunctionPrototype: $FunctionPrototype, — emitted via the Method/call === "static" branch, which happens to be byte-identical to what HolderSelf would emit.
Step-by-step
- Probe reaches
Functionin the intrinsic-constructors loop;copyOwnProperties(Function, "FunctionConstructor", "Function", "static"). Reflect.ownKeys(Function)includes"prototype";desc = { value: Function.prototype, writable: false, enumerable: false, configurable: false }.typeof desc.value === "function"(spec-mandated), so it takes thetype === "function"branch →record({ name: "FunctionPrototype", holder: "FunctionConstructor", key: "prototype", kind: "Method", call: "static", … }).- Normalization loop, line 267:
entry.kind !== "Value"is true →continue. Entry unchanged. .d.tsemitter,case "Method":type = holders["FunctionConstructor"].type = "FunctionConstructor"→PrimordialMethod<FunctionConstructor, "prototype">.- Header emitter groups by
entry.holder = "FunctionConstructor"→ theFunctionPrototyperow appears under the FunctionConstructor table with kindMethod, not under FunctionPrototype with kindSelf.
Why nothing breaks today
- The module's static-
Methodbranch emitsFunctionPrototype: $FunctionPrototype,, identical to whatHolderSelfwould emit. PrimordialMethod<FunctionConstructor, "prototype">resolves:FunctionConstructor extends Record<"prototype", Function>→Function extends (...args) => infer Ris false (the TSFunctioninterface has no bare call signature) → falls through to: Function, exactly whatholders.FunctionPrototype.typewould have emitted.Function.prototypeis{ writable: false, configurable: false }, soPROP("prototype")capture on the constructor is tamper-proof anyway.- %Function.prototype% is a real JSFunction, so any
jsCast<JSFunction*>a Method-kind capture path might do would succeed.
Why it's still worth fixing
REVIEW.md § Correctness: "Fix the whole class in the same PR … parallel switch arms". This is a sibling-site inconsistency: every other XPrototype in the manifest is a HolderSelf on its own holder; FunctionPrototype alone is a Method on FunctionConstructor. If JSCPrimordials.cpp ever gains Method-specific handling that differs from Self (e.g. an unavailable-placeholder path, or a jsCast<JSFunction*> assertion), $FunctionPrototype would silently diverge from every peer with no test flagging it.
Fix
Broaden the normalization guard so the prototype-of-a-Constructor rewrite fires regardless of the probe's typeof classification, e.g.:
for (const entry of entries) {
if (!entry.holder || !entry.key) continue;
const isCtorPrototype = entry.key === "prototype" && entry.holder.endsWith("Constructor");
if (!isCtorPrototype && (entry.kind !== "Value" || entry.valueType !== "object")) continue;
const target = isCtorPrototype
? entry.holder.replace(/Constructor$/, "Prototype")
: entry.name in holders ? entry.name : null;
…
}then regenerate all three artifacts.
…ials-link-time-constants Conflicts resolved: - scripts/build/deps/webkit.ts: kept the autobuild-preview-pr-341 pin (this PR depends on oven-sh/WebKit#341). - src/js/internal/primordials.js: kept the generated file; main's #36318 removed SafePromiseAll from the old hand-written module, which the generated module supersedes entirely.
The merge resolution took ours wholesale for webkit.ts, which dropped main's new webkitTestFFIPath export and broke the build script. Take main's file and keep only the WEBKIT_VERSION override.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
scripts/build/deps/webkit.ts:106-111— DeletingwebkitTestFFIPathhere strands its consumer:scripts/build/ci.tsstill hasimport { webkitTestFFIPath } from "./deps/webkit.ts"at line 29 and calls it at lines 288 and 426. A static named import of a non-existent export fails at module-link time, soci.tswill throw before any code runs and the CI build/upload pipeline breaks. Either restore the export or delete the import and bothif (existsSync(testFFI))blocks inci.tsin this PR (REVIEW.md § Correctness: "Signature changes and renames → grep the whole repo").Extended reasoning...
What the bug is
This PR's only change to
scripts/build/deps/webkit.ts(besides theWEBKIT_VERSIONbump) is the deletion of:export function webkitTestFFIPath(cfg: Config): string { const root = cfg.webkit === "prebuilt" ? prebuiltDestDir(cfg) : depBuildDir(cfg, "WebKit"); return resolve(root, "bin", cfg.windows ? "testFFI.exe" : "testFFI"); }
But
scripts/build/ci.ts— not touched by this PR — still statically imports and uses that function:ci.ts:29—import { webkitTestFFIPath } from "./deps/webkit.ts";ci.ts:288—const testFFI = webkitTestFFIPath(cfg);followed byif (existsSync(testFFI)) { … upload … }ci.ts:426—const testFFI = webkitTestFFIPath(cfg);followed byif (existsSync(testFFI)) { chmodSync(testFFI, 0o755); files.push(testFFI); }
A repo-wide grep confirms
webkitTestFFIPathnow appears only inci.ts— there is no definition anywhere.Why nothing prevents it
This is an ES static named import, not a namespace import or dynamic
require. When Bun evaluatesci.ts, module linking resolves each named import against the exporter's module record;./deps/webkit.tsno longer has an export binding namedwebkitTestFFIPath, so linking fails withSyntaxError: export 'webkitTestFFIPath' not found in './deps/webkit.ts'(or the equivalent Bun message) before line 1 ofci.tsexecutes. There is no fallback — the two call sites are behindexistsSyncguards, but the guard is on the return value, not on the binding, so it never gets that far.The build system doesn't type-check
scripts/at compile time as a merge gate the way it doessrc/, so this doesn't surface until CI actually runsscripts/build/ci.ts— at which point the whole build/upload/artifact-packaging step aborts.Step-by-step proof
- PR diff,
scripts/build/deps/webkit.tshunk: lines 106–110 are removed (-export function webkitTestFFIPath(cfg: Config): string { … }).grep -n 'webkitTestFFIPath\|testFFI' scripts/build/deps/webkit.ts→ no matches. scripts/build/ci.ts:29—import { webkitTestFFIPath } from "./deps/webkit.ts";(unchanged on this branch).- Bun (and any spec-compliant ESM loader) resolves named imports at instantiation: since
./deps/webkit.ts's module record has nowebkitTestFFIPathbinding, ResolveExport returns null and instantiation throws a SyntaxError. - Therefore invoking any entry point that evaluates
scripts/build/ci.ts(the BuildKite build step, artifact upload, packaging) throws immediately — neitherci.ts:288norci.ts:426is ever reached.
Impact
Concrete CI breakage introduced by this PR. The PR description already says "CI will fail on the prebuilt download until [the WebKit] autobuild completes", but this is a separate, permanent failure mode that will not resolve when the autobuild finishes —
ci.tswon't even reach the download step. Per REVIEW.md § Correctness: "Signature changes and renames → grep the whole repo including cfg-gated code and generated-binding inputs; stale call sites compile fine and silently miss the new behavior" — here the stale call site doesn't compile at all. And § Code style: "Delete dead code in the same PR that makes it dead" applies in reverse: deleting an export requires deleting (or updating) its callers atomically.Fix
Two valid options — do one, in this PR:
- Restore
webkitTestFFIPathinscripts/build/deps/webkit.tsif the testFFI upload is still wanted (it's guarded byexistsSync, so it's harmless when the binary is absent). - Remove the import at
ci.ts:29and the twoconst testFFI = webkitTestFFIPath(cfg); if (existsSync(testFFI)) { … }blocks atci.ts:288-292andci.ts:426-430, if the WebKit bump intentionally drops the testFFI artifact.
| `#define JSC_PRIMORDIAL_LAZY_TYPE_PROTOTYPE_HOLDER_${macroName} PrimordialHolder::${holder}Prototype\n#define JSC_PRIMORDIAL_LAZY_TYPE_CONSTRUCTOR_HOLDER_${macroName} PrimordialHolder::${holder}Constructor`, | ||
| ) | ||
| .join("\n")} | ||
|
|
||
| } // namespace JSC |
There was a problem hiding this comment.
🟡 The entry.call === "bound" branch emits ${entry.name}: directly, while every other arm of this emitter (Literal, HolderSelf/Value, Getter/Setter, uncurried Method, static Method, ApplyVariant) wraps the key via jsKey(entry.name). Only Promise constructor statics reach this branch today and all are valid identifiers, so the generated output is byte-identical either way — but if a future bound-receiver constructor gains a static whose primordial name needs quoting (as the RegExp legacy statics already do for the sibling Getter/Setter branches), this line would emit invalid JS. One-token fix: ${jsKey(entry.name)}:.
Extended reasoning...
What the issue is
In generate-primordials.ts's propertyLines emitter, the case "Method": block has three sub-branches:
if (entry.call === "uncurried") propertyLines.push(` ${jsKey(entry.name)}: uncurryThis(${constant(entry)}),`);
else if (entry.call === "bound")
propertyLines.push(
` ${entry.name}: $FunctionPrototypeBind.$call(${constant(entry)}, $${entry.holder!.replace(/Constructor$/, "")}),`,
);
else propertyLines.push(` ${jsKey(entry.name)}: ${constant(entry)},`);The "uncurried" and else (static) arms wrap the property key via jsKey(entry.name); the "bound" arm alone uses raw ${entry.name}. Every other entry.kind case in the surrounding switch — Literal, HolderSelf/Value, Getter/Setter, ApplyVariant — also wraps via jsKey(). jsKey is defined a few lines above as (name) => (/^[A-Za-z_$][\w$]*$/.test(name) ? name : JSON.stringify(name)): it quotes names that are not valid JS identifier property keys.
Why nothing breaks today
Only one probe loop feeds call: "bound" — for (const name of ["Promise"]) { ...; copyOwnProperties(G[name], ${name}Constructor, name, "bound"); } in generate-primordials-probe.js. The resulting entry.name values are PromiseResolve, PromiseReject, PromiseRace, PromiseAll, PromiseAllSettled, PromiseAny, PromiseWithResolvers, PromiseTry, PromiseLength, PromiseName, PromisePrototype — every one matches /^[A-Za-z_$][\w$]*$/, so jsKey(entry.name) === entry.name for all of them. The generated src/js/internal/primordials.js is byte-identical either way, and the checked-in file confirms this (e.g. PromiseResolve: $FunctionPrototypeBind.$call($PromiseResolve, $Promise),).
Why it's worth flagging anyway
This is precisely the pattern REVIEW.md § Correctness calls out: "Grep for every sibling site sharing the pattern: parallel switch arms". The jsKey() wrapper exists specifically because some Node primordial names are not valid unquoted property keys — the RegExp legacy static getters/setters ("RegExpGet$&", "RegExpGet$*", "RegExpGet$`", "RegExpGet$'", "RegExpGet$+") hit the sibling Getter/Setter branches and require quoting to emit parseable JS. If a future constructor is added to the bound-receiver loop (or Promise gains a static whose derived primordial name contains punctuation), this branch would emit RegExpGet$&: $FunctionPrototypeBind.$call(...) — a syntax error in the generated module — while every sibling branch would handle it correctly.
Step-by-step proof
jsKeyat generate-primordials.ts:~322: returnsJSON.stringify(name)iffnamefails/^[A-Za-z_$][\w$]*$/.case "Method":/entry.call === "bound"at ~407-410: emits` ${entry.name}: ...`— nojsKey().- Every peer arm (
"Literal"~397,"HolderSelf"/"Value"~400,"Getter"/"Setter"~403,"Method"/"uncurried"~406,"Method"/else ~411,"ApplyVariant"~425/426) emits` ${jsKey(entry.name)}: ...`. - Probe: only
["Promise"]usescopyOwnProperties(..., "bound");Reflect.ownKeys(Promise)yieldslength,name,resolve,reject,race,all,allSettled,any,withResolvers,prototype,try,[Symbol.species]→getNewKeyproduces identifier-safe names for all →jsKeywould be a no-op → generated output identical. - Hypothetical: an entry
{ name: "FooGet$&", kind: "Method", call: "bound", holder: "FooConstructor" }would emitFooGet$&: $FunctionPrototypeBind.$call(...), which is a JSSyntaxError; the same entry withcall: "static"would emit"FooGet$&": ...and parse.
Impact and fix
Nit — generator-only consistency, zero observable effect on any artifact this PR produces. One-token fix: change ${entry.name}: to ${jsKey(entry.name)}: in the "bound" branch, then regenerate (which will produce no diff).
oven-sh/WebKit#341 synced with WebKit main; autobuild-preview-pr-341-01de5fd4 is published with all 38 assets.
…ials-link-time-constants Brings in #36794 (Upgrade WebKit to 3722912ff800) with the AbstractModuleRecord/ModuleAnalyzer C++/Rust adaptations the synced WebKit preview now requires. Kept the autobuild-preview-pr-341-01de5fd4 pin for WEBKIT_VERSION.
- Restore Float16Array to the manifest: it is part of this engine's surface and internal/util/inspect indexes primordials by a typed array's Symbol.toStringTag, so its constructor must be a member. (The reference-realm oracle test now derives its constructor list from the probe, so the two can no longer drift.) - module epilogue: read `unsafe.prototype` and `items.length` into locals so each is accessed once (bun/no-duplicate-conditional-property-access). - Regenerated against the upgraded WebKit base, which also picks up the newly-added Iterator.prototype.includes builtin.
dylan-conway
changed the title
2 participants
What does this PR do?
Makes
internal/primordialsreal. It becomes Node.js's actualprimordialsobject — frozen, null-prototype references to the original built-ins that survive any prototype pollution or global replacement — so the ~16 builtin modules ported from Node'slib/(node:utilinspect,node:assert, errors, streams, worker messaging, quic, ...) keep working after user code tampers withArray.prototype,RegExp.prototype,Math,Error, and so on.Every value member is a
$Namelink-time constant materialized by JSC (from oven-sh/WebKit#341), so it is pristine no matter when the module loads — including if user code ran first. The member set and semantics are Node ≥ 25'slib/internal/per_context/primordials.js(~840 members): prototype methods and accessors are uncurried, statics are the raw functions, receiver-bound*Applyvariants take(argsArray)as Node's do, and spec constants are inlined. The helpers Node builds by hand —uncurryThis,applyBind,makeSafe, theSafe*collections, theSafePromisecombinators,hardenRegExp,SafeStringPrototypeSearch,SafeArrayPrototypePushApply— are ported from Node. Because Bun loads the module lazily rather than at context bootstrap, theSafe*classes are built from generated pristine descriptor records andhardenRegExpuses the rawRegExpconstants, so nothing reads a live object at load; Safe collections also hand out captured safe iterators.Generated, not hand-written.
src/codegen/generate-primordials.tsruns a probe of Node's construction algorithm inside a pristine build to learn where each member's pristine value lives, then emits three artifacts: JSC's per-holder entry table (JSCPrimordialsTable.h, in the WebKit PR),src/js/internal/primordials.js, andsrc/js/primordials.d.ts. Rerun it when JSC's builtin surface changes; a test fails if the checked-in artifacts drift.bun:internal-for-testingexposesprimordials.audit()(the engine's per-entry manifest) andprimordials.objectfor the tests below. Bumps WebKit to the paired build.How did you verify your code works?
test/js/bun/util/primordials.test.tsiterates the whole manifest rather than a sample: the engine table covers every holder with 0 unavailable entries; the module's member set equals Node's algorithm run live; a differential oracle rebuilds Node's object by value in a pristine realm and compares all ~840 members (presence, typeof, arity, literal values); every entry is identity-equal to the live builtin in untouched and pre-touched globals; direct, roundabout, and structural tampering of every entry — planted before and after first load and across realms — never leaks into a primordial; plus option-off placeholders, Workers, and the$Namebundler path. Ran locally on debug and release builds; existing consumer suites (vm, assert, util-inspect, worker_threads, readline) pass unchanged.Depends on oven-sh/WebKit#341.
[decide:webkit] gate passed · iteration 17 · 12 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 4 rejected · iteration 17
evidence per changed file