starred/terminal-microsoft-1
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 01:04:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix WindowRoot memory leak in SUI (#19826)

Browse Source 
## Summary of the Pull Request
Fixes a memory leak for `IHostedInWindow` in the TerminalSettingsEditor.

The memory leak occurs from `MainPage` creating an object that stores
reference to back to `MainPage` as `IHostedInWindow`. With `IconPicker`
specifically, the cycle would look like `MainPage` --> `NewTabMenu` -->
`IconPicker` --> `MainPage`.

I've audited uses of `IHostedInWindow` in the TerminalSettingsEditor and
replaced them as weak references. I also checked areas that
`WindowRoot()` was called and added a null-check for safety.

## Validation Steps Performed
Verified `MainPage` and `NewTabMenu` dtors are called when the settings
UI closes from...
✅ Launch page (base case - it doesn't have an `IHostedInWindow`)
✅ New Tab Menu page
This commit is contained in:
Carlos Zamora
2026-02-03 14:25:25 -08:00
committed by GitHub
parent 89d82638ab
commit 27aae1f245
17 changed files with 76 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
src/cascadia/TerminalSettingsEditor/ActionsViewModel.cpp
View File

@@ -502,7 +502,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
auto lifetime = get_strong();
static constexpr winrt::guid clientGuidFiles{ 0xbd00ae34, 0x839b, 0x43f6, { 0x8b, 0x94, 0x12, 0x37, 0x1a, 0xfe, 0xea, 0xb5 } };
const auto parentHwnd{ reinterpret_cast<HWND>(_WindowRoot.GetHostingWindow()) };
const auto windowRoot = WindowRoot();
if (!windowRoot)
{
co_return;
}
const auto parentHwnd{ reinterpret_cast<HWND>(windowRoot.GetHostingWindow()) };
auto path = co_await OpenFilePicker(parentHwnd, [](auto&& dialog) {
THROW_IF_FAILED(dialog->SetClientGuid(clientGuidFiles));
try
@@ -525,8 +531,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
auto lifetime = get_strong();
static constexpr winrt::guid clientGuidFolders{ 0xa611027, 0x42be, 0x4665, { 0xaf, 0xf1, 0x3f, 0x22, 0x26, 0xe9, 0xf7, 0x4d } };
;
const auto parentHwnd{ reinterpret_cast<HWND>(_WindowRoot.GetHostingWindow()) };
const auto windowRoot = WindowRoot();
if (!windowRoot)
{
co_return;
}
const auto parentHwnd{ reinterpret_cast<HWND>(windowRoot.GetHostingWindow()) };
auto path = co_await OpenFilePicker(parentHwnd, [](auto&& dialog) {
THROW_IF_FAILED(dialog->SetClientGuid(clientGuidFolders));
try

10
src/cascadia/TerminalSettingsEditor/ActionsViewModel.h
View File

@@ -53,13 +53,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
public:
NavigateToCommandArgs(CommandViewModel command, Editor::IHostedInWindow windowRoot) :
_Command(command),
_WindowRoot(windowRoot) {}
_WeakWindowRoot(windowRoot) {}
Editor::IHostedInWindow WindowRoot() const noexcept { return _WindowRoot; }
Editor::IHostedInWindow WindowRoot() const noexcept { return _WeakWindowRoot.get(); }
Editor::CommandViewModel Command() const noexcept { return _Command; }
private:
Editor::IHostedInWindow _WindowRoot;
winrt::weak_ref<Editor::IHostedInWindow> _WeakWindowRoot;
Editor::CommandViewModel _Command{ nullptr };
};
@@ -141,6 +141,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
winrt::hstring Type() const noexcept { return _descriptor.Type; };
Model::ArgTypeHint TypeHint() const noexcept { return _descriptor.TypeHint; };
bool Required() const noexcept { return _descriptor.Required; };
Editor::IHostedInWindow WindowRoot() const noexcept { return _WeakWindowRoot.get(); }
void WindowRoot(const Editor::IHostedInWindow& value) { _WeakWindowRoot = value; }
// We cannot use the macro here because we need to implement additional logic for the setter
Windows::Foundation::IInspectable EnumValue() const noexcept { return _EnumValue; };
@@ -186,10 +188,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
VIEW_MODEL_OBSERVABLE_PROPERTY(Editor::ColorSchemeViewModel, DefaultColorScheme, nullptr);
VIEW_MODEL_OBSERVABLE_PROPERTY(Windows::Foundation::IInspectable, Value, nullptr);
WINRT_PROPERTY(Windows::Foundation::Collections::IVector<winrt::hstring>, ColorSchemeNamesList, nullptr);
WINRT_PROPERTY(Editor::IHostedInWindow, WindowRoot, nullptr);
private:
Model::ArgDescriptor _descriptor;
winrt::weak_ref<Editor::IHostedInWindow> _WeakWindowRoot{ nullptr };
Windows::Foundation::IInspectable _EnumValue{ nullptr };
Windows::Foundation::Collections::IObservableVector<Microsoft::Terminal::Settings::Editor::EnumEntry> _EnumList;
Windows::Foundation::Collections::IObservableVector<Microsoft::Terminal::Settings::Editor::FlagEntry> _FlagList;

11
src/cascadia/TerminalSettingsEditor/Appearances.cpp
View File

@@ -1423,10 +1423,15 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
safe_void_coroutine Appearances::BackgroundImage_Click(const IInspectable&, const RoutedEventArgs&)
{
auto lifetime = get_strong();
const auto lifetime = get_strong();
const auto parentHwnd{ reinterpret_cast<HWND>(WindowRoot().GetHostingWindow()) };
auto file = co_await OpenImagePicker(parentHwnd);
const auto windowRoot = WindowRoot();
if (!windowRoot)
{
co_return;
}
const auto parentHwnd{ reinterpret_cast<HWND>(windowRoot.GetHostingWindow()) };
const auto file = co_await OpenImagePicker(parentHwnd);
if (!file.empty())
{
Appearance().SetBackgroundImagePath(file);

5
src/cascadia/TerminalSettingsEditor/Appearances.h
View File

@@ -192,6 +192,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// CursorShape visibility logic
bool IsVintageCursor() const;
Editor::IHostedInWindow WindowRoot() const noexcept { return _WeakWindowRoot.get(); }
void WindowRoot(const Editor::IHostedInWindow& value) noexcept { _WeakWindowRoot = value; }
Windows::Foundation::Collections::IObservableVector<Editor::Font> FilteredFontList();
bool ShowAllFonts() const noexcept;
void ShowAllFonts(bool value);
@@ -218,12 +221,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
DEPENDENCY_PROPERTY(Editor::AppearanceViewModel, Appearance);
WINRT_PROPERTY(Editor::ProfileViewModel, SourceProfile, nullptr);
WINRT_PROPERTY(IHostedInWindow, WindowRoot, nullptr);
GETSET_BINDABLE_ENUM_SETTING(BackgroundImageStretchMode, Windows::UI::Xaml::Media::Stretch, Appearance().BackgroundImageStretchMode);
GETSET_BINDABLE_ENUM_SETTING(IntenseTextStyle, Microsoft::Terminal::Settings::Model::IntenseStyle, Appearance().IntenseTextStyle);
private:
winrt::weak_ref<Editor::IHostedInWindow> _WeakWindowRoot{ nullptr };
Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _ViewModelChangedRevoker;
std::array<Windows::UI::Xaml::Controls::Primitives::ToggleButton, 9> _BIAlignmentButtons;
Windows::Foundation::Collections::IMap<uint16_t, Microsoft::Terminal::Settings::Editor::EnumEntry> _FontWeightMap;

10
src/cascadia/TerminalSettingsEditor/IconPicker.cpp
View File

@@ -33,7 +33,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Windows::Foundation::Collections::IObservableVector<Editor::EnumEntry> IconPicker::_BuiltInIcons{ nullptr };
Windows::Foundation::Collections::IObservableVector<Editor::EnumEntry> IconPicker::_IconTypes{ nullptr };
DependencyProperty IconPicker::_CurrentIconPathProperty{ nullptr };
DependencyProperty IconPicker::_WindowRootProperty{ nullptr };
Windows::Foundation::Collections::IObservableVector<Editor::EnumEntry> IconPicker::BuiltInIcons() noexcept
{
@@ -107,15 +106,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
xaml_typename<Editor::IconPicker>(),
PropertyMetadata{ nullptr, PropertyChangedCallback{ &IconPicker::_OnCurrentIconPathChanged } });
}
if (!_WindowRootProperty)
{
_WindowRootProperty =
DependencyProperty::Register(
L"WindowRoot",
xaml_typename<IHostedInWindow>(),
xaml_typename<Editor::IconPicker>(),
PropertyMetadata{ nullptr });
}
}
void IconPicker::_OnCurrentIconPathChanged(const DependencyObject& d, const DependencyPropertyChangedEventArgs& /*e*/)

5
src/cascadia/TerminalSettingsEditor/IconPicker.h
View File

@@ -29,6 +29,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Windows::Foundation::IInspectable CurrentIconType() const noexcept { return _currentIconType; }
void CurrentIconType(const Windows::Foundation::IInspectable& value);
Editor::IHostedInWindow WindowRoot() const noexcept { return _weakWindowRoot.get(); }
void WindowRoot(const Editor::IHostedInWindow& value) noexcept { _weakWindowRoot = value; }
bool UsingNoIcon() const;
bool UsingBuiltInIcon() const;
bool UsingEmojiIcon() const;
@@ -39,7 +42,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
WINRT_OBSERVABLE_PROPERTY(Editor::EnumEntry, CurrentBuiltInIcon, PropertyChanged.raise, nullptr);
DEPENDENCY_PROPERTY(hstring, CurrentIconPath);
DEPENDENCY_PROPERTY(IHostedInWindow, WindowRoot);
private:
static Windows::Foundation::Collections::IObservableVector<Editor::EnumEntry> _BuiltInIcons;
@@ -48,6 +50,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
std::wstring _iconFilter;
Windows::Foundation::IInspectable _currentIconType{};
winrt::hstring _lastIconPath;
winrt::weak_ref<Editor::IHostedInWindow> _weakWindowRoot;
static void _InitializeProperties();
static void _OnCurrentIconPathChanged(const Windows::UI::Xaml::DependencyObject& d, const Windows::UI::Xaml::DependencyPropertyChangedEventArgs& e);

1
src/cascadia/TerminalSettingsEditor/IconPicker.idl
View File

@@ -37,6 +37,5 @@ namespace Microsoft.Terminal.Settings.Editor
static Windows.UI.Xaml.DependencyProperty CurrentIconPathProperty { get; };
IHostedInWindow WindowRoot;
static Windows.UI.Xaml.DependencyProperty WindowRootProperty { get; };
}
}

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

@@ -29,13 +29,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
public:
NavigateToPageArgs(Windows::Foundation::IInspectable viewModel, Editor::IHostedInWindow windowRoot) :
_ViewModel(viewModel),
_WindowRoot(windowRoot) {}
_WeakWindowRoot(windowRoot) {}
Editor::IHostedInWindow WindowRoot() const noexcept { return _WindowRoot; }
Editor::IHostedInWindow WindowRoot() const noexcept { return _WeakWindowRoot.get(); }
Windows::Foundation::IInspectable ViewModel() const noexcept { return _ViewModel; }
private:
Editor::IHostedInWindow _WindowRoot{ nullptr };
winrt::weak_ref<Editor::IHostedInWindow> _WeakWindowRoot{ nullptr };
Windows::Foundation::IInspectable _ViewModel{ nullptr };
};

7
src/cascadia/TerminalSettingsEditor/NewTabMenu.cpp
View File

@@ -25,9 +25,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// but the XAML compiler can't find NewTabMenuListView when we try that. Rather than copying the list of selected items over
// to the view model, we'll just do this instead (much simpler).
NewTabMenuListView().SelectionChanged([this](auto&&, auto&&) {
const auto list = NewTabMenuListView();
MoveToFolderButton().IsEnabled(list.SelectedItems().Size() > 0);
DeleteMultipleButton().IsEnabled(list.SelectedItems().Size() > 0);
MoveToFolderButton().IsEnabled(NewTabMenuListView().SelectedItems().Size() > 0);
DeleteMultipleButton().IsEnabled(NewTabMenuListView().SelectedItems().Size() > 0);
});
Automation::AutomationProperties::SetName(MoveToFolderButton(), RS_(L"NewTabMenu_MoveToFolderTextBlock/Text"));
@@ -44,7 +43,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
const auto args = e.Parameter().as<Editor::NavigateToPageArgs>();
_ViewModel = args.ViewModel().as<Editor::NewTabMenuViewModel>();
_windowRoot = args.WindowRoot();
_weakWindowRoot = args.WindowRoot();
TraceLoggingWrite(
g_hTerminalSettingsEditorProvider,

4
src/cascadia/TerminalSettingsEditor/NewTabMenu.h
View File

@@ -38,13 +38,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void AddFolderNameTextBox_KeyDown(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::Input::KeyRoutedEventArgs& e);
void AddFolderNameTextBox_TextChanged(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::UI::Xaml::Controls::TextChangedEventArgs& e);
Editor::IHostedInWindow WindowRoot() const noexcept { return _windowRoot; }
Editor::IHostedInWindow WindowRoot() const noexcept { return _weakWindowRoot.get(); }
WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
WINRT_OBSERVABLE_PROPERTY(Editor::NewTabMenuViewModel, ViewModel, _PropertyChangedHandlers, nullptr);
private:
Editor::NewTabMenuEntryViewModel _draggedEntry{ nullptr };
Editor::IHostedInWindow _windowRoot;
winrt::weak_ref<Editor::IHostedInWindow> _weakWindowRoot;
void _ScrollToEntry(const Editor::NewTabMenuEntryViewModel& entry);
};

6
src/cascadia/TerminalSettingsEditor/ProfileViewModel.h
View File

@@ -17,13 +17,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
public:
NavigateToProfileArgs(ProfileViewModel profile, Editor::IHostedInWindow windowRoot) :
_Profile(profile),
_WindowRoot(windowRoot) {}
_WeakWindowRoot(windowRoot) {}
Editor::IHostedInWindow WindowRoot() const noexcept { return _WindowRoot; }
Editor::IHostedInWindow WindowRoot() const noexcept { return _WeakWindowRoot ? _WeakWindowRoot.get() : nullptr; }
Editor::ProfileViewModel Profile() const noexcept { return _Profile; }
private:
Editor::IHostedInWindow _WindowRoot;
winrt::weak_ref<Editor::IHostedInWindow> _WeakWindowRoot;
Editor::ProfileViewModel _Profile{ nullptr };
};

9
src/cascadia/TerminalSettingsEditor/Profiles_Advanced.cpp
View File

@@ -27,7 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
const auto args = e.Parameter().as<Editor::NavigateToProfileArgs>();
_Profile = args.Profile();
_windowRoot = args.WindowRoot();
_weakWindowRoot = args.WindowRoot();
TraceLoggingWrite(
g_hTerminalSettingsEditorProvider,
@@ -97,7 +97,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{ L"All Files (*.*)", L"*.*" }
};
const auto parentHwnd{ reinterpret_cast<HWND>(WindowRoot().GetHostingWindow()) };
const auto windowRoot = WindowRoot();
if (!windowRoot)
{
co_return;
}
const auto parentHwnd{ reinterpret_cast<HWND>(windowRoot.GetHostingWindow()) };
auto file = co_await OpenFilePicker(parentHwnd, [](auto&& dialog) {
try
{

4
src/cascadia/TerminalSettingsEditor/Profiles_Advanced.h
View File

@@ -22,12 +22,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void BellSoundAdd_Click(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
til::property_changed_event PropertyChanged;
Editor::IHostedInWindow WindowRoot() { return _windowRoot; };
Editor::IHostedInWindow WindowRoot() const noexcept { return _weakWindowRoot.get(); };
WINRT_PROPERTY(Editor::ProfileViewModel, Profile, nullptr);
private:
Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _ViewModelChangedRevoker;
Editor::IHostedInWindow _windowRoot;
winrt::weak_ref<Editor::IHostedInWindow> _weakWindowRoot;
winrt::Windows::Media::Playback::MediaPlayer _bellPlayer{ nullptr };
bool _bellPlayerCreated{ false };

2
src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp
View File

@@ -24,7 +24,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
const auto args = e.Parameter().as<Editor::NavigateToProfileArgs>();
_Profile = args.Profile();
_windowRoot = args.WindowRoot();
_weakWindowRoot = args.WindowRoot();
if (!_previewControl)
{

4
src/cascadia/TerminalSettingsEditor/Profiles_Appearance.h
View File

@@ -23,7 +23,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void CreateUnfocusedAppearance_Click(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
void DeleteUnfocusedAppearance_Click(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
Editor::IHostedInWindow WindowRoot() { return _windowRoot; };
Editor::IHostedInWindow WindowRoot() const noexcept { return _weakWindowRoot.get(); };
til::property_changed_event PropertyChanged;
WINRT_PROPERTY(Editor::ProfileViewModel, Profile, nullptr);
@@ -36,7 +36,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
std::shared_ptr<ThrottledFunc<>> _updatePreviewControl;
Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _ViewModelChangedRevoker;
Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _AppearanceViewModelChangedRevoker;
Editor::IHostedInWindow _windowRoot;
winrt::weak_ref<Editor::IHostedInWindow> _weakWindowRoot;
};
};

18
src/cascadia/TerminalSettingsEditor/Profiles_Base.cpp
View File

@@ -31,7 +31,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
const auto args = e.Parameter().as<Editor::NavigateToProfileArgs>();
_Profile = args.Profile();
_windowRoot = args.WindowRoot();
_weakWindowRoot = args.WindowRoot();
// Check the use parent directory box if the starting directory is empty
if (_Profile.StartingDirectory().empty())
@@ -113,7 +113,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
};
static constexpr winrt::guid clientGuidExecutables{ 0x2E7E4331, 0x0800, 0x48E6, { 0xB0, 0x17, 0xA1, 0x4C, 0xD8, 0x73, 0xDD, 0x58 } };
const auto parentHwnd{ reinterpret_cast<HWND>(_windowRoot.GetHostingWindow()) };
const auto windowRoot = WindowRoot();
if (!windowRoot)
{
co_return;
}
const auto parentHwnd{ reinterpret_cast<HWND>(windowRoot.GetHostingWindow()) };
auto path = co_await OpenFilePicker(parentHwnd, [](auto&& dialog) {
THROW_IF_FAILED(dialog->SetClientGuid(clientGuidExecutables));
try
@@ -136,7 +142,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
safe_void_coroutine Profiles_Base::StartingDirectory_Click(const IInspectable&, const RoutedEventArgs&)
{
auto lifetime = get_strong();
const auto parentHwnd{ reinterpret_cast<HWND>(_windowRoot.GetHostingWindow()) };
const auto windowRoot = WindowRoot();
if (!windowRoot)
{
co_return;
}
const auto parentHwnd{ reinterpret_cast<HWND>(windowRoot.GetHostingWindow()) };
auto folder = co_await OpenFilePicker(parentHwnd, [](auto&& dialog) {
static constexpr winrt::guid clientGuidFolderPicker{ 0xAADAA433, 0xB04D, 0x4BAE, { 0xB1, 0xEA, 0x1E, 0x6C, 0xD1, 0xCD, 0xA6, 0x8B } };
THROW_IF_FAILED(dialog->SetClientGuid(clientGuidFolderPicker));

4
src/cascadia/TerminalSettingsEditor/Profiles_Base.h
View File

@@ -25,13 +25,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void DeleteConfirmation_Click(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
til::property_changed_event PropertyChanged;
Editor::IHostedInWindow WindowRoot() const noexcept { return _windowRoot; }
Editor::IHostedInWindow WindowRoot() const noexcept { return _weakWindowRoot.get(); }
WINRT_PROPERTY(Editor::ProfileViewModel, Profile, nullptr);
private:
Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _ViewModelChangedRevoker;
winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;
Editor::IHostedInWindow _windowRoot;
winrt::weak_ref<Editor::IHostedInWindow> _weakWindowRoot;
};
};