Skip to content

Commit

Permalink
feat(NavigationView): Include Fix issue with exception being thrown d…
Browse files Browse the repository at this point in the history
…uring cleanup of NavigationView
  • Loading branch information
MartinZikmund committed Sep 18, 2023
1 parent 647c4d8 commit 17e7855
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1333,5 +1333,37 @@ async Task SetPaneConfigAndVerifyToolTips(NavigationViewPaneDisplayMode paneDisp
}
});
}

[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;
});
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.
// MUX Reference NavigationView.cpp, commit 4e2990d
// MUX Reference NavigationView.cpp, commit d8d4f4f

#pragma warning disable 105 // remove when moving to WinUI tree

Expand All @@ -13,6 +13,7 @@
using Microsoft.UI.Xaml.Automation.Peers;
using Microsoft.UI.Xaml.Controls.AnimatedVisuals;
using Uno.Disposables;
using Uno.Foundation.Logging;
using Uno.UI.Helpers.WinUI;
using Windows.ApplicationModel.Core;
using Windows.Foundation;
Expand Down Expand Up @@ -3699,6 +3700,8 @@ private void SetNavigationViewItemRevokers(NavigationViewItem nvi)

private void ClearNavigationViewItemRevokers(NavigationViewItem nvi)
{
RevokeNavigationViewItemRevokers(nvi);

nvi.EventRevoker.Disposable = null;
m_itemsWithRevokerObjects.Remove(nvi);
}
Expand All @@ -3707,11 +3710,35 @@ private void ClearAllNavigationViewItemRevokers()
{
foreach (var nvi in m_itemsWithRevokerObjects)
{
nvi.EventRevoker.Disposable = null;
// 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);

#if !HAS_UNO // TODO Uno specific: Revokers are implemented differently than in WinUI.
nvi.SetValue(s_NavigationViewItemRevokersProperty, nullptr);
#endif
}
catch (Exception ex)
{
if (this.Log().IsEnabled(LogLevel.Error))
{
this.Log().LogError("Failed to clear revokers for NavigationViewItem.", ex);
}
}
}
m_itemsWithRevokerObjects.Clear();
}

private void RevokeNavigationViewItemRevokers(NavigationViewItem nvi)
{
// TODO Uno specific: Revokers are implemented differently than in WinUI.
nvi.EventRevoker.Disposable = null;
}

private void InvalidateTopNavPrimaryLayout()
{
if (m_appliedTemplate && IsTopNavigationView())
Expand Down

0 comments on commit 17e7855

Please sign in to comment.