-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🚧 DRAFT: Correct ancestor XAML memory leak #4187
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
Changes from 9 commits
5420874
8482a04
f6c65a5
6845464
7f04027
90e8a34
f448ee3
bc05c86
e23a895
dde93f3
5540ece
5564df3
a72b060
9e9a223
5ba4557
41b580a
c0f7ff3
b31758d
a582f34
cdf48d0
e62eb8b
fd8aee5
5a523c9
a5fc9eb
e3f5a7f
738b950
435ec70
4ee5606
583d5ae
60169ae
74c3219
08122c3
ce3726f
91093cf
99872b1
5f7a847
3ba061e
0fda83b
197e36e
4a0f38b
71fcd9d
ec4602e
5a7bc1c
a4b9114
ccaf97a
a49a27e
b67ae42
53d9a3e
275a87e
be6f402
a4fd239
67583c0
418a646
8e2a7ac
79d1755
1d3cfc8
723824b
08b48e3
557657b
eab2cc8
03a40ed
91c8cfc
a38d1e5
4983b0e
5be33ce
68a17e2
fda6c40
7876aa9
a3faee9
13747b1
2387ba3
2ac3e4f
b150736
9d66f1a
7c7c290
6e4fb90
76e7c88
becfc10
6e2d6f2
b36c283
70e7a41
02f1076
6686e74
8f10e3b
6e53a50
17e3399
fd99d9d
d6e64c1
169c716
b13fc08
2e7d978
2ab8096
56104c1
b25cdd5
be29388
1c1c370
e40e461
22cae50
1b2e483
1f3dcc7
b39d0ba
db0599a
36f46be
c4f6009
f458d51
bc95b9a
0428550
9195720
a2b3880
d091e6a
9d4809c
58a3cde
c363f62
2ffd1ff
357d2ba
6787487
1ba3228
b605154
17b9b32
f772542
cddc5a6
70c9ef0
3587c40
73e5f15
8eba48b
9b34912
32c3743
ad4fd04
7c205fe
43975b7
ee71863
81b350e
f5a97d4
ab1d3b3
2e9b350
7b7974b
d529d48
94dc7bf
4bc7f21
cc562b3
b2851d3
ab486ec
795c57e
5d1c7bf
bb9dc4b
863e75c
e3c6fe2
f2a8254
72d524c
ed42e41
f48df97
75e53c9
4a651dd
2096e5d
0459a89
8647919
ca6f772
08ffdcf
164eefd
b34f441
83a5bce
c932579
3d03ce7
3401db0
4e327ed
d9e8845
46bd862
fe9ac89
88af510
33bdd30
5ab106e
f3b0e5c
01c9fa3
2404be9
e418928
5d5d86f
836a501
a37860d
8fa7a8d
a867fad
02bbc4d
79f2931
9f20cf4
f606fa8
f9a80ec
10e7668
5e4b0b8
4fd1609
aecbcad
2b84917
cf4122c
ede46a3
8c9890b
eff2ee5
cefaeeb
81504ce
2f9a266
58af745
20c61f9
ea41b48
67f3906
8d0d473
166e101
e4b8a68
8273bca
0ee2c84
cc2a51e
99d587f
e89cac6
20f951e
9cf1630
3f29bde
c5b2bb7
9f8527c
984fe18
0ce4553
55316d4
762818c
fb5b405
21e0825
15a26e4
cd475a3
b276329
8d7f8c2
27c302f
b6ec0bc
5dfc575
1206930
669c355
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using Microsoft.Toolkit.Uwp; | ||
| using Microsoft.Toolkit.Uwp.UI; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using System.Threading.Tasks; | ||
| using Windows.UI.Xaml.Controls; | ||
| using Windows.UI.Xaml.Markup; | ||
|
|
||
| namespace UnitTests.Extensions | ||
| { | ||
| [TestClass] | ||
| public class Test_FrameworkElementExtensions : VisualUITestBase | ||
| { | ||
| [TestCategory("FrameworkElementExtensions")] | ||
| [TestMethod] | ||
| public async Task Test_Ancestor_WeakReference() | ||
| { | ||
| // Need to sim loading the control, and not just load the XAML via XamlReader | ||
| await App.DispatcherQueue.EnqueueAsync(async () => | ||
| { | ||
| var treeRoot = XamlReader.Load( | ||
| @"<Page | ||
| xmlns=""http://schemas.microsoft.com/winfx/2006/xaml/presentation"" | ||
| xmlns:x=""http://schemas.microsoft.com/winfx/2006/xaml"" | ||
| xmlns:controls=""using:Microsoft.Toolkit.Uwp.UI.Controls"" | ||
| xmlns:ui=""using:Microsoft.Toolkit.Uwp.UI""> | ||
|
|
||
| <Grid x:Name=""OuterGrid"" Width=""200"" Background=""Khaki""> | ||
| <Button x:Name=""InnerButton"" ui:FrameworkElementExtensions.AncestorType=""Grid"" /> | ||
| </Grid> | ||
|
|
||
| </Page>") as Page; | ||
|
|
||
| Assert.IsNotNull(treeRoot, "Could not load XAML tree."); | ||
| await SetTestContentAsync(treeRoot); | ||
|
|
||
| // Need to simulate loading the control (and not just rely upon XamlReader.Load) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be handled by the SetTestContentAsync above? as that actually puts the content from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean "... EnqueueAsync above"? In trying to repro the original bug when a XAML control is actually loaded, I found that just spinning up the XAML with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| var frame = new Frame(); | ||
| frame.Navigate(treeRoot.GetType()); | ||
| Assert.IsInstanceOfType(frame.Content, typeof(Page)); | ||
|
|
||
| var btn = treeRoot.FindChild("InnerButton") as Button; | ||
|
|
||
| Assert.IsNull(btn.Parent); | ||
|
|
||
| btn.SetValue(FrameworkElementExtensions.AncestorProperty, new object()); | ||
|
|
||
| Assert.IsNull(btn.Parent); | ||
| }); | ||
| } | ||
|
|
||
| [TestCategory("FrameworkElementExtensions")] | ||
| [TestMethod] | ||
| public async Task Test_Ancestor_WeakRef_UnloadGrid() | ||
| { | ||
| // Need to sim loading the control, and not just load the XAML via XamlReader | ||
| await App.DispatcherQueue.EnqueueAsync(async () => | ||
| { | ||
| var treeRoot = XamlReader.Load( | ||
| @"<Page | ||
| xmlns=""http://schemas.microsoft.com/winfx/2006/xaml/presentation"" | ||
| xmlns:x=""http://schemas.microsoft.com/winfx/2006/xaml"" | ||
| xmlns:controls=""using:Microsoft.Toolkit.Uwp.UI.Controls"" | ||
| xmlns:ui=""using:Microsoft.Toolkit.Uwp.UI""> | ||
|
|
||
| <Grid x:Name=""OuterGrid"" Width=""200"" Background=""Khaki""> | ||
| <Button x:Name=""InnerButton"" ui:FrameworkElementExtensions.AncestorType=""Grid"" /> | ||
| </Grid> | ||
|
|
||
| </Page>") as Page; | ||
|
|
||
| Assert.IsNotNull(treeRoot, "Could not load XAML tree."); | ||
| await SetTestContentAsync(treeRoot); | ||
|
|
||
| // Need to simulate loading the control (and not just rely upon XamlReader.Load) | ||
| var frame = new Frame(); | ||
| frame.Navigate(treeRoot.GetType()); | ||
| Assert.IsInstanceOfType(frame.Content, typeof(Page)); | ||
|
|
||
| var grid = treeRoot.FindChild("OuterGrid") as Grid; | ||
| var button = treeRoot.FindChild("InnerButton") as Button; | ||
|
|
||
| button.SetValue(FrameworkElementExtensions.AncestorProperty, new object()); | ||
|
|
||
| treeRoot.Unloaded += (sender, e) => | ||
| { | ||
| Assert.AreEqual(grid, null); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonder if this is getting called late and causing a problem as immediately after this the test ends... We're also waiting for the Unloaded event from the content itself as part of the VisualUITestBase Cleanup method, so we may need to try forcing your content to unload first before letting the test end? Maybe try calling |
||
| }; | ||
| }); | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want another test where we try and unload the Grid from the visual tree and see if the reference prevents that from happening, right? That's the original bug?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The approach you're seeing here in this draft, was to not rely upon the All the same, we still could test unloading the Grid. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the header here (why CI is failing)