Fix issue with exception being thrown during cleanup of NavigationView. #6797

StephenLPeters · 2022-03-08T00:21:47Z

When NavigationView is being cleaned up in needs to detatch the event handlers on its children NVI's that are still alive, however if those NVI's have already been cleaned up they can be in a state that SetValue(nullptr) throws an exception, which are not allowed during the NavigationView's destructor.

StephenLPeters · 2022-03-08T00:22:59Z

/azp run

azure-pipelines · 2022-03-08T00:23:11Z

Azure Pipelines successfully started running 1 pipeline(s).

kmahone · 2022-03-08T00:45:32Z

I am curious: Why is there a need for 's_NavigationViewItemRevokersProperty'? Why can the revokers not be stored in a std::vector on NavigationView?

StephenLPeters · 2022-03-08T18:57:20Z

I am curious: Why is there a need for 's_NavigationViewItemRevokersProperty'? Why can the revokers not be stored in a std::vector on NavigationView?

I think the original idea is that by attaching these revokers to the NVI the lifetime of the revoker objects would be tied to the NVI and we can set it and forget it. This was the case for multiple years until we identified a problem, the this pointer in the handler could be invalid if the NVI outlives the NavigationView. So we updated this to track the NVI's that have revoker objects set on them so we can clear them when we shut down. Looking closer at this there are definite improvements we can make...

I see three strategies

keep the current model, but replace the this pointer in the handlers with a weak_ref, this should make it so that when the NVI outlives the NV we don't have an invalid raw pointer. Then we can remove all of the tracking.
Keep the current model, and when the navigation view is being destructed iterate over all NVI and revoke all of the attached objects. We don't need to track which nvi's have revoker objects on them. We can be sure they all do.
We can stop attaching these revokers to the NVI itself, keeping them in a map of nvi to revokers objects and then everytime a nvi wants to get cleaned up we will need to revoke all the associated handlers.

@kmahone @lhecker thoughts?

kmahone · 2022-03-08T19:03:37Z

dev/NavigationView/NavigationView.cpp

+            RevokeNavigationViewItemRevokers(nvi);
+            nvi.SetValue(s_NavigationViewItemRevokersProperty, nullptr);
+        }
+        catch (...) {}


Do we know what was causing the exceptions in the first place?

I'm currently looking into it, because I'm now able to reproduce the issue. 🙂

kmahone · 2022-03-08T19:18:26Z

I am curious: Why is there a need for 's_NavigationViewItemRevokersProperty'? Why can the revokers not be stored in a std::vector on NavigationView?

I think the original idea is that by attaching these revokers to the NVI the lifetime of the revoker objects would be tied to the NVI and we can set it and forget it. This was the case for multiple years until we identified a problem, the this pointer in the handler could be invalid if the NVI outlives the NavigationView. So we updated this to track the NVI's that have revoker objects set on them so we can clear them when we shut down. Looking closer at this there are definite improvements we can make...

I see three strategies

keep the current model, but replace the this pointer in the handlers with a weak_ref, this should make it so that when the NVI outlives the NV we don't have an invalid raw pointer. Then we can remove all of the tracking.

Keep the current model, and when the navigation view is being destructed iterate over all NVI and revoke all of the attached objects. We don't need to track which nvi's have revoker objects on them. We can be sure they all do.

We can stop attaching these revokers to the NVI itself, keeping them in a map of nvi to revokers objects and then everytime a nvi wants to get cleaned up we will need to revoke all the associated handlers.

@kmahone @lhecker thoughts?

Would a std::vector<winrt::Foo_revoker> on NavView for each event (Tapped, KeyDown, etc.) be sufficient? Or even a std::vector<NavigationViewItemRevokers>.

Once the NavView is being destroyed, the std::vector would be destroyed and the revokers would get revoked.

Is the problem with this approach that the NavView would keep the NVIs alive even if they are removed from the NavView? Does a winrt::Foo_revoker keep a strong ref to the source element? I'm not sure.

StephenLPeters · 2022-03-08T19:48:15Z

Would a std::vector<winrt::Foo_revoker> on NavView for each event (Tapped, KeyDown, etc.) be sufficient? Or even a std::vector<NavigationViewItemRevokers>.

Once the NavView is being destroyed, the std::vector would be destroyed and the revokers would get revoked.

Is the problem with this approach that the NavView would keep the NVIs alive even if they are removed from the NavView? Does a winrt::Foo_revoker keep a strong ref to the source element? I'm not sure.

The problem with this approach is that the revoker object being stored in NavigationView will keep the DependencyPropertyChangedCallback object alive even after the associated NVI has been cleaned up, that is until the NavigationView is destructed.

StephenLPeters · 2022-03-08T19:54:51Z

I think... the event source has a strong reference to the DependencyPropertyChangedCallback object.. does the revoker object? it might only have a weak ref...

kmahone · 2022-03-08T19:55:38Z

Would a std::vector<winrt::Foo_revoker> on NavView for each event (Tapped, KeyDown, etc.) be sufficient? Or even a std::vector<NavigationViewItemRevokers>.
Once the NavView is being destroyed, the std::vector would be destroyed and the revokers would get revoked.
Is the problem with this approach that the NavView would keep the NVIs alive even if they are removed from the NavView? Does a winrt::Foo_revoker keep a strong ref to the source element? I'm not sure.

The problem with this approach is that the revoker object being stored in NavigationView will keep the DependencyPropertyChangedCallback object alive even after the associated NVI has been cleaned up, that is until the NavigationView is destructed.

And why is that a problem? Is it because when we then try to revoke the event from the now destroyed NVI we crash?

StephenLPeters · 2022-03-08T20:18:28Z

It would result in memory growth as the app remains open. If the NavigationView is the top level UI and never gets cleaned up except as a (as it is designed for) then as items get added and removed from the navigation view the application memory would grow.

StephenLPeters · 2022-03-08T21:19:16Z

Confrimed that the revoker object (usually, and in this case always) has a weak ref to the callback object, so in this case the only down side of this method would be that the vector of revokers will grow for the lifetime of the navigationview.

StephenLPeters · 2022-03-11T00:46:23Z

/azp run

azure-pipelines · 2022-03-11T00:46:35Z

Azure Pipelines successfully started running 1 pipeline(s).

kmahone · 2022-03-11T18:09:00Z

dev/NavigationView/NavigationView.cpp

 {
    for (const auto& nvi : m_itemsWithRevokerObjects)
    {
-        nvi.SetValue(s_NavigationViewItemRevokersProperty, nullptr);
+        try


Add a comment explaining why this is necessary.

StephenLPeters · 2022-03-11T18:56:28Z

/azp run

azure-pipelines · 2022-03-11T18:56:40Z

Azure Pipelines successfully started running 1 pipeline(s).

Attempt to fix issue with exception being thrown during cleanup.

25f793b

ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 8, 2022

StephenLPeters closed this Mar 8, 2022

StephenLPeters reopened this Mar 8, 2022

StephenLPeters marked this pull request as draft March 8, 2022 00:22

zadjii-msft mentioned this pull request Mar 8, 2022

Crash on exit in NavigationViewItemRevokers::~NavigationViewItemRevokers microsoft/terminal#12413

Closed

kmahone reviewed Mar 8, 2022

View reviewed changes

lhecker mentioned this pull request Mar 10, 2022

NavigationViewItemRevokers use WeakRef this pointers #6803

Closed

StephenLPeters mentioned this pull request Mar 11, 2022

NavigationView crashes when full screen #6788

Closed

2 tasks

Add test

269bbef

StephenLPeters added area-NavigationView NavView control team-Controls Issue for the Controls team labels Mar 11, 2022

StephenLPeters changed the title ~~Attempt to fix issue with exception being thrown during cleanup.~~ Fix issue with exception being thrown during cleanup of NavigationView. Mar 11, 2022

StephenLPeters marked this pull request as ready for review March 11, 2022 00:46

kmahone reviewed Mar 11, 2022

View reviewed changes

kmahone approved these changes Mar 11, 2022

View reviewed changes

Add a comment explaining the need for the try catch block

16336bb

ojhad approved these changes Mar 11, 2022

View reviewed changes

StephenLPeters merged commit d8d4f4f into main Mar 12, 2022

StephenLPeters deleted the user/stpete/NavigationViewItemRevokers branch March 12, 2022 16:24

ojhad removed the needs-triage Issue needs to be triaged by the area owners label Mar 14, 2022

bkudiess mentioned this pull request Mar 15, 2022

Cherry-pick PR #6797 to release/2.7 #6833

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with exception being thrown during cleanup of NavigationView. #6797

Fix issue with exception being thrown during cleanup of NavigationView. #6797

StephenLPeters commented Mar 8, 2022 •

edited

Loading

StephenLPeters commented Mar 8, 2022

azure-pipelines bot commented Mar 8, 2022

kmahone commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

kmahone Mar 8, 2022

lhecker Mar 8, 2022

kmahone commented Mar 8, 2022 •

edited

Loading

StephenLPeters commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

kmahone commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

StephenLPeters commented Mar 11, 2022

azure-pipelines bot commented Mar 11, 2022

kmahone Mar 11, 2022

StephenLPeters commented Mar 11, 2022

azure-pipelines bot commented Mar 11, 2022

Fix issue with exception being thrown during cleanup of NavigationView. #6797

Fix issue with exception being thrown during cleanup of NavigationView. #6797

Conversation

StephenLPeters commented Mar 8, 2022 • edited Loading

StephenLPeters commented Mar 8, 2022

azure-pipelines bot commented Mar 8, 2022

kmahone commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

kmahone Mar 8, 2022

Choose a reason for hiding this comment

lhecker Mar 8, 2022

Choose a reason for hiding this comment

kmahone commented Mar 8, 2022 • edited Loading

StephenLPeters commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

kmahone commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

StephenLPeters commented Mar 8, 2022

StephenLPeters commented Mar 11, 2022

azure-pipelines bot commented Mar 11, 2022

kmahone Mar 11, 2022

Choose a reason for hiding this comment

StephenLPeters commented Mar 11, 2022

azure-pipelines bot commented Mar 11, 2022

StephenLPeters commented Mar 8, 2022 •

edited

Loading

kmahone commented Mar 8, 2022 •

edited

Loading