Repository navigation
Conversation
…g with the method's this JavaScriptCore called the tag of super.tag`...` and super[key]`...` with the object the tag was found on (Base.prototype) instead of the receiver that super.tag() uses, and evaluated the key of super[key]`...` before the this TDZ check in derived constructors. oven-sh/WebKit#438 fixes TaggedTemplateNode::emitBytecode; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture. The fixture needs the jsc shell's drainMicrotasks, which the preload now provides from bun:jsc.
|
Warning Review limit reached
Next review available in: 18 minutes 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The Bun-side changes are mechanical (fixture registration, drainMicrotasks preload polyfill, pin bump), but this is a WebKit engine bump on a preview tag that the description says must be repointed to the merged main sha before landing, and it carries three other fork commits — a human should coordinate the land.
What was reviewed:
prebuiltUrl/prebuiltDestDirinscripts/build/deps/webkit.tsalready handleautobuild-preview-*tags (no truncation, correct release URL).jsc.drainMicrotasksexists inbun:jsc(BunJSCModule.h), so the new preload line resolves.- The new fixture's
// @bunfirst line is compatible withparseJSCFlags(skipped before the//@break check), and thefailures-then-throw structure means anyshouldBemiss produces a nonzero exit.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION to a preview build of oven-sh/WebKit#438 (fixing super.tag\...`to call the tag with the method'sthisinstead of the super base) and adds the corresponding JSTests/stress fixture to Bun's jsc-stress suite. Bun-side changes: one-line pin edit inscripts/build/deps/webkit.ts, a new 347-line fixture file, one line registering it in jsc-stress.test.ts, and a one-line drainMicrotaskspolyfill inpreload.js`.
Security risks
None. No user-facing API surface, no auth/crypto/network code. The fixture is a self-contained JS file run in a subprocess with no filesystem or network access.
Level of scrutiny
The Bun-repo diff itself is low-risk and mechanical — I'd approve it in isolation. But the substance of the change is a JavaScriptCore engine bump, and per the PR description this preview pin (a) must be replaced with the merged main sha before landing, and (b) also carries three unrelated WebKit fork commits (#420 vm.Script reuse, #437 threadsafe FFI callback teardown, #421 i64/u64 FFI args) whose Bun-side PRs are separate. Engine bumps and their landing order are a maintainer decision.
Other factors
- The build system already handles
autobuild-preview-*tags:prebuiltUrldoesn't double-prefixautobuild-,prebuiltDestDiruses the whole tag as the cache key, anddownload.tsprints a targeted hint if the preview release disappears — so the pin format is fine. - The fixture is thorough (dot/bracket, static/instance, object literals, arrows, eval, field initializers, generators/async, TDZ ordering, JIT-tier loop, and a negative-control group for non-super tags) and structured so any failing
shouldBeaccumulates into a final throw → nonzero exit → test failure. The description documents fail-before/pass-after and a full JSTests/stress diff between the two engine builds. parseJSCFlagsinjsc-stress.test.tsexplicitly skips the// @bunline before its//@break, so the fixture having no//@ run*directive just yields an empty env (correct — it runs the default variant).- No prior human or bot reviews to address; CI is still building.
|
Agreed on the landing order. To spell it out for whoever lands this:
|
|
oven-sh/WebKit#534 (the |
…fixture oven-sh/WebKit#534 now also carries oven-sh/WebKit#438's commit, which fixes the this of super.tag`...`: both rewrite the same two branches of TaggedTemplateNode::emitBytecode and conflicted. The preview moves to autobuild-preview-pr-534-492e8dd6, and the jsc-stress fixture of #38920 (tagged-templates-super-this.js, its registration and the drainMicrotasks preload global it uses) comes along.
Problem
super.tag\x`inside a method callstagwiththis= the object the property was found on (Base.prototypefor a class, the[[Prototype]]for an object literal method) instead of the receiver.super.tag()in the same method gets the receiver, and so does node; the spec gives both forms the samethis(EvaluateCall step 1.a.i: GetThisValue of a Super Reference). Same forsuper[key]`x``.super()yet,super[key()]\x`evaluatedkey()before throwing the ReferenceError for the uninitializedthis;superkey()and a plainsuper[key()]` read throw first (node does too).bun build --no-bundleprints the tagged template unchanged and the same code misbehaves throughnew Function. The cause is in JavaScriptCore,TaggedTemplateNode::emitBytecode(Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp): one register,base, serves both as the object the tag is looked up on and as the call'sthis. For a super reference it already looked the property up with the right receiver, then movedbase(the home object's prototype) into the call'sthisregister. The super call nodes (FunctionCallDotNode,FunctionCallBracketNode) keep the two apart. Upstream WebKit has the same code..bind(this)and is unaffected); there is no user report. It affects every plain JS/TS class or object literal method that forwards to an inherited tag this way.Fix
...and super[key]...with the method's this WebKit#438: the two super branches ofTaggedTemplateNode::emitBytecodenow do what the call nodes do,ensureThis()(which is also thethisTDZ check) first, then the super base for the lookup, and the call receives thethisregister instead of the super base. Non-super tags emit the same bytecode as before.thisvalue is by definition the enclosing method'sthis(that is what makes it a Super Reference); the lookup base and the call receiver are different objects, and the engine already used the right receiver for the property read, only the call used the wrong one. Matches node.WEBKIT_VERSIONat that PR's preview build (autobuild-preview-pr-438-31f493c1) so CI runs Bun against it. Before landing, TaggedTemplateNode: call super.tag...and super[key]...with the method's this WebKit#438 has to merge andWEBKIT_VERSIONhas to be repointed at the resulting main sha (the build prints that exact instruction once the preview release disappears).f0f60fd2pin it also carries three fork commits that have their own Bun PRs: Let an embedder run a program from an UnlinkedProgramCodeBlock it already holds WebKit#420 (vm.Script compile reuse, vm.Script: compile the source once and link that compile in every context it runs in #38040), [JSC] bun:ffi: a threadsafe JSFFICallback's entry thunk stays callable after its cell and VM are gone WebKit#437 (threadsafe FFI callback teardown, Worker teardown, round 3: node:vm timeout on TerminationDeadline; take-at-landing termination; exit/streams/serve/valkey/Bun.build fixes #38660) and FFI: convert Number arguments of i64/u64 parameters modulo 2^64 WebKit#421 (i64/u64 FFI argument conversion, bun:ffi: convert numbers passed to i64/u64 parameters modulo 2^64 in cc() and the engine #38091). Nothing here depends on them; the existing jsc-stress FFI fixtures pass with this pin, and whichever bump lands first carries them.test/js/bun/jsc-stress/fixtures/tagged-templates-super-this.js, theJSTests/stressfile from the WebKit PR with the usual// @bunfirst line (so Bun hands the source to JSC verbatim, like the other fixtures; without it the transpiler rewritessuper["tag"]tosuper.tagand drops the tag function's own"use strict"), registered injsc-stress.test.ts. It coverssuper.tagandsuper[key](variable, string literal, symbol and index keys) in instance methods, through an intermediate class, static methods, object literal methods (also called through an inheriting object), arrow functions, direct eval, instance and static field initializers, generator and async methods, and a derived constructor aftersuper(); a getter on the super base; the template object, raw strings, substitution values and per-site template object caching; primitive receivers from a strict and a sloppy method, each compared againstsuper.tag(); a replaced super base; the TDZ ordering (neither key nor substitutions run, tag never called); tags that are not super references keeping theirthis; and atestLoopCountloop through the JIT tiers.jsc-stress/preload.jsgainsdrainMicrotasks(frombun:jsc), which the async case uses exactly as it would in the jsc shell (Bump WebKit: allow using declarations in a function nested in a switch case clause #38285 adds the identical line; whichever lands second gets a trivial overlap).f0f60fd2),bun bd test test/js/bun/jsc-stress/jsc-stress.test.ts -t tagged-templates-super-thisfails with 18 of the fixture's 20 groups reporting the prototype asthis(plusevaluated: key,keyfor the TDZ group); the 2 non-super groups pass. The same file passes on node v26 (async group checked separately, node has no synchronous microtask drain).bun bd test test/js/bun/jsc-stress/jsc-stress.test.tspasses all 116 fixtures (debug + ASAN build; the new fixture takes about 0.7s there).JSTests/stressfiles, default variant, on thejscshells of thef0f60fd2and preview release tarballs (details in the WebKit PR). The only deterministic differences are the new test (fail to pass) and 4 FFI tests that belong to the carried FFI: convert Number arguments of i64/u64 parameters modulo 2^64 WebKit#421; everything else that differed between the two runs was load timeouts or concurrent-JIT timing and gives the same result on both engines when rerun alone. Non-super tagged templates compile to the same bytecode as before by construction (thisValuestays null on those paths), and the suite agrees.Background
scripts/build/deps/webkit.tspins which build. An engine fix lands as a WebKit PR plus a pin bump here, andautobuild-preview-pr-*tags let a bump PR run Bun's suite against the WebKit PR before it merges.test/js/bun/jsc-stress/runs files taken verbatim from WebKit'sJSTests/stressunderbun;preload.jssupplies the jsc shell globals they use (noInline,testLoopCount, nowdrainMicrotasks), which is why the engine test can be shared with the WebKit PR as is.super.xlooksxup on the home object's prototype, but a Super Reference remembers the method's ownthisas the receiver, which is what getters and calls through it get.f\...`is a call offwith the template object and the substitution values as arguments, and the spec computes itsthisexactly as forf(...): foro.fit iso; forsuper.fit is the enclosing method'sthis, while the property itself is looked up on the home object's prototype (Base.prototype`). JSC's bytecode generator has separate AST nodes for calls and tagged templates, and only the call nodes handled the super case.ensureThis()in JSC's bytecode generator returns the register holding the function'sthis, after checking it is initialized when the code is a derived constructor (wherethisis in a TDZ untilsuper()returns). Emitting it before the key expression is what fixes the evaluation order.Repro