You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
[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`.
0 commit comments