From 272887a81345e5129193708af340ecb890bae98d Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 24 Apr 2024 20:09:02 -0500 Subject: [PATCH] [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) {