[Java.Interop] Fix JNI global-reference ownership leaks - #12762
simonrozsival wants to merge 37 commits into
Conversation
Retain redirect ownership until method-cache publication succeeds, dispose unpublished candidates, and release cached redirects during teardown. Preserve reentrant lookup and transfer fallback ownership only after enumeration completes. Add deterministic publication, reentrancy, exception, and cache-lifecycle regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Always dispose child member caches during explicit peer-member disposal, even when the owning class has not been initialized. Keep owner class disposal conditional and preserve lazy, repeatable cleanup. Add focused coverage for subclass-only construction, runtime untracking, repeated cache lifecycles, and uninitialized owner disposal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Release transferred input references in finally blocks around wrapper activation and peer construction. Add focused ownership regression coverage for borrowed, local, and global references across success and failure paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use a local java.lang.Class reference when caching getName during MonoVM and CoreCLR startup, then release it immediately. Keep the unused init field reserved to preserve the shared native/managed layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep subclass construction outside cache locks to preserve recursive lookup. Transfer ownership only to the published candidate and dispose losers or candidates whose publication throws. Add deterministic host-JVM coverage for concurrent and recursive publication, exceptional cleanup, winner usability, and runtime untracking. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Dispose temporary class globals when standalone registration does not adopt them. Transfer ownership after marshalling and before JNI can publish callbacks, preserving earlier delegate batches and retaining partial registrations safely until disposal. Add focused regression coverage for empty and failed registration, borrowed references, existing owners, and callback lifetime across repeated and concurrent registration. Addresses finding 7 in #12760. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep weak runnable values under weak Action keys, preserving every pending post without retaining native-canceled callbacks. Let native owner/token matching determine cancellation and GC determine canceled-peer lifetime rather than disposing potentially queued work. Clean up completed callbacks in finally without removing newer mappings, and serialize removal with terminal disposal. Cover native cancellation ownership, queue survival, repeated/reentrant posts, token/handler identity, exceptions, and cache synchronization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bind constructor-race worker threads to the instrumented runtime, guard the complete standalone registration fixture on Android, and assert the shared activation-failure contract without assuming a Mono-specific inner exception. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The moderate empty-registration ownership bug must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
external/Java.Interop/src/Java.Interop/Java.Interop/JniEnvironment.Types.cs — Do not adopt owners for empty native registrations |
What changed in this PR
This pull request fixes JNI global-reference ownership leaks across Java.Interop, Mono.Android callbacks, startup initialization, and native registration.
Changes:
- Cleans cache, transferred-reference, and startup-reference ownership.
- Reworks runnable caching and cancellation lifetimes.
- Makes native registration ownership explicit.
- Adds regression tests for disposal and lifetime behavior.
| File | Reviewed change |
|---|---|
tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj |
Includes lifetime tests. |
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Lang/RunnableCacheTests.cs |
Tests runnable cache ownership. |
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TransferredReferenceTests.cs |
Tests transferred-reference cleanup. |
tests/Mono.Android-Tests/Mono.Android-Tests/Android.OS/CallbackLifetimeTests.cs |
Tests callback lifetime and cancellation. |
src/native/mono/monodroid/monodroid-glue.cc |
Cleans the startup class reference. |
src/native/common/include/managed-interface.hh |
Preserves shared initialization layout. |
src/native/clr/host/host.cc |
Cleans the startup class reference. |
src/Mono.Android/Java.Lang/Throwable.cs |
Cleans transferred handles in failure-safe paths. |
src/Mono.Android/Java.Lang/Thread.cs |
Uses weak runnable cache entries. |
src/Mono.Android/Java.Lang/Object.cs |
Cleans transferred handles in failure-safe paths. |
src/Mono.Android/Android.Views/View.cs |
Updates callback removal for multiple runnables. |
src/Mono.Android/Android.Runtime/JNIEnvInit.cs |
Preserves shared initialization layout. |
src/Mono.Android/Android.OS/Handler.cs |
Updates callback removal for multiple runnables. |
src/Mono.Android/Android.Graphics.Drawables/ScaleDrawable.cs |
Updates drawable callback removal. |
src/Mono.Android/Android.Graphics.Drawables/RotateDrawable.cs |
Updates drawable callback removal. |
src/Mono.Android/Android.Graphics.Drawables/LayerDrawable.cs |
Updates drawable callback removal. |
src/Mono.Android/Android.Graphics.Drawables/InsetDrawable.cs |
Updates drawable callback removal. |
src/Mono.Android/Android.Graphics.Drawables/DrawableContainer.cs |
Updates drawable callback removal. |
src/Mono.Android/Android.Graphics.Drawables/Drawable.cs |
Updates drawable callback removal. |
src/Mono.Android/Android.Graphics.Drawables/ClipDrawable.cs |
Updates drawable callback removal. |
external/Java.Interop/tests/Java.Interop-Tests/java/net/dot/jni/test/ManagedPeerRegistration.java |
Adds the registration fixture. |
external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/ManagedPeerRegistrationTests.cs |
Tests registration ownership and failures. |
external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniSubclassConstructorCacheTests.cs |
Tests constructor cache races. |
external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRedirectCacheOwnershipTests.cs |
Tests redirect cache ownership. |
external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersDisposalTests.cs |
Tests child-cache disposal. |
external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs |
Adds registration observation support. |
external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj |
Includes the registration fixture. |
external/Java.Interop/src/Java.Interop/Java.Interop/ManagedPeer.cs |
Manages registration owners conditionally. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs |
Tracks registration delegates and ownership. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs |
Disposes static redirects. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniMethodInfoCache.cs |
Centralizes redirect-aware cache publication. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs |
Disposes redirects and constructor-race candidates. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs |
Disposes child caches independently. |
external/Java.Interop/src/Java.Interop/Java.Interop/JniEnvironment.Types.cs |
Manages native registration ownership; empty registrations still retain an owner and require correction. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/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
Findings: 0 errors · 1 warning · 0 suggestions
The ownership cleanup is careful and the regression coverage is strong across redirected caches, recursive/concurrent constructor creation, transferred references, callback lifetime, and native registration failure paths. The remaining concern is the public JniType.RegisterNativeMethods() compatibility regression called out inline: repeated registration and unregister/re-register now throw instead of retaining all delegate batches safely.
CI build 1592960 is still in progress. All completed checks are passing; MSBuild+Emulator 5 and the aggregate dotnet-android check were not complete when reviewed.
Generated by Android PR Reviewer for #12762 · copilot · gpt56 · 245.7 AIC · ⌖ 11.3 AIC · ⊞ 21K
Comment /review to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
There was a problem hiding this comment.
❌ Reject — 1 error, 0 warnings, 0 suggestions.
The runnable-cache ownership changes are directionally good and the PR adds substantial regression coverage, but the new synchronization only protects concrete RunnableImplementor.Dispose() calls. Base-typed disposal and finalization can still race native callback removal with deletion of the JNI peer reference, so this needs to be fixed before merge.
CI currently reports 42 successful checks, 1 running, and 1 queued, with no reported failures yet.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12762 · copilot · gpt56 · 352.2 AIC · ⌖ 14.5 AIC · ⊞ 21.4K
Comment /review to run again
| } | ||
| } | ||
|
|
||
| public new void Dispose () |
There was a problem hiding this comment.
🤖 ❌ JNI references — This new Dispose() only takes the runnable lock when the object is disposed through the concrete type. Disposal through Java.Lang.Object/IDisposable, and the finalizer path through IJavaPeerable.Finalized(), bypasses it and calls Dispose(bool) directly. Because the cache uses resurrection-tracking weak references, Remove() can retain a runnable while finalization concurrently clears/deletes its peer after the Handle check, causing the native removal callback to use an invalid JNI reference. Please synchronize every disposal/finalization path with removal, or have removal acquire an independently owned local JNI reference before invoking native code; the current test only covers Run() calling this hidden method.
Rule: JNI reference lifecycle
Fixes #12760.
JNI global references require explicit ownership and cleanup. The audit found paths that discard owning objects without disposal, retain unnecessary class references until runtime shutdown, or keep canceled callbacks alive through managed caches.
Changes
Java.Interop.JniPeerMembershas not initialized its own class.finallyinJava.Lang.Object.GetObject()and the Object/Throwable SetHandle paths. Borrowed-reference and registration behavior remain unchanged.Lifetime details
Canceled callbacks are reclaimed through GC rather than immediate explicit disposal. Java-owned pending callbacks remain alive; keeping an Action alive no longer strongly roots its canceled runnable.
JNI can register part of a native-method batch before returning an error. Such attempts intentionally retain class/delegate ownership until unregistration or disposal. Class-wide rollback would invalidate registrations owned by another instance. Every attempted delegate batch remains rooted across repeated or concurrent registration; registration and unregistration are serialized.
The startup fix removes one otherwise lost slot per normal process startup. It is not a claim of ongoing application-object leakage. Standalone JavaInterop1 registration is distinct from ordinary Android callable-wrapper registration.
Local validation
Built one shared Release SDK, including native runtimes, additional API-level reference assemblies, and local workload configuration. Ran the full Java.Interop host solution and the full Mono.Android runtime suite using that SDK on a dedicated API 35 arm64 emulator.
Final per-suite results, including affected-suite reruns after integration-test corrections:
All host tests and new gref regressions pass. The six trimmable skips cover missing activation constructors, which have different inherited-constructor semantics on that path.
The host run also exposed a JDK-version-dependent exact constant-pool-count assertion in
ModuleInfoTests. The test now validates the semantic module contents without pinning the compiler-internal pool size.Full runs also exposed integration issues in the new tests: explicitly bind race-test workers to the instrumented runtime, keep the standalone registration fixture off Android, and assert the shared activation failure contract without assuming Mono-specific inner exceptions.