Skip to content

Commit

Permalink
[android] fix memory leak with CarouselView (#18584)
Browse files Browse the repository at this point in the history
Fixes: #17726

Adding `CarouselView` to a page and navigating away from it keeps the
page alive forever on Android. I was able to reproduce this in a test.

It took me a while to actually track this one down, but the problem
boils down to `MauiCarouselRecyclerView` doing:

    _carouselViewLayoutListener = new CarouselViewwOnGlobalLayoutListener();
    _carouselViewLayoutListener.LayoutReady += LayoutReady;
    ViewTreeObserver.AddOnGlobalLayoutListener(_carouselViewLayoutListener);

The `ViewTreeObserver` lives longer than the page, and so:

* `ViewTreeObserver` -> `CarouselViewwOnGlobalLayoutListener`
* `event LayoutReady` -> `MauiCarouselRecyclerView`
* `MauiCarouselRecyclerView` -> `CarouselView`
* `CarouselView` -> `Parent.Parent.Parent.*` -> `Page`

Thus the `Page` lives forever.

If we remove the `LayoutReady` event, instead:

* `CarouselViewwOnGlobalLayoutListener` saves `MauiCarouselRecyclerView`
in a weak reference.

* It can just call `LayoutReady()` directly.

* `MauiCarouselRecyclerView` is able to be GC'd now.

* `MauiCarouselRecyclerView.Dispose()` already appropriately will call
  `ClearLayoutListener()`

* Lastly, `ViewTreeObserver?.RemoveOnGlobalLayoutListener(_carouselViewLayoutListener)`

With these changes, the test passes and I don't think anything will leak
now. :)

Other changes:

* I fixed the typo in `CarouselViewwOnGlobalLayoutListener`.
* I sorted the type names in the test alphabetically.
  • Loading branch information
jonathanpeppers authored Nov 8, 2023
1 parent baa86b2 commit 5d71344
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class MauiCarouselRecyclerView : MauiRecyclerView<CarouselView, ItemsView
bool _disposed;

List<View> _oldViews;
CarouselViewwOnGlobalLayoutListener _carouselViewLayoutListener;
CarouselViewOnGlobalLayoutListener _carouselViewLayoutListener;

protected CarouselView Carousel => ItemsView as CarouselView;

Expand Down Expand Up @@ -516,13 +516,11 @@ void AddLayoutListener()
if (_carouselViewLayoutListener != null)
return;

_carouselViewLayoutListener = new CarouselViewwOnGlobalLayoutListener();
_carouselViewLayoutListener.LayoutReady += LayoutReady;

_carouselViewLayoutListener = new CarouselViewOnGlobalLayoutListener(this);
ViewTreeObserver.AddOnGlobalLayoutListener(_carouselViewLayoutListener);
}

void LayoutReady(object sender, EventArgs e)
void LayoutReady()
{
if (!_initialized)
{
Expand All @@ -546,7 +544,6 @@ void ClearLayoutListener()
return;

ViewTreeObserver?.RemoveOnGlobalLayoutListener(_carouselViewLayoutListener);
_carouselViewLayoutListener.LayoutReady -= LayoutReady;
_carouselViewLayoutListener = null;
}

Expand Down Expand Up @@ -594,14 +591,23 @@ protected override void OnMeasure(int widthMeasureSpec, int heightMeasureSpec)

base.OnMeasure(widthMeasureSpec, heightMeasureSpec);
}
}

class CarouselViewwOnGlobalLayoutListener : Java.Lang.Object, ViewTreeObserver.IOnGlobalLayoutListener
{
public EventHandler<EventArgs> LayoutReady;
public void OnGlobalLayout()
class CarouselViewOnGlobalLayoutListener : Java.Lang.Object, ViewTreeObserver.IOnGlobalLayoutListener
{
LayoutReady?.Invoke(this, new EventArgs());
readonly WeakReference<MauiCarouselRecyclerView> _recyclerView;

public CarouselViewOnGlobalLayoutListener(MauiCarouselRecyclerView recyclerView)
{
_recyclerView = new(recyclerView);
}

public void OnGlobalLayout()
{
if (_recyclerView.TryGetTarget(out var recyclerView))
{
recyclerView.LayoutReady();
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ void SetupBuilder()
handlers.AddHandler<Frame, FrameRenderer>();
handlers.AddHandler<Label, LabelHandler>();
handlers.AddHandler<Button, ButtonHandler>();
handlers.AddHandler<CarouselView, CarouselViewHandler>();
handlers.AddHandler<CollectionView, CollectionViewHandler>();
handlers.AddHandler(typeof(Controls.ContentView), typeof(ContentViewHandler));
handlers.AddHandler(typeof(ScrollView), typeof(ScrollViewHandler));
Expand Down Expand Up @@ -315,11 +316,12 @@ await CreateHandlerAndAddToWindow<WindowHandlerStub>(new Window(navPage), async
Title = "Page 2",
Content = new VerticalStackLayout
{
new Label(),
new Button(),
new CarouselView(),
new CollectionView(),
new ContentView(),
new Label(),
new ScrollView(),
new ContentView()
}
};
pageReference = new WeakReference(page);
Expand Down

0 comments on commit 5d71344

Please sign in to comment.