From 3ce27c9f507cbc96cfeeae76b29cf83b1e5e6e25 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 24 Apr 2024 20:09:02 -0500 Subject: [PATCH 1/2] [Mono.Android] fix potential leak of RunnableImplementors (#8900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/dotnet/maui/issues/18757 Context: https://github.com/dotnet/maui/pull/22007 Context: https://github.com/dotnet/maui/blob/440fa7f6ff5602e9cbe23249df5d13c45fb261e1/src/Controls/src/Core/Compatibility/Handlers/ListView/Android/ListViewRenderer.cs#L484-L491 Context: https://github.com/xamarin/monodroid/commit/f4ffb99facfe4f77ed07230a9d4450ea5451e0c5 Context: 5777337e7874a8f2361998ef022db85ba9fc620a [`android.view.View.post(Runnable)`][0] adds a `Runnable` callback to the message queue, to be later run on the UI thread. Commit xamarin/monodroid@f4ffb99f, imported in 5777337e, added a `View.Post(Action)` overload for this method to make it easier to use. There is also a [`View.removeCallbacks(Runnable)`][1] method which allows removing the specified callback from the message queue. A `View.RemoveCallbacks(Action)` method was also added, permitting: Action a = () => …; View v = new View(context); v.Post(a); v.RemoveCallbacks(a); To make this work, we needed a way look up the `java.lang.Runnable` that corresponds to a given `Action`. Enter `RunnableImplementor` and `RunnableImplementor.instances`: namespace Java.Lang; partial class Thread { internal partial class RunnableImplementor : Java.Lang.Object, IRunnable { public RunnableImplementor (Action handler, bool removable = false); public void Run(); static Dictionary instances; public static RunnableImplementor Remove(Action handler); } } which can be used in the `View` overloads (simplified for exposition): namespace Android.Views; partial class View { public bool Post(Action action) => Post(new RunnableImplementor(action, removable: true)); public bool RemoveCallbacks(Action action) => RemoveCallbacks(RunnableImplementor.Remove(action)); } This works, but there's a problem [^0]: when `removable:true` is used, then `handler` is added to `instances`, and `handler` is only removed when: 1. `RunnableImplementor.Run()` is invoked, or 2. `RunnableImplementor.Remove(Action)` is invoked. `RunnableImplementor.Remove(Action)` is only invoked if `View.RemoveAction()` is invoked. Thus, the question: is there a way to use `View.Post()` and *not* invoke `RunnableImplementor.Run()`? Turns Out™, ***Yes***: var v = new View(context); v.Post(() => …); then…do nothing with `v`. While the `View.post(Runnable)` docs state: > Causes the Runnable to be added to the message queue. > The runnable will be run on the user interface thread. that's not *quite* true. More specifically, the `Runnable`s added to the `View` via `View.post(Runnable)` are only *actually* added to the message queue *if and when* the `View` is added to a `Window` [^1]. If the `View` is never added to a `Window`, then *all the `Runnable`s are never invoked*. Which brings us to the C# leak: if we call `View.Post(Action)` and never add the `View` to a `Window`, then `RunnableImplementor.Run()` is never executed. If `RunnableImplementor.Run()` isn't executed, then the `RunnableImplementor` instance will never be removed from `RunnableImplementor.instances`, and as `instances` is a "GC root" it will keep *all of* the `RunnableImplementor` instance, the Java-side `RunnableImplementor` peer instance (via GREF), and the `Action` alive, forever. I could observe this behavior in a MAUI unit test that: 1. Creates a `ListView` 2. Creates the platform view that implements `ListView` 3. Never adds any of these objects to the `Window` 4. Makes sure none of the things leak -- *this fails* Fix this by changing `RunnableImplementor.instances` to be a `ConditionalWeakTable`. This *prevents* `RunnableImplementor.instances` from acting as a GC root, allowing both the `Action` keys and `RunnableImplementor` values to be collected. dotnet/maui#18757 is more or less the same scenario, with one added Reproduction step that should be called out: > * Calling `View.Post(Action)` repeatedly (e.g. in a loop, on a > timer etc) will eventually cause the application to crash with > the message global reference table overflow *This particular part is not solvable*. Android has a GREF limit of ~52000 entries. If you try to create ~52000 GREFs in a way that doesn't allow the GC to collect things, then the app will crash, and there is nothing we can do about it: var v = new View(context); for (int i = 0; i < 53000; ++i) { int x = i; v.Post(() => Console.WriteLine(x)); } // Boom; attempting to add 53k Actions to a View will require // creating 53k `RunnableImplementor` instances, which will // create 53k GREFs, which will cause a Very Bad Time™. TODO: Address [^0] and dispose of the `RunnableImplementor` instance when `View.Post()` returns `false`. [0]: https://developer.android.com/reference/android/view/View#post(java.lang.Runnable) [1]: https://developer.android.com/reference/android/view/View#removeCallbacks(java.lang.Runnable) [2]: https://cs.android.com/android/platform/superproject/+/main:frameworks/base/core/java/android/view/View.java;l=19618 [3]: https://cs.android.com/android/platform/superproject/+/main:frameworks/base/core/java/android/view/View.java;l=21363 [^0]: There's at least two problems; another problem is that we leak if `View.post(Runnable)` returns *false*. This will be addressed later. [^1]: If you never add the `View` to a `Window`, then the `Runnables` just hang around until the GC decides to collect them: 1. When a `View` is *not* attached to a `Window`, then `View.mAttachInfo` is null, [so we call `getRunQueue()`][2]: public boolean post(Runnable action) { … getRunQueue().post(action); return true; } 2. `View.getRunQueue()` creates a `HandlerActionQueue`, sets `View.mRunQueue`. 3. The only place `View.mRunQueue` is "meaningfully used" is within [`View.dispatchAttachedToWindow()`][3], which "transfers" the `Runnables` within the `HandlerActionQueue` to the UI handler. 4. `View.dispatchAttachedToWindow()` is only executed when the `View` is attacked to a `Window`. --- src/Mono.Android/Java.Lang/Thread.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Mono.Android/Java.Lang/Thread.cs b/src/Mono.Android/Java.Lang/Thread.cs index 8c6155f41d3..cc45dd16aed 100644 --- a/src/Mono.Android/Java.Lang/Thread.cs +++ b/src/Mono.Android/Java.Lang/Thread.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using Android.Runtime; @@ -27,7 +28,7 @@ public RunnableImplementor (Action handler, bool removable) this.removable = removable; if (removable) lock (instances) - instances [handler] = this; + instances.AddOrUpdate (handler, this); } public void Run () @@ -41,7 +42,7 @@ public void Run () Dispose (); } - static Dictionary instances = new Dictionary (); + static ConditionalWeakTable instances = new (); public static RunnableImplementor Remove (Action handler) { From 8e1871b032d62840166438b516b4c0c76fb617ef Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Thu, 25 Apr 2024 20:22:26 -0500 Subject: [PATCH 2/2] [Xamarin.Android.Build.Tasks] fix detection of "Android libraries" (#8904) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://github.com/dotnet/maui/issues/18819 Context: https://github.com/kernshen/NoJavaPeer.git Context: https://github.com/xamarin/xamarin-android/pull/4225#issuecomment-583139797 An assembly's assembly references do not include transitive dependencies. Given: // Mono.Android.dll namespace Java.Lang { public partial class Object {} } // MauiLib1.dll namespace MauiLib1 { public class BaseClass : Java.Lang.Object {} } // MauiLib2.dll namespace MauiLib2 { public class DerivedClass3 : MauiLib1.BaseClass {} } then the assembly references for `MauiLib1.dll` will include `Mono.Android.dll` (it directly references a type from it), while the assembly references for `MauiLib2.dll` will include `MauiLib1.dll` (it directly references a type from it) *but* `MauiLib2.dll` *will not* have an assembly reference to `Mono.Android.dll`. This is how things have worked since .NET Framework 1.0. This should not be surprising. [As part of the .NET for Android][0] [`SignAndroidPackage`][1] target, Java Callable Wrappers (JCWs) need to be emitted for all `Java.Lang.Object` subclasses. This in turn requires *loading all assemblies* to *find* the `Java.Lang.Object` subclasses. As a performance optimization, we only load assemblies which we believed could contain `Java.Lang.Object` subclasses: 1. Assemblies with `'%(TargetFrameworkIdentifier)' == 'MonoAndroid'`, which is "carry over" from how Xamarin.Android did things, and works if a .NET for Android project references a Xamarin.Android project. 2. Assemblies with an assembly reference to `Mono.Android.dll`. Assemblies with transitive dependencies were caught by (1)… in Xamarin.Android. With .NET for Android, that is no longer the case: `%(TargetFrameworkIdentifier)` is now always `.NETCoreApp`. This in turn meant that the only assemblies that could be used to generate JCWs were those which directly referenced `Mono.Android.dll`! Enter dotnet/maui#18819 and kernshen/NoJavaPeer, which contains MAUI and .NET for Android solutions with the above transitive reference structure: 1. `MauiLib1.dll` / `AndroidLib1.dll` references `Mono.Android.dll`, exports `BaseClass` 2. `MauiLib2.dll` / `AndroidLib2.dll` references `*Lib1.dll` *and not* `Mono.Android.dll`; exports `DerivedClass3` which inherits `BaseClass`. 3. App project attempts to instantiate `DerivedClass3`. The result: a runtime exception: Only System.NotSupportedException Message=Cannot create instance of type 'MauiLib2.DerivedClass3': no Java peer type found. at Java.Interop.JniPeerMembers.JniInstanceMethods..ctor(Type declaringType) in /Users/runner/work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs:line 22 at Java.Interop.JniPeerMembers.JniInstanceMethods.GetConstructorsForType(Type declaringType) in /Users/runner/work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs:line 77 at Java.Interop.JniPeerMembers.JniInstanceMethods.StartCreateInstance(String constructorSignature, Type declaringType, JniArgumentValue* parameters) in /Users/runner/work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs:line 139 at Java.Lang.Object..ctor() in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net8.0/android-34/mcw/Java.Lang.Object.cs:line 37 at MauiLib1.BaseClass..ctor() at MauiLib2.DerivedClass..ctor() in C:\Project\Maui\MauiApp1\MauiLib2\Platforms\Android\DerivedClass.cs:line 7 at MauiApp1.App..ctor(IServiceProvider serviceProvider) in C:\Project\Maui\MauiApp1\MauiApp1\App.xaml.cs:line 18 The exception occurs because there is no JCW for `DerivedClass3`, and there isn't a JCW for `DerivedClass3` because `MauiLib2.dll` was not processed at all, because it had no assembly reference to `Mono.Android.dll`. As a workaround, update `MauiLib2.dll` to contain an assembly reference to `Mono.Android.dll`. *Fix* this scenario by updating `MonoAndroidHelper.IsMonoAndroidAssembly()` to consider these to be .NET for Android assemblies: 1. Assemblies with `%(TargetFrameworkIdentifier)` *containing* `Android`. (This doesn't actually change anything; it's a simplification.) 2. Assemblies with `%(TargetPlatformIdentifier)` *containing* `Android`. *This* causes `MauiLib2.dll` to be treated as a .NET for Android assembly, fixing the bug. 3. Assemblies with an assembly reference to `Mono.Android.dll`. The addition of check (2) allows assemblies with only transitive (non-) references to `Mono.Android.dll` to be properly considered, allowing JCWs to be emitted for types within them. Update the `BuildWithLibraryTests.ProjectDependencies()` unit test to better check for this scenario. [0]: https://github.com/xamarin/xamarin-android/wiki/Blueprint#after-build [1]: https://learn.microsoft.com/en-us/dotnet/android/building-apps/build-targets#signandroidpackage --- .../BuildWithLibraryTests.cs | 27 +++++++++++++------ .../Utilities/MonoAndroidHelper.cs | 7 ++++- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs index e1083a3cef2..33f75fccbea 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildWithLibraryTests.cs @@ -269,11 +269,7 @@ public void ProjectDependencies ([Values(true, false)] bool projectReference) }; libB.Sources.Clear (); libB.Sources.Add (new BuildItem.Source ("Foo.cs") { - TextContent = () => @"public class Foo { - public Foo () { - var bar = new Bar(); - } - }", + TextContent = () => "public class Foo : Bar { }", }); var libC = new XamarinAndroidLibraryProject () { @@ -283,7 +279,7 @@ public Foo () { }; libC.Sources.Clear (); libC.Sources.Add (new BuildItem.Source ("Bar.cs") { - TextContent = () => "public class Bar { }", + TextContent = () => "public class Bar : Java.Lang.Object { }", }); libC.Sources.Add (new BuildItem ("EmbeddedResource", "Foo.resx") { TextContent = () => InlineData.ResxWithContents ("Cancel") @@ -309,8 +305,8 @@ public Foo () { ProjectName = "AppA", IsRelease = true, Sources = { - new BuildItem.Source ("Bar.cs") { - TextContent = () => "public class Bar : Foo { }", + new BuildItem.Source ("Baz.cs") { + TextContent = () => "public class Baz : Foo { }", }, new BuildItem ("EmbeddedResource", "Foo.resx") { TextContent = () => InlineData.ResxWithContents ("Cancel") @@ -321,6 +317,10 @@ public Foo () { } }; appA.AddReference (libB); + if (!projectReference) { + // @(ProjectReference) implicits adds this reference. For `class Baz : Foo : Bar`: + appA.OtherBuildItems.Add (new BuildItem.Reference ($@"..\{libC.ProjectName}\bin\Release\{libC.TargetFramework}\{libC.ProjectName}.dll")); + } var appBuilder = CreateApkBuilder (Path.Combine (path, appA.ProjectName)); Assert.IsTrue (appBuilder.Build (appA), $"{appA.ProjectName} should succeed"); @@ -332,6 +332,17 @@ public Foo () { helper.AssertContainsEntry ($"assemblies/{libC.ProjectName}.dll"); helper.AssertContainsEntry ($"assemblies/es/{appA.ProjectName}.resources.dll"); helper.AssertContainsEntry ($"assemblies/es/{libC.ProjectName}.resources.dll"); + + var intermediate = Path.Combine (Root, appBuilder.ProjectDirectory, appA.IntermediateOutputPath); + var dexFile = Path.Combine (intermediate, "android", "bin", "classes.dex"); + FileAssert.Exists (dexFile); + + // NOTE: the crc hashes here might change one day, but if we used [Android.Runtime.Register("")] + // LibraryB.dll would have a reference to Mono.Android.dll, which invalidates the test. + string className = "Lcrc6414a4b78410c343a2/Bar;"; + Assert.IsTrue (DexUtils.ContainsClass (className, dexFile, AndroidSdkPath), $"`{dexFile}` should include `{className}`!"); + className = "Lcrc646d2d82b4d8b39bd8/Foo;"; + Assert.IsTrue (DexUtils.ContainsClass (className, dexFile, AndroidSdkPath), $"`{dexFile}` should include `{className}`!"); } [Test] diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs b/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs index 8fb845dd9df..dc768408fe3 100644 --- a/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs +++ b/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs @@ -277,8 +277,13 @@ public static void LogWarning (object log, string msg, params object [] args) #if MSBUILD public static bool IsMonoAndroidAssembly (ITaskItem assembly) { + // NOTE: look for both MonoAndroid and Android var tfi = assembly.GetMetadata ("TargetFrameworkIdentifier"); - if (string.Compare (tfi, "MonoAndroid", StringComparison.OrdinalIgnoreCase) == 0) + if (tfi.IndexOf ("Android", StringComparison.OrdinalIgnoreCase) != -1) + return true; + + var tpi = assembly.GetMetadata ("TargetPlatformIdentifier"); + if (tpi.IndexOf ("Android", StringComparison.OrdinalIgnoreCase) != -1) return true; var hasReference = assembly.GetMetadata ("HasMonoAndroidReference");