FrameworkElementExtensions.Ancestor causes an XAML memory leak

[sylveon]

Describe the bug

Setting the AncestorType property on an object will store the parent of the object as an attached property, causing a reference loop (the parent holds a reference to the child, the child holds a reference to the parent).

• Is this bug a regression in the toolkit? If so, what toolkit version did you last see it work:

Steps to Reproduce

• Can this be reproduced in the Sample App? Technically yes, although the memory usage impact is hard to notice because there is a lot else going on in the sample app.

Steps to reproduce the behavior:

1. Use AncestorType in some XAML content
2. Load the XAML content (eg navigate to it in a frame)
3. Unload the XAML content (eg navigate away from it, remove it from the visual tree)
4. Notice that the XAML elements are never freed correctly, keep taking memory, and weak references will still resolve.

Expected behavior

The elements should be freed.

Screenshots

No screenshots needed.

Environment

NuGet Package(s): N/A

Package Version(s): N/A

Windows 10 Build Number:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [ ] May 2019 Update (18362)
- [x] May 2020 Update (19041)
- [ ] Insider Build (build number: )

App min and target version:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [ ] October 2018 Update (17763)
- [ ] May 2019 Update (18362)
- [x] May 2020 Update (19041)
- [x] Insider Build (22000)

Device form factor:
- [x] Desktop
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio
- [ ] 2017 (version: )
- [ ] 2019 (version: )
- [ ] 2019 Preview (version: )
- [x] 2022 Preview (version 17.0 preview 2.1)

Additional context

Found while doing a C++ port, so bug happens on latest commit. The solution to this issue would be to set the Ancestor attached property to null in the Unloaded event handler.

[ghost]

Hello sylveon, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[michael-hawker]

Thanks @sylveon for the report! We probably need to switch to a weak reference pattern here, eh?

Thoughts on approach @XAML-Knight @Sergio0694?

@XAML-Knight did you want to pick this one up to work on?

[sylveon]

I don't think attached properties can store weak references. At least, not from the C++ side. Adding an Unloaded event handler which just cleared the attached property worked for me to solve the issue.

[michael-hawker]

@sylveon just worried about hitting the unloaded bug and if there's more we can add to mitigate it potentially beyond that. 🙂

[XAML-Knight]

@XAML-Knight did you want to pick this one up to work on?

I'll take a look at this.

[XAML-Knight]

I've opened a draft PR: CommunityToolkit/WindowsCommunityToolkit#4187 , which implements the suggestion from @sylveon

Ancestor property will eventually be set to null (i.e., the unload event fires multiple times, and as stated, will finally set ancestor to null). However, still investigating the significance of the unloading bug #1900, and would we be better off finding a better solution as @michael-hawker has suggested.

[michael-hawker]

Yeah, thinking about it more, while we could setup a WeakReference internally, the usage pattern with Binding to grab the Ancestor would just have to unravel that anyway and then the bound property would still hold a strong-reference I think. It'd basically cascade up the chain of where we use it.

In the end we have to set the Ancestor value to null in order to release all the bound instances of it in the tree... right?

Wonder if we can at least check the basic behavior is occurring properly with a unit test too?

[sylveon]

In my use case I access a property of the ancestor control which is just an IObservableVector, so no reference loop happens at the usage site, the IObservableVector gets freed once both the parent control and the child control gets freed

[michael-hawker]

@sylveon with the move to the new repo I setup some better tests for this, but was unable to see this capture occur: 05ad5d1

I believe the Unloaded event you suggested, would be this one: 022b4bf#diff-580bc71d9c4f313076c44a4b78b36e7cbb2bc490039efc2f789b48bbded56390R81-R93

Any thoughts? Think it has to be a more complex scenario than this one? I tried just on the page removing children and a whole frame navigation... In both cases I saw the reference being cleaned. Thanks!

[sylveon]

@michael-hawker the leak is between TextBlock and ListView in that code (they both hold a strong reference to each other) - a random TextBox won't show it. The leak is obvious when navigating to and away from a page that uses this attached property - memory won't ever drop back to what it was before you opened it, this is more obvious when you're using a control that is intensive on memory of course.

My solution was indeed to just listen to Unloaded within RelativeAncestor::OnAncestorTypeChanged

[michael-hawker]

We think we addressed this in #105, test case doesn't show the issue anymore, added the Unloaded event to be sure. If anyone sees this again, please open a new issue with repro steps or a failing test example.