Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 47 additions & 20 deletions src/Controls/src/Core/Shapes/PathGeometry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ namespace Microsoft.Maui.Controls.Shapes
[ContentProperty("Figures")]
public sealed class PathGeometry : Geometry
{
// Tracks figures whose PropertyChanged and InvalidatePathSegmentRequested events are
// subscribed so we can unsubscribe them even when the collection is cleared (Reset
// action does not populate OldItems).
readonly List<PathFigure> _subscribedFigures = new List<PathFigure>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention / Architectural Layer — This same Clear()-retention leak pattern (Reset action fires with OldItems == null, so the old per-item -= unsubscribe loop never runs) still exists unfixed in two sibling types that use the identical CollectionChanged wiring style:

  • PathFigure.UpdatePathSegmentCollection / OnPathSegmentCollectionChanged (src/Controls/src/Core/Shapes/PathFigure.cs) — figure.Segments.Clear() will still leave oldPathSegment.PropertyChanged -= OnPathSegmentPropertyChanged never called, retaining the PathFigure alive via any surviving PathSegment reference.
  • GeometryGroup.UpdateChildren / OnChildrenCollectionChanged (src/Controls/src/Core/Shapes/GeometryGroup.cs) — group.Children.Clear() has the same gap for Geometry children.

Concrete failing scenario: var seg = new LineSegment(); figure.Segments.Add(seg); figure.Segments.Clear();figure remains reachable through seg.PropertyChanged and a WeakReference<PathFigure> test analogous to FiguresClear_AllowsPathGeometryToBeGarbageCollected would fail here today. Since this PR introduces the exact fix pattern (a subscribed-items tracking list unsubscribed on Reset) for PathGeometry, please confirm whether PathFigure/GeometryGroup should get the same fix in this PR or a tracked follow-up issue — otherwise the underlying bug class remains only partially fixed.


/// <summary>
/// Initializes a new instance of the <see cref="PathGeometry"/> class.
/// </summary>
Expand Down Expand Up @@ -198,17 +203,36 @@ void AddPolyQuad(PathF path, PolyQuadraticBezierSegment polyQuadraticBezierSegme
}
}

void SubscribeFigure(PathFigure figure)
{
figure.PropertyChanged += OnPathFigurePropertyChanged;
figure.InvalidatePathSegmentRequested += OnInvalidatePathSegmentRequested;
_subscribedFigures.Add(figure);
}

void UnsubscribeFigure(PathFigure figure)
{
figure.PropertyChanged -= OnPathFigurePropertyChanged;
figure.InvalidatePathSegmentRequested -= OnInvalidatePathSegmentRequested;
_subscribedFigures.Remove(figure);
}

void UnsubscribeAllFigures()
{
foreach (var figure in _subscribedFigures)
{
figure.PropertyChanged -= OnPathFigurePropertyChanged;
figure.InvalidatePathSegmentRequested -= OnInvalidatePathSegmentRequested;
}
_subscribedFigures.Clear();
}

void UpdatePathFigureCollection(PathFigureCollection oldCollection, PathFigureCollection newCollection)
{
if (oldCollection != null)
{
oldCollection.CollectionChanged -= OnPathFigureCollectionChanged;

foreach (var oldPathFigure in oldCollection)
{
oldPathFigure.PropertyChanged -= OnPathFigurePropertyChanged;
oldPathFigure.InvalidatePathSegmentRequested -= OnInvalidatePathSegmentRequested;
}
UnsubscribeAllFigures();
}

if (newCollection == null)
Expand All @@ -218,34 +242,37 @@ void UpdatePathFigureCollection(PathFigureCollection oldCollection, PathFigureCo

foreach (var newPathFigure in newCollection)
{
newPathFigure.PropertyChanged += OnPathFigurePropertyChanged;
newPathFigure.InvalidatePathSegmentRequested += OnInvalidatePathSegmentRequested;
SubscribeFigure(newPathFigure);
}
}

void OnPathFigureCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
{
if (e.OldItems != null)
if (e.OldItems != null && e.Action != NotifyCollectionChangedAction.Move)
{
foreach (var oldItem in e.OldItems)
{
if (!(oldItem is PathFigure oldPathFigure))
continue;

oldPathFigure.PropertyChanged -= OnPathFigurePropertyChanged;
oldPathFigure.InvalidatePathSegmentRequested -= OnInvalidatePathSegmentRequested;
if (oldItem is PathFigure oldPathFigure)
{
UnsubscribeFigure(oldPathFigure);
}
}
}
if (e.Action == NotifyCollectionChangedAction.Reset)
{
// Clear() raises Reset with OldItems = null; unsubscribe all tracked figures
// to prevent the cleared figures from retaining this PathGeometry alive.
UnsubscribeAllFigures();
}

if (e.NewItems != null)
if (e.NewItems != null && e.Action != NotifyCollectionChangedAction.Move)
{
foreach (var newItem in e.NewItems)
{
if (!(newItem is PathFigure newPathFigure))
continue;

newPathFigure.PropertyChanged += OnPathFigurePropertyChanged;
newPathFigure.InvalidatePathSegmentRequested += OnInvalidatePathSegmentRequested;
if (newItem is PathFigure newPathFigure)
{
SubscribeFigure(newPathFigure);
}
}
}

Expand Down
94 changes: 94 additions & 0 deletions src/Controls/tests/Core.UnitTests/Shapes/PathGeometryTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
using System;
using System.Runtime.CompilerServices;
using Microsoft.Maui.Controls.Shapes;
using Microsoft.Maui.Graphics;
using Xunit;

namespace Microsoft.Maui.Controls.Core.UnitTests.Shapes;

public class PathGeometryTests : BaseTestFixture
{
/// <summary>
/// Figures.Clear() must unsubscribe the cleared PathFigure from the PathGeometry,
/// otherwise the figure retains the geometry alive via its PropertyChanged delegate.
/// </summary>
[Fact]
public void FiguresClear_UnsubscribesFigurePropertyChangedHandler()
{
var geometry = new PathGeometry();
var sharedFigure = new PathFigure { StartPoint = new Point(0, 0) };
geometry.Figures.Add(sharedFigure);

int invalidateCount = 0;
geometry.InvalidatePathGeometryRequested += (s, e) => invalidateCount++;

// Sanity-check: mutating the figure before Clear should trigger invalidation.
sharedFigure.StartPoint = new Point(10, 10);
Assert.Equal(1, invalidateCount);

// Act - Clear() fires CollectionChanged (Reset), which itself calls Invalidate() once.
geometry.Figures.Clear();
int countAfterClear = invalidateCount;

// After Clear, mutating the figure must NOT trigger any further invalidation on the geometry.
sharedFigure.StartPoint = new Point(20, 20);
Assert.Equal(countAfterClear, invalidateCount);
}

/// <summary>
/// Figures.Clear() must unsubscribe the cleared PathFigure's segment-invalidation event
/// from the PathGeometry.
/// </summary>
[Fact]
public void FiguresClear_UnsubscribesFigureSegmentInvalidateHandler()
{
var geometry = new PathGeometry();
var sharedFigure = new PathFigure { StartPoint = new Point(0, 0) };
geometry.Figures.Add(sharedFigure);

int invalidateCount = 0;
geometry.InvalidatePathGeometryRequested += (s, e) => invalidateCount++;

// Sanity-check: adding a segment before Clear should trigger invalidation.
sharedFigure.Segments.Add(new LineSegment { Point = new Point(100, 100) });
Assert.Equal(1, invalidateCount);

// Act - Clear() fires CollectionChanged (Reset), which itself calls Invalidate() once.
geometry.Figures.Clear();
int countAfterClear = invalidateCount;

// After Clear, adding segments to the cleared figure must NOT trigger any further invalidation.
sharedFigure.Segments.Add(new LineSegment { Point = new Point(200, 200) });
Assert.Equal(countAfterClear, invalidateCount);
}

/// <summary>
/// After Figures.Clear(), the PathGeometry must be eligible for garbage collection
/// even when the cleared PathFigure is still alive (shared/rooted elsewhere).
/// </summary>
[Fact]
public void FiguresClear_AllowsPathGeometryToBeGarbageCollected()
{
var sharedFigure = new PathFigure { StartPoint = new Point(0, 0) };
var weakRef = CreateGeometryAndClear(sharedFigure);

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();

// If the bug is present, sharedFigure still holds the geometry alive via
// its PropertyChanged delegate chain, so TryGetTarget would return true.
Assert.False(weakRef.TryGetTarget(out _),
"PathGeometry was retained by the cleared PathFigure (event-handler leak in Figures.Clear()).");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention — adjacent scenario coverage — The new tests only cover a single PathFigure added once and then Clear(). Two adjacent scenarios exercised by the new production logic in PathGeometry.cs have no regression coverage:

  1. Duplicate figures: the same PathFigure instance added twice to Figures (Figures.Add(x); Figures.Add(x);) then a partial Remove(x) followed by Clear() — this exercises _subscribedFigures.Remove(figure) (PathGeometry.cs:217) picking the first matching reference vs. the actual remaining subscription count staying balanced.
  2. Move action: Figures.Move(0, 1) on a 2+ item collection — this exercises the new e.Action != NotifyCollectionChangedAction.Move guards (PathGeometry.cs:251 and :268), added specifically to skip resubscription churn on move; there's currently no test proving a moved figure keeps invalidating correctly and isn't double-subscribed/leaked afterward.
    Both were reasoned about only informally; a regression test for each would lock in the intended behavior against future refactors of this same code area.


// Factored out so the JIT cannot inline the PathGeometry local onto the caller's frame.
[MethodImpl(MethodImplOptions.NoInlining)]
static WeakReference<PathGeometry> CreateGeometryAndClear(PathFigure figure)
{
var geometry = new PathGeometry();
geometry.Figures.Add(figure);
geometry.Figures.Clear();
return new WeakReference<PathGeometry>(geometry);
}
}
Loading