Skip to content

Fix UTF-8 JniType class lookup fallback#1407

Merged
jonathanpeppers merged 9 commits intomainfrom
simon/jnitype-utf8-fallback
Apr 17, 2026
Merged

Fix UTF-8 JniType class lookup fallback#1407
jonathanpeppers merged 9 commits intomainfrom
simon/jnitype-utf8-fallback

Conversation

@simonrozsival
Copy link
Copy Markdown
Member

Summary

Make the UTF-8 JniType(ReadOnlySpan<byte>)/FindClass(ReadOnlySpan<byte>) path behave the same as the existing string overloads.

This change:

  • keeps the raw FindClass() fast path
  • falls back through Class.forName(..., Runtime.ClassLoader) when needed
  • uses NewStringUTF() in the UTF-8 fallback path to avoid allocating a managed UTF-16 string first
  • adds regression tests so both overload families stay aligned

Fixes #1406.

simonrozsival and others added 2 commits April 17, 2026 14:43
Make the ReadOnlySpan<byte> JniType/FindClass path fall back through Class.forName using the runtime class loader, matching the existing string overload semantics. Add regression coverage for Java-style class names so both overload families stay aligned.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid allocating a managed UTF-16 string in the ReadOnlySpan<byte> JniType/Class.forName fallback path by creating the Java name with NewStringUTF instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 17, 2026 13:54
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Aligns UTF-8 ReadOnlySpan<byte> class lookup behavior with existing string overload semantics in Java.Interop, ensuring consistent resolution via FindClass() fast path with a Class.forName(..., Runtime.ClassLoader) fallback, and adds regression tests to keep the overload families in sync.

Changes:

  • Refactors TryFindClass(string) to delegate fallback logic into a shared helper.
  • Adds UTF-8 fallback path using NewStringUTF() to avoid allocating a managed UTF-16 string before calling Class.forName(...).
  • Adds tests validating UTF-8 FindClass and JniType constructor fallback parity with the string overloads.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Java.Interop/Java.Interop/JniEnvironment.Types.cs Adds shared fallback logic and extends UTF-8 FindClass(ReadOnlySpan<byte>) to match string lookup semantics.
tests/Java.Interop-Tests/Java.Interop/JniTypeUtf8Test.cs Adds regression tests ensuring UTF-8 overloads behave like the existing string overloads.

Comment thread src/Java.Interop/Java.Interop/JniEnvironment.Types.cs Outdated
Comment thread src/Java.Interop/Java.Interop/JniEnvironment.Types.cs Outdated
@simonrozsival simonrozsival added the copilot `copilot-cli` or other AIs were used to author this label Apr 17, 2026
simonrozsival and others added 7 commits April 17, 2026 16:06
Limit the NewStringUTF helper to function-pointer builds so other backends continue to compile, and include the UTF-8 class name in the fallback exception message for better diagnostics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refactor the string and UTF-8 class lookup fallback paths to share a single implementation built around a Java string reference. This preserves the NewStringUTF optimization while removing duplicated fallback logic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inline the UTF-8 fallback Java string creation at the TryFindClass(ReadOnlySpan<byte>) callsite so it matches the string overload pattern and avoids an unnecessary wrapper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Delete the obsolete TryLoadClassWithFallback(ReadOnlySpan<byte>) overload now that the UTF-8 callsite creates and disposes the Java string inline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Inline the single-use RawNewStringUTF helper and keep managed class-name conversion on the final error path only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refactor TryLoadClassWithFallback to return success via an out JniObjectReference so callers can defer class-name allocation until the final not-found branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop the single-use GetStringClassName helper and decode the UTF-8 class name directly at the final not-found throw site.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

🤖 PR Review — Fix UTF-8 JniType class lookup fallback

Bug fixed: The ReadOnlySpan<byte> overload of FindClass did not fall back through Class.forName() when JNI FindClass failed — unlike the string overload. This meant Java-style names like "java.lang.Object"u8 would fail for UTF-8 but succeed for strings.

Key changes

Area What changed
TryLoadClassWithFallback New shared helper extracts the Class.forName() fallback from the string path so both overloads use it
NewJavaNameFromUtf8 Converts UTF-8 JNI name (/.) and calls NewStringUTF to avoid a managed UTF-16 allocation
String overload Refactored to call TryLoadClassWithFallback instead of inline logic
Tests Two new tests verify both FindClass and JniType constructor fallback parity

Assessment

Looks good overall. The refactoring correctly preserves the original fallback semantics:

  • When Class.forName succeeds → returns class (unchanged)
  • When both fail with throwOnError=true → throws pendingException (the original Java exception)
  • The new InvalidOperationException is only reached in the edge case where pendingException is null (previously would've been NullReferenceException from throw pendingException! — so this is a bug fix)

NewJavaNameFromUtf8 is well-designed: stackalloc for ≤256 bytes, proper null termination, ExceptionDispatchInfo to preserve stack trace from NewStringUTF failures.

Tests adequately cover the fix by using dot-separated names that force the Class.forName fallback path.

Minor nit

There appears to be trailing whitespace on the throw new InvalidOperationException line in the string path (~line 70 of the diff). Not a functional issue, but doesn't match the rest of the file's style.

@jonathanpeppers jonathanpeppers merged commit ce075ce into main Apr 17, 2026
2 checks passed
@jonathanpeppers jonathanpeppers deleted the simon/jnitype-utf8-fallback branch April 17, 2026 20:58
simonrozsival added a commit that referenced this pull request Apr 21, 2026
…o avoid unnecessary global refs

Previously, TryLoadClassWithFallback eagerly called GetExceptionForThrowable
before trying Class.forName(), creating a managed JavaException (with a JNI
global ref, getMessage(), getCause(), getStackTrace()) even when Class.forName()
would succeed immediately after. This was especially costly on the UTF-8
FindClass path introduced in PR #1407, which now falls back through
Class.forName() where it previously did not.

The fix defers managed exception creation: try Class.forName() first using only
raw JNI operations, and only materialize the managed exception if we actually
need to throw. In the common case (Class.forName succeeds or throwOnError=false),
no managed exception is created and no global ref is allocated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jonathanpeppers pushed a commit that referenced this pull request Apr 21, 2026
… to fix global ref leak (#1410)

Previously, TryLoadClassWithFallback eagerly called GetExceptionForThrowable
before trying Class.forName(), creating a managed JavaException (with a JNI
global ref, getMessage(), getCause(), getStackTrace()) even when Class.forName()
would succeed immediately after. This was especially costly on the UTF-8
FindClass path introduced in PR #1407, which now falls back through
Class.forName() where it previously did not.

The fix defers managed exception creation: try Class.forName() first using only
raw JNI operations, and only materialize the managed exception if we actually
need to throw. In the common case (Class.forName succeeds or throwOnError=false),
no managed exception is created and no global ref is allocated.

### [Java.Interop] Add tests verifying TryFindClass does not leak global refs

Adds regression tests for both UTF-8 and string TryFindClass overloads,
verifying that repeated lookups of non-existent classes do not leak JNI
global references. These tests exercise the TryLoadClassWithFallback code
path (throwOnError=false) that was the source of the leak.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

copilot `copilot-cli` or other AIs were used to author this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JniType(ReadOnlySpan<byte>) should match JniType(string) lookup semantics

3 participants