Skip to content

Commit

Permalink
[Mono.Android] fix potential leak of RunnableImplementors (#8900)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
jonathanpeppers authored Apr 25, 2024
1 parent 11dd4fe commit 3ce27c9
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/Mono.Android/Java.Lang/Thread.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

using Android.Runtime;
Expand Down Expand Up @@ -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 ()
Expand All @@ -41,7 +42,7 @@ public void Run ()
Dispose ();
}

static Dictionary<Action, RunnableImplementor> instances = new Dictionary<Action, RunnableImplementor> ();
static ConditionalWeakTable<Action, RunnableImplementor> instances = new ();

public static RunnableImplementor Remove (Action handler)
{
Expand Down

0 comments on commit 3ce27c9

Please sign in to comment.