Repository: microsoft/terminal (language: cpp)

Issue:
Windows Terminal crashes when emptying settings.json with settings window open
### Windows Terminal version

1.14.1962.0

### Windows build number

10.0.19043.0

### Other Software

N/A

### Steps to reproduce

1. Open Terminal
2. Open Settings
3. Open JSON file
4. In external editor, CTRL+A to select the contents of the settings.json and press delete
5. Save


### Expected Behavior

No crash.

### Actual Behavior

Windows Terminal repopulates the settings.json file with defaults and then crashes with:

```
Unhandled exception at 0x00007FFCE568FB62 (KernelBase.dll) in WindowsTerminal.exe: 0xC000027B: An application-internal exception has occurred (parameters: 0x000002CDCB9A0970, 0x000000000000000A).
```

Which is being triggered from `TerminalApp.dll!winrt::TerminalApp::implementation::AppLogic::_ApplyStartupTaskStateChange$_ResumeCoro$1() Line 1004`.

Edit: Feedback Hub URL: https://aka.ms/AAhm2q3



Discussion / hints:
Possibly related to #12333.
I'll triage this as a separate bug for now, but cross my fingers and hope that it's the root cause of #12333. dmps loading now......
If need be, I created another feedback at https://aka.ms/AAhmblm this morning since my original didn't appear for over 12-13 hours under My Feedback.
Hmm. Coroutines are weird, but this looks like the error it's trying to report is `"The operation attempted to access data outside the valid range.."`. 

Interesting bit of the stack is:
```
2e (Inline Function) --------`--------     rpcrt4!Ndr64pSendReceive+0x2f [minio\rpc\ndr64\cltcall.cxx @ 184] 
2f 000000aa`24d9dab0 00007ffc`e6abc538     rpcrt4!NdrpClientCall3+0x3a4 [minio\rpc\ndr64\cltcall.cxx @ 328] 
30 000000aa`24d9de20 00007ffc`e6bcf642     combase!ObjectStublessClient+0x138 [onecore\com\combase\ndr\ndrole\amd64\stblsclt.cxx @ 369] 
31 000000aa`24d9e1b0 00007ffc`613f652d     combase!ObjectStubless+0x42 [onecore\com\combase\ndr\ndrole\amd64\stubless.asm @ 176] 
32 000000aa`24d9e200 00007ffc`613a8d24     TerminalApp!winrt::impl::consume_Windows_ApplicationModel_IStartupTask<winrt::Windows::ApplicationModel::IStartupTask>::State+0x2d [C:\a\_work\1\s\src\cascadia\TerminalApp\Generated Files\winrt\Windows.ApplicationModel.h @ 1077] 
33 000000aa`24d9e240 00007ffc`613a4d71     TerminalApp!winrt::TerminalApp::implementation::AppLogic::_ApplyStartupTaskStateChange$_ResumeCoro$1+0x1c4 [C:\a\_work\1\s\src\cascadia\TerminalApp\AppLogic.cpp @ 1004] 
34 (Inline Function) --------`--------     TerminalApp!std::experimental::coroutine_handle<void>::resume+0x9
```

Kinda looks like
```c++
        const auto task = co_await StartupTask::GetAsync(StartupTaskName);

        switch (task.State())
        {
```

Like, `task` was null? 

<hr>

Naw, this is in xaml. There's a stowed xaml exception:

```
    Windows_UI_Xaml!CUIElement::Measure+0x4bd  (7ffccced5dfd) [onecoreuap\windows\dxaml\xcp\core\core\elements\uielement.cpp @ 4063]
    Windows_UI_Xaml!CUIElement::Measure+0x4bd  (7ffccced5dfd) [onecoreuap\windows\dxaml\xcp\core\core\elements\uielement.cpp @ 4063]
    Windows_UI_Xaml!CUIElement::Measure+0x4bd  (7ffccced5dfd) [onecoreuap\windows\dxaml\xcp\core\core\elements\uielement.cpp @ 4063]
    Windows_UI_Xaml!CLayoutManager::UpdateLayout+0x107  (7ffcccecc987) [onecoreuap\windows\dxaml\xcp\core\layout\layoutmanager.cpp @ 279]
    Windows_UI_Xaml!CUIElement::UpdateLayout+0x3d  (7ffccce3d309) [onecoreuap\windows\dxaml\xcp\core\core\elements\uielement.cpp @ 3761]
    Windows_UI_Xaml!DirectUI::UIElementGenerated::UpdateLayout+0x47  (7ffccce3cac7) [onecoreuap\windows\dxaml\xcp\dxaml\lib\winrtgeneratedclasses\uielement.g.cpp @ 4804]
    Microsoft_UI_Xaml!winrt::impl::consume_Windows_UI_Xaml_IUIElement<winrt::Microsoft::UI::Xaml::Controls::ItemsRepeater>::UpdateLayout+0x53  (7ffc5427ebd3) [C:\a\_work\1\s\BuildOutput\Intermediates\x64\Microsoft.UI.Xaml\obj\Generated Files\winrt\Windows.UI.Xaml.h @ 3021]
    Microsoft_UI_Xaml!NavigationView::OnMenuItemsSourceCollectionChanged+0x24  (7ffc543041b4) [C:\a\_work\1\s\dev\NavigationView\NavigationView.cpp @ 3251]
    Microsoft_UI_Xaml!winrt::impl::delegate<winrt::Windows::UI::Xaml::SizeChangedEventHandler,<lambda_778e218533f9d63cc7aafcdd2f5bac52> >::Invoke+0x3b  (7ffc542cd5db) [C:\a\_work\1\s\BuildOutput\Intermediates\x64\Microsoft.UI.Xaml\obj\Generated Files\winrt\Windows.Foundation.h @ 894]
    Microsoft_UI_Xaml!event_base<event_source<winrt::Windows::UI::Xaml::Interop::NotifyCollectionChangedEventHandler>,winrt::Windows::UI::Xaml::Interop::NotifyCollectionChangedEventHandler,tracker_ref<winrt::Windows::UI::Xaml::Interop::NotifyCollectionChangedEventHandler,0,IUnknown *> >::operator  (7ffc5421fd87) [C:\a\_work\1\s\dev\inc\event.h @ 77]
    Microsoft_UI_Xaml!InspectingDataSource::OnVectorChanged+0xe4  (7ffc541d4294) [C:\a\_work\1\s\dev\Repeater\InspectingDataSource.cpp @ 260]
    Microsoft_UI_Xaml!winrt::impl::delegate<winrt::Windows::Foundation::TypedEventHandler<winrt::Microsoft::UI::Xaml::Controls::Layout,winrt::Windows::Foundation::IInspectable>,<lambda_5d92c9e3076376e1f0ac0972ab590101> >::Invoke+0x3b  (7ffc5425b9fb) [C:\a\_work\1\s\BuildOutput\Intermediates\x64\Microsoft.UI.Xaml\obj\Generated Files\winrt\Windows.Foundation.h @ 883]
    Microsoft_UI_Xaml!winrt::Windows::Foundation::Collections::VectorChangedEventHandler<winrt::Windows::Foundation::IInspectable>::operator  (7ffc54263f4a) [C:\a\_work\1\s\BuildOutput\Intermediates\x64\Microsoft.UI.Xaml\obj\Generated Files\winrt\Windows.Foundation.Collections.h @ 690]
    Microsoft_UI_Xaml!event_base<event_source<winrt::Windows::Foundation::Collections::VectorChangedEventHandler<winrt::Windows::Foundation::IInspectable> >,winrt::Windows::Foundation::Collections::VectorChangedEventHandler<winrt::Windows::Foundation::IInspectable>,tracker_ref<winrt::Windows::Foundation::Collections::VectorChangedEventHandler<winrt::Windows::Foundation::IInspectable>,0,IUnknown *> >::operator  (7ffc54263eef) [C:\a\_work\1\s\dev\inc\event.h @ 77]
    Microsoft_UI_Xaml!ObservableVectorInnerImpl<VectorOptions<winrt::Windows::Foundation::IInspectable,1,0,1,0>,TStorageWrapperImpl<winrt::Windows::Foundation::IInspectable,0> >::RaiseChildrenChanged+0x118  (7ffc54263df8) [C:\a\_work\1\s\dev\Collections\Vector.h @ 341]
    Microsoft_UI_Xaml!winrt::impl::produce<VectorBase<winrt::Windows::Foundation::IInspectable,1,0,1,0,VectorOptions<winrt::Windows::Foundation::IInspectable,1,0,1,0> >,winrt::Windows::Foundation::Collections::IVector<winrt::Windows::Foundation::IInspectable> >::ReplaceAll+0x4f  (7ffc542f16df) [C:\a\_work\1\s\BuildOutput\Intermediates\x64\Microsoft.UI.Xaml\obj\Generated Files\winrt\Windows.Foundation.Collections.h @ 624]
    Microsoft_Terminal_Settings_Editor!winrt::Microsoft::Terminal::Settings::Editor::implementation::MainPage::UpdateSettings+0x1db 
```

That looks more right. `MainPage::UpdateSettings` -> replaces all the elements of the nav view -> `NavigationView::OnMenuItemsSourceCollectionChanged`

If I need to look at anything from VS 2022's debugger view for you, let me know. Just be a bit more explicit, since I hardly ever use that.
Nah, I think we can repro this pretty readily. This is probably just more fallout from doing weird stuff to our nav view. Doing this a few times seemed like I would get a friggen wild set of items in the nave view, until eventually the app would crash. 
@zadjii-msft Did you need a (mini)dump for this?
Nah, this one's got a consistent repro. Just need someone to take the time to look into it at this point. 
see also:
* #9273
* #10390

This is the SAME THING.

https://github.com/microsoft/terminal/blob/2d66dc44f5abef975bed86af12cb5facb6729a1a/src/cascadia/TerminalSettingsEditor/MainPage.cpp#L152-L157

That `ReplaceAll` call, causes a XAML exception higher in the stack. 

We can't
* `Clear`
* `RemoveAtEnd` in a loop
* Just `RemoveAtEnd` once: nope
* Just `RemoveAt(0)` once: that crashes too.

Hmm.
@chingucoding Okay, I'm way out on a limb here. But I was looking at https://github.com/microsoft/microsoft-ui-xaml/pull/3138, which was the fix for https://github.com/microsoft/microsoft-ui-xaml/issues/2818, and thought you might have an idea here. 

TL;DR: we're calling `IVector<>::ReplaceAll()` on our `NavigationView`'s `MenuItems` to replace the list when the settings reloads (demonstrated above by just deleting your existing settings, forcing us to generate a fresh set). However, when the list decreases in length[1], it seems like the NavView just _crashes_. 

You think if I somehow converted our `std::vector<MUX::Controls::NavigationViewItem>` into something that could be used as a `MenuItemSource`, that I could swap out the entries that way?


[1]: it's not _totally_ clear what triggers the crash, but that seems close enough to the truth. 

## EDIT 

Yes that absolutely works. It's horrifying but it works. 

Fix patch:
diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp
index 493347479cf..b1b2a834933 100644
--- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp
+++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp
@@ -117,6 +117,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
             lastBreadcrumb = _breadcrumbs.GetAt(size - 1);
         }
 
+        // Collect all the values out of the old nav view item source
         auto menuItems{ SettingsNav().MenuItems() };
 
         // We'll remove a bunch of items and iterate over it twice.
@@ -156,7 +157,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
                 }),
             menuItemsSTL.end());
 
-        menuItems.ReplaceAll(menuItemsSTL);
+        // Now, we've got a list of just the static entries again. Lets take
+        // those and stick them back into a new winrt vector, and set that as
+        // the source again.
+        //
+        // By setting MenuItemsSource in its entirety, rather than manipulating
+        // MenuItems, we avoid a crash in WinUI.
+        auto newSource = winrt::single_threaded_vector<IInspectable>(std::move(menuItemsSTL));
+        SettingsNav().MenuItemsSource(newSource);
 
         // Repopulate profile-related menu items
         _InitializeProfilesList();
@@ -209,7 +217,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         // Couldn't find the selected item, fallback to first menu item
         // This happens when the selected item was a profile which doesn't exist in the new configuration
         // We can use menuItemsSTL here because the only things they miss are profile entries.
-        const auto& firstItem{ menuItemsSTL.at(0).as<MUX::Controls::NavigationViewItem>() };
+        const auto& firstItem{ SettingsNav().MenuItems().GetAt(0).as<MUX::Controls::NavigationViewItem>() };
         SettingsNav().SelectedItem(firstItem);
         _Navigate(unbox_value<hstring>(firstItem.Tag()), BreadcrumbSubPage::None);
     }
@@ -527,7 +535,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     void MainPage::_InitializeProfilesList()
     {
-        const auto menuItems = SettingsNav().MenuItems();
+        const auto& itemSource{ SettingsNav().MenuItemsSource() };
+        if (!itemSource)
+        {
+            // There wasn't a MenuItemsSource set yet? The only way that's
+            // possible is if we haven't used
+            // _MoveXamlParsedNavItemsIntoItemSource to move the hardcoded menu
+            // entries from XAML into our runtime menu item source. Do that now.
+
+            _MoveXamlParsedNavItemsIntoItemSource();
+        }
+        const auto menuItems = SettingsNav().MenuItemsSource().try_as<IVector<IInspectable>>();
 
         // Manually create a NavigationViewItem for each profile
         // and keep a reference to them in a map so that we
@@ -557,6 +575,37 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         menuItems.Append(addProfileItem);
     }
 
+    // BODGY
+    // Does the very wacky business of moving all our MenuItems that we
+    // hardcoded in XAML into a runtime MenuItemsSource. We'll then use _that_
+    // MenuItemsSource as the source for our nav view entries instead. This
+    // lets us hardcode the initial entries in precompiled XAML, but then adjust
+    // the items at runtime. Without using a MenuItemsSource, the NavView just
+    // crashes when items are removed (see GH#13673)
+    void MainPage::_MoveXamlParsedNavItemsIntoItemSource()
+    {
+        if (SettingsNav().MenuItemsSource())
+        {
+            // We've already copied over the original items to a source. We can
+            // just skip this now.
+            return;
+        }
+
+        auto menuItems{ SettingsNav().MenuItems() };
+        // Remove all the existing items, and move them to a separate vector
+        // that we'll use as a MenuItemsSource. By doing this, we avoid a WinUI
+        // bug (MUX#6302) where modifying the NavView.Items() directly causes a
+        // crash. By leaving these static entries in XAML, we maintain the
+        // benefit of instantiating them from the XBF, rather than at runtime.
+        //
+        // --> Copy it into an STL vector to simplify our code and reduce COM overhead.
+        std::vector<IInspectable> menuItemsSTL(menuItems.Size(), nullptr);
+        menuItems.GetMany(0, menuItemsSTL);
+
+        auto newSource = winrt::single_threaded_vector<IInspectable>(std::move(menuItemsSTL));
+        SettingsNav().MenuItemsSource(newSource);
+    }
+
     void MainPage::_CreateAndNavigateToNewProfile(const uint32_t index, const Model::Profile& profile)
     {
         const auto newProfile{ profile ? profile : _settingsClone.CreateNewProfile() };
diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.h b/src/cascadia/TerminalSettingsEditor/MainPage.h
index 321331b6a8e..bdcfa5b974d 100644
--- a/src/cascadia/TerminalSettingsEditor/MainPage.h
+++ b/src/cascadia/TerminalSettingsEditor/MainPage.h
@@ -68,6 +68,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         void _Navigate(const Editor::ProfileViewModel& profile, BreadcrumbSubPage subPage);
 
         void _UpdateBackgroundForMica();
+        void _MoveXamlParsedNavItemsIntoItemSource();
 
         winrt::Microsoft::Terminal::Settings::Editor::ColorSchemesPageViewModel _colorSchemesPageVM{ nullptr };
 


Test patch: