Microsoft-ui-xaml: Is ItemsRepeater leaky

[harvinders]

Describe the bug
@ranjeshj
When navigating away from a page containing ItemsRepeater, ItemsRepeater does not release the all the Item views.

Steps to reproduce the bug
Please get the repo app from here

Steps to reproduce the behavior:

1. Click on a image, it takes you to next page
2. Click back button
3. Repeat [1] and [2] multiple times
4. End with the clicking on the Image
5. Take a memory snapshot and check the StoryView count,

Expected behavior
ItemsRepeater should not leak

Version Info

NuGet package version: 2.4.0-prerelease.200203002

| Windows 10 version | Saw the problem? |
| :--------------------------------- | :-------------------- |
| Insider Build (xxxxx) | |
| November 2019 Update (18363) | |
| May 2019 Update (18362) | |
| October 2018 Update (17763) | Yes|
| April 2018 Update (17134) | |
| Fall Creators Update (16299) | |
| Creators Update (15063) | |

| Device form factor | Saw the problem? |
| :-------------------- | :------------------- |
| Desktop | Yes |
| Mobile | |
| Xbox | |
| Surface Hub | |
| IoT | |

Additional context

[ranjeshj]

@harvinders I think the issue is garbage collection is not kicking in and cleaning up yet. Try adding a destructor on MainPage and see if it is getting hit. It will not get hit every time you navigate back and forth. At some point (when gc runs) it will get hit and everything will get cleaned up.

Alternatively you can try forcing a gc by running gc.collect() in your code to see things getting cleaned up.

You don't have to worry about it as long as it gets cleaned up at some point. If you never see it getting cleaned up or if you see your memory usage going up continuously without dropping down then it is likely a leak.

[harvinders]

@ranjeshj I added following code on the DetailPage (The page I navigate to) and still see the count keep on increasing.

        protected override void OnNavigatedTo(NavigationEventArgs e)
        {
            base.OnNavigatedTo(e);

            ViewModel = e.Parameter as StoryViewModel;
            GC.Collect();
            GC.WaitForPendingFinalizers();
        }

[harvinders]

Hi @ranjeshj @StephenLPeters

I have updated and simplified repo here.

The repo shows that if I use GridView instead of ItemsRepeater, there is no leak, however when I start using ItemsRepeater it starts leaking. I have added GC calls already in the code so that GC should not be a factor. For quick summary, I am attaching the diagnostic view of heap using visual studio.

You would have to uncomment code in MainPage.xaml to switch between GridView and ItemsRepeater

GridView

Look for NewsView and NewsViewModel

Opening the application (Heap at snapshot#1)

Tapping and moving to detail page. (Heap at snapshot#2)

Press back and come back to main page. (compared snapshot#3 with snapshot#2 above)

Move to and fro, settled on main page after some time. (compared snapshot#4 with snapshot#3 above)

ItemsRepeater

Opening the application (Heap at snapshot#1)

Please note, instead of 13 NewsView 14 gets created. Compared to GridView one more is created everytime.

Tapping and moving to detail page. (Heap at snapshot#2)

13 NewsView are removed

Press back and come back to main page. (compared snapshot#3 with snapshot#2 above)

NewsView count is 28, (2 more than GridView)

Move to and fro, settled on main page after 5 times. (compared snapshot#4 with snapshot#3 above)

NewsView count is increases by 5

[chingucoding]

I think the ItemsRepeater is leaking. We create 14 views, 13 with the actual item from the ItemsSource and one with no item set in binding. After navigating, we clear all elements that have an item set and then recreate 14 items, 13 with item and one without.

[chingucoding]

Unless someone (from the team or the community) is working on this issue right now, I would like to work on this!

@ranjeshj @AnaWishnoff FYI

[anawishnoff]

@chingucoding Would be great for you to work on this, thank you!

[chingucoding]

I was able to track down where we create the element that does not get cleared:
https://github.com/microsoft/microsoft-ui-xaml/blob/c5fa8a6b45efe820b80aa994751801df553117f7/dev/Repeater/ItemsRepeater.cpp#L626

This if case creates one item as part of loading the datatemplate, however this item will never get cleared. Assigning the loaded template or the framework element to a field that only sits in that scope also does not clear the item.

@StephenLPeters @ranjeshj Shouldn't the item be cleared when we leave the scope? Is there a way we can manually force clearing/freeing such items (delete and free both crash)? Unfortunately, I haven't found much information on this.

[ranjeshj]

I don't think that line can be causing the leak. These are ref counted and should get collected automatically once it leaves that scope.

[chingucoding]

@ranjeshj I looked at all the items that got eventually got garbage collected and looked at the "indices" of those items and I am very sure that this is the only item that does not get collected.

I tested this the following way:

1. Have a UserControl with a number property which defaults to -1
2. Have list of items that expose numbers in some way (all numbers are 0 and above)
3. Have a DataTemplate which uses said UserControl and binds the number property to the numbers exposed by the collection.

Then I opened the page with the repeater, saw that the usercontrol got created 11 times, once mentioned in the line above, and the other 10 times somewhere in the ElementFactory I think.

Then I left the page, and re entered the page. That is when the "old" usercontrols got cleared. Then I checked which numbers where being cleared. Every number in my "numbers collection" was present in a single UserControl. No cleared UserControl had the number -1.

Since the line linked above does not set the datacontext and thus not override the number, the number for that UserControl must be -1. As we didn't find any UserControl that got gc'ed with the number property being -1, that item has to be the one not being cleared.

[ranjeshj]

When you mean 'cleared' do you mean to say that ElementClearing got called ? This particular instance was not prepared so it won't get cleared. Can you check if the destructor gets called ? I would expect the destructor to get called once we are out of the scope for this particular element.

[chingucoding]

With "cleared" I meant the destructor of said UserControl got called. And the strange thing is that when we leave the scope of the line linked above, there is no call to the destructor, instead we keep creating new controls in the ElementFactory.

[ranjeshj]

That is interesting. I would have expected it to get collected. Perhaps you can try breaking that line out of the condition block and see if that helps. @jevansaks @kmahone Any ideas what might be happening here ?

[chingucoding]

Unfortunately that doesn't help either. I've tried the following combinations:

            const auto content = dataTemplate.LoadContent();
            const auto element = content.as<winrt::FrameworkElement>();
            if (element == nullptr) {
                // We have a DataTemplate which is empty, so we need to set it to true
                m_isItemTemplateEmpty = true;
            }

            const auto element = dataTemplate.LoadContent().as<winrt::FrameworkElement>();
            if (element == nullptr) {
                // We have a DataTemplate which is empty, so we need to set it to true
                m_isItemTemplateEmpty = true;
            }

Also I've tried recycling the element with the following code, however that didn't work either:

            const auto element = dataTemplate.LoadContent().as<winrt::FrameworkElement>();
            if (element == nullptr) {
                // We have a DataTemplate which is empty, so we need to set it to true
                m_isItemTemplateEmpty = true;
            }

            // Recycle template
            const auto factoryArgs = winrt::make_self<ElementFactoryRecycleArgs>();
            factoryArgs->Element(element);
            m_itemTemplateWrapper.RecycleElement(static_cast<winrt::ElementFactoryRecycleArgs>(*factoryArgs));

[jevansaks]

@ranjeshj you did work in the DataTemplate code a long time ago to cache the last-built element for an optimization. Is that coming into play here?

Also if the test app is C# then there's very possibly some .NET GC behavior coming into play that's keeping things alive. I've definitely seen oddities with the Frame control where things on a Page don't get cleaned up until you go backwards and then go forwards to a different Page instance.

I think the best way to dig in is write a C++ app to see if direct references (no GC) leaks. If it does then the problem is in native code somewhere for sure. If not then we should suspect a GC problem, which could be mitigated with weak_refs on whatever link you want to be cleaned up.

[chingucoding]

Created a C++ version of the repro project and it seems that is issue is also existent there too.
We create one more view than we clean up, the repro project can be found here: https://github.com/chingucoding/RepeaterMemoryLeak

[ranjeshj]

Created a C++ version of the repro project and it seems that is issue is also existent there too.
We create one more view than we clean up, the repro project can be found here: https://github.com/chingucoding/RepeaterMemoryLeak

Thanks @chingucoding. Could you run an experiment to further track it down? Can you try providing a custom ElementFactory as ItemTemplate instead of DataTemplate. If it does not leak with a custom element factory, that will scope down the leak to DataTemplate. Thanks.

[chingucoding]

I've swapped out the DataTemplate in the linked repo with a custom ElementFactory, and with that, we always clear the same number of items, that we created, resulting in no memory leaks. So it is an issue with the DataTemplate not clearing the items correctly, given that an ElementFactory behaves as expected and results in no memory leak.

Btw: Thanks @harvinders for the repro project, that helped a lot here!

[ranjeshj]

Thanks @chingucoding . That helps narrow it down. We could probably repro the issue with just a DataTemplate (no repeater). When the DataTemplate goes out of scope it looks like it is not cleaning all the containers recycled into it. It looks like navigating back and forth is making it show up more because we use a different DataTemplate object each time.

One mitigation could be to take the DataTemplate out of the page and have it in App.Resorces. Perf wise, that is much better because you are now not throwing away and creating new elements when navigating across the pages. The elements get recycled into the same DataTemplate instance.

That said, this does look like a bug in DataTemplate that needs to be fixed (post WinUI3 since DataTemplate code is in OS XAML)

[StephenLPeters]

@harvinders to see @chingucoding and @ranjeshj 's latest updates

[harvinders]

@StephenLPeters , @chingucoding , @ranjeshj If I am not mistaken the early experiments that I did I compared ItemsRepeater with ListView keeping other things as such. If the issue is with DataTempate should we also not see the leak in ListView? However, I did not saw any leaks there when I did my early experiements.

[jevansaks]

I'm going to try debugging through and see what clues I can find about the leaked objects. I agree with @harvinders that if we don't see this issue with ListView that I suspect the issue is somewhere in between ItemsRepeater and DataTemplate. When I looked at this yesterday I didn't see any outstanding pointers to the leaked object so I suspect that these are ref count leaks. Such things are very uncommon with the type of coding patterns we use in this codebase but I've seen it when there's some subtle unsafe things happening (like reinterpret_casts). Anyhow ... I'll debug for a bit.

[ranjeshj]

Thanks @jevansaks.
@harvinders ListView does recycling on its own internally (does not leverage the IElementFactory). IElementFactory was introduced to decouple recycling from the collection controls when ItemsRepeater was created. It looks like we are leaking somewhere in this new codepath. Given that custom implementations of IElementFactory are not leaking (as @chingucoding investigated), I suspect the leak is somewhere in DataTemplate or its usage.

[jevansaks]

From what I can see so far, the call in ItemsRepeater to LoadContent here:
https://github.com/microsoft/microsoft-ui-xaml/blob/6265117611bfd5bb84a3c582c67071775fa9a1ca/dev/Repeater/ItemsRepeater.cpp#L624

Is returning an element that has a ref count of 2 instead of 1. It doesn't appear to be doing anything different than the other LoadContent() call that produces the items which are then going into the tree. But I wonder if LoadContent is adding a reference under the assumption that it will eventually be added into the tree and without parenting the element that we've broken some implicit contract?

[jevansaks]

It appears that there's some kind of expectation of how we're supposed to handle the return value of DataTemplate.LoadContent() -- we can't just discard the result like we are doing here.

The good news is that another workaround in addition to the IElementFactory approach is a DataTemplateSelector in place of the direct DataTemplate works as well.

We'll continue to investigate what's going on here.

[jevansaks]

So we understand the problem and it is a bug in DataTemplate::LoadContent. We will work to get a fix in WinUI3 for it but in the meantime we'll need some kind of workaround in ItemsRepeater. The only way I can see to work around it is that the element returned by LoadContent needs to be added to a UIElementCollection. It can be immediately removed after it's added.

The other option is to reconsider the code where we're calling LoadContent at all. Why is it important that we set the m_isItemTemplateEmpty flag ? We don't do that if DataTemplateSelector returns empty things, do we?

But if it is important then I think the simplest workaround is add the item to the Repeater's own Children and then immediately remove it. Something like:

    if (auto content = dataTemplate.LoadContent().as<winrt::FrameworkElement>())
    {
        // Due to bug <To be filed> we need to get the framework to take ownership of the extra implicit ref that was returned by LoadContent
        auto children = Children();
        children.Append(content);
        children.RemoveAtEnd();
    }
    else
    {
        // We have a DataTemplate which is empty, so we need to set it to true
        m_isItemTemplateEmpty = true;
    }

@chingucoding it sounds like you have a repro in MUXControlsTestApp where you can check this. Can you try this out?

[StephenLPeters]

@jevansaks did you want to open a new issue for the Winui3 work and leave your context there or is it pretty much all here?

[jevansaks]

Yes @StephenLPeters I'll create another issue for the DataTemplate::LoadContent bug, we can use this one to track the ItemsRepeater workaround.

[chingucoding]

@jevansaks Your workaround works, with it, we don't have a memory leak due to that LoadContent call!

You can find the testcode I used here: https://github.com/chingucoding/microsoft-ui-xaml/tree/itemsrepeaterleak-testcode
Note that the API test is broken though, not entirely sure why.

The other option is to reconsider the code where we're calling LoadContent at all. Why is it important that we set the m_isItemTemplateEmpty flag ? We don't do that if DataTemplateSelector returns empty things, do we?

It seems that we use that flag to request 0 space if we determined that the DataTemplate is empty. Maybe we should do the same for an empty DataTemplateSelector, though for that we would need to go through all items we got from that which does not sound ideal.

[jevansaks]

Thanks @chingucoding ! Did you build a new test to verify this functionality? If not I'll continue my tinkering to build one.

Note that the API test is broken though,

Which API test is broken with this change?

[chingucoding]

Oh right, should have mentioned this. I've added both a test UI and tried to add an API test for this, which you can find here.
However that test does not work as expected, maybe this is a timing issue?

[jevansaks]

Check out LeakTests.cs for API tests that verify leaks in C#. The problem is that you'll need to poke the GC to get the garbage collector to run and clean up the objects. Try adding this just before your last check:

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

[chingucoding]

Adding the snippet worked, the test works now as expected! If you like, you can use that test, if you want to create a PR for this issue 😄

I've also updated the test on the linked branch.

[jevansaks]

Adding the snippet worked, the test works now as expected! If you like, you can use that test, if you want to create a PR for this issue 😄

You did all the hard work, you should create the PR! But I'm happy to create the PR if you'd prefer. :)

[chingucoding]

You did all the hard work, you should create the PR! But I'm happy to create the PR if you'd prefer. :)

You were the one finding a fix for this issue not me, so feel free to create the PR :)

[ranjeshj]

m_isItemTemplateEmpty flag is quite straightforward when we are passed in a DataTemplate. I suspect that when using a template selector, we don't know if the selected template is empty or not until much later. What if one of the templates is empty and the others are not. We cannot use the same logic (if m_isItemTemplateEmpty is true, we currently short-circuit measure and report zero desired size for the repeater). So we did not support the case where if the data template is empty in a template selector is empty. Instead we just don't support that case. One can workaround by providing a template that contains a single element of 0 size. This was discussed in the PR which added m_isItemTemplateEmpty flag.

[jevansaks]

You did all the hard work, you should create the PR! But I'm happy to create the PR if you'd prefer. :)

You were the one finding a fix for this issue not me, so feel free to create the PR :)

Acknowledged @chingucoding , I'll get the PR ready.