TextDecoder: emit U+FFFD for malformed UTF-16 - #235
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are focused, match the stated follow-up goals, and are supported by targeted new tests for the UTF-16 edge cases.
Pull request overview
This PR is a follow-up to prior review comments in #220 and #230, tightening TimeoutDispatcher’s timeout-id handling to avoid unsafe reuse paths and aligning the UTF-16 TextDecoder implementation with the Encoding Standard’s replacement behavior for malformed input.
Changes:
- Removes the unused
DispatchImplpath and ensuresTimeoutDispatcher::Dispatchalways allocates a fresh timeout id. - Updates UTF-16 decoding to emit U+FFFD for a trailing odd byte and for unpaired surrogates, matching replacement-mode behavior.
- Adds unit tests covering malformed UTF-16 edge cases for both
utf-16leandutf-16be, and updates the polyfill README accordingly.
File summaries
| File | Description |
|---|---|
| Tests/UnitTests/Scripts/tests.ts | Adds JS tests for malformed UTF-16 replacement behavior in TextDecoder. |
| Polyfills/TextDecoder/Source/TextDecoder.cpp | Implements replacement-mode malformed UTF-16 handling (odd byte + unpaired surrogates). |
| Polyfills/TextDecoder/README.md | Documents the updated malformed UTF-16 replacement behavior. |
| Polyfills/Scheduling/Source/TimeoutDispatcher.h | Removes the now-unused DispatchImpl declaration. |
| Polyfills/Scheduling/Source/TimeoutDispatcher.cpp | Removes DispatchImpl and makes Dispatch always allocate a fresh id to avoid unsafe reuse/collision behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
DispatchImpl still accepted an id after BabylonJS#220, but Dispatch always passed 0. If a future caller reused an existing id, unordered_map::insert would keep the old Timeout and m_timeMap would gain a second entry pointing at it; Clear() erases only one. Always allocate a fresh id. The UTF-16 decoder dropped a trailing odd byte and passed unpaired surrogates through to Napi::String. The Encoding Standard replacement mode requires U+FFFD for both, in either endianness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc
The previous wording said errors were "not detected" while also describing U+FFFD replacement. fatal still does not throw; replacement is the supported behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da74bc94-a7dc-4817-bd81-59b5c6b123fc
205707d to
5c81b40
Compare
bkaradzic-microsoft
changed the title
|
@bghgary Corrected the description: I also rebased this PR onto the latest |
Summary
Follow-up to #230 (comment).
The UTF-16 decoder previously dropped a trailing odd byte and created JavaScript strings from unpaired surrogates. Replacement mode in the Encoding Standard requires U+FFFD for both, in either endianness.
DecodeUtf16now:The README describes this behavior and the still-unsupported
fataloption.Timer cleanup after rebase
Correction to the original description, per Gary's review:
Dispatchalways passed0, so theid == 0branch was always taken. The nonzero ID-reuse path was dead.The latest
mainalready includes the timer refactor from #232, which removesDispatchImpland guards against duplicate-ID insertion. The rebase keeps that implementation unchanged and drops this PR's superseded timer changes. The remaining diff contains only TextDecoder, its documentation, and the UTF-16 regression cases.Regression coverage
U+D800then'A') produces"\uFFFDA""\uFFFD"Existing valid-pair and BOM cases are unchanged.