Clear DataContext of cleared elements in ItemsRepeater

dev/Repeater/APITests/Common/Mocks/MockRecyclingTestFactory.cs

@@ -0,0 +1,29 @@
+using Microsoft.UI.Xaml.Controls;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests.Common.Mocks
+{
+    public class MockRecyclingTestFactory : RecyclingElementFactory
+    {
+
+        public List<object> DataContextsInGetElement = new List<object>();
+
+        protected override UIElement GetElementCore(Microsoft.UI.Xaml.Controls.ElementFactoryGetArgs args)
+        {
+            var uiElement = base.GetElementCore(args);
+            if(uiElement is FrameworkElement frameworkElement)
+            {
+                DataContextsInGetElement.Add(frameworkElement.DataContext);
+            }
+            return base.GetElementCore(args);
+        }
+
+        protected override void RecycleElementCore(Microsoft.UI.Xaml.Controls.ElementFactoryRecycleArgs args)
+        {
+            base.RecycleElementCore(args);
+        }
+
+    }
+}

dev/Repeater/APITests/Repeater_APITests.projitems

@@ -18,6 +18,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Common\LayoutExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Common\FlowLayoutDerived.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Common\Mocks\MockNonVirtualizingLayout.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Common\Mocks\MockRecyclingTestFactory.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Common\NamedGroup.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Common\NonVirtualStackLayout.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Common\OrientationBasedMeasures.cs" />

dev/Repeater/APITests/ViewManagerTests.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See LICENSE in the project root for license information.
 
 using Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests.Common;
@@ -759,6 +759,76 @@ public void ValidateFocusMoveOnElementClearedWithUniqueIds()
                     () => { ValidateCurrentFocus(repeater, 0 /*expectedIndex */, "3" /* expectedContent */); }
                 });
         }
+
+        [TestMethod]
+        // Why does this test work?
+        // When the elements get created from the RecyclingElementFactory, we get already "existing" data templates.
+        // However, the reason for the crash in #2384 is that those "empty" data templates actually still had their data context
+        // The data context of course only gets set after we give the ViewManager the data template.
+        // Thus, the DataContext we see in GetElementCore is the OLD datacontext.
+        // If that data context is not null, that means it did not get cleared when the element was recycled, which is the wrong behavior.
+        // To check if the clearing is working correctly, we log all data contexts, and check if all of them are null.
+        public void ValidateElementClearingClearsDataContext()
+        {
+            ItemsRepeater repeater = null;
+            MockRecyclingTestFactory elementFactory = null;
+            int elementClearingRaisedCount = 0;
+            ItemsRepeaterScrollHost scroller = null;
+            Log.Comment("Initialize ItemsRepeater");
+            RunOnUIThread.Execute(() =>
+            {
+                elementFactory = new MockRecyclingTestFactory();
+                elementFactory.RecyclePool = new RecyclePool();
+                elementFactory.Templates["Item"] = (DataTemplate)XamlReader.Load(
+                    @"<DataTemplate xmlns='http://schemas.microsoft.com/winfx/2006/xaml/presentation'> " + "<Button Content=\"{Binding}\" />" + @"</DataTemplate>");
+
+                repeater = CreateRepeater(Enumerable.Range(0, 100),
+                    elementFactory);
+
+                repeater.Layout = new StackLayout();
+
+                var scrollViewer = new ScrollViewer() { Content = repeater };
+                scroller = new ItemsRepeaterScrollHost() 
+                {
+                    Height = 200,
+                    Width = 200,
+                    ScrollViewer = scrollViewer
+                };
+
+                Content = scroller;
+                scroller.UpdateLayout();
+                repeater.ElementClearing += Repeater_ElementClearing;
+            });
+
+            IdleSynchronizer.Wait();
+
+            Log.Comment("Scrolling ItemsRepeater around to clear and recycle elements");
+            RunOnUIThread.Execute(() =>
+            {
+                scroller.ScrollViewer.ChangeView(null, 400, null, true);
+                scroller.ScrollViewer.ChangeView(null, 600, null, true);
+                scroller.ScrollViewer.ChangeView(null, 800, null, true);
+            });
+
+            IdleSynchronizer.Wait();
+
+            Log.Comment("Verify ItemsRepeater cleared data contexts correctly");
+            Verify.IsTrue(elementClearingRaisedCount > 0, "ItemsRepeater should have cleared some elements");
+            foreach (object o in elementFactory.DataContextsInGetElement)
+            {
+                // TL;DR; on why we check this:
+                // All DataContexts that are present on our returned ItemTemplate(Shim) get logged inside DataContextsInGetElement.
+                // If one of them is not null, that means the DataContext did not get cleared correctly which is a bug.
+                Verify.IsNull(o, "Datacontext should be null on a recycled ItemTemplate");
+            }
+
+            void Repeater_ElementClearing(ItemsRepeater sender, Microsoft.UI.Xaml.Controls.ItemsRepeaterElementClearingEventArgs args)
+            {
+                elementClearingRaisedCount++;
+            }
+        }
+
+
         private void MoveFocusToIndex(ItemsRepeater repeater, int index)
         {
             var element = repeater.TryGetElement(index) as Control;

dev/Repeater/ViewManager.cpp

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
+// Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See LICENSE in the project root for license information.
 
 #include <pch.h>
@@ -97,6 +97,15 @@ void ViewManager::ClearElement(const winrt::UIElement& element, bool isClearedDu
     }
 }
 
+// We need to clear the datacontext to prevent crashes from happening,
+//  however we only do that if we were the ones setting it.
+// That is when one of the following is the case (numbering taken from line ~642):
+// 1.2    No ItemTemplate, data is not a UIElement
+// 2.1    ItemTemplate, data is not FrameworkElement
+// 2.2.2  Itemtemplate, data is FrameworkElement, ElementFactory returned Element different to data
+//
+// In all of those three cases, we the ItemTemplateShim is NOT null.
+// Luckily when we create the items, we store whether we were the once setting the DataContext.
 void ViewManager::ClearElementToElementFactory(const winrt::UIElement& element)
 {
     m_owner->OnElementClearing(element);
@@ -134,6 +143,16 @@ void ViewManager::ClearElementToElementFactory(const winrt::UIElement& element)
 
     auto virtInfo = ItemsRepeater::GetVirtualizationInfo(element);
     virtInfo->MoveOwnershipToElementFactory();
+
+    // During creation of this object, we were the one setting the DataContext, so clear it now.
+    if (virtInfo->MustClearDataContext())
+    {
+        if (const auto elementAsFE = element.try_as<winrt::FrameworkElement>())
+        {
+            elementAsFE.DataContext(nullptr);
+        }
+    }
+
     m_phaser.StopPhasing(element, virtInfo);
     if (m_lastFocusedElement == element)
     {
@@ -693,6 +712,8 @@ winrt::UIElement ViewManager::GetElementFromElementFactory(int index)
         // which means that the element has been recycled and not created from scratch.
         REPEATER_TRACE_PERF(L"ElementRecycled");
     }
+    // Clear flag
+    virtInfo->MustClearDataContext(false);
 
     if (data != element)
     {
@@ -731,6 +752,7 @@ winrt::UIElement ViewManager::GetElementFromElementFactory(int index)
             }();
 
             elementAsFE.DataContext(elementDataContext);
+            virtInfo->MustClearDataContext(true);
         }
         else
         {

dev/Repeater/VirtualizationInfo.h

@@ -48,6 +48,9 @@ class VirtualizationInfo : public winrt::implements<VirtualizationInfo, winrt::I
     winrt::IInspectable Data() const { return m_data.get(); }
     winrt::IDataTemplateComponent DataTemplateComponent() const { return m_dataTemplateComponent.get(); }
 
+    bool MustClearDataContext() const { return m_mustClearDataContext; }
+    void MustClearDataContext(bool mustClearDataContext) { m_mustClearDataContext = mustClearDataContext; }
+
     static constexpr int PhaseNotSpecified = std::numeric_limits<int>::min();
     static constexpr int PhaseReachedEnd = -1;
 
@@ -90,7 +93,8 @@ class VirtualizationInfo : public winrt::implements<VirtualizationInfo, winrt::I
     int m_phase{ PhaseNotSpecified };
     bool m_keepAlive{ false };
     bool m_autoRecycleCandidate{ false };
+    bool m_mustClearDataContext{ false };
 
     weak_ref<winrt::IInspectable> m_data;
     weak_ref<winrt::IDataTemplateComponent> m_dataTemplateComponent;
-};
+};