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

Fix memory leak on touchBehavior android #2113

Merged
merged 16 commits into from
Aug 16, 2024

Conversation

pictos
Copy link
Member

@pictos pictos commented Aug 11, 2024

Description of Change

This PR fixes the memory leak on Android's implementation of TouchBehavior, and adds a new type WeakWrapper, that is to help maintainer and contributors to create and use WeakReference types.

From my investigations the issue was because of the strong TouchBehavior reference inside the AccessibilityListener, changing to be a WeakReference fixes the leak, as you can see in the video below.

Screenbits.2024-08-11_162437.mp4

also, from the video, you can see it will say there's a leak, but in fact the object will take a while to be free, probably it implements a Finalizer which will make it live longer. Just saying, because some user can complain about it.

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

@pictos pictos requested a review from a team August 11, 2024 23:35
@pictos pictos force-pushed the pj/memory-leak-fix-touchbehavior branch from 3dfa8be to 5ceea9f Compare August 12, 2024 06:55
brminnick
brminnick previously approved these changes Aug 13, 2024
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pedro!!

Just FYI - I removed WeakWrapper since we were only using it once in TouchBehavior.android.cs; we can always add it later if we end up relying on WeakReference<T> more.

I also updated Dispose(bool) to properly dispose of the WeakReference.Target be setting it to null.

@brminnick brminnick enabled auto-merge (squash) August 13, 2024 18:28
`Install Tizen Workload` now fails fast
@pictos
Copy link
Member Author

pictos commented Aug 13, 2024

@brminnick the Dispose method isn't being called, because of that I used the WeakReference, with that there's no need to clean anything. The reason the cleanup isn't being executed is because the Element is set to null before the OnDetaching method being called. I strongly recommend to revert the change to previous commits

@brminnick
Copy link
Collaborator

the Dispose method isn't being called

Is this a bug in CommunityToolkit.Maui? If so, we should fix it.

I see we dispose our instance of this class (accessibilityListener.Dispose();) in partial void PlatformDispose(), so I assume in your research that you found it to be a bug in net8.0-android, and obviously there's nothing we can do about that.

I used the WeakReference, with that there's no need to clean anything.

Two thoughts:

  1. It's still a best practice to set WeakReference.Target to null when we are disposing the object; doing this does not introduce any harm
  2. Since AccessibilityListener implements IDisposable, we should continue override Dispose(bool) and follow best practices; if one day net8.0-android fixes this Dispose bug we will be prepared.

@pictos
Copy link
Member Author

pictos commented Aug 14, 2024

Is this a bug in CommunityToolkit.Maui? If so, we should fix it.

I'm not sure if it's a bug or the way the BaseBehavior works, I assumed isn't a bug. The Element here is null because this piece is executed before, leading the clean-up to leave early.

So, there's no bug in the .net-android

It's still a best practice to set WeakReference.Target to null when we are disposing the object; doing this does not introduce any harm

not sure about this one... But in this case, it doesn't make any difference just add more code that will not be executed

@brminnick brminnick merged commit f0f6059 into main Aug 16, 2024
7 checks passed
@brminnick brminnick deleted the pj/memory-leak-fix-touchbehavior branch August 16, 2024 15:09
@rafalka
Copy link
Contributor

rafalka commented Aug 16, 2024

Hi,
Good job but there's another memory leak on Android...
Take look into OnDetachedFrom()

	protected override void OnDetachedFrom(VisualElement bindable, AView platformView)
	{
		base.OnDetachedFrom(bindable, platformView);

		view = platformView;

		if (Element is null)
		{
			return;
		}
...

We exiting when Element is null.
The thing is that Element is always null because it returns a View base.OnDetachedFrom() calls UnassignViewAndBingingContext() which sets it to null.

I think that this null check is spare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] TouchBehavior Memory Leak once the behavior has been included in a DataTemplate
5 participants