Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ override Microsoft.Maui.Controls.Platform.Compatibility.ShellSectionRenderer.OnH
~override Microsoft.Maui.Controls.Handlers.Items.RecyclerViewScrollListener<TItemsView, TItemsViewSource>.OnScrollStateChanged(AndroidX.RecyclerView.Widget.RecyclerView recyclerView, int newState) -> void
~override Microsoft.Maui.Controls.Handlers.Items.SelectableItemsViewAdapter<TItemsView, TItemsSource>.IsSelectionEnabled(Android.Views.ViewGroup parent, int viewType) -> bool
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ override Microsoft.Maui.Controls.Platform.Compatibility.ShellItemRenderer.ViewDi
~override Microsoft.Maui.Controls.Platform.Compatibility.ShellSectionRenderer.DidMoveToParentViewController(UIKit.UIViewController parent) -> void
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
override Microsoft.Maui.Controls.Handlers.Items2.StructuredItemsViewController2<TItemsView>.UpdateFlowDirection() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ override Microsoft.Maui.Controls.Platform.Compatibility.ShellItemRenderer.ViewDi
~override Microsoft.Maui.Controls.Platform.Compatibility.ShellSectionRenderer.DidMoveToParentViewController(UIKit.UIViewController parent) -> void
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
override Microsoft.Maui.Controls.Handlers.Items2.StructuredItemsViewController2<TItemsView>.UpdateFlowDirection() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
override Microsoft.Maui.Controls.Shapes.Shape.OnPropertyChanged(string? propertyName = null) -> void
override Microsoft.Maui.Controls.GraphicsView.OnBindingContextChanged() -> void
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
override Microsoft.Maui.Controls.Shapes.Shape.OnPropertyChanged(string? propertyName = null) -> void
override Microsoft.Maui.Controls.GraphicsView.OnBindingContextChanged() -> void
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
override Microsoft.Maui.Controls.Shapes.Shape.OnPropertyChanged(string? propertyName = null) -> void
override Microsoft.Maui.Controls.GraphicsView.OnBindingContextChanged() -> void
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
override Microsoft.Maui.Controls.Shapes.Shape.OnPropertyChanged(string? propertyName = null) -> void
override Microsoft.Maui.Controls.GraphicsView.OnBindingContextChanged() -> void
override Microsoft.Maui.Controls.TitleBar.OnBindingContextChanged() -> void
~override Microsoft.Maui.Controls.SwipeItems.OnPropertyChanged(string propertyName = null) -> void
55 changes: 55 additions & 0 deletions src/Controls/src/Core/SwipeView/SwipeItems.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,42 @@ public class SwipeItems : Element, IList<ISwipeItem>, INotifyCollectionChanged
{
readonly ObservableCollection<Maui.ISwipeItem> _swipeItems;

/// <summary>
/// Notifies the owning <see cref="SwipeView"/> (resolved via <see cref="Element.Parent"/>)
/// that something in this collection has changed so the handler can refresh the platform UI.
/// </summary>
/// <remarks>
/// Using <see cref="Element.Parent"/> (rather than a captured-closure subscription on the
/// SwipeView side) avoids leaking the owning SwipeView when a SwipeItems instance is
/// cached/shared across multiple SwipeView instances. When SwipeItems is reassigned to a
/// new owner, <c>AddLogicalChild</c> reassigns <see cref="Element.Parent"/> so notifications
/// always go to the current owner.
/// </remarks>
void NotifyOwner()
{
if (Parent is not SwipeView swipeView)
{
return;
}

if (this == swipeView.LeftItems)
{
swipeView.Handler?.UpdateValue(nameof(SwipeView.LeftItems));
}
else if (this == swipeView.RightItems)
{
swipeView.Handler?.UpdateValue(nameof(SwipeView.RightItems));
}
else if (this == swipeView.TopItems)
{
swipeView.Handler?.UpdateValue(nameof(SwipeView.TopItems));
}
else if (this == swipeView.BottomItems)
{
swipeView.Handler?.UpdateValue(nameof(SwipeView.BottomItems));
}
}

/// <summary>
/// Initializes a new instance of the <see cref="SwipeItems"/> class with the specified swipe items.
/// </summary>
Expand All @@ -33,6 +69,22 @@ public SwipeItems(IEnumerable<ISwipeItem> swipeItems)
_swipeItems.CollectionChanged += OnSwipeItemsChanged;
}

// Override OnPropertyChanged so we can notify the owning SwipeView when Mode /
// SwipeBehaviorOnInvoked change. Filtering by property name avoids a Handler.UpdateValue
// storm on every base-Element property change (Parent, BindingContext, Style, etc.).
// The notification goes through Parent (a weak reference managed by AddLogicalChild),
// so the owning SwipeView is never rooted by this SwipeItems instance (issue #35481).
protected override void OnPropertyChanged([System.Runtime.CompilerServices.CallerMemberName] string propertyName = null)
{
base.OnPropertyChanged(propertyName);

if (propertyName == ModeProperty.PropertyName ||
propertyName == SwipeBehaviorOnInvokedProperty.PropertyName)
{
NotifyOwner();
}
}

/// <summary>
/// Initializes a new instance of the <see cref="SwipeItems"/> class.
/// </summary>
Expand Down Expand Up @@ -157,6 +209,9 @@ void OnSwipeItemsChanged(object sender, NotifyCollectionChangedEventArgs notifyC
}

CollectionChanged?.Invoke(this, notifyCollectionChangedEventArgs);

// Notify the owning SwipeView so its handler can update the platform UI.
NotifyOwner();
}

IEnumerator IEnumerable.GetEnumerator()
Expand Down
54 changes: 21 additions & 33 deletions src/Controls/src/Core/SwipeView/SwipeView.cs
Original file line number Diff line number Diff line change
Expand Up @@ -163,44 +163,21 @@ static void OnSwipeItemsChanged(BindableObject bindable, object oldValue, object

if (oldValue is SwipeItems oldItems)
{
oldItems.CollectionChanged -= SwipeItemsCollectionChanged;
oldItems.PropertyChanged -= SwipeItemsPropertyChanged;
swipeView.RemoveLogicalChild(oldItems);
}

if (newValue is SwipeItems newItems)
{
newItems.CollectionChanged += SwipeItemsCollectionChanged;
newItems.PropertyChanged += SwipeItemsPropertyChanged;
// AddLogicalChild reassigns newItems.Parent to this SwipeView. When SwipeItems is
// cached/shared across multiple SwipeView instances, subsequent assignments
// reparent it away from the previous owner, so previous SwipeViews are not held
// alive through this collection. SwipeItems itself self-subscribes to its own
// CollectionChanged / PropertyChanged events and uses Parent (this SwipeView) to
// notify the handler — we intentionally do NOT add closure-based subscriptions
// here, since those captured `swipeView` and prevented GC when SwipeItems was
// shared across SwipeViews (issue #35481).
swipeView.AddLogicalChild(newItems);
}

void SwipeItemsPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (sender is SwipeItems swipeItems)
SendChange(swipeItems);
}

void SwipeItemsCollectionChanged(object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e)
{
if (sender is SwipeItems swipeItems)
SendChange(swipeItems);
}

void SendChange(SwipeItems swipeItems)
{
if (swipeItems == swipeView.LeftItems)
swipeView?.Handler?.UpdateValue(nameof(LeftItems));

if (swipeItems == swipeView.RightItems)
swipeView?.Handler?.UpdateValue(nameof(RightItems));

if (swipeItems == swipeView.TopItems)
swipeView?.Handler?.UpdateValue(nameof(TopItems));

if (swipeItems == swipeView.BottomItems)
swipeView?.Handler?.UpdateValue(nameof(BottomItems));
}
}

public event EventHandler<SwipeStartedEventArgs> SwipeStarted;
Expand Down Expand Up @@ -295,13 +272,24 @@ bool ISwipeView.IsOpen
protected override void OnChildAdded(Element child)
{
base.OnChildAdded(child);
child.PropertyChanged += OnPropertyChanged;

// Skip SwipeItems children: they are logical children for visual-tree purposes only;
// subscribing to their PropertyChanged would create a strong reference from a
// potentially cached/long-lived SwipeItems back to this SwipeView.
if (child is not SwipeItems)
{
child.PropertyChanged += OnPropertyChanged;
}
}

protected override void OnChildRemoved(Element child, int oldLogicalIndex)
{
base.OnChildRemoved(child, oldLogicalIndex);
child.PropertyChanged -= OnPropertyChanged;

if (child is not SwipeItems)
{
child.PropertyChanged -= OnPropertyChanged;
}
}

void OnPropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e)
Expand Down
91 changes: 91 additions & 0 deletions src/Controls/tests/Core.UnitTests/SwipeViewTests.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System;
using System.Collections.Generic;
using Microsoft.Maui.Graphics;
using Microsoft.Maui.Platform;
using Xunit;
Expand Down Expand Up @@ -566,5 +568,94 @@ public void SwipeViewRediscoversScrollParentWhenTemplateRootIsReparented()
var scrollView2 = new ScrollView { Content = contentView };
Assert.Equal(scrollView2, GetPrivateField(swipeView, "_scrollParent"));
}

// Regression test for https://github.com/dotnet/maui/issues/35481
// A SwipeItems instance that is cached (e.g. in a static dictionary) and shared
// across multiple SwipeViews must not keep prior SwipeView instances alive.
[Fact]
public void CachedSwipeItemsDoesNotKeepSwipeViewAlive()
{
// Simulate the repro: one long-lived SwipeItems cache shared across many SwipeViews.
var cachedSwipeItems = new SwipeItems { new SwipeItem { Text = "Delete" } };

// Allocate the SwipeViews in a non-inlined helper so the locals do not remain
// rooted on the test method's stack frame in Debug builds.
var swipeViewRefs = CreateSwipeViewsSharingCachedItems(cachedSwipeItems, 20);

// Mutate the cached SwipeItems after the SwipeViews are no longer referenced.
// Before the fix, the CollectionChanged/PropertyChanged subscriptions on
// SwipeItems held a strong reference back to every SwipeView that had ever
// used this cache, so this mutation would keep them all alive.
cachedSwipeItems.Add(new SwipeItem { Text = "Archive" });

ForceFullGC();

GC.KeepAlive(cachedSwipeItems);

Assert.All(swipeViewRefs, r => Assert.False(r.IsAlive,
"SwipeView was kept alive by the cached SwipeItems — issue #35481 regression."));
}

[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
static List<WeakReference> CreateSwipeViewsSharingCachedItems(SwipeItems shared, int count)
{
var refs = new List<WeakReference>(count);
for (int i = 0; i < count; i++)
{
var sv = new SwipeView { RightItems = shared };
// Simulate a CollectionView recycling the row: the SwipeView reassigns its
// RightItems before going out of scope, releasing the Parent back-reference
// from the cached SwipeItems. Before the fix, the closure-based
// CollectionChanged/PropertyChanged subscriptions on the cached SwipeItems
// still held this SwipeView alive even after this reassignment.
sv.RightItems = new SwipeItems();
refs.Add(new WeakReference(sv));
}
return refs;
}

// Regression test for https://github.com/dotnet/maui/issues/35481
// Replacing RightItems with a new SwipeItems instance must release any back-reference
// from the previous (cached) SwipeItems to the SwipeView. Without the fix, the cached
// SwipeItems' CollectionChanged/PropertyChanged delegates kept the SwipeView alive
// even after it was logically unhooked.
[Fact]
public void ReplacingCachedSwipeItemsReleasesPreviousOwnerReference()
{
// Cache: held by external code (the user's static dictionary in the repro).
var cachedRightItems = new SwipeItems { new SwipeItem { Text = "Done" } };

var swipeViewRef = CreateSwipeViewAssignThenReplace(cachedRightItems);

// Mutate the cached SwipeItems after the SwipeView is unhooked, just like the repro.
cachedRightItems.Add(new SwipeItem { Text = "Archive" });

ForceFullGC();

GC.KeepAlive(cachedRightItems);

Assert.False(swipeViewRef.IsAlive,
"Replaced SwipeView was kept alive by its previously-assigned cached SwipeItems — issue #35481 regression.");
}

[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
static WeakReference CreateSwipeViewAssignThenReplace(SwipeItems cachedRightItems)
{
var sv = new SwipeView { RightItems = cachedRightItems };
// Replace the assignment — the previous (cached) SwipeItems is now logically
// unhooked but kept alive by the caller. It must not retain the SwipeView.
sv.RightItems = new SwipeItems { new SwipeItem { Text = "Replaced" } };
return new WeakReference(sv);
}

static void ForceFullGC()
{
for (int i = 0; i < 5; i++)
{
GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, blocking: true, compacting: true);
GC.WaitForPendingFinalizers();
GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced, blocking: true, compacting: true);
}
}
}
}
Loading