[Mono.Android] Fix JNI local reference disposal with ownership flags - #12380
[Mono.Android] Fix JNI local reference disposal with ownership flags#12380jonathanpeppers wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a local JNI reference leak when JniHandleOwnership includes non-ownership flags (notably DoNotRegister) by masking those flags before choosing the disposal path in JNIEnv.DeleteRef. It also adds an on-device regression test to ensure local reference counts remain stable after creating/disposing many local string references.
Changes:
- Mask
JniHandleOwnershipto only transfer-ownership bits before switching disposal logic inJNIEnv.DeleteRef. - Add a regression test that creates/disposes 600 local string references using
TransferLocalRef | DoNotRegisterand verifies no local-ref accumulation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaConvertTest.cs | Adds a regression test to catch local-reference accumulation when DoNotRegister is combined with transfer ownership. |
| src/Mono.Android/Android.Runtime/JNIEnv.cs | Fixes DeleteRef to ignore non-ownership flags when selecting the deletion path, ensuring local refs are disposed correctly. |
f3b8228 to
6b98592
Compare
JavaConvert creates temporary primitive and string wrappers with TransferLocalRef combined with DoNotRegister. JNIEnv.DeleteRef previously switched on the full flags value, so the combined value matched no ownership case and leaked the local reference. Mask non-ownership flags before selecting the disposal path. Add a deterministic regression that consumes a local string reference with the affected flag combination and verifies the thread-local reference count remains unchanged. Fixes #10589 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b5fb7339-61f9-45af-bdac-4bdcdee623d9
6b98592 to
99716ed
Compare
|
/review |
jonathanpeppers
added
the
ready-to-review
|
@dalexsoto review |
|
✅ Android PR Reviewer completed successfully!
|
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
Masking non-ownership flags before JNI reference disposal correctly handles DoNotRegister combinations, and the deterministic local-reference-count regression covers the leak path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 0 warnings · 1 suggestion
The ownership-mask fix correctly addresses the reported local-reference leak, and the deterministic reference-count assertion is well targeted. The remaining suggestion is to cover the global-reference branch that this same implementation change also affects.
CI is still pending: 42 of 44 checks are complete and passing; MSBuild+Emulator 2 is running and the aggregate dotnet-android check remains queued.
Generated by Android PR Reviewer for #12380 · gpt56 · 93 AIC · ⌖ 19 AIC · ⊞ 25.6K
Comment /review to run again
|
|
||
| using (var value = new Java.Lang.String ( | ||
| JNIEnv.NewString ("value"), | ||
| JniHandleOwnership.TransferLocalRef | JniHandleOwnership.DoNotRegister)) { |
There was a problem hiding this comment.
🤖 💡 Testing — DeleteRef() now changes handling for both TransferLocalRef | DoNotRegister and TransferGlobalRef | DoNotRegister, but this regression only protects the local-reference branch. Please add or parameterize a global-reference case using GlobalReferenceCount so the other newly supported ownership combination cannot regress unnoticed.
Rule: Bug fixes need regression tests
Description
JavaConvertcreates temporary primitive and string wrappers usingJniHandleOwnership.TransferLocalRef | JniHandleOwnership.DoNotRegister.JNIEnv.DeleteRef()previously switched on the full flags value, so this combination matched no ownership case and the local reference was not deleted.Mask non-ownership flags before selecting the JNI disposal path. This fixes the local-reference accumulation reached through
JavaDictionarykeys/values,JavaSet/JavaCollection,System.Linq.Extensions.ToEnumerator_Dispose, andJavaConvert, includingAndroidMessageHandlerresponse-header enumeration.Add a deterministic regression that consumes one local string reference with the affected ownership combination and verifies the thread-local JNI reference count remains unchanged. The old code leaves the count at
initial + 1, so no table-size-dependent loop or GC/local-frame workaround is needed.Fixes #10589
Testing