-
Notifications
You must be signed in to change notification settings - Fork 569
Fix JNI global ref leak in TypeManager.Activate and ValueManager.ActivateViaReflection #11111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
simonrozsival
wants to merge
5
commits into
main
from
dev/simonrozsival/investigate-11101-layoutinflater-memory-leak
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
15fa01d
Add diagnostic logging for peer lifecycle tracing
simonrozsival 65c7a53
Add RemovePeer logging for peer cleanup tracking
simonrozsival 7debed6
Fix race condition: use ConstructPeer before constructor in activation
simonrozsival 527cb22
Fix SimpleValueManager + guard diagnostic logging
simonrozsival 00e04ed
Add stress test for global ref leak during repeated inflation
simonrozsival File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
129 changes: 129 additions & 0 deletions
129
tests/Mono.Android-Tests/Mono.Android-Tests/Android.Views/InflatedCustomViewTests.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| using System; | ||
| using Android.App; | ||
| using Android.Content; | ||
| using Android.Runtime; | ||
| using Android.Util; | ||
| using Android.Views; | ||
| using Java.Interop; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace Xamarin.Android.RuntimeTests; | ||
|
|
||
| // https://github.com/dotnet/android/issues/11101 | ||
| [TestFixture] | ||
| public class InflatedCustomViewTests | ||
| { | ||
| [Test] | ||
| public void InflatedCustomView_HasValidPeerReference () | ||
| { | ||
| var inflater = LayoutInflater.From (Application.Context)!; | ||
| var layout = inflater.Inflate (Resource.Layout.inflated_custom_view, null, false)!; | ||
|
|
||
|
Comment on lines
+19
to
+21
|
||
| // Find our custom view in the inflated layout | ||
| var customView = FindCustomView (layout); | ||
|
|
||
| Assert.IsNotNull (customView, "Custom view should be found in inflated layout"); | ||
|
|
||
| // After inflation via Java-initiated activation, the peer should have a | ||
| // properly managed global JNI reference, not a raw local ref with Invalid type. | ||
| var peerRef = customView!.PeerReference; | ||
| Assert.IsTrue (peerRef.IsValid, "PeerReference should be valid"); | ||
|
Comment on lines
+29
to
+30
|
||
| Assert.AreNotEqual ( | ||
| JniObjectReferenceType.Invalid, | ||
| peerRef.Type, | ||
| "PeerReference.Type should not be Invalid — it should be a Global ref"); | ||
|
|
||
| // The peer should be registered so PeekObject can find it | ||
| var peeked = Java.Lang.Object.PeekObject (customView.Handle); | ||
| Assert.IsNotNull (peeked, "PeekObject should find the registered peer"); | ||
| Assert.AreSame (customView, peeked, "PeekObject should return the same instance"); | ||
| } | ||
|
|
||
| [Test] | ||
| public void InflatedCustomView_CanBeCollected () | ||
| { | ||
| WeakReference? weakRef = null; | ||
|
|
||
| // Create and discard the inflated view on a separate thread | ||
| // to avoid any local variable keeping it alive | ||
| var t = new System.Threading.Thread (() => { | ||
| var inflater = LayoutInflater.From (Application.Context)!; | ||
| var layout = inflater.Inflate (Resource.Layout.inflated_custom_view, null, false)!; | ||
| var customView = FindCustomView (layout); | ||
| Assert.IsNotNull (customView, "Custom view should be found in inflated layout"); | ||
| weakRef = new WeakReference (customView); | ||
| }); | ||
| t.Start (); | ||
| t.Join (); | ||
|
|
||
| // Force GC + bridge processing | ||
| GC.Collect (); | ||
| GC.WaitForPendingFinalizers (); | ||
| GC.Collect (); | ||
| GC.WaitForPendingFinalizers (); | ||
|
|
||
| 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)"); | ||
|
Comment on lines
+65
to
+67
|
||
| } | ||
|
|
||
| // Stress test: repeated inflation + GC to trigger the race condition | ||
| // between Activate.SetPeerReference and ConstructPeer. Under the bug, | ||
| // each race hit leaks a JNI global ref, so gref count grows unboundedly. | ||
| [Test] | ||
| public void InflatedCustomView_RepeatedInflation_DoesNotLeakGlobalRefs () | ||
| { | ||
| int initialGrefCount = Java.Interop.JniEnvironment.Runtime.GlobalReferenceCount; | ||
|
|
||
| for (int i = 0; i < 50; i++) { | ||
| var t = new System.Threading.Thread (() => { | ||
| var inflater = LayoutInflater.From (Application.Context)!; | ||
| inflater.Inflate (Resource.Layout.inflated_custom_view, null, false); | ||
| }); | ||
| t.Start (); | ||
| t.Join (); | ||
| } | ||
|
|
||
| GC.Collect (); | ||
| GC.WaitForPendingFinalizers (); | ||
| GC.Collect (); | ||
| GC.WaitForPendingFinalizers (); | ||
|
|
||
| int finalGrefCount = Java.Interop.JniEnvironment.Runtime.GlobalReferenceCount; | ||
|
|
||
| // Allow some tolerance — other code may allocate/release grefs. | ||
| // The key assertion is that gref count doesn't grow proportionally | ||
| // to the number of inflations (under the bug, each inflation | ||
| // leaks ~1 gref, so 50 inflations would leak ~50 grefs). | ||
| int leaked = finalGrefCount - initialGrefCount; | ||
| Assert.Less (leaked, 10, | ||
| $"Global reference count grew by {leaked} after 50 inflations — " + | ||
| $"expected near-zero growth after GC (initial={initialGrefCount}, final={finalGrefCount})"); | ||
| } | ||
|
|
||
| static InflatedCustomView? FindCustomView (View root) | ||
| { | ||
| if (root is InflatedCustomView customView) | ||
| return customView; | ||
|
|
||
| if (root is ViewGroup viewGroup) { | ||
| for (int i = 0; i < viewGroup.ChildCount; i++) { | ||
| var child = viewGroup.GetChildAt (i); | ||
| if (child is InflatedCustomView found) | ||
| return found; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
|
|
||
| // A simple custom view that can be inflated from XML | ||
| public sealed class InflatedCustomView : View | ||
| { | ||
| public InflatedCustomView (Context? context) : base (context) { } | ||
| public InflatedCustomView (nint javaReference, JniHandleOwnership transfer) : base (javaReference, transfer) { } | ||
| public InflatedCustomView (Context? context, IAttributeSet? attrs) : base (context, attrs) { } | ||
| public InflatedCustomView (Context? context, IAttributeSet? attrs, int defStyleAttr) : base (context, attrs, defStyleAttr) { } | ||
| public InflatedCustomView (Context? context, IAttributeSet? attrs, int defStyleAttr, int defStyleRes) : base (context, attrs, defStyleAttr, defStyleRes) { } | ||
| } | ||
11 changes: 11 additions & 0 deletions
11
tests/Mono.Android-Tests/Mono.Android-Tests/Resources/layout/inflated_custom_view.axml
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <FrameLayout | ||
| xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent"> | ||
|
|
||
| <Xamarin.Android.RuntimeTests.InflatedCustomView | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" /> | ||
|
|
||
| </FrameLayout> |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peeris declared in the pattern match (if (newobj is IJavaPeerable peer)) and is out of scope after theifblock. The logging aftercinfo.Invoke()referencespeer, which will not compile. Move the post-ctor log inside theifblock, or hoist a separateIJavaPeerable peervariable outside theifand assign it before invoking the constructor.