Fix TabItem memory leak

[DmitryZhelnin]

What does the pull request do?

Moves update of TabStripPlacement to TabContol.

What is the current behavior?

Subscription is not disposed and TabItem controls remain in memory.

Fixed issues

Fixes #12416

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0038279-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[TomEdwardsEnscape]

The trouble with doing this when TabItem is detached from the visual tree tree is that it can be frequently detached as the user clicks around an application, causing the property to stop updating at unpredictable times. This behaviour can be reproduced in the Avalonia control gallery, which has a nested TabControl in its "TabControl" section. Enter and exit this section and you will see that OnDetachedFromVisualTree is called.

In the specific case of TabStripPlacement, the property not updating when the control is detached is unlikely to lead to real-world problems. However, if a property is defined then it really ought to behave correctly at all times, as if nothing else someone might copy or extend this code in the future.

Alternatives

When I start thinking about dynamically adding and removing tab items, I start questioning whether property makes sense at all. It's entirely legal for a single TabItem to be within the Items collection of multiple TabControls, as long as it is only attached to the visual tree in one location at a time. What should the value be in this scenario?

Perhaps it would be better to remove this property (or mark it as obsolete), then have templates instead bind to $parent[TabControl].TabStripPlacement. This makes retrieving the value an explicit visual tree operation, which nobody would expect to work when the TabItem was detached.

Another alternative is switching to logical tree attach/detach. These events are much more predictable. But are they raised when adding a tab item dynamically?

A third option is to use a weak event subscription and let the garbage collector take care of things.

[TomEdwardsEnscape]

Wait a second, the documentation for AvaloniaObject.GetObservable says that "The subscription is created using a weak reference". So it shouldn't be causing a leak in the first place!

[DmitryZhelnin]

As I understand, weak reference points to the TabControl, so the whole TabControl can be garbage collected even if any TabItem is still alive and referenced by another object.

[DmitryZhelnin]

@TomEdwardsEnscape I agree with you on the point whether property makes sense at all. Looks like TabStripPlacement can be removed from TabItem control.

[Gillibald]

Managing subscriptions via logical tree should work

[DmitryZhelnin]

Or we can move update logic to the TabControl. In this case no subscriptions needed, but setter of TabItem.TabStripPlacement has to be internal. For example:

TabStripPlacementProperty.Changed.AddClassHandler<TabControl>((x, args) => x.UpdateTabStripPlacement());

private void UpdateTabStripPlacement()
{
    for (int i = 0; i < Items.Count; i++)
    {
        var control = ContainerFromIndex(i);
        if (control is TabItem tabItem)
        {
            tabItem.TabStripPlacement = TabStripPlacement;
        }
    }
}

And additionally call UpdateTabStripPlacement in OnApplyTemplate after ItemsPresenterPart was initialized.

[rabbitism]

@TomEdwardsEnscape I agree with you on the point whether property makes sense at all. Looks like TabStripPlacement can be removed from TabItem control.

Placement is widely used for custom styles. (like menu like TabControl with items on left side with an indicator. )

[TomEdwardsEnscape]

I worked out where the strong reference is coming from. It's not from GetObservable but from the lambda method in the subsequent Subscribe call. This is implicitly capturing the TabItem.

The class below avoids this, but still leaves WeakObserver objects lying around (though these are at least very small).

The proper fix for this is a "weak subscribe" method that uses ConditionalWeakTable to send notifications to the right targets, while still allowing the observers to be GCed.

Moving the update logic to the TabControl instead, as Dmitry suggests, would also be fine with me. TabControl generates TabItems anyway, so it's not adding a new dependency.

protected void SubscribeToOwnerProperties(AvaloniaObject owner)
{
    _ownerSubscriptions = owner.GetObservable(TabControl.TabStripPlacementProperty).Subscribe(new WeakObserver(this));
}

private class WeakObserver : IObserver<Dock>
{
    private GCHandle _weakRef;

    public WeakObserver(TabItem owner)
    {
        _weakRef = GCHandle.Alloc(owner, GCHandleType.Weak);
    }

    ~WeakObserver()
    {
        _weakRef.Free();
    }

    public void OnCompleted()
    {
        if (_weakRef.Target is TabItem tabItem)
        {
            tabItem.TabStripPlacement = null;
        }

        _weakRef.Free();
        GC.SuppressFinalize(this);
    }

    public void OnError(Exception error)
    {
        // ignore
    }

    public void OnNext(Dock value)
    {
        if (_weakRef.Target is TabItem tabItem)
        {
            tabItem.TabStripPlacement = value;
        }
    }
}

[Gillibald]

Or we can move update logic to the TabControl. In this case no subscriptions needed, but setter of TabItem.TabStripPlacement has to be internal. For example:

TabStripPlacementProperty.Changed.AddClassHandler<TabControl>((x, args) => x.UpdateTabStripPlacement());

private void UpdateTabStripPlacement()
{
    for (int i = 0; i < Items.Count; i++)
    {
        var control = ContainerFromIndex(i);
        if (control is TabItem tabItem)
        {
            tabItem.TabStripPlacement = TabStripPlacement;
        }
    }
}

And additionally call UpdateTabStripPlacement in OnApplyTemplate after ItemsPresenterPart was initialized.

@DmitryZhelnin Please update the PR to use this approach and we can merge

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0038716-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[DmitryZhelnin]

@Gillibald PR is updated.

BTW I tried to run this test

Avalonia/tests/Avalonia.LeakTests/ControlTests.cs

Line 422 in 06b336d

public void TabItem_Is_Freed()

and neither new version nor old one are passing. But if I take memory snapshots I can see that fixed version is working as intended. Are these tests out of date or I should fix it?

[maxkatz6]

You can iterate over ItemsPresenterPart children instead, without calling ContainerFromIndex for each item. Which in theory can be virtualized too (==a lot of unnecessary iterations).

[DmitryZhelnin]

Done

[maxkatz6]

Removal of this method is a breaking change. As noticed from the CI fail.
Can yo keep this method empty with Obsolete attribute? Just in case.

[maxkatz6]

@DmitryZhelnin

[DmitryZhelnin]

@maxkatz6 done

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0039810-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0039817-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]