URL: WebKit parser follow-ups + reuse input string for href, cache the last base URL - #39368
Conversation
…ion, host:port and IPv4 fast path, oven-sh/WebKit#454) No-Verification-Needed: version pin bump
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 69 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. 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 (1)
Comment |
|
Updated 4:50 PM PT - Aug 16th, 2026
✅ @Jarred-Sumner, your commit 2aa42536907649e6cc7b5be3e94cab0a33729607 passed in 🧪 To try this PR locally: bunx bun-pr 39368That installs a local version of the PR into your bun-39368 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — single-line WebKit version pin bump.
Checked that only WEBKIT_VERSION changed in scripts/build/deps/webkit.ts; no other logic touched. The new hash is a valid 40-char hex sha, so the autobuild-<hash> tag/URL derivation and the 16-char cache-dir slice both keep working. Substantive URL-parser changes live in oven-sh/WebKit#454 and were validated there (43-config CI, TestWTF, differential fuzz).
Extended reasoning...
Overview
This PR updates the WEBKIT_VERSION constant in scripts/build/deps/webkit.ts from 0cbb4a194653… to c6cfe90c6064…. It is a one-line change to a string literal that determines which prebuilt WebKit tarball is downloaded (or which commit is checked out in local mode). All actual code changes — SIMD table-lookup classification and the host:port/IPv4 fast path in URLParser — live in oven-sh/WebKit#454 and are not part of this diff.
Security risks
None from the Bun-side diff. The value is a git commit hash consumed only by the build system to construct a GitHub release download URL and a cache directory name. It is not user-controllable at runtime and does not flow into any parsing or network path in the shipped binary.
Level of scrutiny
Low for the Bun diff itself: this is a routine dependency version bump following the established pattern (see recent commits like #39273 which bumped the same constant). The value is a well-formed 40-hex sha, so prebuiltUrl prefixes it with autobuild- correctly and prebuiltDestDir's .slice(0, 16) still yields a distinct cache key. The substantive changes were reviewed, differentially fuzzed, and CI-tested across 43 configs in the WebKit repo; Bun CI will exercise the full integration once the prebuilt tarballs publish.
Other factors
No CODEOWNERS entry covers this path. No prior reviewer comments to address. The PR description explicitly notes the WebKit download step will fail until prebuilts publish (~1h), which is expected and gates merge on CI going green — a human does not need to re-verify the hash by hand.
…h#39368) Only Source/WTF/wtf/URLParser.cpp changed upstream (167+/42-); the FFI tree is untouched, so the OHOS symlink headers and perl pin carry over. Verified: URL suite 24/24, buffer 634/634, blob 104/104, fetch 363/363.
Jarred-Sumner
changed the title
…nycode without full ICU (#39468) ### What does this PR do? URL binding follow-ups on top of #39368 (the parser work lives in WebKit; these are the parts a standalone URL library can't do, plus the one place Node still beat us): - **`href` / `toString()` / `toJSON()` reuse the constructor's `JSString`** when the URL was already canonical — WebKit's parser result *is* the input `StringImpl` in that case — and otherwise cache the last `JSString` produced on the wrapper. Also makes `new URL(rel, urlObject)` cheaper (the base is stringified through it). - **Last-base cache on `JSVMClientData`**: `new URL(input, base)` / `URL.parse` / `URL.canParse` with a string base re-parsed and re-validated the base on every call; it is nearly always the same string (configured origin, request URL), so the last valid one is remembered per VM. - **Literal `xn--` labels judged without full ICU ToASCII** (`ASCIIHostPunycodeCheck.h`): `hasValidPunycodeHost()` ran `uidna_nameToASCII` (~600 ns) whenever a host contained a literal `xn--` label. For an all-ASCII host the only thing ToASCII can reject is an ACE label, so this mirrors `icu::UTS46::processLabel` for those: RFC 3492 decode with `u_strFromPunycode`'s failure rules → must be unchanged under the `uts46` normalizer → no U+FFFD → no leading combining mark. Labels that would then need the BiDi / CONTEXTJ rules still go to ICU, so verdicts are ICU's by construction. - Cheaper "does the host contain `xn--`" probe (was a generic substring search on every parse). - `bench/snippets/url-kinds.mjs`. | ns/op, same box | before | after | node 26 | |---|---|---|---| | `URL.canParse("https://xn--a.com/")` (invalid punycode) | 870 | **312** | 290 | | `URL.canParse("https://xn--ls8h.com/p")` (valid punycode) | 826 | **425** | 581 | | `new URL("../assets/logo.svg", pageUrlString)` | 401 | **263** | 838 | | `new URL("../assets/logo.svg", urlObject)` | 562 | **371** | 886 | | `new URL(u).href` / `.toString()` | 185 / 184 | **147 / 147** | 462 / 453 | ### How did you verify your code works? - The punycode check was differentially tested against ICU's `uidna_nameToASCII` (Bun's flags/allowed errors) on ~380k hosts — every IdnaTestV2/toascii input's ACE form, mutations of those, random `xn--` labels, mixed case: 0 disagreements (128k valid, 251k invalid, 19k deferred to ICU). - New `url.test.ts` cases: literal punycode verdicts (valid/invalid/uppercase/RTL/ZWJ/combining-mark/overflow, all cross-checked with Node 26), repeated/alternating/invalid string bases incl. error `.input`/`.base`, href/toString/toJSON consistency across mutation. - `test/js/web/url`, `test/js/node/url`, node `test-whatwg-url-*`, `test-url-*`, `test-whatwg-url-custom-domainto`, `test-url-domain-ascii-unicode` pass on a release build. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
What does this PR do?
WebKit bump →
c6cfe90c6064(oven-sh/WebKit#454): table-lookup (pshufb/tbl) SIMD classification in the URL scanners;host:portand dotted-quad IPv4 hosts stay on the parser's fast path. (oven-sh/WebKit#463 — SIMD scan for percent-encoded/Unicode hosts — is up separately and can be folded in once merged.)Bun-side, things a standalone URL library can't do:
new URL(s).href/toString()/toJSON(): when nothing needed rewriting, WebKit's parser result is the inputStringImpl, so return the constructor's ownJSStringinstead of allocating another; the lastJSStringproduced forhrefis cached on the wrapper either way (also makesnew URL(rel, urlObject)cheaper, since the base is stringified through it).new URL(input, base)/URL.parse/URL.canParsewith a string base re-parsed and re-validated the base every call; it is nearly always the same string (configured origin, request URL), so the last valid base is remembered per thread.hasValidParsedHost's "does the host containxn--" check ran a generic substring search on every URL; look for--instead.bench/snippets/url-kinds.mjs:new URL()/canParse/parseover realistic URL shapes.Same-machine, both non-LTO, ns per call (previous = the #452-era build):
new URL("http://localhost:3000/api/users/42")new URL("http://192.168.1.10:8080/metrics")new URL("http://[2001:db8::…]:8080/status")new URL(longCdnPath)/ 2 KB querynew URL("../assets/logo.svg", pageUrlString)new URL("?page=3", listUrlString)new URL("../assets/logo.svg", urlObject)new URL(u).href/.toString()URL.canParse(pathQueryHash)How did you verify your code works?
test/js/web/url(new cases for repeated/alternating/invalid string bases incl. error.input/.base, and href/toString/toJSON consistency across mutation),test/js/node/url, nodetest-whatwg-url-*/test-url-*pass on a release build against WebKitc6cfe90c+ #463.