Don't crash when the user tries to duplicate a tab with a profile that no longer exists

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp

@@ -70,6 +70,10 @@ namespace TerminalAppLocalTests
 
         TEST_METHOD(TestTerminalArgsForBinding);
 
+        TEST_METHOD(FindMissingProfile);
+        TEST_METHOD(MakeSettingsForProfileThatDoesntExist);
+        TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);
+
         TEST_CLASS_SETUP(ClassSetup)
         {
             InitializeJsonReader();
@@ -2089,4 +2093,144 @@ namespace TerminalAppLocalTests
             VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
         }
     }
+    void SettingsTests::FindMissingProfile()
+    {
+        // Test that CascadiaSettings::FindProfile returns null for a GUID that
+        // doesn't exist
+        const std::string settingsString{ R"(
+        {
+            "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name" : "profile0",
+                    "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
+                },
+                {
+                    "name" : "profile1",
+                    "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}"
+                }
+            ]
+        })" };
+        const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
+        auto settings = CascadiaSettings::FromJson(settingsJsonObj);
+
+        const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
+        const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
+        const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");
+
+        const Profile* const profile1 = settings->FindProfile(guid1);
+        const Profile* const profile2 = settings->FindProfile(guid2);
+        const Profile* const profile3 = settings->FindProfile(guid3);
+
+        VERIFY_IS_NOT_NULL(profile1);
+        VERIFY_IS_NOT_NULL(profile2);
+        VERIFY_IS_NULL(profile3);
+
+        VERIFY_ARE_EQUAL(L"profile0", profile1->GetName());
+        VERIFY_ARE_EQUAL(L"profile1", profile2->GetName());
+    }
+
+    void SettingsTests::MakeSettingsForProfileThatDoesntExist()
+    {
+        // Test that MakeSettings throws when the GUID doesn't exist
+        const std::string settingsString{ R"(
+        {
+            "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name" : "profile0",
+                    "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1
+                },
+                {
+                    "name" : "profile1",
+                    "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
+                    "historySize": 2
+                }
+            ]
+        })" };
+        const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
+        auto settings = CascadiaSettings::FromJson(settingsJsonObj);
+
+        const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
+        const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
+        const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");
+
+        try
+        {
+            auto terminalSettings = settings->BuildSettings(guid1);
+            VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
+            VERIFY_ARE_EQUAL(1, terminalSettings.HistorySize());
+        }
+        catch (...)
+        {
+            VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
+        }
+
+        try
+        {
+            auto terminalSettings = settings->BuildSettings(guid2);
+            VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
+            VERIFY_ARE_EQUAL(2, terminalSettings.HistorySize());
+        }
+        catch (...)
+        {
+            VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
+        }
+
+        VERIFY_THROWS(auto terminalSettings = settings->BuildSettings(guid3), wil::ResultException, L"This call to BuildSettings should fail");
+
+        try
+        {
+            const auto [guid, termSettings] = settings->BuildSettings(nullptr);
+            VERIFY_ARE_NOT_EQUAL(nullptr, termSettings);
+            VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
+        }
+        catch (...)
+        {
+            VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
+        }
+    }
+
+    void SettingsTests::MakeSettingsForDefaultProfileThatDoesntExist()
+    {
+        // Test that MakeSettings _doesnt_ throw when we load settings with a
+        // defaultProfile that's not in the list, we validate the settings, and
+        // then call MakeSettings(nullopt). The validation should ensure that
+        // the default profile is something reasonable
+        const std::string settingsString{ R"(
+        {
+            "defaultProfile": "{6239a42c-3333-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name" : "profile0",
+                    "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1
+                },
+                {
+                    "name" : "profile1",
+                    "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
+                    "historySize": 2
+                }
+            ]
+        })" };
+        const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
+        auto settings = CascadiaSettings::FromJson(settingsJsonObj);
+        settings->_ValidateSettings();
+
+        VERIFY_ARE_EQUAL(2u, settings->_warnings.size());
+        VERIFY_ARE_EQUAL(2u, settings->_profiles.size());
+        VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid());
+        try
+        {
+            const auto [guid, termSettings] = settings->BuildSettings(nullptr);
+            VERIFY_ARE_NOT_EQUAL(nullptr, termSettings);
+            VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
+        }
+        catch (...)
+        {
+            VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
+        }
+    }
+
 }

src/cascadia/LocalTests_TerminalApp/TabTests.cpp

@@ -15,6 +15,7 @@ using namespace Microsoft::Console;
 using namespace TerminalApp;
 using namespace WEX::Logging;
 using namespace WEX::TestExecution;
+using namespace WEX::Common;
 
 namespace TerminalAppLocalTests
 {
@@ -53,6 +54,8 @@ namespace TerminalAppLocalTests
         TEST_METHOD(CreateSimpleTerminalXamlType);
         TEST_METHOD(CreateTerminalMuxXamlType);
 
+        TEST_METHOD(TryDuplicateBadTab);
+
         TEST_CLASS_SETUP(ClassSetup)
         {
             InitializeJsonReader();
@@ -164,4 +167,126 @@ namespace TerminalAppLocalTests
         VERIFY_SUCCEEDED(result);
     }
 
+    void TabTests::TryDuplicateBadTab()
+    {
+        // Create a tab with a profile with GUID 1
+        // Reload the settings so that GUID 1 is no longer in the list of profiles
+        // Try calling _DuplicateTabViewItem on tab 1
+        // No new tab should be created (and more importantly, the app should not crash)
+        //
+        // Created to test GH#2455
+
+        const std::string settingsJson0{ R"(
+        {
+            "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name" : "profile0",
+                    "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+                    "historySize": 1
+                },
+                {
+                    "name" : "profile1",
+                    "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
+                    "historySize": 2
+                }
+            ]
+        })" };
+
+        const std::string settingsJson1{ R"(
+        {
+            "defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
+            "profiles": [
+                {
+                    "name" : "profile1",
+                    "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
+                    "historySize": 2
+                }
+            ]
+        })" };
+
+        VerifyParseSucceeded(settingsJson0);
+        auto settings0 = std::make_shared<CascadiaSettings>(false);
+        VERIFY_IS_NOT_NULL(settings0);
+        settings0->_ParseJsonString(settingsJson0, false);
+        settings0->LayerJson(settings0->_userSettings);
+        settings0->_ValidateSettings();
+
+        VerifyParseSucceeded(settingsJson1);
+        auto settings1 = std::make_shared<CascadiaSettings>(false);
+        VERIFY_IS_NOT_NULL(settings1);
+        settings1->_ParseJsonString(settingsJson1, false);
+        settings1->LayerJson(settings1->_userSettings);
+        settings1->_ValidateSettings();
+
+        const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
+        const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
+        const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");
+
+        // This is super wacky, but we can't just initialize the
+        // com_ptr<impl::TerminalPage> in the lambda and assign it back out of
+        // the lambda. We'll crash trying to get a weak_ref to the TerminalPage
+        // during TerminalPage::Create() below.
+        //
+        // Instead, create the winrt object, then get a com_ptr to the
+        // implementation _from_ the winrt object. This seems to work, even if
+        // it's weird.
+        winrt::TerminalApp::TerminalPage projectedPage{ nullptr };
+        winrt::com_ptr<winrt::TerminalApp::implementation::TerminalPage> page{ nullptr };
+
+        Log::Comment(NoThrowString().Format(L"Construct the TerminalPage"));
+        auto result = RunOnUIThread([&projectedPage, &page, settings0]() {
+            projectedPage = winrt::TerminalApp::TerminalPage();
+            page.copy_from(winrt::get_self<winrt::TerminalApp::implementation::TerminalPage>(projectedPage));
+            page->_settings = settings0;
+        });
+        VERIFY_SUCCEEDED(result);
+
+        VERIFY_IS_NOT_NULL(page);
+        VERIFY_IS_NOT_NULL(page->_settings);
+
+        Log::Comment(NoThrowString().Format(L"Create() the TerminalPage"));
+        result = RunOnUIThread([&page]() {
+            VERIFY_IS_NOT_NULL(page);
+            VERIFY_IS_NOT_NULL(page->_settings);
+            page->Create();
+
+            // I think in the tests, we don't always set the focused tab on
+            // creation. Doesn't seem to be a problem in the real app, but
+            // probably indicative of a problem.
+            //
+            // Manually set it here, so that later, the _GetFocusedTabIndex call
+            // in _DuplicateTabViewItem will have a sensible value.
+            page->_SetFocusedTabIndex(0);
+        });
+        VERIFY_SUCCEEDED(result);
+
+        result = RunOnUIThread([&page]() {
+            VERIFY_ARE_EQUAL(1u, page->_tabs.size());
+        });
+        VERIFY_SUCCEEDED(result);
+
+        Log::Comment(NoThrowString().Format(L"Duplicate the first tab"));
+        result = RunOnUIThread([&page]() {
+            page->_DuplicateTabViewItem();
+            VERIFY_ARE_EQUAL(2u, page->_tabs.size());
+        });
+        VERIFY_SUCCEEDED(result);
+
+        Log::Comment(NoThrowString().Format(
+            L"Change the settings of the TerminalPage so the first profile is "
+            L"no longer in the list of profiles"));
+        result = RunOnUIThread([&page, settings1]() {
+            page->_settings = settings1;
+        });
+        VERIFY_SUCCEEDED(result);
+
+        Log::Comment(NoThrowString().Format(L"Duplicate the tab, and don't crash"));
+        result = RunOnUIThread([&page]() {
+            page->_DuplicateTabViewItem();
+            VERIFY_ARE_EQUAL(2u, page->_tabs.size(), L"We should gracefully do nothing here - the profile no longer exists.");
+        });
+        VERIFY_SUCCEEDED(result);
+    }
+
 }