Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zadjii-msft wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a8bd503
I believe this is the bugfix
zadjii-msft Jan 31, 2020
648373d
Try writing tests
zadjii-msft Jan 31, 2020
029b676
clean up these tests for PR
zadjii-msft Jan 31, 2020
0a6f72c
Fix the tests o__o
zadjii-msft Jan 31, 2020
9849fe9
Merge remote-tracking branch 'origin/master' into dev/migrie/b/2455-w…
zadjii-msft Jan 31, 2020
963a830
Add the VERIFY_THROWS for dustin
zadjii-msft Jan 31, 2020
8ed39cd
Merge branch 'master' into dev/migrie/b/2455-with-new-tests
zadjii-msft Mar 18, 2020
82ad8bb
I _think_ this will pull the settings from the control, but I haven't…
zadjii-msft Mar 18, 2020
eafe9c8
Wrap Application::Current in try/catch since that breaks tests
zadjii-msft Mar 18, 2020
908fd74
Try to clone the settings directly from the control
zadjii-msft Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ namespace TerminalAppLocalTests

TEST_METHOD(TestTerminalArgsForBinding);

TEST_METHOD(FindMissingProfile);
TEST_METHOD(MakeSettingsForProfileThatDoesntExist);
TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);

TEST_CLASS_SETUP(ClassSetup)
{
InitializeJsonReader();
Expand Down Expand Up @@ -2089,4 +2093,152 @@ 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");
}

try
{
auto terminalSettings = settings->BuildSettings(guid3);
Comment thread
zadjii-msft marked this conversation as resolved.
Outdated
VERIFY_IS_TRUE(false, L"This call to BuildSettings should fail");
}
catch (...)
{
VERIFY_IS_TRUE(true, L"This call to BuildSettings successfully failed");
}

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");
}
}

}
125 changes: 125 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -53,6 +54,8 @@ namespace TerminalAppLocalTests
TEST_METHOD(CreateSimpleTerminalXamlType);
TEST_METHOD(CreateTerminalMuxXamlType);

TEST_METHOD(TryDuplicateBadTab);

TEST_CLASS_SETUP(ClassSetup)
{
InitializeJsonReader();
Expand Down Expand Up @@ -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);
}

}
Loading
⚡