Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mono.Android] fix potential leak in Java.Lang.Thread #8900

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

jonathanpeppers
Copy link
Member

Fixes: dotnet/maui#18757
Context: dotnet/maui#22007
Context: https://cs.android.com/android/platform/superproject/+/main:frameworks/base/core/java/android/view/View.java;l=19612

If you do something like:

new View(myContext).Post(() => {
    // do something
});

If the View is never added to the Window (IsAttachedToWindow is false), the Runnable will never be executed, with data stored in this dictionary:

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

This is a problem if the Action:

  • Is an instance method, will cause the Action.Target to live forever

  • Is an anonymous method with captured variables, will cause the captured variables to live forever

I could observe this behavior in a MAUI unit test that:

  • Creates a ListView

  • Creates the platform view that implements ListView

  • Never adds any of these objects to the Window

  • Makes sure none of the things leak -- this fails

This seems less likely to occur in a real application, but it is still a bug.

Fixes: dotnet/maui#18757
Context: dotnet/maui#22007
Context: https://cs.android.com/android/platform/superproject/+/main:frameworks/base/core/java/android/view/View.java;l=19612

If you do something like:

    new View(myContext).Post(() => {
        // do something
    });

If the `View` is never added to the `Window` (`IsAttachedToWindow` is
false), the `Runnable` will never be executed, with data stored in
this dictionary:

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

This is a problem if the `Action`:

* Is an instance method, will cause the `Action.Target` to live forever

* Is an anonymous method with captured variables, will cause the
  captured variables to live forever

I could observe this behavior in a MAUI unit test that:

* Creates a `ListView`

* Creates the platform view that implements `ListView`

* Never adds any of these objects to the `Window`

* Makes sure none of the things leak -- *this fails*

This seems less likely to occur in a real application, but it is still
a bug.
@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Apr 24, 2024

I manually tested in MAUI by doing:

<Reference Include="D:\src\xamarin-android\bin\Debug\dotnet\packs\Microsoft.Android.Runtime.34.android-arm\34.99.0-preview.5.265\runtimes\android-arm\lib\net9.0\Mono.Android.dll" />

I can see my code changes:
image

image

(26 seconds, because I had breakpoints)

@jonpryor
Copy link
Member

Context: https://github.com/dotnet/maui/issues/18757
Context: https://github.com/dotnet/maui/pull/22007
Context: https://cs.android.com/android/platform/superproject/+/main:frameworks/base/core/java/android/view/View.java;l=19612
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 need 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 `Runnables` 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 3k GREFs, which will cause a Very Bad Time™.

[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.

@jonpryor jonpryor merged commit 3ce27c9 into dotnet:main Apr 25, 2024
47 checks passed
grendello added a commit that referenced this pull request Apr 25, 2024
* main:
  [Mono.Android] fix potential leak of RunnableImplementors (#8900)
grendello pushed a commit that referenced this pull request Apr 25, 2024
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`.
grendello added a commit that referenced this pull request Apr 25, 2024
* main: (26 commits)
  [Mono.Android] fix potential leak of RunnableImplementors (#8900)
  [build] Bump $(AndroidNetPreviousVersion) to 34.0.95 (#8898)
  [docs] Reorganize public Documentation (#8889)
  Bump external/Java.Interop from `06214ff` to `6cfa78c` (#8879)
  Localized file check-in by OneLocBuild Task (#8894)
  $(AndroidPackVersionSuffix)=preview.5; net9 is 34.99.0.preview.5 (#8896)
  Bump to xamarin/monodroid@a5742221b3 (#8893)
  Remove MonoArchive_BaseUri from Configurables.cs (#8890)
  Bump to xamarin/xamarin-android-binutils/L_18.1.4-8.0.0@758d2e7 (#8885)
  [Mono.Android] Bind API-VanillaIceCream Beta 1 (#8891)
  [AndroidToolTask] Log tool output as error  (#8861)
  [Xamarin.Android.Build.Tasks] Remove "Xamarin" from messages (#8884)
  [Mono.Android] Bind API-VanillaIceCream Developer Preview 2 (#8741)
  Bump to dotnet/installer@22ffa42d6c 9.0.100-preview.4.24221.5 (#8887)
  Bump external/xamarin-android-tools from `37d79c9` to `05f9a90` (#8869)
  Bump external/Java.Interop from `e1c7832` to `06214ff` (#8878)
  Bump to dotnet/installer@7380c301c1 9.0.100-preview.4.24215.2 (#8874)
  [Mono.Android] Commit baseline PublicAPI files for API-35 (#8840)
  Add a unit test to check environment processing (#8856)
  Don't use azureedge.net CDN (#8846)
  ...
@jonathanpeppers jonathanpeppers deleted the Java.Lang.Thread-Leak branch April 25, 2024 16:31
grendello added a commit that referenced this pull request Apr 26, 2024
* main:
  [Xamarin.Android.Build.Tasks] fix detection of "Android libraries" (#8904)
  [Mono.Android] fix potential leak of RunnableImplementors (#8900)
jonathanpeppers added a commit that referenced this pull request Apr 29, 2024
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`.
jonathanpeppers added a commit that referenced this pull request May 6, 2024
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`.
grendello added a commit that referenced this pull request May 6, 2024
* release/8.0.2xx:
  [Mono.Android] fix potential leak of RunnableImplementors (#8900)
  Bump to xamarin/monodroid@21aed734 (#8905)
  [Xamarin.Android.Build.Tasks] fix detection of "Android libraries" (#8904)
  [ci] Do not use @self annotation for templates (#8783)
  Bump to xamarin/xamarin-android-tools/release/8.0.1xx@d50747cb (#8892)
  [Xamarin.Android.Build.Tasks] Remove "Xamarin" from messages (#8884)
  [ci] Use NuGetAuthenticate@1 (#8877)
  [ci] Migrate to the 1ES template (#8876)
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants