[Mono.Android] fix global ref leak in TypeManager.Activate#11112
[Mono.Android] fix global ref leak in TypeManager.Activate#11112jonathanpeppers wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a JNI global reference leak when Java activates managed peers via TypeManager.Activate, preventing repeated ConstructPeer calls in constructor chains from creating duplicate global refs (notably impacting LayoutInflater.Inflate with custom C# views).
Changes:
- Update
TypeManager.Activateto create an uninitialized peer withActivatable|Replaceablestate and useValueManager.ConstructPeerfor correct peer construction/registration. - Promote
GetUninitializedObjectto a shared helper so both activation and proxy creation consistently set the required managed peer state. - Add a regression test that inflates a custom view repeatedly and asserts JNI global ref counts remain stable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Mono.Android/Java.Interop/TypeManager.cs |
Fixes activation path to construct peers correctly and avoid duplicate global refs during constructor chaining. |
tests/Mono.Android-Tests/Mono.Android-Tests/Android.Widget/CustomWidgetTests.cs |
Adds regression coverage to detect global ref growth when inflating custom views from XML. |
simonrozsival
left a comment
There was a problem hiding this comment.
This is a good fix for TypeManager.Activate. We need to apply the same fix to JavaMarshalValueManage.ActivateViaReflection (CoreCLR) and SimpleValueManager.ActivateViaReflection (and possibly other implementations of value managers we have for various purposes) in follow-up PRs.
|
The new test works well, it shows the memory leak on CoreCLR and Native AOT: Maybe we should apply the fix to all the "VMs" already in this PR |
c64fdb9 to
31a44fa
Compare
Round 1 acts as warmup, letting background threads settle. Round 2 measures with a clean baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GlobalReferenceCount is process-wide and affected by Android system services, GC bridge processing, and finalizer threads. Using 100 inflations makes a real leak (300+ delta) far exceed any background noise, while a threshold of 100 tolerates device variability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes: #11101
Fixes: #10989
Context: 5c23bcda8 (PR #9640)
The Java.Interop Unification (5c23bcd) changed
Objectto extendJavaObject, introducing an additionalConstructPeercall in the constructor chain.TypeManager.Activatewas updated to useSetPeerReferencebut never set theActivatablestate flag. Without it, eachConstructPeercall in the constructor chain (JavaObject()andObject.SetHandle()) creates a new JNI global ref, overwriting the previous one without deleting it — leaking 3 global refs perLayoutInflater.Inflatecall.Before the fix, the new test fails with:
This went unnoticed because the
Activatepath is only triggered when Java creates .NET objects (not the other way around). The two main scenarios are Activity recreation and custom C# views in Android XML layouts. Most developers use .NET MAUI, which has a single Activity and does not use custom C# views in Android layout XML files, so neither scenario was commonly hit.Changes:
Promote
GetUninitializedObjectfrom a local function inCreateProxyto a shared static method. It setsActivatable | Replaceable, which tellsConstructPeerto return early and not create duplicate global refs.Have
ActivatecallGetUninitializedObjectthenConstructPeerto create one global ref whilejobjectis still a valid JNI local ref. TheActivatableflag then prevents duplicates during the constructor chain.Add regression test that inflates custom views and asserts JNI global reference count does not grow.