Skip to content

handle unpaired surrogates on websocket close#6176

Merged
anonrig merged 1 commit intomainfrom
yagiz/fix-websocket-spec-compl
Feb 25, 2026
Merged

handle unpaired surrogates on websocket close#6176
anonrig merged 1 commit intomainfrom
yagiz/fix-websocket-spec-compl

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Feb 25, 2026

I don't think this change substitutes for a compat flag but I'm open to be convinced.

@anonrig anonrig requested review from a team as code owners February 25, 2026 16:09
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Feb 25, 2026

The code and tests look good. Let me now summarize my review findings:

  1. The core change is correct: Changing kj::String to jsg::USVString for the close() reason and CloseEvent.Initializer.reason aligns with the WebSocket spec and HTML spec.

  2. No compat flag needed: I agree with the author. Sending lone surrogates in WebSocket close reasons is not something reasonable code depends on, and this is a spec compliance fix.

  3. The orDefault pattern: kj::mv(reason).orDefault(jsg::USVString(kj::str())) — this creates an empty USVString from an empty kj::String. Since USVString's constructor has a KJ_DASSERT(isValidUtf8()), and an empty string is valid UTF-8, this is fine.

  4. Tests are good: The new unit tests cover lone surrogates, multiple surrogates, and properly paired surrogates.

  5. WPT re-enablement: Close-reason-unpaired-surrogates.any.js expectedFailures removed (should now pass), and Close-onlyReason.any.js re-enabled with cleanup code. CI will validate these.

I don't see any actionable issues with this PR. The change is straightforward, spec-correct, well-tested, and doesn't need a compat flag.

LGTM

github run

Copy link
Member

@npaun npaun left a comment

Choose a reason for hiding this comment

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

Let's see if anyone insists on a compat flag though before merging

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 25, 2026

Merging this PR will degrade performance by 12.22%

❌ 1 regressed benchmark
✅ 69 untouched benchmarks
⏩ 129 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
Encode_ASCII_32[TextEncoder][0/0/32] 3 ms 3.5 ms -12.22%

Comparing yagiz/fix-websocket-spec-compl (355e4ef) with main (9e7c111)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.62%. Comparing base (9e7c111) to head (355e4ef).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6176      +/-   ##
==========================================
- Coverage   70.63%   70.62%   -0.02%     
==========================================
  Files         413      413              
  Lines      109819   109819              
  Branches    18096    18096              
==========================================
- Hits        77576    77559      -17     
- Misses      21436    21452      +16     
- Partials    10807    10808       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

@anonrig anonrig merged commit 85a3a0f into main Feb 25, 2026
27 of 29 checks passed
@anonrig anonrig deleted the yagiz/fix-websocket-spec-compl branch February 25, 2026 19:18
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.

4 participants