[Mono.Android] follow up remaining value manager activation fixes#11115
Closed
simonrozsival wants to merge 3 commits intodev/peppers/fix-activate-gref-leakfrom
Closed
[Mono.Android] follow up remaining value manager activation fixes#11115simonrozsival wants to merge 3 commits intodev/peppers/fix-activate-gref-leakfrom
simonrozsival wants to merge 3 commits intodev/peppers/fix-activate-gref-leakfrom
Conversation
Fixes: #11101 Fixes: #10989 Context: 5c23bcda8 (PR #9640) The Java.Interop Unification (5c23bcd) changed `Object` to extend `JavaObject`, introducing an additional `ConstructPeer` call in the constructor chain. `TypeManager.Activate` was updated to use `SetPeerReference` but never set the `Activatable` state flag. Without it, each `ConstructPeer` call in the constructor chain (`JavaObject()` and `Object.SetHandle()`) creates a new JNI global ref, overwriting the previous one without deleting it — leaking 3 global refs per `LayoutInflater.Inflate` call. Before the fix, the new test fails with: Global reference leak detected: 30 extra global refs after inflating/GC'ing 10 custom views. Before=207, After=237 This went unnoticed because the `Activate` path 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 `GetUninitializedObject` from a local function in `CreateProxy` to a shared static method. It sets `Activatable | Replaceable`, which tells `ConstructPeer` to return early and not create duplicate global refs. - Have `Activate` call `GetUninitializedObject` then `ConstructPeer` to create one global ref while `jobject` is still a valid JNI local ref. The `Activatable` flag then prevents duplicates during the constructor chain. - Add regression test that inflates custom views and asserts JNI global reference count does not grow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up runtime + generator changes to close remaining Java→managed activation paths that could recreate the same peer and leak JNI global references, plus additional test coverage to prevent regressions.
Changes:
- Update reflection-based activation paths to create a proper global ref via
ConstructPeer()before invoking managed constructors, withActivatable|Replaceablestate set upfront. - Add
JavaPeerProxy.ConstructActivatedPeer()and emit calls to it in the trimmable typemap generator for inherited-ctor activation/UCO paths. - Add/extend tests to validate gref-leak behavior and ensure the generator references the new helper.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Android.Widget/CustomWidgetTests.cs | Adds a regression test asserting that inflating custom views doesn’t leak global refs. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Extends generator tests to assert the new helper is referenced. |
| src/Mono.Android/Microsoft.Android.Runtime/SimpleValueManager.cs | Fixes reflection activation to set state and construct the peer before invoking ctors. |
| src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalValueManager.cs | Same activation fix as SimpleValueManager for the JavaMarshal path. |
| src/Mono.Android/Java.Interop/TypeManager.cs | Adjusts legacy activation to use uninitialized object + ConstructPeer() before ctor invocation. |
| src/Mono.Android/Java.Interop/JavaPeerProxy.cs | Introduces ConstructActivatedPeer() helper for generated proxy activation paths. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Emits calls to ConstructActivatedPeer() in inherited-ctor CreateInstance/UCO constructor IL. |
simonrozsival
changed the title
…opilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
force-pushed
the
dev/simonrozsival/11101-valuemanager-followup
branch
from
798dfeb to
d49eaab
Compare
jonathanpeppers
force-pushed
the
dev/peppers/fix-activate-gref-leak
branch
from
c64fdb9 to
31a44fa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #11112.
This PR fixes:
JavaMarshalValueManager.ActivateViaReflection()SimpleValueManager.ActivateViaReflection()