[Mono.Android] Prevent premature JNI handle collection - #12315
Conversation
Keep managed JNI handle owners alive until native calls have consumed their handles, and return stable local references for temporary wrappers. Add concurrent collection coverage for ToLocalJniHandle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6105414-1c2c-4696-8d01-6507bda3e4be
There was a problem hiding this comment.
Pull request overview
This PR addresses a JNI lifetime hazard where managed handle owners can be collected after a .Handle/borrowed-handle access but before the corresponding native/JNI operation consumes it, leading to invalid handles. The change set hardens several managed→JNI call sites with GC.KeepAlive(...), updates conversions to prefer stable local references, and adds a regression test that stresses JNIEnv.ToLocalJniHandle() under concurrent GC/finalization.
Changes:
- Add
GC.KeepAlive(...)at a number of JNI boundary call sites to prevent premature collection of managed handle owners. - Route legacy
Android.Runtime.JavaObject.GetHandle(object)throughJavaConvert.ToLocalJniHandle(object)and adjustJavaConvert’s JavaObject wrapping to useToLocalJniHandle. - Add a GC-pressure regression test for
JNIEnv.ToLocalJniHandle().
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JnienvTest.cs | Adds concurrent-GC regression coverage for JNIEnv.ToLocalJniHandle() using a finalizable handle owner. |
| src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs | Keeps certificate object alive across JNI handle consumption. |
| src/Mono.Android/Java.Interop/Runtime.cs | Ensures IJavaObject passed to IsGCUserPeer stays alive through handle usage. |
| src/Mono.Android/Java.Interop/JavaObjectExtensions.cs | Keeps cast source instance alive across GetObject(..., Handle, ...) conversions. |
| src/Mono.Android/Java.Interop/JavaConvert.cs | Makes temporary Android.Runtime.JavaObject wrapper conversion return a stable local ref via JNIEnv.ToLocalJniHandle. |
| src/Mono.Android/Android.Widget/TextView.cs | Keeps listener/receiver alive across CallVoidMethod in event hookup. |
| src/Mono.Android/Android.Widget/ArrayAdapter.cs | Adds GC.KeepAlive(context) after constructor/static JNI calls and keeps comparator/this alive in JNI calls. |
| src/Mono.Android/Android.Widget/AdapterViewAnimator.cs | Adds GC.KeepAlive(this/value) around adapter getter/setter JNI calls. |
| src/Mono.Android/Android.Widget/AdapterView.cs | Keeps constructor parameters alive across StartCreateInstance/FinishCreateInstance calls. |
| src/Mono.Android/Android.Widget/AbsListView.cs | Keeps adapter/receiver alive across SetAdapter JNI calls. |
| src/Mono.Android/Android.Runtime/XmlReaderPullParser.cs | Keeps parser-reader wrappers alive across NewLocalRef(handle) creation. |
| src/Mono.Android/Android.Runtime/JNIEnv.cs | Keeps IJavaObject alive across local-ref creation; keeps array elements alive during copy/set into Java arrays. |
| src/Mono.Android/Android.Runtime/JavaObject.cs | Consolidates legacy GetHandle(object) conversion through JavaConvert.ToLocalJniHandle. |
| src/Mono.Android/Android.Runtime/AndroidEnvironment.cs | Keeps throwable alive across JNIEnv.Throw(Handle). |
Use an explicit marshaling failure when ArrayAdapter creation returns null. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6105414-1c2c-4696-8d01-6507bda3e4be
Use a derived Java peer because Java.Lang.Object does not expose a public constructor. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6105414-1c2c-4696-8d01-6507bda3e4be
Derive the finalizable owner directly from Java.Lang.Object so Android packaging accepts it as an IJavaObject. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6105414-1c2c-4696-8d01-6507bda3e4be
Preserve fallback JavaObject peer mappings during collection conversion and validate the generated test peer without assuming its runtime class name. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6105414-1c2c-4696-8d01-6507bda3e4be
jonathanpeppers
added
the
ready-to-review
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
1 warning · 0 errors
The lifetime changes consistently keep managed owners alive while JNI consumes borrowed handles, and consolidating JavaObject.GetHandle() through local-reference conversion fixes the temporary-wrapper lifetime problem. CI is green across all 44 checks.
The new regression test does not cover the vulnerable IJavaObject fallback branch and is likely to pass without the fix; please adjust the test owner so the test proves the intended race is fixed.
Generated by Android PR Reviewer for #12315 · gpt56 · 141 AIC · ⌖ 10.1 AIC · ⊞ 25.3K
Comment /review to run again
Extract the IJavaObject fallback into an internal helper so the regression test can exercise the vulnerable borrowed-handle path while using a package-valid Java peer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6105414-1c2c-4696-8d01-6507bda3e4be
|
@dalexsoto review |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
Re-reviewed the current changes. JNI ownership, rooting, disposal, and fallback paths look correct; the regression test exercises the vulnerable path, and all exact-head checks pass. No blocking issues found.
Summary
JNIEnv.ToLocalJniHandle()with concurrent GC pressureJavaObject.GetHandle()conversion throughJavaConvertFixes #5405
Testing