fix(js): pin the ICU default locale to navigator.language - #742
Open
ntdatt812 wants to merge 2 commits into
Open
fix(js): pin the ICU default locale to navigator.language#742ntdatt812 wants to merge 2 commits into
ntdatt812 wants to merge 2 commits into
Conversation
Intl resolved its default through ICU, which nothing in the engine set, so it reported whatever locale the V8 build carried. navigator.language, navigator.languages and the Accept-Language default were all pinned to en-US already, so a page reading Intl next to either of the others saw a browser disagreeing with itself. The report measured en-AU against en-US on a Windows release build. Pin ICU once, before the first isolate exists and under the isolate creation lock, from a single DEFAULT_LOCALE constant that names why the three surfaces have to agree. The host default is not reproducible across machines: on Linux ICU already answers en-US, so a test that only asserts the surfaces agree passes with or without the fix. The regression test forces a different default first and asserts the engine still names its own, which is safe because nextest runs each test in its own process. Fixes h4ckf0r0day#734.
V8 resolves the ICU default locale once per isolate and caches it at the first Intl use, so a pin that lands after a page has formatted anything silently does nothing. The test now formats, moves the host default a second time underneath the warmed isolate, and formats again, asserting both the reported locale and en-US number grouping. Raised by yinnho on h4ckf0r0day#734.
Author
|
Obstacle course, for the CONTRIBUTING pre-PR item I could not tick from a unit test. Companion repo at the ref your CI pins (
No delta. The single failure is the same stage on both, and it fails on untouched So it is a property of this host rather than of the change, and I have not chased it further since it is outside the diff. It is deterministic rather than flaky here: re-running just that stage with Everything else in this PR is unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #734.
What changed
Three surfaces name a locale, and only two of them were pinned:
navigator.languageis"en-US"andnavigator.languagesis["en-US", "en"], both hardcoded injs/bootstrap.js.obscura-netsendsAccept-Language: en-US,en;q=0.9.Intlresolves its default through ICU, and nothing undercrates/ever set one.grep -rn "set_default_locale" crates/was empty.So the third value was a property of whichever V8 build you happened to be running, not of the browser identity Obscura presents. The report measured
en-AUfromIntlagainsten-USfromnavigator.languageon a Windows release build. A page that reads two of the three sees a browser disagreeing with itself, which is the kind of internal inconsistency identity checks look for, so this sits in the stealth priority the README states.The disagreement being one-sided is what decides the fix: two surfaces are already pinned, one is inherited, so agreeing means pinning the third rather than loosening the other two. That also keeps what scripts read consistent with what actually goes out on the wire.
One constant and one call, in
crates/obscura-js/src/runtime.rs:DEFAULT_LOCALEnames the value and, in its doc comment, the two other places that have to stay equal to it.pin_default_locale()callsv8::icu::set_default_localeinside aOnce.It is called from
with_base_url_and_proxybeforeJsRuntime::new, inside the existingISOLATE_CREATE_LOCKblock. Both parts matter: ICU is consulted when a page first constructs anIntlobject, so an isolate built ahead of the call would answer from the host default, and the lock keeps the write from racing an isolate being constructed on another connection thread. That is the onlyJsRuntime::newsite in the crate, so CLI and CDP both go through it.A note on the constant, which does not need an answer before merging
DEFAULT_LOCALEduplicates a string that also lives inbootstrap.jsand inobscura-net. I kept it that way because threading one constant across a Rust crate boundary and a JS bundle is a larger change than this bug warrants, and the doc comment records the coupling so the next person to change one has a pointer to the others. If you would rather have a single source, tell me where you want it to live and I will send that separately.Validation
The interesting part of this one is that the obvious test does not work.
A test that just asserts the three surfaces agree passes on
main, unfixed, because ICU on a Linux runner already defaults toen-US. The reported default was a Windows build's. A host default is not reproducible across machines, so asserting agreement proves nothing about whether the engine pins anything.test_intl_default_locale_is_pinned_not_inherited_from_the_hosttherefore forces a different default first and asserts the engine still names its own. That is safe because nextest runs each test in its own process, so the global never reaches a sibling test.Red on this branch with the
pin_default_locale()call commented out, which is the exact shape of the reported bug, three inherited values next to two pinned ones:Green with the call restored.
@yinnho raised a V8 subtlety on the issue that the test now encodes: V8 resolves the ICU default locale once per isolate and caches it at the first
Intluse, so a pin landing after a page has formatted anything silently does nothing. Their literal suggestion, format then change the configured language then format again, does not translate directly because Obscura has no runtime language switch. The half that does translate is that the pin has to survive the host default moving underneath a warmed isolate, so the test now formats, moves the host default a second time, and formats again in the same isolate.That second block is load-bearing, and what it reports without the pin is a direct measurement of the caching they described. Host default forced to
fr-FR, isolate built and warmed, host default then moved tode-DE:The warmed isolate answers
fr-FR, notde-DE, and formats with French narrow-no-break-space grouping. It never saw the second change. That is the cache, and it is whypin_default_localeruns beforeJsRuntime::newrather than anywhere later. With the pin in place both surfaces readen-USand the number formats as1,234.5.Since this changes shared JS-runtime code, I ran the whole four-configuration sequence from CONTRIBUTING rather than only
obscura-js, in arust:latestcontainer so the result matches CI rather than my Windows host:cargo build --release -p obscura-cli --bins --features rendercargo nextest run --release --features render --no-fail-fastcargo build --release -p obscura-cli --bins --no-default-featurescargo nextest run --release -p obscura --no-fail-fastcargo nextest run --release --workspace --exclude obscura --exclude obscura-render --no-default-features --no-fail-fastcargo check --release -p obscura-render --no-default-featuresI have not run
cargo fmtover the tree, per CONTRIBUTING; the added lines are rustfmt-clean on their own.Rendering
Not applicable. No layout, paint, screenshot, screencast or PDF code is touched.
Performance
The call is inside a
std::sync::Once, on the isolate-construction path, already underISOLATE_CREATE_LOCK. It runs once per process and adds nothing per request, per page, or per script. There is no change to any hot path.Checklist