Skip to content

Fix JNI global ref leak in TypeManager.Activate and ValueManager.ActivateViaReflection#11111

Closed
simonrozsival wants to merge 5 commits intomainfrom
dev/simonrozsival/investigate-11101-layoutinflater-memory-leak
Closed

Fix JNI global ref leak in TypeManager.Activate and ValueManager.ActivateViaReflection#11111
simonrozsival wants to merge 5 commits intomainfrom
dev/simonrozsival/investigate-11101-layoutinflater-memory-leak

Conversation

@simonrozsival
Copy link
Copy Markdown
Member

Summary

Fix a JNI global reference leak in TypeManager.Activate() and ValueManager.ActivateViaReflection() that causes memory leaks when Java-initiated peer activation (e.g., LayoutInflater.Inflate with custom .NET views, Activity recreation) triggers GC during the constructor chain.

Fixes: #11101
Related: #10989

Root Cause

The regression was introduced in the "Java.Interop Unification" (#9640, commit 5c23bcda8) when Java.Lang.Object was rewritten to extend Java.Interop.JavaObject.

Net9 (safe): Activate() set o.handle = jobject directly. The Object(IntPtr, JniHandleOwnership) constructor had a guard:

if (handle_type != 0)  // bridge sets this to JNIGlobalRefType
    return;            // skip SetHandle → no duplicate global ref

Net10 (leaks): Activate() calls SetPeerReference(new JniObjectReference(jobject)), which allocates a JniObjectReferenceControlBlock with a raw local ref (handle_type = Invalid). The handle_type guard was removed during the unification. When the constructor chain's ConstructPeer runs:

  1. It captures PeerReference (which may have been modified by bridge processing during a GC)
  2. Calls NewGlobalRef() — creating a second global ref
  3. Overwrites the control block — orphaning the first global ref

The orphaned global ref is never deleted, keeping the Java object alive forever.

Fix

Replace SetPeerReference with Activatable + ConstructPeer before the constructor invocation in all three activation paths:

  • TypeManager.Activate() — ACW activation path (Mono VM via AndroidValueManager.ActivatePeer)
  • JavaMarshalValueManager.ActivateViaReflection() — CoreCLR path via ManagedPeer.Construct
  • SimpleValueManager.ActivateViaReflection() — test/desktop path

This ensures:

  1. The control block has a proper global ref from the start (no intermediate Invalid-type handle)
  2. The Activatable flag prevents the constructor chain's ConstructPeer from creating a duplicate (equivalent of net9's handle_type != 0 guard)

Testing

Added device tests in InflatedCustomViewTests:

  • HasValidPeerReference — verifies activation produces Global ref type + PeekObject registration
  • CanBeCollected — verifies inflated custom views are GC'd after discarding
  • RepeatedInflation_DoesNotLeakGlobalRefs — stress test: 50 inflate cycles, asserts gref count stays stable

Also added diagnostic logging (guarded by Logger.LogGlobalRef) at Activate, SetHandle, AddPeer, RemovePeer, and FinalizePeer. Enable with debug.mono.log=gref, filter with adb logcat -s monodroid-peer.

simonrozsival and others added 5 commits April 14, 2026 18:16
Add 'monodroid-peer' tagged logging to trace:
- TypeManager.Activate: PeerReference state before/after ctor
- Object.SetHandle: ConstructPeer entry with existing PeerRef state
- AndroidValueManager.AddPeer: registration, hash collisions, replacements
- AndroidValueManager.FinalizePeer: resurrection vs disposal decisions

Also add a device test for the #11101 inflation scenario.

Filter with: adb logcat -s monodroid-peer

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace SetPeerReference with Activatable + ConstructPeer in both
TypeManager.Activate() and JavaMarshalValueManager.ActivateViaReflection().

The race condition: Between SetPeerReference (which stores a raw JNI
local ref with Invalid type in the control block) and ConstructPeer
(which promotes it to a global ref), if a GC triggers bridge
processing, the bridge's take_weak_global_ref_jni calls
DeleteGlobalRef on what is actually a local ref (JNI error), then
take_global_ref_jni creates a global ref that gets orphaned when
ConstructPeer later creates its own global ref and overwrites the
control block. The orphaned global ref keeps the Java object alive
forever.

The fix calls ConstructPeer BEFORE the constructor, ensuring the
control block always contains a proper global ref. The Activatable
flag prevents the constructor chain's ConstructPeer from creating a
duplicate ref.

Fixes: #11101

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix the same SetPeerReference race in SimpleValueManager.
ActivateViaReflection() for completeness (same pattern as the
TypeManager and JavaMarshalValueManager fixes).

Guard all monodroid-peer diagnostic logging behind Logger.LogGlobalRef
to avoid string allocation overhead in production. Enable with:
  debug.mono.log=gref

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests that 50 consecutive inflate+discard cycles don't cause
unbounded global ref growth. Under the bug, each race hit during
Activate leaks ~1 gref, so this test would see ~50 leaked grefs.
With the fix, gref count returns to near-baseline after GC.

This also validates the fix for issue #10989 (same root cause
with Activity recreation — Activities also go through n_Activate).

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

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

Fixes a JNI global reference leak during Java-initiated peer activation by ensuring peers are marked Activatable and ConstructPeer is performed before invoking managed constructors, preventing duplicate/orphaned global refs. Adds regression tests around LayoutInflater.Inflate() and extra diagnostic logging to help validate peer/ref lifecycle behavior.

Changes:

  • Update activation paths (TypeManager.Activate() and *ValueManager.ActivateViaReflection()) to set Activatable and call ConstructPeer prior to constructor invocation.
  • Add targeted logging (behind Logger.LogGlobalRef) around handle/peer registration and finalization to diagnose gref/peer behavior.
  • Add an inflation-based device regression test and corresponding AXML layout to validate peer reference correctness and non-leaking behavior.

Reviewed changes

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

Show a summary per file
File Description
tests/Mono.Android-Tests/Mono.Android-Tests/Resources/layout/inflated_custom_view.axml Adds layout resource used to inflate a custom managed View from XML.
tests/Mono.Android-Tests/Mono.Android-Tests/Android.Views/InflatedCustomViewTests.cs Adds regression tests for activation/collection/gref stability when inflating custom views.
src/Mono.Android/Microsoft.Android.Runtime/SimpleValueManager.cs Switches reflection activation path to Activatable + ConstructPeer pre-ctor.
src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs Same as above for JavaMarshal/CoreCLR activation path.
src/Mono.Android/Java.Lang/Object.cs Adds gref diagnostics around SetHandle() and refines options passed to ConstructPeer.
src/Mono.Android/Java.Interop/TypeManager.cs Updates Java->managed activation to Activatable + ConstructPeer pre-ctor and adds diagnostics.
src/Mono.Android/Android.Runtime/AndroidRuntime.cs Adds gref/peer-table diagnostics for AddPeer/RemovePeer/FinalizePeer paths.

Comment on lines 213 to +217
cinfo.Invoke (newobj, parms);
if (Logger.LogGlobalRef) {
Logger.Log (LogLevel.Info, "monodroid-peer",
FormattableString.Invariant ($"Activate: after ctor PeerRef={peer.PeerReference} PeerRef.Type={peer.PeerReference.Type} State={peer.JniManagedPeerState} IdentityHashCode=0x{peer.JniIdentityHashCode:x}"));
}
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

peer is declared in the pattern match (if (newobj is IJavaPeerable peer)) and is out of scope after the if block. The logging after cinfo.Invoke() references peer, which will not compile. Move the post-ctor log inside the if block, or hoist a separate IJavaPeerable peer variable outside the if and assign it before invoking the constructor.

Copilot uses AI. Check for mistakes.
Comment on lines +19 to +21
var inflater = LayoutInflater.From (Application.Context)!;
var layout = inflater.Inflate (Resource.Layout.inflated_custom_view, null, false)!;

Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

Avoid the null-forgiving operator (!) in tests. LayoutInflater.From(Application.Context) and Inflate(...) are non-null; remove ! and, if you need to enforce non-nullability, use explicit checks/assertions rather than !.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +29 to +30
var peerRef = customView!.PeerReference;
Assert.IsTrue (peerRef.IsValid, "PeerReference should be valid");
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

Avoid using the null-forgiving operator (customView!). Since FindCustomView() can return null, replace customView! with an explicit null check (e.g., fail the test if null) and then use a non-null local variable for subsequent assertions.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +65 to +67
Assert.IsNotNull (weakRef, "WeakReference should have been created");
Assert.IsFalse (weakRef!.IsAlive,
"Custom view should be collected after GC — if it's still alive, there is a memory leak (https://github.com/dotnet/android/issues/11101)");
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

Avoid the null-forgiving operator on weakRef (weakRef!). Prefer an explicit null-check (e.g., if (weakRef == null) Assert.Fail(...)) and then use a non-null local variable for the IsAlive assertion.

Copilot generated this review using guidance from repository custom instructions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak in LayoutInflater.Inflate method when using custom view in XML layout.

2 participants