Commit 0315e89
committed
[Mono.Android] fix potential leak of RunnableImplementors (#8900)
Context: dotnet/maui#18757
Context: dotnet/maui#22007
Context: https://github.com/dotnet/maui/blob/440fa7f6ff5602e9cbe23249df5d13c45fb261e1/src/Controls/src/Core/Compatibility/Handlers/ListView/Android/ListViewRenderer.cs#L484-L491
Context: xamarin/monodroid@f4ffb99
Context: 5777337
[`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 5777337, 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<Action, RunnableImplementor> 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<Action, RunnableImplementor>`. 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`.1 parent ea43008 commit 0315e89
1 file changed
+3
-2
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
| 3 | + | |
3 | 4 | | |
4 | 5 | | |
5 | 6 | | |
| |||
27 | 28 | | |
28 | 29 | | |
29 | 30 | | |
30 | | - | |
| 31 | + | |
31 | 32 | | |
32 | 33 | | |
33 | 34 | | |
| |||
41 | 42 | | |
42 | 43 | | |
43 | 44 | | |
44 | | - | |
| 45 | + | |
45 | 46 | | |
46 | 47 | | |
47 | 48 | | |
| |||
0 commit comments