From 25f793bc2a7bb05192584aa332643ed8e3abfc0a Mon Sep 17 00:00:00 2001
From: Stephen Peters <stephenlpeters@gmail.com>
Date: Mon, 7 Mar 2022 16:18:44 -0800
Subject: [PATCH 1/3] Attempt to fix issue with exception being thrown during
 cleanup.

---
 dev/NavigationView/NavigationView.cpp         | 20 +++++++++++++++++--
 dev/NavigationView/NavigationView.h           |  3 ++-
 .../NavigationViewItemRevokers.h              |  8 ++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp
index 3cafecb6b4..f03b03d47d 100644
--- a/dev/NavigationView/NavigationView.cpp
+++ b/dev/NavigationView/NavigationView.cpp
@@ -3418,19 +3418,35 @@ void NavigationView::SetNavigationViewItemRevokers(const winrt::NavigationViewIt
 
 void NavigationView::ClearNavigationViewItemRevokers(const winrt::NavigationViewItem& nvi)
 {
+    RevokeNavigationViewItemRevokers(nvi);
     nvi.SetValue(s_NavigationViewItemRevokersProperty, nullptr);
     m_itemsWithRevokerObjects.erase(nvi);
 }
 
-void NavigationView::ClearAllNavigationViewItemRevokers()
+void NavigationView::ClearAllNavigationViewItemRevokers() noexcept
 {
     for (const auto& nvi : m_itemsWithRevokerObjects)
     {
-        nvi.SetValue(s_NavigationViewItemRevokersProperty, nullptr);
+        try
+        {
+            RevokeNavigationViewItemRevokers(nvi);
+            nvi.SetValue(s_NavigationViewItemRevokersProperty, nullptr);
+        }
+        catch (...) {}
     }
     m_itemsWithRevokerObjects.clear();
 }
 
+void NavigationView::RevokeNavigationViewItemRevokers(const winrt::NavigationViewItem& nvi)
+{
+    if (auto const revokers = nvi.GetValue(s_NavigationViewItemRevokersProperty))
+    {
+        if (auto const revokersAsNVIR = revokers.try_as<NavigationViewItemRevokers>()) {
+            revokersAsNVIR->RevokeAll();
+        }
+    }
+}
+
 void NavigationView::InvalidateTopNavPrimaryLayout()
 {
     if (m_appliedTemplate && IsTopNavigationView())
diff --git a/dev/NavigationView/NavigationView.h b/dev/NavigationView/NavigationView.h
index bd8213c660..1f7085f495 100644
--- a/dev/NavigationView/NavigationView.h
+++ b/dev/NavigationView/NavigationView.h
@@ -187,7 +187,8 @@ class NavigationView :
     inline static GlobalDependencyProperty s_NavigationViewItemRevokersProperty{ nullptr };
     void SetNavigationViewItemRevokers(const winrt::NavigationViewItem& nvi);
     void ClearNavigationViewItemRevokers(const winrt::NavigationViewItem& nvi);
-    void ClearAllNavigationViewItemRevokers();
+    void ClearAllNavigationViewItemRevokers() noexcept;
+    void RevokeNavigationViewItemRevokers(const winrt::NavigationViewItem& nvi);
     std::set<winrt::NavigationViewItem> m_itemsWithRevokerObjects;
 
     void InvalidateTopNavPrimaryLayout();
diff --git a/dev/NavigationView/NavigationViewItemRevokers.h b/dev/NavigationView/NavigationViewItemRevokers.h
index e7592189f9..82c7d04c6c 100644
--- a/dev/NavigationView/NavigationViewItemRevokers.h
+++ b/dev/NavigationView/NavigationViewItemRevokers.h
@@ -11,4 +11,12 @@ class NavigationViewItemRevokers : public winrt::implements<NavigationViewItemRe
     winrt::UIElement::GotFocus_revoker gotFocusRevoker{};
     PropertyChanged_revoker isSelectedRevoker{};
     PropertyChanged_revoker isExpandedRevoker{};
+
+    void RevokeAll() {
+        if (tappedRevoker) tappedRevoker.revoke();
+        if (keyDownRevoker) keyDownRevoker.revoke();
+        if (gotFocusRevoker) gotFocusRevoker.revoke();
+        if (isSelectedRevoker) isSelectedRevoker.revoke();
+        if (isExpandedRevoker) isExpandedRevoker.revoke();
+    }
 };

From 269bbef5b52a900c93890902f515238329997cd4 Mon Sep 17 00:00:00 2001
From: Stephen Peters <stephenlpeters@gmail.com>
Date: Thu, 10 Mar 2022 16:43:21 -0800
Subject: [PATCH 2/3] Add test

---
 .../NavigationViewTests.cs                    | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs
index c3b3a6eff0..c3540c524d 100644
--- a/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs
+++ b/dev/NavigationView/NavigationView_ApiTests/NavigationViewTests.cs
@@ -1263,5 +1263,39 @@ void SetPaneConfigAndVerifyToolTips(NavigationViewPaneDisplayMode paneDisplayMod
                 }
             });
         }
+
+        [TestMethod]
+        public void VerifyNVIOutlivingNVDoesNotCrash()
+        {
+            NavigationViewItem menuItem1 = null;
+            RunOnUIThread.Execute(() =>
+            {
+                var navView = new NavigationView();
+                menuItem1 = new NavigationViewItem();
+
+                navView.MenuItems.Add(menuItem1);
+                Content = navView;
+                Content.UpdateLayout();
+
+                navView.MenuItems.Clear();
+                Content = menuItem1;
+                Content.UpdateLayout();
+            });
+
+            IdleSynchronizer.Wait();
+
+            RunOnUIThread.Execute(() =>
+            {
+                GC.Collect();
+            });
+
+            IdleSynchronizer.Wait();
+
+            RunOnUIThread.Execute(() =>
+            {
+                // NavigationView has a handler on NVI's IsSelected DependencyPropertyChangedEvent.
+                menuItem1.IsSelected = !menuItem1.IsSelected;
+            });
+        }
     }
 }
\ No newline at end of file

From 16336bbc75a3a5016236add07ebc37645612e69f Mon Sep 17 00:00:00 2001
From: Stephen L Peters <stpete@microsoft.com>
Date: Fri, 11 Mar 2022 10:56:11 -0800
Subject: [PATCH 3/3] Add a comment explaining the need for the try catch block

---
 dev/NavigationView/NavigationView.cpp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp
index f03b03d47d..966cc20ecf 100644
--- a/dev/NavigationView/NavigationView.cpp
+++ b/dev/NavigationView/NavigationView.cpp
@@ -3427,6 +3427,10 @@ void NavigationView::ClearAllNavigationViewItemRevokers() noexcept
 {
     for (const auto& nvi : m_itemsWithRevokerObjects)
     {
+        // ClearAllNavigationViewItemRevokers is only called in the destructor, where exceptions cannot be thrown.
+        // If the associated NV has not yet been cleaned up, we must detach these revokers or risk a call into freed
+        // memory being made.  However if they have been cleaned up these calls will throw. In this case we can ignore
+        // those exceptions.
         try
         {
             RevokeNavigationViewItemRevokers(nvi);