Skip to content

fix(web): sanitize unparseable browser locale before engine bootstrap - #370

Merged
Catrya merged 4 commits into
MostroP2P:mainfrom
Matobi98:fix/227-web-locale-crash
Sep 7, 2026
Merged

fix(web): sanitize unparseable browser locale before engine bootstrap#370
Catrya merged 4 commits into
MostroP2P:mainfrom
Matobi98:fix/227-web-locale-crash

Conversation

@Matobi98

@Matobi98 Matobi98 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Part of #227: on web, an unparseable browser locale (the POSIX C locale, an underscored en_US, or empty) crashed the app before runApp with a blank page and nothing logged.

Root cause, pinned down with a full stack trace (the original issue narrowed it to "somewhere in early main()" but not further):

RangeError: Incorrect locale information provided
at new Locale ()
at Object.EnginePlatformDispatcher_parseBrowserLanguages (main.dart.js:2494:21)
at Object.EnginePlatformDispatcher$ (main.dart.js:2471:14)
at CanvasKitRenderer.initialize$0 (main.dart.js:73965:16)

This is inside the Flutter web engine's own bootstrap (CanvasKitRenderer.initialize), not in this app's code — it fires before Firebase.initializeApp or RustLib.init() ever run. That rules out both of the issue's other suspects and means no app-level try/catch around main() can guard it; the fix has to happen before the engine reads the locale at all.

Fix

Added a small inline script to web/index.html, before flutter_bootstrap.js loads (same pattern already used there for coi-serviceworker.min.js): it validates navigator.language/navigator.languages with Intl.DateTimeFormat (the same check that throws inside the engine), normalizes underscored tags (en_USen-US), and falls back to en-US for anything still invalid (C, empty). Note POSIX is not one of those: unlike the single-letter C, it is a structurally valid five-letter language subtag, so Intl accepts it and it passes through untouched — correctly, since it never crashed anything. If the shadowing itself is refused by the browser, the script warns and leaves the locale alone rather than throwing. By the time the engine reads it, the browser only ever reports a valid tag.

Testing

Manual, against a real release bundle, both directions — with navigator.language(s) shadowed to C before the sanitizer runs, since Chrome's DevTools rejects C in its locale override field ("Locale must contain alphabetic characters"):

  • Without the fix → blank page, RangeError: Incorrect locale information provided in the console, flutter-view never created.
  • With the fix → navigator.languages reports ["en-US"] and the app renders.
  • Same result for "" (empty locale).
  • Note: real browsers never report a broken tag like C themselves (they self-sanitize), which is why this only shows up in CI/headless/embedded environments — matches the "low severity" framing in the issue.

Automated regression guard: smoke.mjs now reads the locale from SMOKE_LOCALE (defaults to en-US, unchanged). web-build.yml reruns the release-bundle smoke test with SMOKE_LOCALE=C right after the existing one. Verified locally against a real bundle (build-web.sh --release + flutter build web --release, Flutter 3.38.2), stripping only the sanitizer <script> from the built index.html between runs so both sides come from the same build:

en-US C
with sanitizer passes fully passes fully
without sanitizer passes fully Flutter view never mounted

"Passes fully" means all four smoke assertions, Rust bridge call included.

Acceptance criteria (from #227)

  • Exact call site identified and recorded (commented on the issue with the full stack trace)
  • App starts and renders with locale forced to C, en_US†, and "" — verified against a real release bundle; the CI step only exercises C, since Playwright normalizes the other two before the page sees them
  • No unhandled exception escapes main() for a bad locale — it's normalized before the engine ever sees it
  • Firebase.initializeApp was already unaffected — the crash happens earlier, before that line runs
  • CI case loads the release bundle under an invalid locale and asserts the view mounts
  • locale: 'en-US' pin in smoke.mjs kept for determinism, documented as no longer load-bearing for this bug

en_US gets normalized to en-US by the sanitizer like any other case; a separate Accept-Language: en_US fetch failure I hit while testing that one specific tag turned out to be a pre-existing Playwright/Chromium quirk unrelated to this fix (reproduces identically with the sanitizer removed) — not covered by this PR.

An unparseable navigator.language (POSIX "C"/"POSIX", underscored
"en_US", or empty) crashes the Flutter web engine's own bootstrap
(EnginePlatformDispatcher_parseBrowserLanguages, called from
CanvasKitRenderer.initialize) with an uncaught RangeError before
runApp — an unrecoverable blank page with nothing logged. The crash
is inside the engine's compiled JS, before Firebase.initializeApp or
RustLib.init() ever run, so no app-level try/catch can guard it.

Sanitize navigator.language/languages in web/index.html, before
flutter_bootstrap.js loads: normalize underscored tags, fall back to
en-US for anything Intl.DateTimeFormat still rejects. By the time the
engine reads it, the browser only ever reports a valid tag.

Add a CI regression case: web-build.yml reruns the release-bundle
smoke test with the locale forced to "C" (smoke.mjs now honors
SMOKE_LOCALE). Verified both directions locally against the real
bundle — fails without the fix, passes with it.

Fixes MostroP2P#227
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 1f8e7066-c565-43e8-a022-c62ce7216483


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Catrya
Catrya previously approved these changes Sep 2, 2026

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I reproduced the crash end to end on the Flutter version CI pins, confirmed the fix removes it, and confirmed the new CI step genuinely fails without it. The diagnosis in the description is correct down to the frame.

End-to-end A/B (Flutter 3.38.2, Chromium 141)

Same built bundle in both runs — the only difference is whether the sanitizer script is present in index.html, stripped at serve time — with Playwright forcing locale: 'C':

sanitizer present sanitizer removed
navigator.languages "en-US" "C"
flutter-view mounted yes no
RangeError: Incorrect locale information provided no yes
<body> children flt-semantics-placeholder, script, script, flt-announcement-host, flutter-view script, flt-announcement-host

Two runs each, identical results. Without the fix the engine dies mid-bootstrap and the flutter-view element is never created — the page is not merely empty, the view never exists. That last row is also why the new CI step is not decorative: smoke.mjs asserts flutter-view mounts and that no page errors occurred, and both fail here.

Why this only reproduces on the pinnedWorth recording, because it nearly sent me pt showed no crash at all — I was buildingwith Flutter 3.35.7. The engine changed bet

  • 3.35.7: parseBrowserLanguages does lds ui.Locale in pure Dart.
    ui.Locale('C') validates nothing and neve
  • 3.38.2 (FLUTTER_VERSION in all threomLocale(language), and DomLocaleis declared@js('Intl.Locale') (engine/src/gine/dom.dart:2448-2450). That is exactly the new Locale ()` frame in you

Measured in Chromium 141: new Intl.Locale(orrect locale information provided — the
message verbatim. So this is a regression ie, not by this app, and it is invisible to
anyone still on 3.35.

One consequence for the CI guard: what makee change. A line in the workflow comment
saying so — the guard tests Intl.Locale, ince 3.38 — would keep it from quietly
becoming a no-op if that parser ever change

The validity oracle matches the engine'

The sanitizer validates with Intl.DateTimeFormat; the engine throws from Intl.Locale. I checked they agrsince a mismatch is how this fix could sile POSIX, en_US, en-US, c, zz, "", und, x, en-US-POSIX, both accept or both reject, all ten. The oracle is sound. ## Minor- **POSIX is not an invalid tag, and the * Measured: Intl.Locale('POSIX'),Intl.DateTimeFormat('POSIX') and Intl.gell succeed — it is a structurally validfive-letter language subtag. With Playwrighnavigator.languagesis still["POSIX"]after the script runs. That is the correct behaviour, since it never crashed anything, but the description says thesanitizer "falls back to en-US for anythi, empty)", which is wrong about POSIX.Issue #227 carries the same imprecision.- Two of the three acceptance-criteria ta Playwright, so the CI guard really onlycovers C. Measured:

locale "C"      -> the browser reports "Cs intended)
locale "en_US"  -> the browser reports "en-US"   (Playwright normalizes before the page sees it)
locale ""       -> the browser reports "e is used)

The footnote gets close for en_US but a; the simpler fact is that the underscoredtag never reaches the page at all. Worth stating so nobody reads those two criteria as guarded.

  • **Neither Object.defineProperty call issValidBcp47is; the writes are not. If abrowser ever refused to shadownavigator.language`, the inline script would throw — before the engine, and in
    exactly the embedded/kiosk environments thit's "no page errors" assertion would fail on it. Cheap hardening for a script whose whole job is to run in hostile environments.

Also verified

  • Script ordering in the built bundle, serviceworkerat offset 1426, sanitizer at 2060,flutter_bootstrap.jsat 3717. The sine script and the bootstrap isasync`, soit always runs first.
  • The repo's own web guard still passes.dart→ 16/16, including the assertion that the isolation shim precedesflutter_bootstrap.js. - **Full suite on the merged tree, built wi **293 passed, 0 failed**; flutter analyze` → zero issues in hand-written code. Goldens included.

Not verified

                                                                                                        - **Screenshots do not distinguish the two  because my bundle has no Rust core(`web/pkg` needs `wasm-pack`, which I did n engine boots but `main()` then fails at`RustLib.init()` and paints nothing. The DOve is the evidence; a real side-by-sidepicture would need the wasm core.
  • The repo's actual smoke.mjs could nits mostroBridgeReady assertion needs theRust bridge. I used a targeted checker over the same bundle instead, serving it cross-origin isolated under /app/ as the real one does.
  • The "" and en_US cases, per the Playwright limitation above.

…ening

Catrya's review of MostroP2P#370. No behavior change to the sanitizer itself beyond
never throwing.

- Guard both Object.defineProperty writes with try/catch. A browser may
  expose navigator.language(s) as non-configurable; an uncaught error there
  would be the blank page this script exists to prevent, in exactly the
  locked-down environments it targets. Both writes share the block so a
  partial shadow cannot leave language and languages disagreeing.
- Correct the POSIX claim. "POSIX" is a structurally valid five-letter
  language subtag: Intl accepts it and it passes through untouched, unlike
  the single-letter "C". Verified both oracles (Intl.DateTimeFormat, used
  here, and Intl.Locale, which the engine throws from) agree on all of
  C/POSIX/en_US/en-US/c/zz/""/und/x/en-US-POSIX.
- Record that the CI guard only exercises "C": Playwright normalizes the
  locale before the page sees it, so "en_US" arrives as a valid "en-US" and
  "" falls back to the default.
- Record that the crash only exists on Flutter >= 3.38, where
  parseBrowserLanguages moved from a pure-Dart ui.Locale that validated
  nothing to Intl.Locale — so the guard silently stops testing anything if
  FLUTTER_VERSION moves below it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011kR2ZS83zniZMXankK1Fry
@Matobi98

Matobi98 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Review round 1 — pushed in e90568a. Thanks for the measurements, they caught
a real imprecision and a real hole.

1. try/catch around both Object.defineProperty calls — the only behavior
change. Both writes share one block, so a partial shadow can't leave
language and languages disagreeing; the failure path degrades to a
console.warn. warn rather than error on purpose: smoke.mjs:175 only
records error-type console messages, so a failed shadow won't fail the
bundle test for something that isn't a bundle defect. Also added an early
return when there is nothing to change, so a valid locale never attempts the
write at all.

2. POSIX corrected. You're right, and I reproduced it: Intl.Locale,
Intl.DateTimeFormat and my isValidBcp47 all accept POSIX and all reject
C, en_US, "", c and x — agreement on all ten tags you listed. The
comment no longer lists POSIX among the broken tags and now says explicitly
that it passes through untouched, which is the correct behavior. The PR
description above is corrected too; #227 carries the same imprecision and
should be edited.

3 & 4. Both recorded in the workflow comment: that Playwright normalizes
the locale before the page sees it, so the CI step only exercises C while
the sanitizer covers all three; and that the crash only exists on
Flutter >= 3.38, so if FLUTTER_VERSION ever moves below it the step passes
for the wrong reason.

Verified by hand

The app was run in a real Chrome (flutter run -d chrome with the COOP/COEP
headers, so the sanitizer is the one this PR adds) and driven through a full
trade against a local regtest (Polar + mostrod + relay), with mostro-cli as
the maker: take → buyer invoice → seller pays the hold → fiat-sent → release,
settling in LND (invoice SETTLED, hold payment SUCCEEDED, 12966 sats). The
whole flow behaves exactly as it does without this branch. With a valid
browser locale the sanitizer is a no-op, which is the intended behavior, and
nothing it emits appeared in the console — so the script sits in front of the
engine bootstrap without costing anything on the normal path.

The broken-locale case was also reproduced by hand, though not in a browser
that genuinely reports one: forcing it through Chrome's DevTools doesn't work
(the Sensors locale field rejects C with "Locale must contain alphabetic
characters"), so I served the release bundle with navigator.language(s)
shadowed to C before the sanitizer runs. Blank page and RangeError: Incorrect locale information provided without the fix, app renders with it.
A faithful simulation of the container/kiosk browser, not the browser itself.

Verified by script

You noted your bundle had no Rust core, so I ran the A/B you couldn't: a real
build-web.sh --release + flutter build web --release under the pinned
3.38.2, then the repo's own smoke.mjs against that bundle, stripping only
the sanitizer <script> from the built index.html between runs — same
build on both sides.

en-US C
with sanitizer passes fully passes fully
without passes fully Flutter view never mounted

Full pass means all four assertions, Rust bridge call included. One red cell,
and it's the one the guard exists for.

Independently, the browser-side script was exercised against C, en_US,
"", POSIX and es-AR, with defineProperty both permitted and forced to
throw: before this commit the hostile case threw an uncaught error, now it
warns and continues.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested — light. The diagnosis is right, the fix is at the only layer that can work, and I reproduced the differential against a real bundle. What needs to change is the issue linkage and two lines in the sanitizer.

The root cause is confirmed at source level

Not just from the stack trace — this is Flutter 3.38.2's own SDK, flutter_web_sdk/lib/_engine/engine/platform_dispatcher.dart:916:

static List<ui.Locale> parseBrowserLanguages() {
  final List<String>? languages = _browserLanguagesOverride ?? domWindow.navigator.languages;
  if (languages == null || languages.isEmpty) { return const <ui.Locale>[_defaultLocale]; }
  for (final String language in languages) {
    final DomLocale domLocale = DomLocale(language);   // ← throws here

and dom.dart:2443:

@JS('Intl.Locale')
extension type DomLocale._(JSObject _) implements JSObject {
  external DomLocale(String tag, [DomLocaleOptions? options]);

It runs from EnginePlatformDispatcher's field initializer, i.e. engine bootstrap, before runApp. Your conclusion that no app-level try/catch can reach it is correct, and the HTML shim is the only lever. Good call.

One thing that falls out of this and matters below: the engine reads navigator.languages only. It never touches navigator.language.

Reproduced against a real release bundle

I built the branch (build-web.sh --release + flutter build web --release --base-href "/app/" --pwa-strategy=none, Flutter 3.38.2) and ran your smoke.mjs four times, stripping only the sanitizer <script> from build/web/index.html between runs so both sides come from one build:

SMOKE_LOCALE=en-US SMOKE_LOCALE=C
with sanitizer ✓ isolated, ✓ view mounted ✓ isolated, ✓ view mounted
without sanitizer ✓ isolated, ✓ view mounted ✓ isolated, ✗ the Flutter view never mounted + RangeError: Incorrect locale information provided

Exactly one of the four dies earlier than the others, and it is the cell your table predicts. The differential holds.

Caveat, stated plainly: my bundle had no web/pkg — building the wasm core here needs a rustup component I did not want to install on this machine — so smoke assertion 3 (the Rust bridge) fails in all four runs. It is a constant across the matrix and does not affect the locale comparison, but it means I could not independently confirm your "passes fully, Rust bridge call included". CI is green on your side, which covers it.

The Intl.DateTimeFormat proxy is sound

Worth recording, since the sanitizer validates with Intl.DateTimeFormat while the engine parses with Intl.Locale, and nothing guarantees those agree. I checked 33 tags — grandfathered (i-klingon, zh-min-nan, art-lojban, en-GB-oed, cel-gaulish, sgn-BE-FR), extensions (en-US-u-ca-gregory, ja-JP-u-ca-japanese), private use (qaa-Qaaa-QM-x-southern, x-private), malformed (en-, -en, en US, C.UTF-8, en_US@posix, *, 12, ab-CDE, en-a-bbb-a-ccc):

divergences between Intl.DateTimeFormat and Intl.Locale: 0

Both are gated by the same structural validity check in ECMA-402, so this is not a coincidence that could drift. No gap.

And the no-op path does what it claims — measured in Chromium: en-US, POSIX and es-419 come out byte-identical, no defineProperty attempted.

Blocking 1 — the atomicity comment is not true

From round 1:

// Both writes go together so a partial shadow can't leave language and
// languages disagreeing.

It can. With navigator.languages exposed as non-configurable — the exact case the try/catch exists for:

before: { lang: 'C',     langs: ['C'] }
after : { lang: 'en-US', langs: ['C'] }
warn  : TypeError: Cannot redefine property: languages

The first defineProperty has already applied when the second throws, and the catch cannot undo it. So the comment states a guarantee the code does not provide.

The ordering makes it worse rather than harmless: languages is the one the engine reads, and it is attempted second. In the partial-failure case the fix does nothing and leaves the two properties disagreeing — the worst of both. Two options, either is fine:

  • write languages first, then language — then a partial failure has already fixed the crash and only leaves language stale; or
  • check Object.getOwnPropertyDescriptor(navigator, ...).configurable for both before writing either, and bail if either is locked.

Then the comment matches the code.

Blocking 2 — "Fixes #227" with two acceptance criteria unimplemented

The issue lists six. Two are not met on this branch:

  • "No unhandled exception escapes main(); a locale that cannot be used falls back to a default and is logged rather than thrown" — there is no top-level guard. This locale is normalized before it can throw, which is not the same thing.
  • "Firebase.initializeApp cannot take down startup for any reason"lib/core/app_bootstrap.dart:53 is still on UnsupportedError catch (e), unchanged. The AC asks for hardening, not for a verdict on whether Firebase caused this particular crash; the checkbox is ticked with reasoning that answers the second question.

The issue is explicit that this is the part that matters most — "2. Make startup survivable regardless of where it turns out to be. ... The second is the one that generalises." As it stands, merging auto-closes #227 with its generalising half undone.

I'd rather not see it folded in here: the sanitizer is a clean, self-contained change and the startup guard is a different concern. So: change the body to "Part of #227" and open a follow-up for the top-level guard + the Firebase catch, or say in the issue why those two are being dropped.

Suggestions

  1. pages_bundle_test.dart doesn't guard the script order. That file exists for exactly this class ("Every one of these, when wrong, yields a blank page — guarded statically"), and it already asserts shimAt < bootstrapAt. One more index comparison would cover the sanitizer, and unlike the CI step it cannot pass for the wrong reason.

  2. The CI locale step has no negative control. Your own comment admits it can go green for the wrong reason if Flutter changes. selftest.mjs states the principle already — "The healthy fixture is the control: it is what makes a failure in the other two mean 'the error was detected'". A second run against a copy of build/web with the <script> stripped, expecting exit 1, makes the guard self-validating. That is exactly the run I did by hand above, and it works.

  3. SMOKE_LOCALE uses process.env.SMOKE_LOCALE || 'en-US', so SMOKE_LOCALE='' silently becomes en-US — the empty-locale case can't be expressed through the knob its name advertises. ?? fixes it.

One thing you got right that's worth recording

The issue told you to add the case to selftest.mjs's CASES table. That would have been wrong — selftest.mjs runs against three static HTML fixtures with no Flutter engine in them, so a locale case there would prove nothing. Putting it in web-build.yml against the real bundle is the correct place. Worth a line in #227 so the next person doesn't "fix" it back.

What I verified

  • The crash site in the Flutter 3.38.2 SDK sources, including that the engine reads navigator.languages and never navigator.language.
  • 4 smoke runs on a real release bundle, both locales × with/without the sanitizer.
  • 33-tag agreement between Intl.DateTimeFormat and Intl.Locale.
  • Sanitizer behaviour in Chromium for C, c, POSIX, es-419, en-US, plus the non-configurable-languages case.
  • That Playwright's locale: 'C' genuinely reaches the page as navigator.language === 'C' — so the CI step exercises the real broken state — and both of your caveats: en_US is normalized to en-US before the page sees it, and '' falls back to the system locale.
  • The sanitizer survives flutter build web into build/web/index.html verbatim, with script order coi-serviceworker → sanitizer → flutter_bootstrap.js, so the "shim must stay first" constraint and pages_bundle_test.dart both still hold.
  • All three CI checks green on e90568a.

…ontrol

Blocking 1: the sanitizer's try/catch was not the transaction its comment
claimed. If the first defineProperty applied and the second threw, the
first could not be undone — and since the engine reads navigator.languages
only, a partial shadow both left the two properties disagreeing and failed
to fix the crash. Check that neither is a non-configurable own property
before writing either, and bail out with a warning if one is locked.
Nothing can rescue that case, but the fix no longer half-applies.

Suggestions:

- web/index.html carries a <!-- locale-sanitizer --> marker and
  pages_bundle_test.dart asserts shim < sanitizer < flutter_bootstrap.js.
  Unlike the CI step, a static check cannot pass for the wrong reason;
  verified it fails when the script is moved after the bootstrap.
- web-build.yml runs a negative control after the invalid-locale smoke
  test: the same bundle with the sanitizer stripped must fail under
  locale "C". Without it that step goes green whether or not the fix is
  present or still needed. The strip is asserted to have happened, so a
  no-op cannot turn the control into a rubber stamp.
- smoke.mjs uses `??` instead of `||` for SMOKE_LOCALE, so the empty
  locale — one of the three broken tags — can actually be expressed
  instead of silently becoming en-US.

Part of MostroP2P#227
@grunch
grunch requested a review from Catrya September 7, 2026 13:36
grunch
grunch previously approved these changes Sep 7, 2026

@grunch grunch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — strict pass, verified on the merged tree

Verdict: ✅ Approve. No critical or high findings. Two medium findings on the new CI control step, one suggestion that would close the "CI only covers C" gap for real, two nits. None of them changes the sanitizer's behaviour, which is correct.

What I verified

  • Merge with main (0ce3d14) is clean; the four files are untouched on main.
  • test/web/pages_bundle_test.dart on the merged tree: 17/17, including the new ordering guard (shim → sanitizer → flutter_bootstrap.js).
  • CI on 09b4859: all four checks green, including the new positive step and the negative control, so the control genuinely fails without the sanitizer today.
  • The app reads locales from the sanitized source only. The Dart side derives its language from WidgetsBinding.instance.platformDispatcher.locales (settings_provider.dart:144), which the engine populates from navigator.languages. There is no findSystemLocale() / Intl.systemLocale path reading the raw navigator.language behind the sanitizer's back — I grepped for it.
  • Round-2 changes hold up. isLockedDown on both properties before either write closes the partial-shadow hole from round 1; the residual try/catch is now reachable only for a non-extensible navigator (Object.preventExtensions), where the first defineProperty throws before applying, so no partial write is possible there either. The comment above it could say that in one line instead of reading as belt-and-braces for the case it just excluded — nit, no change required.
  • No CSP in web/index.html, so the inline script is not blocked; nothing in the change touches lib/src/rust/ or the bridge surface.

Findings (ranked)

# Severity Where Summary
1 ⚠️ Medium web-build.yml:244 The control treats any non-zero exit as "died of the locale bug". Grep the log for the RangeError signature.
2 ⚠️ Medium web-build.yml:244 The control burns the full 120 s TIMEOUT_MS on every PR (measured: 2 min 1 s vs ~1 s for the positive steps). Cap it with SMOKE_TIMEOUT_MS.
3 💡 Suggestion smoke.mjs:173 addInitScript can shadow navigator.languages before the sanitizer runs, covering en_US, "" and mixed lists in CI — the Playwright locale limitation is not a browser limitation.
4 🔧 Nit index.html:89 Object.freeze the shadow array to match the spec-shaped navigator.languages.

Findings 1 and 2 are worth doing before merge because they are small and the control is the part of this PR most likely to rot silently; 3 can be a follow-up. I'm approving now so the choice is the author's.

Recorded for #227

Agree with the earlier round that "Part of #227" is the right linkage: the top-level main() guard and the app_bootstrap.dart:53 Firebase catch remain open there.

Comment thread .github/workflows/web-build.yml Outdated
Comment thread .github/workflows/web-build.yml
Comment thread test/web/smoke/smoke.mjs
Comment thread web/index.html Outdated
…hadow

Three findings from grunch's review of 09b4859. None changes the sanitizer's
behaviour on any locale.

1. The negative control accepted any failure as the expected one. It only
   checked the exit code, and smoke.mjs exits non-zero for five unrelated
   reasons — plus the ones where Chromium never launches or the port is
   taken — so the control could report "failed as expected" while proving
   nothing about this bug. It now captures the run's output and requires the
   RangeError signature in it, failing the job when the bundle dies of
   anything else. Verified against a real release bundle: the signature is
   present, so the control has been failing for the right reason; the risk
   was future rot, not a current hole.

2. That control burned the full 120s TIMEOUT_MS on every PR, waiting on a
   flutter-view element the crash guarantees will never appear. Capped at
   20s for this step only — the positive steps keep the generous default,
   which they need to instantiate the wasm core on a cold runner. Measured:
   121s -> 21s.

3. The shadow array is now frozen, matching the HTML spec's guarantee for
   navigator.languages. Both assignments, since the empty-result branch is
   the one the "C" case actually uses.

grunch's third finding — shadowing navigator.languages via addInitScript so
CI can cover en_US and "" as well, which Playwright's locale option cannot
reach — is left as a follow-up, as he suggested.

Verified: pages_bundle_test.dart 17/17; smoke.mjs against a real release
bundle (build-web.sh --release + flutter build web --release, Flutter
3.38.2) passes fully under both en-US and C with the sanitizer, and the
control fails with the RangeError without it.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants