[ios] fix memory leak in Image #15062

jonathanpeppers · 2023-05-12T20:59:29Z

Image has two different "cycles" on iOS:

ImageHandler -> MauiImageView -> ImageHandler
- via the WindowChanged event
ImageHandler -> ImageSourcePartLoader -> ImageHandler
- via Handler, Func<IImageSourcePart?>, and Action<PlatformImage?>

This causes any MauiImageView to live forever.

To solve these issues:

Get rid of the MauiImageView.WindowChanged event, and use a WeakReference to the handler instead.
ImageSourcePartLoader now only has a WeakReference to the handler.
This requires a new ISetImageHandler interface to be used by several handler types involving images.

Hopefully the changes here are using [Obsolete] correctly to make things backwards compatible & less breaking changes.

Unsure yet if this fully solves #14664, but at least one part of it.

src/Core/src/Platform/ImageSourcePartLoader.cs

jonathanpeppers · 2023-05-16T16:14:09Z

This does not fully solve: #14664

It appears there are other cycles in the customer sample:

Here is one of them:

maui/src/Controls/src/Core/Platform/GestureManager/GestureManager.cs

Lines 15 to 18 in c3f269c

    
           readonly IControlsView _view; 
        
           object? _containerView; 
        
           object? _platformView; 
        
           object? _handler;

We can merge this one as a starting place, and maybe adding the views to the screen in my test would trigger more issues?

src/Core/src/Platform/ImageSourcePartLoader.cs

mattleibow · 2023-05-16T20:26:01Z

src/Core/src/Platform/iOS/MauiImageView.cs

@@ -7,18 +7,34 @@ namespace Microsoft.Maui.Platform
 {
 	public class MauiImageView : UIImageView
 	{
+		WeakReference<ImageHandler>? _handler;


Maybe this can be a public property? This way we don't have to change API at all much - the event still works - we just don't use it? Sort of like the iOS bending of a weak delegate OR an event? We can still use the existing iOS constructors.

We can mark the bad one as obsolete just to let people know why - and us. Rather than just "it will be removed", maybe we can be "Use Handler instead. iOS platform views should not have strong references to handlers or cross-platform views."

src/Core/src/Handlers/Image/ISetImageHandler.cs

mattleibow · 2023-05-16T20:40:04Z

src/Core/src/Platform/iOS/MauiImageView.cs

+		{
+			if (_handler is not null && _handler.TryGetTarget(out var handler))
+			{
+				handler.NotifyWindowChanged();


I am not 100% happy with views reaching out into internal handlers as this means this view will never work for anything else, but for now maybe it is fine? The method of catching the iOS window change is not great and is a bit iffy - we could actually do all this in cross platform code. We now have a Window property. But this is more for another issue.

@PureWeen thoughts?

If we made this one public on IImageHandler and gave it a proper name, it would mean other IImageHandler could work?

rmarinho · 2023-05-30T23:56:49Z

/rebase

Context: dotnet#14664 (comment) `Image` has two different "cycles" on iOS: * `ImageHandler` -> `MauiImageView` -> `ImageHandler` * via the `WindowChanged` event * `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler` * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>` This causes any `MauiImageView` to live forever. To solve these issues: * Get rid of the `MauiImageView.WindowChanged` event, and use a `WeakReference` to the handler instead. * `ImageSourcePartLoader` now only has a `WeakReference` to the handler. * This requires a new `ISetImageHandler` interface to be used by several handler types involving images. Hopefully the changes here are using `[Obsolete]` correctly to make things backwards compatible & less breaking changes. Unsure yet if this fully solves dotnet#14664, but at least one part of it.

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

rmarinho · 2023-06-01T10:36:05Z

src/Core/src/Handlers/Image/ImageHandler.iOS.cs

 			PlatformView.Image = obj;

-		void OnWindowChanged(object? sender, EventArgs e)
+		public void OnWindowChanged()


Do we want this to be a public API ? doesn't seem consistent with other Handlers

rmarinho

My only comment is related with WindowChanged not sure if we want that public.

jonathanpeppers · 2023-06-01T13:51:45Z

If it isn't public, then no one can implement IImageHandler?. I did it mainly because of this comment:

#15062 (comment)

But, let me know what to do to get this merged.

* [ios] fix memory leak in Image Context: #14664 (comment) `Image` has two different "cycles" on iOS: * `ImageHandler` -> `MauiImageView` -> `ImageHandler` * via the `WindowChanged` event * `ImageHandler` -> `ImageSourcePartLoader` -> `ImageHandler` * via `Handler`, `Func<IImageSourcePart?>`, and `Action<PlatformImage?>` This causes any `MauiImageView` to live forever. To solve these issues: * Get rid of the `MauiImageView.WindowChanged` event, and use a `WeakReference` to the handler instead. * `ImageSourcePartLoader` now only has a `WeakReference` to the handler. * This requires a new `ISetImageHandler` interface to be used by several handler types involving images. Hopefully the changes here are using `[Obsolete]` correctly to make things backwards compatible & less breaking changes. Unsure yet if this fully solves #14664, but at least one part of it. * Update src/Core/src/Platform/ImageSourcePartLoader.cs Co-authored-by: Matthew Leibowitz <mattleibow@live.com> * Update MauiImageView.cs * Update MauiImageView.cs * ISetImageHandler -> IImageSourcePartSetter * IImageHandler.OnWindowChanged() is now public * Fix IImageSourcePartSetter namespace * MauiImageView._handler can be readonly --------- Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

PureWeen · 2023-06-28T16:50:51Z

There are currently a number of PRs that are all working together to fix the memory leak issues on iOS. Any of these PRs backported individually will most likely not fix the issues that users are currently seeing. We have a few reproductions that we are testing against where we still haven't quite fixed all of the memory leaks. Backporting this PR would most likely have a higher chance of causing regressions (38 files changed) than it would actually fixing the overall problem.

.NET8 is now part of VS so if you can verify your scenarios against .NET8 that will ensure we've fixed as many leaks as possible for .NET8.

If there are still leaks that users are finding in .NET8 after the release of .NET8 those will have a decent chance of being backported. We should be through the larger sets of changes by the time we hit .NET8 so post .NET8 changes should be smaller/safer.

jonathanpeppers force-pushed the ImageDoesNotLeak branch from c1e580f to 1643aba Compare May 12, 2023 21:22

jonathanpeppers added platform/iOS 🍎 legacy-area-perf Startup / Runtime performance labels May 12, 2023

rolfbjarne reviewed May 15, 2023

View reviewed changes

src/Core/src/Platform/ImageSourcePartLoader.cs Outdated Show resolved Hide resolved

jonathanpeppers force-pushed the ImageDoesNotLeak branch from 2367013 to 878e6cf Compare May 15, 2023 13:59

jonathanpeppers marked this pull request as ready for review May 15, 2023 15:33

jonathanpeppers requested review from mattleibow and PureWeen May 15, 2023 15:33

mattleibow reviewed May 16, 2023

View reviewed changes

jonathanpeppers force-pushed the ImageDoesNotLeak branch from d8af8fe to ab79dd5 Compare May 30, 2023 14:45

jonathanpeppers and others added 8 commits May 30, 2023 23:57

Update src/Core/src/Platform/ImageSourcePartLoader.cs

75d3e60

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>

Update MauiImageView.cs

d247846

Update MauiImageView.cs

f057f3b

ISetImageHandler -> IImageSourcePartSetter

71249db

IImageHandler.OnWindowChanged() is now public

3401281

Fix IImageSourcePartSetter namespace

22f4def

MauiImageView._handler can be readonly

1480730

github-actions bot force-pushed the ImageDoesNotLeak branch from ab79dd5 to 1480730 Compare May 30, 2023 23:57

rmarinho reviewed Jun 1, 2023

View reviewed changes

rmarinho approved these changes Jun 1, 2023

View reviewed changes

rmarinho merged commit 556f1c7 into dotnet:main Jun 14, 2023
28 checks passed

jonathanpeppers deleted the ImageDoesNotLeak branch June 14, 2023 13:35

Redth added the backport/suggested The PR author or issue review has suggested that the change should be backported. label Jun 20, 2023

PureWeen added the backport/NO This change should not be backported. It may break customers. label Jun 28, 2023

kcoop mentioned this pull request Jul 11, 2023

Tracing GC on iOS yields mono_coop_cond_signal Cannot transition thread 0x2016c7280 from STATE_BLOCKING with DO_BLOCKING dotnet/runtime#88642

Closed

jonathanpeppers added memory-leak 💦 Memory usage grows / objects live forever and removed legacy-area-perf Startup / Runtime performance labels Jul 12, 2023

APopatanasov mentioned this pull request Jul 26, 2023

[regression/8.0.0-preview.5.8529] Provide API to set ImageSource to the ImageSourcePartLoader #16367

Closed

jonathanpeppers mentioned this pull request Aug 31, 2023

Memory leaks - After running 24 hours it crashes #17049

Open

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] fix memory leak in Image #15062

[ios] fix memory leak in Image #15062

jonathanpeppers commented May 12, 2023

jonathanpeppers commented May 16, 2023 •

edited

Loading

mattleibow May 16, 2023

mattleibow May 16, 2023

mattleibow May 16, 2023

jonathanpeppers May 16, 2023

rmarinho commented May 30, 2023

rmarinho Jun 1, 2023

rmarinho left a comment

jonathanpeppers commented Jun 1, 2023

PureWeen commented Jun 28, 2023

[ios] fix memory leak in Image #15062

[ios] fix memory leak in Image #15062

Conversation

jonathanpeppers commented May 12, 2023

jonathanpeppers commented May 16, 2023 • edited Loading

mattleibow May 16, 2023

Choose a reason for hiding this comment

mattleibow May 16, 2023

Choose a reason for hiding this comment

mattleibow May 16, 2023

Choose a reason for hiding this comment

jonathanpeppers May 16, 2023

Choose a reason for hiding this comment

rmarinho commented May 30, 2023

rmarinho Jun 1, 2023

Choose a reason for hiding this comment

rmarinho left a comment

Choose a reason for hiding this comment

jonathanpeppers commented Jun 1, 2023

PureWeen commented Jun 28, 2023

jonathanpeppers commented May 16, 2023 •

edited

Loading