Skip to content

lib: correct windows-1252 decoding in TextDecoder - #60737

Closed
yashwantbezawada wants to merge 3 commits into
nodejs:mainfrom
yashwantbezawada:fix-windows-1252-decoding
Closed

lib: correct windows-1252 decoding in TextDecoder#60737
yashwantbezawada wants to merge 3 commits into
nodejs:mainfrom
yashwantbezawada:fix-windows-1252-decoding

Conversation

@yashwantbezawada

Copy link
Copy Markdown

Description

Fixes#56542

This PR corrects the Windows-1252 decoding in TextDecoder by disabling the Latin-1 fast path for Windows-1252 encoding.

Problem

The TextDecoder was incorrectly using the Latin-1 fast path (via simdutf's convert_latin1_to_utf8) for Windows-1252 encoding. This caused incorrect decoding of bytes in the 0x80-0x9F range.

The root cause is that Windows-1252 differs from ISO-8859-1 (Latin-1) in this byte range:

  • ISO-8859-1: Bytes 0x80-0x9F are undefined/control characters that map directly to Unicode (e.g., 0x92 → U+0092)
  • Windows-1252: These bytes map to specific printable characters (e.g., 0x92 → U+2019 RIGHT SINGLE QUOTATION MARK ')

Solution

Disable the Latin-1 fast path for Windows-1252 by setting this[kLatin1FastPath] = false. This forces the decoder to use the ICU converter (getConverter()), which correctly handles Windows-1252 character mappings according to the WHATWG Encoding Standard.

Changes

Test Coverage

The new test file test/parallel/test-whatwg-encoding-custom-windows-1252.js verifies:

  1. Specific issue case: byte 0x92 correctly decodes to U+2019 (')
  2. All 32 characters in the 0x80-0x9F range according to WHATWG spec
  3. Common Windows-1252 encoding aliases (windows-1252, cp1252, x-cp1252)
  4. Realistic text samples with mixed special characters

References

@nodejs-github-botnodejs-github-bot added encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run. labels Nov 15, 2025
@yashwantbezawadayashwantbezawada changed the title fix: correct Windows-1252 decoding in TextDecoderlib: correct windows-1252 decoding in TextDecoderNov 15, 2025
Comment threadlib/internal/encoding.js Outdated
Comment threadtest/parallel/test-whatwg-encoding-custom-windows-1252.js
@Renegade334Renegade334 added semver-major PRs that contain breaking changes and should be released in the next major version. web-standards Issues and PRs related to Web APIs labels Nov 16, 2025
The TextDecoder was incorrectly using the Latin-1 fast path for
windows-1252 encoding, which caused incorrect decoding of bytes
in the 0x80-0x9F range.
The issue occurs because windows-1252 differs from ISO-8859-1
(Latin-1) in this byte range. The simdutf library's
convert_latin1_to_utf8 function directly maps bytes to Unicode
codepoints (e.g., 0x92 → U+0092), which is correct for
ISO-8859-1 but incorrect for windows-1252, where 0x92 should
map to U+2019 (RIGHT SINGLE QUOTATION MARK ').
This fix disables the Latin-1 fast path for windows-1252,
forcing the decoder to use the ICU converter which correctly
handles the windows-1252 specific character mappings according
to the WHATWG Encoding Standard.
The fix includes comprehensive tests for all 32 affected
characters (bytes 0x80-0x9F) to prevent regression.
Fixes: nodejs#56542
Refs: https://encoding.spec.whatwg.org/#windows-1252
@codecov

codecovBot commented Nov 16, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.56%. Comparing base (2271d2d) to head (de6c369).
⚠️ Report is 476 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #60737 +/- ##
==========================================
+ Coverage 88.53% 88.56% +0.02% 
==========================================
Files 703 703 Lines 208226 208254 +28 Branches 40145 40172 +27 ==========================================
+ Hits 184352 184437 +85 + Misses 15884 15822 -62 - Partials 7990 7995 +5 
Files with missing linesCoverage Δ
lib/internal/encoding.js99.50% <100.00%> (-0.01%)⬇️

... and 60 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The Latin-1 fast path was incorrectly enabled only for windows-1252
encoding, which differs from ISO-8859-1 (Latin-1) in the 0x80-0x9F
range. Since windows-1252 cannot use the Latin-1 fast path (it requires
different character mappings via ICU), and no other encoding uses it,
the entire Latin-1 fast path mechanism has been removed.
This simplifies the code while fixing the windows-1252 decoding issue.
Windows-1252 now correctly uses the ICU decoder for all characters.
Fixes: nodejs#56542
Comment threadlib/internal/encoding.js
@anonriganonrig removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 23, 2025
Remove the decodeLatin1 import from encoding_binding as it is no longer
used after disabling the Latin-1 fast path for Windows-1252.
@ChALkeR

ChALkeR commented Nov 29, 2025

Copy link
Copy Markdown
Member

decodeLatin1 has no usage outside of this and should be just removed, it was added under a wrong assumption
this PR leaves it present but unused

i noticed this PR just now, but I already filed #60889

Tests here are useful though!

@yashwantbezawada

Copy link
Copy Markdown
Author

Makes sense - #60889 is a more complete fix since it removes the C++ code too.

Happy to open a separate PR to add the Windows-1252 test file after #60889 lands, or I can add it directly to #60889 if that's easier. Let me know what works best.

@ChALkeR

ChALkeR commented Jan 17, 2026

Copy link
Copy Markdown
Member

This can be closed, #61093 landed, which removed the broken codepath.
It also added tests for all single-byte encodings.

@yashwantbezawada

Copy link
Copy Markdown
Author

Closing this - looks like #61118 already landed and fixed the issue. Thanks!

@ChALkeR

ChALkeR commented Jan 31, 2026

Copy link
Copy Markdown
Member

upd: the correct link that fixed that was #61093, i linked a follow-up accidentally

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

Labels

encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.needs-ciPRs that need a full CI run.web-standardsIssues and PRs related to Web APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextDecoder incorrectly decodes 0x92 and several other characters for Windows-1252

5 participants

@yashwantbezawada@ChALkeR@anonrig@Renegade334@nodejs-github-bot