Repository navigation
Fix modified-UTF-8 corruption in JNI string marshaling - #1
Merged
Merged
Conversation
This was referenced Aug 29, 2026
This was referenced Sep 5, 2026
GetJNIString()/NewJNIString() used env->GetStringUTFChars()/NewStringUTF(), which use JNI's "modified UTF-8" (CESU-8) encoding: supplementary-plane characters (e.g. emoji) are encoded as a pair of 3-byte surrogate sequences instead of one 4-byte UTF-8 sequence, and NUL is encoded as an overlong 2-byte sequence. The result was treated as standard UTF-8 by CefString, silently corrupting any string containing a supplementary-plane character or an embedded NUL crossing the JNI boundary in either direction. Since CefString is UTF-16 internally with direct buffer access, and jchar/char16_t are both exactly 16 bits, the fix goes through JNI's non-UTF string functions (NewString()/GetStringChars()) instead, copying UTF-16 directly with no encoding ambiguity and no transcoding needed. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01BLqFiEqAPbHc3MifgpNjA8
ModifiedUtf8Test covers both directions of the bug fixed in the previous commit, isolated so a bug in one direction can't be masked by the other: - nativeToJavaAstralCharacterSurvivesTitleChange: a supplementary-plane character generated entirely inside JS (String.fromCodePoint) is read back into Java via onTitleChange, exercising NewJNIString(). - javaToNativeAstralCharacterSurvivesExecuteJavaScript: a Java string containing the same character is passed to executeJavaScript() and verified inside JS (result reported back as a plain-ASCII PASS/FAIL), exercising GetJNIString() without the readback itself going back through NewJNIString(). Both tests use a new OSR (off-screen-rendered) browser via TestFrame's new createBrowser(url, useOSR) overload, added because OSR browsers close via a pure Java-side handshake and don't depend on native window-destroy notification -- unlike TestFrame's default windowed mode, whose close path currently hangs in this environment (tracked separately, matches upstream java-cef#364; unrelated to this fix). Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01BLqFiEqAPbHc3MifgpNjA8
Thrameos
force-pushed
the
fix/modified-utf8-jni-strings
branch
from
September 6, 2026 03:04
992e168 to
609f172
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1 +/- ##
============================================
+ Coverage 39.61% 41.62% +2.01%
- Complexity 761 868 +107
============================================
Files 248 253 +5
Lines 14990 15773 +783
Branches 2461 2650 +189
============================================
+ Hits 5938 6566 +628
- Misses 7813 7895 +82
- Partials 1239 1312 +73 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
javaToNativeAstralCharacterSurvivesExecuteJavaScript() terminated on the first onTitleChange call, which can be an initial title defaulting to the page URL before executeJavaScript()'s title assignment actually lands -- same class of flake DisplayHandlerTest already had to guard against (onTitleChange/onAddressChange firing more than once). Filter to only the JS-assigned "PASS"/"FAIL:..." value. Found via a real CI failure on this PR after rebasing onto master. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HBFmQs6JDypYP5qdC99yDq
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6
Summary
GetJNIString()/NewJNIString()innative/jni_util.cppusedenv->GetStringUTFChars()/env->NewStringUTF(), JNI's modified UTF-8 (CESU-8) encoding, but treated the result as standard UTF-8 on theCefStringside. Supplementary-plane characters (emoji, etc.) and embedded NULs were silently corrupted crossing the JNI boundary in either direction.UTFstring functions (NewString()/GetStringChars()) instead, copyingCefString's native UTF-16 buffer directly (jchar/char16_tare both exactly 16 bits) -- no transcoding, no ambiguity.NewJNIString/ScopedJNIStringnow takeconst CefString&instead ofconst std::string&; every call site already passed aCefStringor a literal that constructs one implicitly).ModifiedUtf8Test.java, reproducing both directions of the bug in isolation (native->Java viaonTitleChange, Java->native viaexecuteJavaScript, each checked so a bug in one direction can't be masked by the other) -- both failed before the fix, both pass after.TestFrame.createBrowser(url, useOSR)overload andOsrSmokeTest.java: the new regression tests use an OSR (off-screen-rendered) browser rather thanTestFrame's default windowed mode, since OSR closes via a pure Java-side handshake. This sidesteps an unrelated, separately-reproduced windowed-browser close hang in this dev environment that matches upstream java-cef#364 (not fixed by or related to this PR).Test plan
libjcef.socleanly (ninja jcef), zero compile errors.ModifiedUtf8Testreproduced the bug pre-fix (mangled titleð,FAIL:fffdreplacement-character corruption) and passes post-fix.OsrSmokeTest(existing OSR browser lifecycle coverage) -- no regression.🤖 Generated with Claude Code
https://claude.ai/code/session_01BLqFiEqAPbHc3MifgpNjA8