IntlLocale: throw RangeError (not TypeError) when ICU canonicalization fails on a structurally valid tag - #350
IntlLocale: throw RangeError (not TypeError) when ICU canonicalization fails on a structurally valid tag#350robobun wants to merge 1 commit into
Conversation
…n fails on a structurally valid tag initializeLocale() validates the input tag with isStructurallyValidLanguageTag before reaching either canonicalize() or toCanonical(). When those return failure the tag is well-formed BCP-47; the only failure mode is ICU's ULOC_FULLNAME_CAPACITY buffer overflowing on long (23+ variant) tags. canonicalizeLocaleList in IntlObject.cpp surfaces the same canonicalization failure as RangeError, so Intl.DateTimeFormat, Intl.NumberFormat, Intl.Collator, Intl.getCanonicalLocales and String.prototype.localeCompare all throw RangeError for the identical input while Intl.Locale alone threw TypeError. V8 also throws RangeError here. A RangeError is what user code's locale-validation catch guards check for, so the inconsistency made Intl.Locale uniquely slip past them.
|
Warning Review limit reached
Next review available in: 12 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward error-type swap for spec/V8 consistency.
What was reviewed:
- Both call sites:
throwRangeErrorhas the same signature asthrowTypeErrorand is used identically throughoutIntlLocale.cppand the rest ofruntime/. - Confirmed
canonicalizeLocaleListinIntlObject.cpp:821throws RangeError on the same failure path, so this bringsIntl.Localein line. - No control-flow change — both sites still
return;immediately after throwing.
Extended reasoning...
Overview
Two-line functional change in Source/JavaScriptCore/runtime/IntlLocale.cpp: two throwTypeError(...) calls in IntlLocale::initializeLocale become throwRangeError(...), plus explanatory comments. These sites fire when ICU canonicalization fails on a tag that has already passed isStructurallyValidLanguageTag (i.e., ICU's ULOC_FULLNAME_CAPACITY overflow). The equivalent path in canonicalizeLocaleList (IntlObject.cpp:821) already throws RangeError, and every other error branch in initializeLocale (invalid tag, invalid language/script/region/variants/calendar/etc.) also throws RangeError. This makes the two outliers consistent.
Security risks
None. No new inputs are accepted, no buffers are resized, no parsing logic changes. Only the constructor of the thrown error object differs. throwRangeError(JSGlobalObject*, ThrowScope&, ASCIILiteral) is a well-established helper used ~258 times across runtime/ with the same signature as throwTypeError.
Level of scrutiny
Low. This is a targeted error-type correction with a clear rationale (match other Intl entry points and V8) and no behavioral change on the happy path. The failure paths still return immediately after throwing. The added comments accurately describe why RangeError is correct here.
Other factors
- The PR description references a companion Bun PR that will add a regression test in
test/js/web/intl/intl.test.ts. - No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
- Bug-hunting system found nothing.
Preview Builds
|
2 participants
Problem
new Intl.Locale(tag)throws TypeError ("failed to initialize Locale") for a structurally valid BCP-47 tag that exceeds ICU's internalULOC_FULLNAME_CAPACITYbuffer during canonicalization. Every other Intl entry point (Intl.DateTimeFormat,Intl.NumberFormat,Intl.Collator,Intl.getCanonicalLocales,String.prototype.localeCompare) throws RangeError for the same input, becausecanonicalizeLocaleListinIntlObject.cppreports canonicalization failure as RangeError. V8 also throws RangeError here.So
catch (e) { if (e instanceof RangeError) /* invalid locale */ }misses only theIntl.Localeconstructor.Cause
IntlLocale::initializeLocalecallsisStructurallyValidLanguageTagat the top (throwing RangeError on failure), so by the timeLocaleIDBuilder::canonicalize()/toCanonical()run the tag is known to be well-formed. Their only failure mode is ICU's fixed-size buffer overflow. The two failure sites threwthrowTypeErrorwhile the equivalent path incanonicalizeLocaleListthrowscreateRangeError.Fix
Change both sites to
throwRangeError, matchingcanonicalizeLocaleListand V8.Branched from
549170099226f816(Bun's currentWEBKIT_VERSION) so the preview tarball is a minimal diff against what Bun links today. A companion Bun PR will bumpWEBKIT_VERSIONto this preview and add a regression test intest/js/web/intl/intl.test.ts.