Repository navigation
Conversation
Point WEBKIT_VERSION at the preview build of oven-sh/WebKit#614, which merges upstream WebKit main at ccdcb8a026 into the fork. Upstream 64168e4d90 adds GlobalObjectMethodTable::moduleTypeIsAllowed ahead of moduleLoaderImportModule, so the five method tables get a nullptr slot for it. The new test pins four engine fixes from the range and the host-defined import attribute path.
The serialized bytecode of the three corpus libraries that contain async functions and generators (happy-dom, undici, libraries.js) changed with upstream 4fa7b55ae4, which stops emitting tail calls in generator and async function bodies. CI produced the same three hashes on all eleven platforms, which is the update-the-snapshot case the test describes.
|
Warning Review limit reached
On-demand reviews are free for the next 10 days. After that, they cost $0.25 per reviewed file. Or wait 17 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Comment |
|
Status Reproduced with bun 1.4.3 against Node 26: // TZ=America/New_York bun repro.js ; TZ=Pacific/Apia bun repro.js ; same with node
function rt(t){var d=new Date(t);return new Date(d.getFullYear(),d.getMonth(),d.getDate(),d.getHours(),d.getMinutes(),d.getSeconds(),d.getMilliseconds()).getTime()}
[-8.64e15, -8.64e15+36e5, 8.64e15, 8.64e15-18e6].forEach(function(t){
var d=new Date(t);
console.log(t, new Date(t).setMilliseconds(d.getMilliseconds()), new Date(t).setHours(d.getHours()), rt(t)); // all four columns must be equal
});bun 1.4.3, The engine fix is oven-sh/WebKit#615, now rebased on oven-sh/WebKit The branch is stacked on #42177, because bun needs that PR's embedder changes to build against a WebKit past #614. Only the last commit is this PR's. Merge order: #42177, then oven-sh/WebKit#615, then this PR with the pin moved to the merge commit. CI on 93787d3 (build 114066): 180 of 181 jobs pass, and the new test passes on every platform. The one red job is |
![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. Since it bumps the WebKit prebuilt — and to a preview tag that, as the description notes, must be swapped for the merged oven-sh/WebKit#615 SHA before landing — a human should gate the merge.
What was reviewed:
- The round-trip test: each
check()writes a unique key intoresults/expectedand the aggregate.toEqualcovers every setter, the multi-arg constructor, and the ±1 ms NaN boundary — no vacuous or overwritten entries. process.env.TZhandling:setTimeZoneasserts the epoch offset so a zone that didn't take effect fails loudly, andafterAllrestores/deletes the original value.- The east-of-Greenwich block:
offset / msPerHouris an integer for Tokyo/Apia/Kiritimati atmaxTimeValue, so thesetHoursidentity holds; the-5 * msPerHourstarting point keeps the pre-set value in range for all three.
Extended reasoning...
Overview
This PR changes two files. scripts/build/deps/webkit.ts moves WEBKIT_VERSION from the pinned commit dfd696443b9b… to autobuild-preview-pr-615-c1a60940, the autobuild artifact of oven-sh/WebKit#615. test/js/bun/jsc/date-local-time-utc-conversion.test.ts is new: it exercises the JSC-side fix that PR carries by round-tripping the ten time values nearest ±8.64e15 ms through the multi-argument Date constructor and every local setter across 13 IANA zones (via in-process process.env.TZ), then pins zone-specific edge cases for New York's local-mean-time offset at the minimum bound and Tokyo/Apia/Kiritimati at the maximum, plus Date.parse with an explicit UTC offset.
Security risks
None identified. The test is pure in-process Date arithmetic with no network, no filesystem writes beyond the harness, no subprocesses, and no user-controlled input parsing. The WebKit pin change is a build-time artifact selector; the security-relevant question is provenance of the prebuilt, which is why the preview tag must become a merged SHA before landing.
Level of scrutiny
High, and human-gated. WebKit is the JavaScript engine — any bump changes execution semantics for every JS program Bun runs. The repo's own vendoring guidance (.claude/docs/landing-prs.md) is explicit: never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds); swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor first. The PR author already flags this, so the defer is about ensuring a human confirms the swap and the cross-platform prebuilt availability rather than raising a new objection.
Other factors
The test file is well-placed alongside the existing webkit-upgrade-*.test.ts files in test/js/bun/jsc/. It follows the aggregated-.toEqual convention from test/CLAUDE.md, uses describe.each/test.each, has no sleeps/timeouts/hardcoded ports, and restores the TZ global in afterAll. I checked that each check() label is unique per timeValue (no silent overwrites in the results map), that the fields.slice(...) spreads land on the right setter overloads, and that offset / msPerHour is integral for the three chosen east-of-Greenwich zones so setHours receives whole hours. The PR description states 10 of 43 tests fail on bun 1.4.3 and pass on the bumped build, satisfying the "fails for the right reason" bar. No CODEOWNERS entry covers these paths.
|
Agreed on the merge gate. Once oven-sh/WebKit#615 lands I move |
Its tree is identical to the preview build CI ran against (autobuild-preview-pr-614-3db1d92e).
…ar the time value limits use the zone's own offset (WebKit bump for oven-sh/WebKit#615) Pin WebKit to the preview build of oven-sh/WebKit#615 and add a test. JSC remapped a local time value outside the time value range to a year between 2008 and 2035 before it asked ICU for the UTC offset, so a local setter or the multi-argument constructor on a valid Date within one UTC offset of either end of the range was off by the difference between the two offsets, or NaN.
84c51a2 to
93787d3
Compare
|
The diff grew to 7 files because this branch is now stacked on #42177. oven-sh/WebKit#615 had to be rebased past oven-sh/WebKit#614, and bun needs the embedder changes of #42177 to build against that WebKit. Five of the seven files are #42177's and are reviewed there. This PR's own change is still the last commit only: the pin in |
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "dfd696443b9ba87c4f516d1c724f0abacdd8768a"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-615-8030fb83"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-615-8030fb83, an ephemeral preview-PR release tag; once oven-sh/WebKit#615 merges or closes that tag is garbage-collected and every fresh build (CI cold cache, new contributor, bun run clean cache) 404s downloading the WebKit prebuilt. Fix: before merge, swap the pin to the merged upstream SHA (40-hex commit on oven-sh/WebKit main) whose autobuild-<sha> release carries every platform × flavor artifact — never land a preview-pr-* tag on main. The PR description acknowledges this ("Do not merge before oven-sh/WebKit#615 lands"), but the pin as committed is the ephemeral shape.
Extended reasoning...
prebuiltUrl() in scripts/build/deps/webkit.ts:68-75 builds https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-615-8030fb83/bun-webkit-<os>-<arch><suffix>.tar.gz. Preview-PR autobuild releases on oven-sh/WebKit are transient — they exist only while the PR is open and are deleted after merge/close. On the base branch the pin is the merged SHA dfd696443b9ba87c4f516d1c724f0abacdd8768a, whose autobuild-<sha> release is permanent. After this PR lands and #615 is merged upstream, bun run build on any machine without a warm $BUN_INSTALL/build-cache/webkit-preview-pr-615-8030fb83* extraction hits a 404 in download.ts and the build aborts. test/internal/source-lints/webkit-prebuilt-url.test.ts:122-124 explicitly accepts autobuild-* tags, so no automated check blocks this. The repo's dependency-bump rule (landing-prs.md → Dependencies & vendoring) forbids merging pins to unmerged-PR builds for exactly this reason.
Verification: normal — acknowledged in diff: the PR description states "The pin is a preview tag, not a merge commit. Do not merge before oven-sh/WebKit#615 lands. I move the pin to the merged commit then." That flags the hazard but does not resolve it; the code as it stands is what merges. scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from the 40-hex merged SHA… | normal — acknowledged in PR…
There was a problem hiding this comment.
Agreed, and intended: this thread stays open as the merge blocker. The preview pin is what lets CI here exercise the engine change while oven-sh/WebKit#615 is in review. After #42177 and oven-sh/WebKit#615 land I rebase on main, push the 40-hex merge commit here, check that its autobuild-<sha> release carries every os, arch and debug/lto/asan tarball, and resolve this thread with that push.
Stacked on #42177 (WebKit upgrade to ccdcb8a026). Until that merges, the diff also shows its commits. Only the last commit belongs to this PR:
scripts/build/deps/webkit.tsandtest/js/bun/jsc/date-local-time-utc-conversion.test.ts.Problem
Datesetter, the multi-argumentDateconstructor andDate.parseof an offset-less string give a shifted time value orNaNfor a valid Date within one UTC offset of either end of the time value range (±8.64e15 ms).TZ=America/New_York:d = new Date(-8.64e15 + 36e5); d.setHours(d.getHours())movesdby 56 min 2 s andnew Date(-8.64e15).setMilliseconds(0)isNaN.TZ=Pacific/Apia: every local setter on the last 13 hours of the range isNaN. Node 26 returns the identity, as the note on UTC(t) requires.TZ=UTCis not affected, and the getters were already right.DateCache::DSTCache::localTimeOffset()(vendor/WebKit/Source/JavaScriptCore/runtime/JSDateMath.cpp) remapped a local time value outside the range to the same month and day in a year between 2008 and 2035 before it asked ICU for the offset, because its offset cache cannot hold a time outside the range. So New York got EDT -4:00 instead of its local mean time -4:56:02, and Apia got its 2008 offset -11:00 instead of +13:00.Fix
autobuild-preview-pr-615-8030fb83, the preview build of [JSC] Date: convert a local time to UTC with the zone's own offset when the local time is outside the time value range WebKit#615. That is oven-sh/WebKitmain(4b9ff9959a, the pin of Upgrade WebKit to ccdcb8a026 #42177) plus one commit:localTimeOffset()asks ICU directly for a time outside the range, the V8 date parser path goes through the same guardedlocalTimeToMS()as the setters, and WTF's own parser no longer clips the local value before it subtracts the offset.mainand move the pin to the merged commit.test/js/bun/jsc/date-local-time-utc-conversion.test.ts(new, 43 tests). It round-trips the ten time values nearest the limits through the constructor and every local setter in 13 zones, and pins the New York, Tokyo, Apia and Kiritimati edge cases and theDate.parseforms. bun 1.4.3 fails 10 of the 43,bun bd testwith this pin (debug + ASAN) passes all.v8-date-parser.test.js,cron-local-time.test.ts, thesetTimeZonetest inbun-jsc.test.tsandtest-datetime-change-notify.jspass with the pin.Background
t + offset(t)into fields, edits one, recomposes a local time valuel, and storesTimeClip(l - offset(l)). Fortnear a limit,lis past it by up to the offset.UCalendarbehind a small interval cache ported from V8. For an instant before a zone's first rule ICU returns the zone's local mean time, which is why dates that old have offsets like -4:56:02. V8 asks ICU the same way, without the remap.process.env.TZ, which bun applies to the running process, and restores the original value at the end.Notes
autobuild-preview-pr-615-c1a60940, built on the previous WebKit pin (dfd696443b). Upgrade to upstream WebKit ccdcb8a026 WebKit#614 then merged with upstream 320788@main, which adds the same ±1-day narrowing guard that revision carried. [JSC] Date: convert a local time to UTC with the zone's own offset when the local time is outside the time value range WebKit#615 is now rebased on it and keeps only what upstream does not have. bun cannot build against a WebKit past SegmentationFault (26 or big number): fetch().then() #614 without the embedder changes of Upgrade WebKit to ccdcb8a026 #42177 (GlobalObjectMethodTablegained a slot), so this branch sits on top of that PR.main(4b9ff9959a), the JSC stress test of [JSC] Date: convert a local time to UTC with the zone's own offset when the local time is outside the time value range WebKit#615 fails in the same 8 zones (constructor and setters) as bun 1.4.3 does here, so the upgrade of Upgrade WebKit to ccdcb8a026 #42177 alone does not fix this.cron-parse.test.ts"impossible day/month (Feb 30) returns null quickly" asserts a 50 ms wall clock bound and takes about 155 ms on a local debug + ASAN build (6 ms on release 1.4.3). The same run showed thatBun.cron.parse("0 0 30 2 *", new Date(8.64e15 - 86400000 * 400), { tz: "UTC" })does not return on a build with the SegmentationFault (26 or big number): fetch().then() #614 WebKit. That comes from upstream's change tocomputeGregorianDateTime, not from this PR, and is tracked separately for Upgrade WebKit to ccdcb8a026 #42177.JSTests/stress/date-local-time-utc-conversion-at-time-value-limits.jsunder four option sets, test262built-ins/Date,annexB/built-ins/Date,intl402/Dateandintl402/DateTimeFormatin six zones, the mozilla Date tests and theJSTests/complextime zone tests, all compared against the jsc shell of WebKitmain.Dateagainst Node 26 across seven non-UTC zones. Only the local-to-UTC direction differed, and only within 14 hours of either end of the range.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_bytecode_portable.test.ts