🚧 Correct ancestor XAML memory leak #4236

XAML-Knight · 2021-09-09T23:04:27Z

Fixes CommunityToolkit/Windows#107

Also #4187

PR Type

What kind of change does this PR introduce?

Bugfix (correct memory leak)

What is the current behavior?

Steps to Reproduce:

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

What is the new behavior?

The elements should be freed.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

…to local/AncestorTheUnloaded

…to dev/AncestorTheUnloaded

ghost · 2021-09-09T23:04:31Z

Thanks XAML-Knight for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

michael-hawker · 2021-09-09T23:29:11Z

UnitTests/UnitTests.UWP/Extensions/Test_FrameworkElementExtensions.cs

+
+                treeRoot.Unloaded += (sender, e) =>
+                {
+                    Assert.AreEqual(grid, null);


We probably want to have a value set here that we check is set below after the await SetTestContent as well, otherwise if this doesn't fire then we never know, but we want to make sure it fires, eh?

XAML-Knight · 2021-09-14T17:24:07Z

An issue has been raised (#4246) describing the challenges when running these unit tests via the build script. The workaround for now is to apply the [Ignore] attribute to the unit tests.

michael-hawker · 2021-09-15T22:43:42Z

If I uncomment the tests, both are failing for me even locally. Though the Test Explorer seems to be very insistent about them still being ignored, had to like completely rebuild and then run in Debug mode.

I don't think the Unload event is testing what we expect to occur here. Ideally, locally at least, we want the test to fail without the fix applied and then to pass when we apply the fix.

I also noticed that we're missing unregistering the Loaded event in the Loaded event so that'll be hanging on to another reference to the parent as well:

WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI/Extensions/FrameworkElement/FrameworkElementExtensions.RelativeAncestor.cs

Lines 79 to 85 in 6fd3327

    
           private static void FrameworkElement_Loaded(object sender, RoutedEventArgs e) 
        
           { 
        
               if (sender is FrameworkElement fe) 
        
               { 
        
                   SetAncestor(fe, fe.FindAscendant(GetAncestorType(fe))); 
        
               } 
        
           }

Rosuavio

Could the issues with this test be similar to the issues we solved in the test in #4255 (commit 377a884) ?

Rosuavio · 2021-09-16T21:09:22Z

UnitTests/UnitTests.UWP/Extensions/Test_FrameworkElementExtensions.cs

+
+                // Need to simulate loading the control (and not just rely upon XamlReader.Load)
+                var frame = new Frame();
+                frame.Navigate(treeRoot.GetType());


You might need to await this by doing another await App.DispatcherQueue.EnqueueAsync()
Perhaps?

Suggested change

frame.Navigate(treeRoot.GetType());

await App.DispatcherQueue.EnqueueAsync(() => frame.Navigate(treeRoot.GetType()));

Rosuavio · 2021-09-16T21:09:27Z

UnitTests/UnitTests.UWP/Extensions/Test_FrameworkElementExtensions.cs

+
+                Assert.IsNull(btn.Parent);
+
+                btn.SetValue(FrameworkElementExtensions.AncestorProperty, new object());


Suggested change

btn.SetValue(FrameworkElementExtensions.AncestorProperty, new object());

await App.DispatcherQueue.EnqueueAsync(() => btn.SetValue(FrameworkElementExtensions.AncestorProperty, new object()));

Rosuavio · 2021-09-16T21:09:57Z

UnitTests/UnitTests.UWP/Extensions/Test_FrameworkElementExtensions.cs

+
+                // Need to simulate loading the control (and not just rely upon XamlReader.Load)
+                var frame = new Frame();
+                frame.Navigate(treeRoot.GetType());


Suggested change

frame.Navigate(treeRoot.GetType());

await App.DispatcherQueue.EnqueueAsync(() => frame.Navigate(treeRoot.GetType()));

Rosuavio · 2021-09-16T21:10:21Z

UnitTests/UnitTests.UWP/Extensions/Test_FrameworkElementExtensions.cs

+                var grid = treeRoot.FindChild("OuterGrid") as Grid;
+                var button = treeRoot.FindChild("InnerButton") as Button;
+
+                button.SetValue(FrameworkElementExtensions.AncestorProperty, new object());


Suggested change

button.SetValue(FrameworkElementExtensions.AncestorProperty, new object());

await App.DispatcherQueue.EnqueueAsync(() => button.SetValue(FrameworkElementExtensions.AncestorProperty, new object()));

michael-hawker · 2021-09-17T01:18:19Z

...ft.Toolkit.Uwp.UI/Extensions/FrameworkElement/FrameworkElementExtensions.RelativeAncestor.cs

        }

        /// <summary>
        /// Attached <see cref="DependencyProperty"/> for retrieving a parent <see cref="object"/> for the <see cref="AncestorProperty"/>
        /// </summary>
        public static readonly DependencyProperty AncestorProperty =
-            DependencyProperty.RegisterAttached("Ancestor", typeof(object), typeof(FrameworkElementExtensions), new PropertyMetadata(null));
+            DependencyProperty.RegisterAttached("Ancestor", typeof(WeakReference<object>), typeof(FrameworkElementExtensions), new PropertyMetadata(null));


By changing the type here we break using it in XAML as intended. This can be demonstrated with the TokenizingTextBox as it uses this feature to grab the TokenSpacing property from its parent.

michael-hawker · 2023-06-29T23:40:46Z

Closing, superseded by work done in CommunityToolkit/Windows#105

XAML-Knight added 23 commits August 19, 2021 09:50

Wire up Unload event

357d2ba

Added FrameworkElementExtension test

d529d48

add test file, wip framework extension

b2851d3

Apply WeakReference

8c9890b

Added missing header

2f9a266

Place DP props on one line

c5b2bb7

Remove blank line

55316d4

Add test for grid unload

762818c

Apply Await

cd475a3

Aid the unload effort

27c302f

Test added to proj

1206930

Merge remote-tracking branch 'XAML-Knight/dev/AncestorTheUnloaded' in…

669c355

…to local/AncestorTheUnloaded

Wire up Unload event

45ce6ad

Added FrameworkElementExtension test

547cc92

add test file, wip framework extension

6b049eb

Apply WeakReference

a529c02

Added missing header

f35da93

Place DP props on one line

9b937b9

Remove blank line

46e6072

Add test for grid unload

eccae8a

Apply Await

82b2819

Aid the unload effort

f99184c

Merge remote-tracking branch 'XAML-Knight/dev/AncestorTheUnloaded' in…

48e323c

…to dev/AncestorTheUnloaded

XAML-Knight self-assigned this Sep 9, 2021

ghost requested review from michael-hawker, azchohfi and Rosuavio September 9, 2021 23:04

ghost added bug 🐛 An unexpected issue that highlights incorrect behavior extensions ⚡ labels Sep 9, 2021

XAML-Knight added the in progress 🚧 label Sep 9, 2021

XAML-Knight removed their assignment Sep 9, 2021

XAML-Knight marked this pull request as draft September 9, 2021 23:07

XAML-Knight mentioned this pull request Sep 9, 2021

🚧 DRAFT: Correct ancestor XAML memory leak #4187

Closed

8 tasks

michael-hawker reviewed Sep 9, 2021

View reviewed changes

michael-hawker added this to the 7.1 milestone Sep 14, 2021

XAML-Knight mentioned this pull request Sep 14, 2021

Unit tests which pass in VS Test Explorer fail when ran via build script #4246

Closed

XAML-Knight added 2 commits September 14, 2021 10:40

Ignore Unit Tests to get build to pass

afb83c5

Set dummy variable, as check that runs after main body of test

70c5a8e

XAML-Knight changed the title ~~🚧 DRAFT: Correct ancestor XAML memory leak~~ Correct ancestor XAML memory leak Sep 14, 2021

XAML-Knight marked this pull request as ready for review September 14, 2021 22:05

Rosuavio reviewed Sep 16, 2021

View reviewed changes

Detach from loaded, and test improvements

8396bf9

michael-hawker reviewed Sep 17, 2021

View reviewed changes

michael-hawker marked this pull request as draft September 17, 2021 01:24

XAML-Knight changed the title ~~Correct ancestor XAML memory leak~~ 🚧 Correct ancestor XAML memory leak Sep 17, 2021

michael-hawker removed this from the 7.1 milestone Sep 20, 2021

michael-hawker closed this Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚧 Correct ancestor XAML memory leak #4236

🚧 Correct ancestor XAML memory leak #4236

XAML-Knight commented Sep 9, 2021

ghost commented Sep 9, 2021

michael-hawker Sep 9, 2021

XAML-Knight commented Sep 14, 2021

michael-hawker commented Sep 15, 2021

Rosuavio left a comment

Rosuavio Sep 16, 2021

Rosuavio Sep 16, 2021

Rosuavio Sep 16, 2021

Rosuavio Sep 16, 2021

michael-hawker Sep 17, 2021

michael-hawker commented Jun 29, 2023

	frame.Navigate(treeRoot.GetType());
	await App.DispatcherQueue.EnqueueAsync(() => frame.Navigate(treeRoot.GetType()));


		Assert.IsNull(btn.Parent);

		btn.SetValue(FrameworkElementExtensions.AncestorProperty, new object());

	button.SetValue(FrameworkElementExtensions.AncestorProperty, new object());
	await App.DispatcherQueue.EnqueueAsync(() => button.SetValue(FrameworkElementExtensions.AncestorProperty, new object()));

🚧 Correct ancestor XAML memory leak #4236

🚧 Correct ancestor XAML memory leak #4236

Conversation

XAML-Knight commented Sep 9, 2021

Fixes CommunityToolkit/Windows#107

Also #4187

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

ghost commented Sep 9, 2021

michael-hawker Sep 9, 2021

Choose a reason for hiding this comment

XAML-Knight commented Sep 14, 2021

michael-hawker commented Sep 15, 2021

Rosuavio left a comment

Choose a reason for hiding this comment

Rosuavio Sep 16, 2021

Choose a reason for hiding this comment

Rosuavio Sep 16, 2021

Choose a reason for hiding this comment

Rosuavio Sep 16, 2021

Choose a reason for hiding this comment

Rosuavio Sep 16, 2021

Choose a reason for hiding this comment

michael-hawker Sep 17, 2021

Choose a reason for hiding this comment

michael-hawker commented Jun 29, 2023