diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index 4efbb57a2..350426f92 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -886,14 +886,19 @@ namespace winrt::TerminalApp::implementation // actual file you wrote. So listen for that too. wil::FolderChangeEvents::FileName | wil::FolderChangeEvents::LastWriteTime, [this, settingsBasename = settingsPath.filename()](wil::FolderChangeEvent, PCWSTR fileModified) { - static const auto appState{ ApplicationState::SharedInstance() }; + // DO NOT create a static reference to + // ApplicationState::SharedInstance here. See + // https://github.com/microsoft/terminal/pull/11222/files/9ff2775122a496fb8b1bcc7a0b83a64ce5b26c5f#r719627541 + // for why. ApplicationState::SharedInstance already caches it's + // own static ref. + const winrt::hstring modifiedBasename{ std::filesystem::path{ fileModified }.filename().c_str() }; if (modifiedBasename == settingsBasename) { _reloadSettings->Run(); } - else if (appState.IsStatePath(modifiedBasename)) + else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename)) { _reloadState(); } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp index 1874dd24a..cd0485280 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.cpp +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.cpp @@ -13,7 +13,6 @@ static constexpr std::wstring_view stateFileName{ L"state.json" }; static constexpr std::wstring_view elevatedStateFileName{ L"elevated-state.json" }; -static constexpr std::wstring_view unelevatedStateFileName{ L"user-state.json" }; static constexpr std::string_view TabLayoutKey{ "tabLayout" }; static constexpr std::string_view InitialPositionKey{ "initialPosition" }; @@ -91,7 +90,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ApplicationState::ApplicationState(const std::filesystem::path& stateRoot) noexcept : _sharedPath{ stateRoot / stateFileName }, - _userPath{ stateRoot / unelevatedStateFileName }, _elevatedPath{ stateRoot / elevatedStateFileName }, _throttler{ std::chrono::seconds(1), [this]() { _write(); } } { @@ -101,9 +99,21 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // The destructor ensures that the last write is flushed to disk before returning. ApplicationState::~ApplicationState() { + TraceLoggingWrite(g_hSettingsModelProvider, + "ApplicationState_Dtor_Start", + TraceLoggingDescription("Event at the start of the ApplicationState destructor"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + // This will ensure that we not just cancel the last outstanding timer, // but instead force it to run as soon as possible and wait for it to complete. _throttler.flush(); + + TraceLoggingWrite(g_hSettingsModelProvider, + "ApplicationState_Dtor_End", + TraceLoggingDescription("Event at the end of the ApplicationState destructor"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); } // Re-read the state.json from disk. @@ -116,8 +126,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static const auto sharedPath{ _sharedPath.filename() }; static const auto elevatedPath{ _elevatedPath.filename() }; - static const auto userPath{ _userPath.filename() }; - return filename == sharedPath || filename == elevatedPath || filename == userPath; + return filename == sharedPath || filename == elevatedPath; } // Deserializes the state.json and user-state (or elevated-state if @@ -130,7 +139,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::string errs; std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; - // First get shared state out of `state.json` into us + // First get shared state out of `state.json`. const auto sharedData = _readSharedContents().value_or(std::string{}); if (!sharedData.empty()) { @@ -140,19 +149,31 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); } - FromJson(root, FileSource::Shared); - } - - // Then, try and get anything in user-state/elevated-state - if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) - { - Json::Value root; - if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) + // - If we're elevated, we want to only load the Shared properties + // from state.json. We'll then load the Local props from + // `elevated-state.json` + // - If we're unelevated, then load _everything_ from state.json. + if (::Microsoft::Console::Utils::IsElevated()) { - throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); - } + // Only load shared properties if we're elevated + FromJson(root, FileSource::Shared); - FromJson(root, FileSource::Local); + // Then, try and get anything in elevated-state + if (const auto localData{ _readLocalContents().value_or(std::string{}) }; !localData.empty()) + { + Json::Value root; + if (!reader->parse(localData.data(), localData.data() + localData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + FromJson(root, FileSource::Local); + } + } + else + { + // If we're unelevated, then load everything. + FromJson(root, FileSource::Shared | FileSource::Local); + } } } CATCH_LOG() @@ -166,8 +187,49 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { Json::StreamWriterBuilder wbuilder; - _writeSharedContents(Json::writeString(wbuilder, ToJson(FileSource::Shared))); - _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + // When we're elevated, we've got to be tricky. We don't want to write + // our window state, allowed commandlines, and other Local properties + // into the shared `state.json`. But, if we only serialize the Shared + // properties to a json blob, then we'll omit windowState entirely, + // _removing_ the window state of the unelevated instance. Oh no! + // + // So, to be tricky, we'll first _load_ the shared state to a json blob. + // We'll then serialize our view of the shared properties on top of that + // blob. Then we'll write that blob back to the file. This will + // round-trip the Local properties for the unelevated instances + // untouched in state.json + // + // After that's done, we'll write our Local properties into + // elevated-state.json. + if (::Microsoft::Console::Utils::IsElevated()) + { + std::string errs; + std::unique_ptr reader{ Json::CharReaderBuilder::CharReaderBuilder().newCharReader() }; + Json::Value root; + + // First load the contents of state.json into a json blob. This will + // contain the Shared properties and the unelevated instance's Local + // properties. + const auto sharedData = _readSharedContents().value_or(std::string{}); + if (!sharedData.empty()) + { + if (!reader->parse(sharedData.data(), sharedData.data() + sharedData.size(), &root, &errs)) + { + throw winrt::hresult_error(WEB_E_INVALID_JSON_STRING, winrt::to_hstring(errs)); + } + } + // Layer our shared properties on top of the blob from state.json, + // and write it back out. + _writeSharedContents(Json::writeString(wbuilder, _toJsonWithBlob(root, FileSource::Shared))); + + // Finally, write our Local properties back to elevated-state.json + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local))); + } + else + { + // We're unelevated, this is easy. Just write everything back out. + _writeLocalContents(Json::writeString(wbuilder, ToJson(FileSource::Local | FileSource::Shared))); + } } CATCH_LOG() @@ -192,8 +254,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // * return std::optional by value // At the time of writing the former version skips missing fields in the json, // but we want to explicitly clear state fields that were removed from state.json. + // + // GH#11222: We only load properties that are of the same type (Local or + // Shared) which we requested. If we didn't want to load this type of + // property, just skip it. #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ - if (parseSource == source) \ + if (WI_IsFlagSet(parseSource, source)) \ state->name = JsonUtils::GetValueForKey>(root, key); MTSM_APPLICATION_STATE_FIELDS(MTSM_APPLICATION_STATE_GEN) @@ -203,9 +269,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ApplicationState::ToJson(FileSource parseSource) const noexcept { Json::Value root{ Json::objectValue }; + return _toJsonWithBlob(root, parseSource); + } + Json::Value ApplicationState::_toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept + { { auto state = _state.lock_shared(); + + // GH#11222: We only write properties that are of the same type (Local + // or Shared) which we requested. If we didn't want to serialize this + // type of property, just skip it. #define MTSM_APPLICATION_STATE_GEN(source, type, name, key, ...) \ if (WI_IsFlagSet(parseSource, source)) \ JsonUtils::SetValueForKey(root, key, state->name); @@ -257,7 +331,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { return ::Microsoft::Console::Utils::IsElevated() ? ReadUTF8FileIfExists(_elevatedPath, true) : - ReadUTF8FileIfExists(_userPath, false); + ReadUTF8FileIfExists(_sharedPath, false); } // Method Description: @@ -291,7 +365,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } else { - WriteUTF8FileAtomic(_userPath, content); + WriteUTF8FileAtomic(_sharedPath, content); } } diff --git a/src/cascadia/TerminalSettingsModel/ApplicationState.h b/src/cascadia/TerminalSettingsModel/ApplicationState.h index ed6db33fe..80bde4cf1 100644 --- a/src/cascadia/TerminalSettingsModel/ApplicationState.h +++ b/src/cascadia/TerminalSettingsModel/ApplicationState.h @@ -91,6 +91,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _write() const noexcept; void _read() const noexcept; + Json::Value _toJsonWithBlob(Json::Value& root, FileSource parseSource) const noexcept; + std::optional _readSharedContents() const; void _writeSharedContents(const std::string_view content) const; std::optional _readLocalContents() const; diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index c2b8dcda2..4111ef66e 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -819,8 +819,30 @@ winrt::Windows::Foundation::IAsyncAction AppHost::_SaveWindowLayouts() if (_logic.ShouldUsePersistedLayout()) { - const auto layoutJsons = _windowManager.GetAllWindowLayouts(); - _logic.SaveWindowLayoutJsons(layoutJsons); + try + { + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_SaveWindowLayouts_Collect", + TraceLoggingDescription("Logged when collecting window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + const auto layoutJsons = _windowManager.GetAllWindowLayouts(); + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_SaveWindowLayouts_Save", + TraceLoggingDescription("Logged when writing window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + _logic.SaveWindowLayoutJsons(layoutJsons); + } + catch (...) + { + LOG_CAUGHT_EXCEPTION(); + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_SaveWindowLayouts_Failed", + TraceLoggingDescription("An error occured when collecting or writing window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + } } co_return; @@ -841,6 +863,12 @@ winrt::fire_and_forget AppHost::_SaveWindowLayoutsRepeat() // per 10 seconds, if a save is requested by another source simultaneously. if (_getWindowLayoutThrottler.has_value()) { + TraceLoggingWrite(g_hWindowsTerminalProvider, + "AppHost_requestGetLayout", + TraceLoggingDescription("Logged when triggering a throttled write of the window state"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + _getWindowLayoutThrottler.value()(); } }