Skip to content

fix(build): emit ASCII-only bundles so doctrenderer's no-ICU V8 can parse them - #81

Merged
chrip merged 2 commits into
mainfrom
fix/80-terser-ascii-only
Aug 27, 2026
Merged

fix(build): emit ASCII-only bundles so doctrenderer's no-ICU V8 can parse them#81
chrip merged 2 commits into
mainfrom
fix/80-terser-ascii-only

Conversation

@chrip

@chrip chrip commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Terser was configured with neither format.ascii_only nor format.quote_keys, so the webpack pipeline emitted the LaTeX symbol table's supplementary-plane ("astral") keys as bare identifiers -- 131 of them per bundle in word/ and slide/ sdk-all.js, alongside 43348 raw non-ASCII bytes.

doctrenderer/x2t execute these bundles on a V8 built with v8_enable_i18n_support=false (core: Common/3dParty/v8/tools/8.9/*/nc-build.sh). Without ICU its reduced Unicode tables do not classify astral characters as ID_Start, so it rejects the entire bundle with "SyntaxError: Invalid or unexpected token". ScriptCompiler::Compile then returns an empty MaybeLocal and an unguarded ToLocalChecked() aborts the process -- the "Fatal error in v8::ToLocalChecked / Empty MaybeLocal" and "Illegal instruction (core dumped)" that make Euro-Office 9.3.4 fail on first start and on every document save.

The Closure Compiler build this replaced escaped all non-ASCII to \uXXXX, so the ASCII-only invariant held implicitly. The webpack migration dropped it silently, and no gate noticed: every JS check in check-build.yml runs on Node and headless Chromium, both full-ICU, which parse these bundles happily.

  • format.ascii_only restores the invariant.
  • format.quote_keys is redundant with ascii_only today but pins the behaviour against a future Terser bump. It must sit inside format; Terser rejects a top-level quote_keys outright with "quote_keys is not a supported option".
  • Two unit tests assert the options and that Terser escapes astral keys rather than emitting them bare.
  • A CI step asserts the emitted bundles contain no byte > 0x7F, which is the only gate here capable of catching this class of regression.

Verified against the v9.3.4 release image: all eight bundles rebuild with zero non-ASCII bytes and zero bare astral keys, x2t -create-js-cache exits 0 with all four caches written (previously aborted leaving a 0-byte cache), and documentserver-generate-allfonts.sh completes cleanly.

Fixes #80

Assisted-by: ClaudeCode:claude-opus-5

…arse them

Terser was configured with neither format.ascii_only nor format.quote_keys, so
the webpack pipeline emitted the LaTeX symbol table's supplementary-plane
("astral") keys as bare identifiers -- 131 of them per bundle in word/ and
slide/ sdk-all.js, alongside 43348 raw non-ASCII bytes.

doctrenderer/x2t execute these bundles on a V8 built with
v8_enable_i18n_support=false (core: Common/3dParty/v8/tools/8.9/*/nc-build.sh).
Without ICU its reduced Unicode tables do not classify astral characters as
ID_Start, so it rejects the entire bundle with "SyntaxError: Invalid or
unexpected token". ScriptCompiler::Compile then returns an empty MaybeLocal and
an unguarded ToLocalChecked() aborts the process -- the "Fatal error in
v8::ToLocalChecked / Empty MaybeLocal" and "Illegal instruction (core dumped)"
that make Euro-Office 9.3.4 fail on first start and on every document save.

The Closure Compiler build this replaced escaped all non-ASCII to \uXXXX, so
the ASCII-only invariant held implicitly. The webpack migration dropped it
silently, and no gate noticed: every JS check in check-build.yml runs on Node
and headless Chromium, both full-ICU, which parse these bundles happily.

- format.ascii_only restores the invariant.
- format.quote_keys is redundant with ascii_only today but pins the behaviour
  against a future Terser bump. It must sit inside `format`; Terser rejects a
  top-level quote_keys outright with "`quote_keys` is not a supported option".
- Two unit tests assert the options and that Terser escapes astral keys rather
  than emitting them bare.
- A CI step asserts the emitted bundles contain no byte > 0x7F, which is the
  only gate here capable of catching this class of regression.

Verified against the v9.3.4 release image: all eight bundles rebuild with zero
non-ASCII bytes and zero bare astral keys, `x2t -create-js-cache` exits 0 with
all four caches written (previously aborted leaving a 0-byte cache), and
documentserver-generate-allfonts.sh completes cleanly.

Fixes #80

Assisted-by: ClaudeCode:claude-opus-5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
@chrip
chrip requested a review from a team as a code owner August 27, 2026 09:21
@chrip
chrip requested review from Alex-Arsys, MonaAghili and moodyjmz and removed request for a team August 27, 2026 09:21
@moodyjmz

Copy link
Copy Markdown
Member

Review

TL;DR: Approve with reservations. The shipped fix is correct — I reproduced the build with the exact pinned toolchain (terser@5.49.0, terser-webpack-plugin@5.6.1, webpack@5.108.4) and confirmed ascii_only: true + quote_keys: true together produce fully-quoted output, resolving #80. But the PR's own reasoning about why it works is backwards, and as a direct consequence the new behavioral regression test can't fail and the CI gate can't detect the specific failure mode it's meant to guard against.

Findings

1. The code comment has the mechanism backwards. build/webpack.sdk.factory.mjs:314-320 claims ascii_only alone "forces astral keys to be quoted and escaped" and calls quote_keys "redundant." Built through the actual pinned webpack+terser-webpack-plugin+terser pipeline:

ascii_only only        → \u{1d538}:"\\doubleA"    ← BARE identifier (ASCII text, but unquoted)
ascii_only+quote_keys  → "\u{1d538}":"\\doubleA"  ← quoted, as shipped

quote_keys is the load-bearing fix, not decoration. \u{1d538} as a bare IdentifierName still resolves to the astral code point for ID_Start classification — a no-ICU V8 rejects it the same as the raw UTF-8 byte. The comment, if trusted later, invites deleting the option that's actually doing the work.

2. The regression test that's supposed to catch this can never fail. build/test/webpack-sdk-terser-options.test.cjs:135-141 calls terser.minify(src, terserOptions) directly, bypassing terser-webpack-plugin's ecma injection entirely. Ran it standalone: with or without quote_keys, the output comes back quoted, because terser's default (no ecma set) behaves conservatively. So this test would still pass even if quote_keys were deleted from the real config tomorrow.

3. CI's grep gate is blind to this exact residual mode. .github/workflows/check-build.yml:161-166 greps for bytes > 0x7F. \u{1d538}: is pure ASCII text — confirmed grep -P '[^\x00-\x7F]' reports clean on it. The config-presence unit test (terserOptions.format.quote_keys === true, lines 118-126) does correctly catch an outright deletion of the option, so this isn't a hole today — but the intended behavioral coverage is not exercising the real failure mode.

4. A second, untouched Terser pass exists. build/scripts/deploy-assets.cjs:86-91 minifies Native/*.js, libfont/engine/* (the fonts_*.js files #80 itself names as concatenated by doctrenderer), spell/*, hash/*, zlib/engine/* — no ascii_only/quote_keys, outside the CI glob. ASCII-clean today only incidentally (the sole non-ASCII content is Cyrillic in comments, stripped by comments: false). Not broken now, but nothing prevents it from becoming the next #80.

5. Minor CI hardening. [ -e "$f" ] || continue makes an unmatched glob a silent pass; grep -qP's exit 2 (grep error) is indistinguishable from "no match."

Checked and not problems: format block is unconditional across module/platform; quote_keys correctly nested inside format (top-level is rejected by Terser); terserOptionsOf() reads the live config object; require('terser') resolves the same pinned version the build uses; CI glob/output-path/chunk-naming line up; grep -P is available on ubuntu-latest; dev builds skip Terser with no hazard.

Suggestion before merge: correct the comment at webpack.sdk.factory.mjs:314-320, and set ecma: 2020 (or derive it from the same source terser-webpack-plugin uses) in the terser.minify() call at webpack-sdk-terser-options.test.cjs:135 so the test actually exercises the code path it's named for.

… coverage gaps

Review of #81 (thanks @moodyjmz) found the mechanism written into the config
comment was backwards, and that both new gates were consequently unable to
detect the failure they exist for. Verified each point against the real
pipeline before changing anything.

quote_keys is the load-bearing option, not the redundant one. Built with the
pinned toolchain and only ascii_only, this pipeline emits 131 keys of the form

    \u{1d552}:"\\doublea"

which contains no byte > 0x7F yet is still a bare astral identifier, and the
no-ICU V8 rejects it exactly as it rejects the raw UTF-8 spelling. Measured
against the v9.3.4 image: ascii_only alone still aborts x2t and leaves a 0-byte
sdk-all.cache; ascii_only + quote_keys writes a valid 5.3 MB cache. Both options
stay, with the comment now describing what actually happens.

- The behavioral test called terser.minify() without `ecma`, a path the real
  build never takes: Terser's default is conservative and quotes astral keys
  regardless, so the test passed with or without quote_keys. It now pins
  ecma to what terser-webpack-plugin injects, and a companion test asserts the
  bare key *does* reappear without quote_keys, so the guard cannot go blind
  again. Confirmed by deleting quote_keys from the config: 4 tests now fail,
  including the behavioral one, where previously none did.

- The CI gate greped for bytes > 0x7F and so was blind to the escaped form.
  Replaced with build/scripts/check-bundle-ascii.cjs, which checks both
  invariants (no raw non-ASCII, no bare astral key in raw or \u{...} form),
  reports GitHub annotations, and fails when the glob matches nothing instead
  of silently passing. The test suite imports its regexes so the two cannot
  drift apart.

- deploy-assets.cjs ran a second, separate Terser pass over Native/*.js and
  libfont/engine/fonts_*.js -- files doctrenderer concatenates into the same
  script it compiles -- with neither option set and outside the gate's reach.
  ASCII-clean today by luck; given the same options so it stays that way.

Full suite: 44/44. Rebuilt word/sdk-all.js with the final config and confirmed
x2t writes a valid cache.

Assisted-by: ClaudeCode:claude-opus-5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
@moodyjmz

Copy link
Copy Markdown
Member

Verified the follow-up commit (991547e) directly rather than taking the commit message on faith — pulled the diff, re-derived Terser's actual identifier-printing rule (output.js: the gate is ecma < 2015, a boolean threshold, not a specific version — so pinning ecma: 2020 in the test is a safe proxy for whatever terser-webpack-plugin injects), and confirmed the glob pattern resolves against the real deploy layout at the pinned glob@8.1.0.

All four points from the review above are correctly addressed:

  • The format.ascii_only/quote_keys comment in webpack.sdk.factory.mjs now correctly states quote_keys is the load-bearing option, with the exact \u{1d552}: failure mode spelled out.
  • The behavioral test now pins ecma: 2020 so it actually exercises the code path the real build takes, and adds a "guards the guard" test asserting the bare key does reappear if quote_keys is removed — so this can't quietly go blind again.
  • The CI gate is now build/scripts/check-bundle-ascii.cjs, checking both raw non-ASCII and bare-astral-key in both raw and escaped form, sharing its regexes with the test file.
  • deploy-assets.cjs's second Terser pass over Native/*.js/libfont/engine/fonts_*.js now gets the same options, closing the latent gap.

Nice catch-and-fix cycle. Approving.

@moodyjmz moodyjmz 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.

Approving — see review comments above; the follow-up commit correctly addressed all findings.

@chrip
chrip merged commit 1e80c88 into main Aug 27, 2026
4 checks passed
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.

Terser emits bare astral object keys in sdk-all.js; doctrenderer's no-ICU V8 rejects them and x2t aborts (9.3.4 regression)

2 participants