Visitar URL original
fix(transform): Register brush, geometry and group handlers weakly (p… · Singulink/uno@be41da6 · GitHub
Skip to content

Commit be41da6

Browse files
mikernetmikernet
committed
fix(transform): Register brush, geometry and group handlers weakly (port)
Port of the review round on unoplatform#25102 (704e17f, 7031c45 and 77899d4). Transforms are multi-parent shareable, so a transform shared by many short-lived brushes, geometries or transform groups rooted each of them through the strong Transform.Changed event (and a geometry also roots the Path it draws or the element it clips). Brush, Geometry and TransformGroup now hold a weak RegisterChanged registration instead, and the strong Changed events on Transform and Projection, which had no subscribers left, are removed. TransformGroup also drops its child registrations on a Reset, so Children.Clear() no longer keeps the removed children alive.
1 parent 8d0c8ed commit be41da6

7 files changed

Lines changed: 330 additions & 47 deletions

File tree

‎src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml/Given_UIElement.DataContext.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,10 @@ public async Task SingleParentNonFE_Parent_Collected_Then_Next_Parent_Counts_As_
256256
Assert.AreEqual((Windows.UI.Color)SolidColorBrush.ColorProperty.GetMetadata(typeof(SolidColorBrush)).DefaultValue, brush.Color, "1. with no DataContext the bound property falls back to its default");
257257

258258
// The collected parent still counts: the brush has had two parents, so inheritance is off for good, exactly as
259-
// when the first parent is still alive (MultiParentNonFE_*). The outcome must not depend on collection timing.
259+
// when the first parent is still alive (MultiParentNonFE_*). The outcome must not depend on GC timing.
260+
// Known WinUI divergence: a destroyed WinUI parent removes itself from its values (ReleaseImpl ->
261+
// ResetReferencesFromChildren -> RemoveParent), so the next consumer is a single parent again. Uno has no such
262+
// hook, so a collected parent keeps counting; only an explicit removal or the ALC sweep frees the slot.
260263
secondOwner.DataContext = new { Color = Colors.Green };
261264
Assert.AreNotEqual(Colors.Green, brush.Color, "2. once it has been attached to multiple \"parent\", dc should no longer propagate");
262265

‎src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Media/Given_Transform.cs‎

Lines changed: 266 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ public async Task When_RenderTransform_Shared_By_Popup_Style_Then_Discarded_Popu
9393

9494
var root = new Grid { Width = 200, Height = 200 };
9595
TestServices.WindowHelper.WindowContent = root;
96-
await TestServices.WindowHelper.WaitForIdle();
96+
// The popups take the root's XamlRoot, so the root must be in the live tree first.
97+
await TestServices.WindowHelper.WaitForLoaded(root);
9798

9899
try
99100
{
@@ -128,7 +129,7 @@ public async Task When_RenderTransform_Shared_By_Popup_Style_Then_Discarded_Popu
128129
}
129130
finally
130131
{
131-
VisualTreeHelper.CloseAllPopups(TestServices.WindowHelper.XamlRoot);
132+
UITestHelper.CloseAllPopups();
132133
TestServices.WindowHelper.WindowContent = null;
133134
}
134135

@@ -190,6 +191,269 @@ public async Task When_Projection_Shared_By_Style_Then_Discarded_Elements_Collec
190191
GC.KeepAlive(projection);
191192
}
192193

194+
[TestMethod]
195+
[GitHubWorkItem("https://github.com/unoplatform/uno/issues/25099")]
196+
public async Task When_Transform_Shared_By_Brushes_Then_Discarded_Brushes_Collected()
197+
{
198+
// Transforms are shareable: one used by many brushes (e.g. a resource referenced from a template) outlives
199+
// them and must not keep the discarded ones alive.
200+
var transform = new TranslateTransform();
201+
202+
var root = new StackPanel { Width = 100, Height = 100, Background = new SolidColorBrush(Colors.White) };
203+
TestServices.WindowHelper.WindowContent = root;
204+
await TestServices.WindowHelper.WaitForIdle();
205+
206+
try
207+
{
208+
var brushRefs = CreateElementsWithTransformedBrush(root, transform, count: 5);
209+
await TestServices.WindowHelper.WaitForIdle();
210+
211+
root.Children.Clear();
212+
await TestServices.WindowHelper.WaitForIdle();
213+
214+
foreach (var brushRef in brushRefs)
215+
{
216+
Assert.IsTrue(await TestHelper.TryWaitUntilCollected(brushRef), "A discarded brush using the shared transform was not collected.");
217+
}
218+
219+
// Red on the left half, blue on the right; moving the relative transform by half the width turns it all red.
220+
var survivor = new Border { Width = 40, Height = 10, HorizontalAlignment = HorizontalAlignment.Left, Background = CreateSplitBrush(transform) };
221+
root.Children.Add(survivor);
222+
await TestServices.WindowHelper.WaitForLoaded(survivor);
223+
await TestServices.WindowHelper.WaitForIdle();
224+
225+
var before = await UITestHelper.ScreenShot(root);
226+
ImageAssert.HasColorAt(before, 30, 5, Colors.Blue, tolerance: 5);
227+
228+
GC.Collect(2);
229+
GC.WaitForPendingFinalizers();
230+
GC.Collect(2);
231+
232+
transform.X = 0.5;
233+
await TestServices.WindowHelper.WaitForIdle();
234+
235+
var after = await UITestHelper.ScreenShot(root);
236+
ImageAssert.HasColorAt(after, 30, 5, Colors.Red, tolerance: 5);
237+
}
238+
finally
239+
{
240+
TestServices.WindowHelper.WindowContent = null;
241+
}
242+
243+
GC.KeepAlive(transform);
244+
}
245+
246+
[TestMethod]
247+
[GitHubWorkItem("https://github.com/unoplatform/uno/issues/25099")]
248+
public async Task When_Transform_Shared_By_Geometries_Then_Discarded_Elements_Collected()
249+
{
250+
// A geometry listens to its transform and the element it clips to the geometry, so a transform shared across
251+
// geometries must not keep the discarded geometries, and through them their elements, alive.
252+
var transform = new TranslateTransform();
253+
254+
var root = new StackPanel { Width = 100, Height = 100, Background = new SolidColorBrush(Colors.White) };
255+
TestServices.WindowHelper.WindowContent = root;
256+
await TestServices.WindowHelper.WaitForIdle();
257+
258+
try
259+
{
260+
var elementRefs = CreateElementsWithTransformedClip(root, transform, count: 5);
261+
await TestServices.WindowHelper.WaitForIdle();
262+
263+
root.Children.Clear();
264+
await TestServices.WindowHelper.WaitForIdle();
265+
266+
foreach (var elementRef in elementRefs)
267+
{
268+
Assert.IsTrue(await TestHelper.TryWaitUntilCollected(elementRef), "A discarded element clipped by a geometry using the shared transform was not collected.");
269+
}
270+
271+
// The clip shows the first 10 pixels of the element; moving it by 20 shows the 20-30 range instead.
272+
var survivor = CreateTransformedClipElement(transform);
273+
root.Children.Add(survivor);
274+
await TestServices.WindowHelper.WaitForLoaded(survivor);
275+
await TestServices.WindowHelper.WaitForIdle();
276+
277+
var before = await UITestHelper.ScreenShot(root);
278+
ImageAssert.HasColorAt(before, 5, 5, Colors.Red, tolerance: 5);
279+
ImageAssert.DoesNotHaveColorAt(before, 25, 5, Colors.Red, tolerance: 5);
280+
281+
GC.Collect(2);
282+
GC.WaitForPendingFinalizers();
283+
GC.Collect(2);
284+
285+
transform.X = 20;
286+
await TestServices.WindowHelper.WaitForIdle();
287+
288+
var after = await UITestHelper.ScreenShot(root);
289+
ImageAssert.HasColorAt(after, 25, 5, Colors.Red, tolerance: 5);
290+
ImageAssert.DoesNotHaveColorAt(after, 5, 5, Colors.Red, tolerance: 5);
291+
}
292+
finally
293+
{
294+
TestServices.WindowHelper.WindowContent = null;
295+
}
296+
297+
GC.KeepAlive(transform);
298+
}
299+
300+
[TestMethod]
301+
[GitHubWorkItem("https://github.com/unoplatform/uno/issues/25099")]
302+
public async Task When_Transform_Shared_By_TransformGroups_Then_Discarded_Groups_Collected()
303+
{
304+
// A TransformGroup listens to its children, so a child transform shared across groups must not keep the
305+
// discarded groups alive.
306+
var transform = new ScaleTransform { ScaleX = 2, ScaleY = 2 };
307+
308+
var root = new StackPanel { Width = 100, Height = 100, Background = new SolidColorBrush(Colors.White) };
309+
TestServices.WindowHelper.WindowContent = root;
310+
await TestServices.WindowHelper.WaitForIdle();
311+
312+
try
313+
{
314+
var groupRefs = CreateElementsWithTransformGroup(root, transform, count: 5);
315+
await TestServices.WindowHelper.WaitForIdle();
316+
317+
root.Children.Clear();
318+
await TestServices.WindowHelper.WaitForIdle();
319+
320+
foreach (var groupRef in groupRefs)
321+
{
322+
Assert.IsTrue(await TestHelper.TryWaitUntilCollected(groupRef), "A discarded TransformGroup containing the shared transform was not collected.");
323+
}
324+
325+
var survivor = new Border
326+
{
327+
Width = 10,
328+
Height = 10,
329+
Background = new SolidColorBrush(Colors.Red),
330+
HorizontalAlignment = HorizontalAlignment.Left,
331+
RenderTransform = new TransformGroup { Children = { transform } },
332+
};
333+
root.Children.Add(survivor);
334+
await TestServices.WindowHelper.WaitForLoaded(survivor);
335+
await TestServices.WindowHelper.WaitForIdle();
336+
337+
var before = await UITestHelper.ScreenShot(root);
338+
ImageAssert.HasColorAt(before, 15, 5, Colors.Red, tolerance: 5);
339+
ImageAssert.DoesNotHaveColorAt(before, 25, 5, Colors.Red, tolerance: 5);
340+
341+
GC.Collect(2);
342+
GC.WaitForPendingFinalizers();
343+
GC.Collect(2);
344+
345+
transform.ScaleX = 3;
346+
await TestServices.WindowHelper.WaitForIdle();
347+
348+
var after = await UITestHelper.ScreenShot(root);
349+
ImageAssert.HasColorAt(after, 25, 5, Colors.Red, tolerance: 5);
350+
}
351+
finally
352+
{
353+
TestServices.WindowHelper.WindowContent = null;
354+
}
355+
356+
GC.KeepAlive(transform);
357+
}
358+
359+
[TestMethod]
360+
[GitHubWorkItem("https://github.com/unoplatform/uno/issues/25099")]
361+
public async Task When_TransformGroup_Children_Cleared_Then_Removed_Child_Collected()
362+
{
363+
// Clear() reports a Reset without the removed items; a long-lived group rebuilt in code must still let go of
364+
// the transforms it no longer contains, and keep following the ones added afterwards.
365+
var group = new TransformGroup();
366+
var childRef = AddChildTransform(group);
367+
368+
group.Children.Clear();
369+
370+
Assert.IsTrue(await TestHelper.TryWaitUntilCollected(childRef), "A transform cleared from a TransformGroup was not collected.");
371+
372+
var newChild = new ScaleTransform { ScaleX = 3 };
373+
group.Children.Add(newChild);
374+
newChild.ScaleX = 4;
375+
376+
Assert.AreEqual(4, group.Value.M11, "The group no longer follows a child added after Clear().");
377+
}
378+
379+
[MethodImpl(MethodImplOptions.NoInlining)]
380+
private static WeakReference AddChildTransform(TransformGroup group)
381+
{
382+
var child = new ScaleTransform { ScaleX = 2 };
383+
group.Children.Add(child);
384+
return new WeakReference(child);
385+
}
386+
387+
private static LinearGradientBrush CreateSplitBrush(Transform relativeTransform)
388+
=> new()
389+
{
390+
StartPoint = new(0, 0),
391+
EndPoint = new(1, 0),
392+
GradientStops =
393+
{
394+
new GradientStop { Color = Colors.Red, Offset = 0 },
395+
new GradientStop { Color = Colors.Red, Offset = 0.5 },
396+
new GradientStop { Color = Colors.Blue, Offset = 0.5 },
397+
new GradientStop { Color = Colors.Blue, Offset = 1 },
398+
},
399+
RelativeTransform = relativeTransform,
400+
};
401+
402+
private static Border CreateTransformedClipElement(Transform transform)
403+
=> new()
404+
{
405+
Width = 40,
406+
Height = 10,
407+
HorizontalAlignment = HorizontalAlignment.Left,
408+
Background = new SolidColorBrush(Colors.Red),
409+
Clip = new RectangleGeometry { Rect = new(0, 0, 10, 10), Transform = transform },
410+
};
411+
412+
[MethodImpl(MethodImplOptions.NoInlining)]
413+
private static List<WeakReference> CreateElementsWithTransformedBrush(Panel root, Transform transform, int count)
414+
{
415+
var refs = new List<WeakReference>(count);
416+
417+
for (var i = 0; i < count; i++)
418+
{
419+
var brush = CreateSplitBrush(transform);
420+
root.Children.Add(new Border { Width = 40, Height = 10, Background = brush });
421+
refs.Add(new WeakReference(brush));
422+
}
423+
424+
return refs;
425+
}
426+
427+
[MethodImpl(MethodImplOptions.NoInlining)]
428+
private static List<WeakReference> CreateElementsWithTransformedClip(Panel root, Transform transform, int count)
429+
{
430+
var refs = new List<WeakReference>(count);
431+
432+
for (var i = 0; i < count; i++)
433+
{
434+
var element = CreateTransformedClipElement(transform);
435+
root.Children.Add(element);
436+
refs.Add(new WeakReference(element));
437+
}
438+
439+
return refs;
440+
}
441+
442+
[MethodImpl(MethodImplOptions.NoInlining)]
443+
private static List<WeakReference> CreateElementsWithTransformGroup(Panel root, Transform transform, int count)
444+
{
445+
var refs = new List<WeakReference>(count);
446+
447+
for (var i = 0; i < count; i++)
448+
{
449+
var group = new TransformGroup { Children = { transform } };
450+
root.Children.Add(new Border { Width = 10, Height = 10, RenderTransform = group });
451+
refs.Add(new WeakReference(group));
452+
}
453+
454+
return refs;
455+
}
456+
193457
[MethodImpl(MethodImplOptions.NoInlining)]
194458
private static async Task<List<WeakReference>> CreateStyledPopups(Panel root, Style style, int count)
195459
{

‎src/Uno.UI/UI/Xaml/Media/Brush.cs‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -86,20 +86,23 @@ internal virtual void OnPropertyChanged2(DependencyPropertyChangedEventArgs args
8686

8787
OnInvalidateRender();
8888

89-
if (args.Property == TransformProperty || args.Property == RelativeTransformProperty)
89+
// Weak registrations: a transform shared across brushes (e.g. a resource used in a template) must not
90+
// root the discarded ones.
91+
if (args.Property == TransformProperty)
9092
{
91-
if (args.NewValue is Transform newTransform)
92-
{
93-
newTransform.Changed += OnTransformChange;
94-
}
95-
96-
if (args.OldValue is Transform oldTransform)
97-
{
98-
oldTransform.Changed -= OnTransformChange;
99-
}
93+
_transformChangedRegistration?.Dispose();
94+
_transformChangedRegistration = (args.NewValue as Transform)?.RegisterChanged(OnTransformChange);
95+
}
96+
else if (args.Property == RelativeTransformProperty)
97+
{
98+
_relativeTransformChangedRegistration?.Dispose();
99+
_relativeTransformChangedRegistration = (args.NewValue as Transform)?.RegisterChanged(OnTransformChange);
100100
}
101101
}
102102

103+
private IDisposable? _transformChangedRegistration;
104+
private IDisposable? _relativeTransformChangedRegistration;
105+
103106
private void OnTransformChange(object? sender, EventArgs args) => OnInvalidateRender();
104107

105108
#region Opacity Dependency Property

‎src/Uno.UI/UI/Xaml/Media/Geometry.cs‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -66,19 +66,16 @@ public Transform Transform
6666
new FrameworkPropertyMetadata(default(Transform), propertyChangedCallback: (s, args) => ((Geometry)s).OnTransformChanged(args))
6767
);
6868

69+
private IDisposable _transformChangedRegistration;
70+
6971
private void OnTransformChanged(DependencyPropertyChangedEventArgs args)
7072
{
7173
RaiseGeometryChanged();
7274

73-
if (args.OldValue is Transform oldValue)
74-
{
75-
oldValue.Changed -= OnTransformSubChanged;
76-
}
77-
78-
if (args.NewValue is Transform newValue)
79-
{
80-
newValue.Changed += OnTransformSubChanged;
81-
}
75+
// Weak registration: a transform shared across geometries must not root the discarded ones, and through
76+
// their GeometryChanged handlers the paths and clipped elements using them.
77+
_transformChangedRegistration?.Dispose();
78+
_transformChangedRegistration = (args.NewValue as Transform)?.RegisterChanged(OnTransformSubChanged);
8279
}
8380

8481
private void OnTransformSubChanged(object sender, EventArgs e)

0 commit comments

Comments
 (0)