Skip to content

Commit

Permalink
Fix SUI race conditions when reloading settings (#10390)
Browse files Browse the repository at this point in the history 
## 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 ✔️

(cherry picked from commit b034fc9)

# Conflicts:
#	src/cascadia/TerminalApp/AppLogic.cpp
#	src/cascadia/TerminalApp/AppLogic.h
  • Loading branch information
@lhecker @DHowett
lhecker authored and DHowett committed Jul 7, 2021
1 parent 39f3f23 commit dbe688d
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 123 deletions.
85 changes: 32 additions & 53 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ namespace winrt::TerminalApp::implementation
});
_root->Create();

_ApplyTheme(_settings.GlobalSettings().Theme());
_RefreshThemeRoutine();
_ApplyStartupTaskStateChange();

TraceLoggingWrite(
Expand Down Expand Up @@ -895,36 +895,25 @@ 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_await winrt::resume_after(FileActivityQuiesceTime);
_ReloadSettings();
_settingsReloadQueued.store(false);
co_return;
}
}

fire_and_forget AppLogic::_LoadErrorsDialogRoutine()
{
co_await winrt::resume_foreground(_root->Dispatcher());
auto weakSelf = get_weak();

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_after(std::chrono::milliseconds(100));
co_await winrt::resume_foreground(_root->Dispatcher());

_ShowLoadWarningsDialog();
if (auto self{ weakSelf.get() })
{
_ReloadSettings();
_settingsReloadQueued.store(false);
}
}

fire_and_forget AppLogic::_RefreshThemeRoutine()
void AppLogic::_RefreshThemeRoutine()
{
co_await winrt::resume_foreground(_root->Dispatcher());

// Refresh the UI theme
_ApplyTheme(_settings.GlobalSettings().Theme());
}

Expand Down Expand Up @@ -959,39 +948,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)
{
case StartupTaskState::Disabled:
switch (task.State())
{
case StartupTaskState::Disabled:
if (tryEnableStartupTask)
{
if (tryEnableStartupTask)
{
co_await task.RequestEnableAsync();
}
break;
co_await task.RequestEnableAsync();
}
case StartupTaskState::DisabledByUser:
break;
case StartupTaskState::DisabledByUser:
// TODO: GH#6254: define UX for other StartupTaskStates
break;
case StartupTaskState::Enabled:
if (!tryEnableStartupTask)
{
// TODO: GH#6254: define UX for other StartupTaskStates
break;
}
case StartupTaskState::Enabled:
{
if (!tryEnableStartupTask)
{
task.Disable();
}
break;
}
task.Disable();
}
break;
}
}
CATCH_LOG();
Expand All @@ -1009,12 +985,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
Expand Down
6 changes: 1 addition & 5 deletions src/cascadia/TerminalApp/AppLogic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,14 @@ 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();

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);
Expand Down
48 changes: 18 additions & 30 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
// 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();
if (needRefreshUI)
{
_RefreshUIForSettingsReload();
}

// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();
}

void TerminalPage::Create()
Expand Down Expand Up @@ -1830,7 +1825,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.
Expand Down Expand Up @@ -1885,17 +1880,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
Expand Down
6 changes: 2 additions & 4 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -67,8 +67,6 @@ namespace winrt::TerminalApp::implementation
winrt::hstring ApplicationDisplayName();
winrt::hstring ApplicationVersion();

winrt::hstring ThirdPartyNoticesLink();

winrt::fire_and_forget CloseWindow();

void ToggleFocusMode();
Expand Down Expand Up @@ -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();
Expand Down
67 changes: 37 additions & 30 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() };
Expand All @@ -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
Expand All @@ -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>() })
{
Expand All @@ -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;
}
}
}
Expand All @@ -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;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/MainPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit dbe688d

Please sign in to comment.