starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-04-10 16:21:06 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Fix SUI race conditions when reloading settings (#10390)

Browse Source 
## Summary of the Pull Request

This commit fixes various race conditions regarding the settings UI. It's unsafe to write to class members from background threads without acquiring mutexes or yielding to the main thread first.
By changing the settings reload code path to yield to the main thread early, we're able to cut down on code complexity and unsafe member accesses.

## PR Checklist
* [x] Closes #9273
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Settings UI reloads without crashing ✔️
This commit is contained in:
Leonard Hecker
2021-06-11 02:38:10 +02:00
committed by GitHub
parent 5bb8148ef9
commit b034fc9ae5
6 changed files with 94 additions and 126 deletions
Download Patch File Download Diff File Expand all files Collapse all files

91
src/cascadia/TerminalApp/AppLogic.cpp
View File

@@ -307,7 +307,7 @@ namespace winrt::TerminalApp::implementation
_root->Create();
_ApplyLanguageSettingChange();
_ApplyTheme(_settings.GlobalSettings().Theme());
_RefreshThemeRoutine();
_ApplyStartupTaskStateChange();
TraceLoggingWrite(
@@ -896,10 +896,18 @@ namespace winrt::TerminalApp::implementation
// this stops us from reloading too many times or too quickly.
fire_and_forget AppLogic::_DispatchReloadSettings()
{
static constexpr auto FileActivityQuiesceTime{ std::chrono::milliseconds(50) };
if (!_settingsReloadQueued.exchange(true))
if (_settingsReloadQueued.exchange(true))
{
co_return;
}
auto weakSelf = get_weak();
co_await winrt::resume_after(std::chrono::milliseconds(100));
co_await winrt::resume_foreground(_root->Dispatcher());
if (auto self{ weakSelf.get() })
{
co_await winrt::resume_after(FileActivityQuiesceTime);
_ReloadSettings();
_settingsReloadQueued.store(false);
}
@@ -918,27 +926,8 @@ namespace winrt::TerminalApp::implementation
}
}
fire_and_forget AppLogic::_LoadErrorsDialogRoutine()
void AppLogic::_RefreshThemeRoutine()
{
co_await winrt::resume_foreground(_root->Dispatcher());
const winrt::hstring titleKey = USES_RESOURCE(L"ReloadJsonParseErrorTitle");
const winrt::hstring textKey = USES_RESOURCE(L"ReloadJsonParseErrorText");
_ShowLoadErrorsDialog(titleKey, textKey, _settingsLoadedResult);
}
fire_and_forget AppLogic::_ShowLoadWarningsDialogRoutine()
{
co_await winrt::resume_foreground(_root->Dispatcher());
_ShowLoadWarningsDialog();
}
fire_and_forget AppLogic::_RefreshThemeRoutine()
{
co_await winrt::resume_foreground(_root->Dispatcher());
// Refresh the UI theme
_ApplyTheme(_settings.GlobalSettings().Theme());
}
@@ -973,39 +962,26 @@ namespace winrt::TerminalApp::implementation
return;
}
auto weakThis{ get_weak() };
co_await winrt::resume_foreground(_root->Dispatcher(), CoreDispatcherPriority::Normal);
if (auto page{ weakThis.get() })
{
StartupTaskState state;
bool tryEnableStartupTask = _settings.GlobalSettings().StartOnUserLogin();
StartupTask task = co_await StartupTask::GetAsync(StartupTaskName);
const auto tryEnableStartupTask = _settings.GlobalSettings().StartOnUserLogin();
const auto task = co_await StartupTask::GetAsync(StartupTaskName);
state = task.State();
switch (state)
switch (task.State())
{
case StartupTaskState::Disabled:
if (tryEnableStartupTask)
{
case StartupTaskState::Disabled:
co_await task.RequestEnableAsync();
}
break;
case StartupTaskState::DisabledByUser:
// TODO: GH#6254: define UX for other StartupTaskStates
break;
case StartupTaskState::Enabled:
if (!tryEnableStartupTask)
{
if (tryEnableStartupTask)
{
co_await task.RequestEnableAsync();
}
break;
}
case StartupTaskState::DisabledByUser:
{
// TODO: GH#6254: define UX for other StartupTaskStates
break;
}
case StartupTaskState::Enabled:
{
if (!tryEnableStartupTask)
{
task.Disable();
}
break;
}
task.Disable();
}
break;
}
}
CATCH_LOG();
@@ -1023,12 +999,15 @@ namespace winrt::TerminalApp::implementation
if (FAILED(_settingsLoadedResult))
{
_LoadErrorsDialogRoutine();
const winrt::hstring titleKey = USES_RESOURCE(L"ReloadJsonParseErrorTitle");
const winrt::hstring textKey = USES_RESOURCE(L"ReloadJsonParseErrorText");
_ShowLoadErrorsDialog(titleKey, textKey, _settingsLoadedResult);
return;
}
else if (_settingsLoadedResult == S_FALSE)
if (_settingsLoadedResult == S_FALSE)
{
_ShowLoadWarningsDialogRoutine();
_ShowLoadWarningsDialog();
}
// Here, we successfully reloaded the settings, and created a new

6
src/cascadia/TerminalApp/AppLogic.h
View File

@@ -124,19 +124,15 @@ namespace winrt::TerminalApp::implementation
::TerminalApp::AppCommandlineArgs _appArgs;
::TerminalApp::AppCommandlineArgs _settingsAppArgs;
int _ParseArgs(winrt::array_view<const hstring>& args);
static TerminalApp::FindTargetWindowResult _doFindTargetWindow(winrt::array_view<const hstring> args,
const Microsoft::Terminal::Settings::Model::WindowingMode& windowingBehavior);
void _ShowLoadErrorsDialog(const winrt::hstring& titleKey, const winrt::hstring& contentKey, HRESULT settingsLoadedResult);
void _ShowLoadWarningsDialog();
bool _IsKeyboardServiceEnabled();
void _ShowKeyboardServiceDisabledDialog();
void _ApplyLanguageSettingChange();
fire_and_forget _LoadErrorsDialogRoutine();
fire_and_forget _ShowLoadWarningsDialogRoutine();
fire_and_forget _RefreshThemeRoutine();
void _RefreshThemeRoutine();
fire_and_forget _ApplyStartupTaskStateChange();
void _OnLoaded(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);

48
src/cascadia/TerminalApp/TerminalPage.cpp
View File

@@ -92,31 +92,26 @@ namespace winrt::TerminalApp::implementation
}
}
winrt::fire_and_forget TerminalPage::SetSettings(CascadiaSettings settings, bool needRefreshUI)
void TerminalPage::SetSettings(CascadiaSettings settings, bool needRefreshUI)
{
_settings = settings;
auto weakThis{ get_weak() };
co_await winrt::resume_foreground(Dispatcher());
if (auto page{ weakThis.get() })
// Make sure to _UpdateCommandsForPalette before
// _RefreshUIForSettingsReload. _UpdateCommandsForPalette will make
// sure the KeyChordText of Commands is updated, which needs to
// happen before the Settings UI is reloaded and tries to re-read
// those values.
_UpdateCommandsForPalette();
CommandPalette().SetActionMap(_settings.ActionMap());
if (needRefreshUI)
{
// Make sure to _UpdateCommandsForPalette before
// _RefreshUIForSettingsReload. _UpdateCommandsForPalette will make
// sure the KeyChordText of Commands is updated, which needs to
// happen before the Settings UI is reloaded and tries to re-read
// those values.
_UpdateCommandsForPalette();
CommandPalette().SetActionMap(_settings.ActionMap());
if (needRefreshUI)
{
_RefreshUIForSettingsReload();
}
// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();
_RefreshUIForSettingsReload();
}
// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();
}
void TerminalPage::Create()
@@ -1833,7 +1828,7 @@ namespace winrt::TerminalApp::implementation
// This includes update the settings of all the tabs according
// to their profiles, update the title and icon of each tab, and
// finally create the tab flyout
winrt::fire_and_forget TerminalPage::_RefreshUIForSettingsReload()
void TerminalPage::_RefreshUIForSettingsReload()
{
// Re-wire the keybindings to their handlers, as we'll have created a
// new AppKeyBindings object.
@@ -1888,17 +1883,10 @@ namespace winrt::TerminalApp::implementation
tabImpl->SetActionMap(_settings.ActionMap());
}
auto weakThis{ get_weak() };
co_await winrt::resume_foreground(Dispatcher());
// repopulate the new tab button's flyout with entries for each
// profile, which might have changed
if (auto page{ weakThis.get() })
{
_UpdateTabWidthMode();
_CreateNewTabFlyout();
}
_UpdateTabWidthMode();
_CreateNewTabFlyout();
// Reload the current value of alwaysOnTop from the settings file. This
// will let the user hot-reload this setting, but any runtime changes to

6
src/cascadia/TerminalApp/TerminalPage.h
View File

@@ -54,7 +54,7 @@ namespace winrt::TerminalApp::implementation
// put it in our inheritance graph. https://github.com/microsoft/microsoft-ui-xaml/issues/3331
STDMETHODIMP Initialize(HWND hwnd);
winrt::fire_and_forget SetSettings(Microsoft::Terminal::Settings::Model::CascadiaSettings settings, bool needRefreshUI);
void SetSettings(Microsoft::Terminal::Settings::Model::CascadiaSettings settings, bool needRefreshUI);
void Create();
@@ -67,8 +67,6 @@ namespace winrt::TerminalApp::implementation
winrt::hstring ApplicationDisplayName();
winrt::hstring ApplicationVersion();
winrt::hstring ThirdPartyNoticesLink();
winrt::fire_and_forget CloseWindow();
void ToggleFocusMode();
@@ -296,7 +294,7 @@ namespace winrt::TerminalApp::implementation
winrt::Microsoft::Terminal::Control::TermControl _InitControl(const winrt::Microsoft::Terminal::Settings::Model::TerminalSettingsCreateResult& settings,
const winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection& connection);
winrt::fire_and_forget _RefreshUIForSettingsReload();
void _RefreshUIForSettingsReload();
void _SetNonClientAreaColors(const Windows::UI::Color& selectedTabColor);
void _ClearNonClientAreaColors();

67
src/cascadia/TerminalSettingsEditor/MainPage.cpp
View File

@@ -65,13 +65,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// - settings - the new settings source
// Return value:
// - <none>
fire_and_forget MainPage::UpdateSettings(Model::CascadiaSettings settings)
void MainPage::UpdateSettings(const Model::CascadiaSettings& settings)
{
_settingsSource = settings;
_settingsClone = settings.Copy();
co_await winrt::resume_foreground(Dispatcher());
// Deduce information about the currently selected item
IInspectable selectedItemTag;
auto menuItems{ SettingsNav().MenuItems() };
@@ -83,34 +81,43 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
}
// remove all profile-related NavViewItems by populating a std::vector
// with the ones we want to keep.
// NOTE: menuItems.Remove() causes an out-of-bounds crash. Using ReplaceAll()
// gets around this crash.
std::vector<IInspectable> menuItemsSTL;
for (const auto& item : menuItems)
{
if (const auto& navViewItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
if (const auto& tag{ navViewItem.Tag() })
{
if (tag.try_as<Editor::ProfileViewModel>())
// We'll remove a bunch of items and iterate over it twice.
// --> 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);
// We want to refresh the list of profiles in the NavigationView.
// In order to add profiles we can use _InitializeProfilesList();
// But before we can do that we have to remove existing profiles first of course.
// This "erase-remove" idiom will achieve just that.
menuItemsSTL.erase(
std::remove_if(
menuItemsSTL.begin(),
menuItemsSTL.end(),
[](const auto& item) -> bool {
if (const auto& navViewItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
// don't add NavViewItem pointing to a Profile
continue;
}
else if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == addProfileTag)
if (const auto& tag{ navViewItem.Tag() })
{
// don't add the "Add Profile" item
continue;
if (tag.try_as<Editor::ProfileViewModel>())
{
// remove NavViewItem pointing to a Profile
return true;
}
if (const auto& stringTag{ tag.try_as<hstring>() })
{
if (stringTag == addProfileTag)
{
// remove the "Add Profile" item
return true;
}
}
}
}
}
}
menuItemsSTL.emplace_back(item);
}
return false;
}),
menuItemsSTL.end());
menuItems.ReplaceAll(menuItemsSTL);
// Repopulate profile-related menu items
@@ -123,7 +130,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// refresh the current page using the SelectedItem data we collected before the refresh
if (selectedItemTag)
{
for (const auto& item : menuItems)
for (const auto& item : menuItemsSTL)
{
if (const auto& menuItem{ item.try_as<MUX::Controls::NavigationViewItem>() })
{
@@ -138,7 +145,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// found the one that was selected before the refresh
SettingsNav().SelectedItem(item);
_Navigate(*stringTag);
co_return;
return;
}
}
}
@@ -151,7 +158,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// found the one that was selected before the refresh
SettingsNav().SelectedItem(item);
_Navigate(*profileTag);
co_return;
return;
}
}
}

2
src/cascadia/TerminalSettingsEditor/MainPage.h
View File

@@ -13,7 +13,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
MainPage() = delete;
MainPage(const Model::CascadiaSettings& settings);
fire_and_forget UpdateSettings(Model::CascadiaSettings settings);
void UpdateSettings(const Model::CascadiaSettings& settings);
void OpenJsonKeyDown(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::KeyRoutedEventArgs const& args);
void OpenJsonTapped(Windows::Foundation::IInspectable const& sender, Windows::UI::Xaml::Input::TappedRoutedEventArgs const& args);