UCommonWidgetCarousel Memory Leak introduced in UE4.27 still in UE5.5

Development Programming & Scripting UI
, ,
1

Add and remove children or ClearChildren off of a CommonWidgetCarousel, notice how Destruct is not called on the child widgets. And if you’re using a CommonActivatableWidget, OnDeactivated doesn’t get called either. I tested using BP with a CommonWidgetCarousel, added several instances of a BP object with breakpoints set on event node for Construct and Destruct. Construct was reliably called, Destruct was not.

If you look at the CachedSlotWidgets variable you’ll notice it’s only cleared in ReleaseSlateResources. It never releases the children slot content. And if for some reason RebuildWidget is called it only ever continues adding to CachedSlotWidgets, never freeing the old data.

Note, this bug has been around since at least UE 4.27 when I first notice it being added, and looking at UE 5.6 the bug appears to still be there. Also it’s not a permanent leak as long as the Carousel parent is eventually deleted, everything will get cleaned up then; if there’s a long running carousel that’s being added/removed from in production that would be a problem.

This might not cover all cases, but at least adding this change helps fix it for me: (see below code sample).

I also updated `ClearChildren()` to include `CachedSlotWidgets.Empty()` just in case `RemoveChildAt(…)` didn’t address everything. And I updated `RebuildWidget()` to maintain a function local temp copy of `CachedSlotWidgets` before emptying and rebuilding `CachedSlotWidgets` in case stale data accidentally got left in.

When you guys look at the current use of CachedSlotWidgets in CommonWidgetCarousel, unless I’m missing something, it does stand out as an issue to you too right?

`// CommonWidgetCarousel.h:
protected:
virtual void OnSlotRemoved(UPanelSlot* InSlot) override;

// CommonWidgetCarousel.cpp:
void UCommonWidgetCarousel::OnSlotRemoved(UPanelSlot* InSlot)
{
if (InSlot == nullptr)
{
return;
}

if (InSlot->Content != nullptr)
{
const TSharedPtr SafeWidget = InSlot->Content->GetCachedWidget();
if (SafeWidget.IsValid())
{
CachedSlotWidgets.Remove(SafeWidget.ToSharedRef());
}
}
}`

2

Steps to Reproduce

  1. Add a CommonWidgetCarousel to a widget of your preference.
  2. Add child widgets to the carousel.
  3. Remove child widgets or ClearChildren off of the carousel.
  4. Notice how child widgets Destruct() is not called for BP or native.
    1. If child widget is a CommonActivatableWidget, OnDeactivated() doesn’t get called either if the widget was auto-activated.
  5. From looking at BP widget debug drop down, notice child widget instances remain referenced.
3

Hi,

I’d agree that the behavior here is a bit bizarre, especially since it’s inconsistent. We only add things to CachedSlotWidgets in RebuildWidget, so anything added dynamically won’t be cached. The caching isn’t strictly necessary since we maintain three slots (previous, current, next) to support the transition, so we can see dynamic widgets being constructed once they are in one of those three slots and destroyed when they leave. Knowing that, we could consider just removing CachedSlotWidgets altogether and letting the widgets construct/destruct naturally.

My only hesitation there is that the caching may be necessary to avoid hitching if the contents of the carousel are very heavy, as a widget would be constructed each time the page transitions. If we do want to keep the current behavior and ensure every child of the carousel stays constructed, we should add widgets to CachedSlotWidgets in UCommonWidgetCarousel::OnSlotAdded in addition to the changes you proposed. I’ll check with some folks to see if we should make that change or if it’s safe to remove the caching outright.

Best,

Cody

4

Hi,

I checked in some improvements for 5.7, we’re now more consistent about the caching behavior of widgets added at runtime vs. edit time and I’ve added a new property to explicitly set whether or not we should cache (since there are memory tradeoffs to consider). You can take a look at CL# 43006373 if you want to backport those changes.

Best,

Cody

5

THANKS CODY! ~