Skip to content
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.

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

[1.18+] On Windows 10, Terminal reserves ~15 MB of memory per window opened at the same time #15384

Closed
zadjii-msft opened this issue May 18, 2023 · 7 comments · Fixed by #18215
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented May 18, 2023


Note: 📌 Pinned comment: #15384 (comment)


This is from me deploying to a Win10 19041 VM.

Closed a second window, then hovered over the first. At some point got:

>	Windows.UI.Xaml.Controls.dll!winrt::throw_hresult(const winrt::hresult result) Line 4524	C++
 	[Inline Frame] Windows.UI.Xaml.Controls.dll!winrt::check_hresult(winrt::hresult) Line 4612	C++
 	Windows.UI.Xaml.Controls.dll!winrt::impl::consume_Windows_UI_Xaml_IDependencyObject<AcrylicBrush>::GetValue(const winrt::Windows::UI::Xaml::DependencyProperty & dp) Line 1045	C++
 	[Inline Frame] Windows.UI.Xaml.Controls.dll!AcrylicBrushProperties::AlwaysUseFallback() Line 150	C++
 	Windows.UI.Xaml.Controls.dll!AcrylicBrush::UpdateAcrylicBrush() Line 996	C++
    ...

This one's a hard failfast that can't be continued. Window also seems to be in a normal state? In a normal _messagePump?

This happens every time I hover the split button after closing another window on Windows 10, but not on Windows 11.

Originally posted by @zadjii-msft in #15364 (comment)

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels May 18, 2023
@zadjii-msft
Copy link
Member Author

zadjii-msft commented May 18, 2023

Relevant threads:

Basically, when we close a DesktopWindowXamlSource, it calls to Windows_UI_Xaml!DirectUI::MetadataAPI::Reset, which resets the XAML metadata provider for the process. So, closing one DWXS on one thread fucks all the other threads with XAML.

This was fixed in Windows 11 by os.2020!5837001. That wasn't backported to Windows 10. We may be in a bad spot.

  • Not release 1.18 for Windows 10, until that's fixed?
  • Hold the release entirely until we find some workaround?
  • Manually disable process model v3 on Windows 10 (is that even possible?)
  • Leak the DesktopWindowsXamlSource (ew)
  • Use some sort of threadpool to hold on to expired windows, and re-use them for subsequent windows (this will still consume extra memory, but only up to "max number of simultaneous windows" instead of "the total number of windows opened")
  • Ship to Win11. Hold off on shipping to win10 until we do the threadpool thing?

@zadjii-msft zadjii-msft added Severity-Crash Crashes are real bad news. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Area-Windowing Window frame, quake mode, tearout labels May 19, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.18 milestone May 19, 2023
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 19, 2023
zadjii-msft added a commit that referenced this issue May 22, 2023
Re: #15384

I'm not sure I've even got the words for this.
@zadjii-msft
Copy link
Member Author

After #15397, closing a window on Win10 will simply leak the window. This will result in a ~15 MB memory leak, which is Bad but less bad than full on crashing.

I'm working on a way to re-use old threads. That'll cause the leak to be scaled on N="max number of simultaneously open windows" rather than M="total number of closed windows over the life of the Terminal process"

@zadjii-msft zadjii-msft changed the title [1.18+] Closing one window on Windows 10 will crash the Terminal [1.18+] Closing windows on Windows 10 leaks ~15 MB of memory May 22, 2023
@zadjii-msft zadjii-msft added the Issue-Bug It either shouldn't be doing this or needs an investigation. label May 22, 2023
@zadjii-msft zadjii-msft self-assigned this May 22, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label May 22, 2023
zadjii-msft added a commit that referenced this issue May 22, 2023
Re: #15384

Basically, when we close a `DesktopWindowXamlSource`, it calls to `Windows_UI_Xaml!DirectUI::MetadataAPI::Reset`, which resets the XAML metadata provider _for the process_. So, closing one DWXS on one thread will force an A/V next time another thread tries to do something like... display a tooltip. Not immediately, but surely soon enough.

This was fixed in Windows 11 by os.2020!5837001. That wasn't backported to Windows 10.

This will cause a ~15MB memory leak PER WINDOW. OBVIOUSLY, this is bad, but it's less bad than crashing. 

We're gonna keep using #15384 for other ideas here too.
DHowett pushed a commit that referenced this issue May 23, 2023
Re: #15384

Basically, when we close a `DesktopWindowXamlSource`, it calls to `Windows_UI_Xaml!DirectUI::MetadataAPI::Reset`, which resets the XAML metadata provider _for the process_. So, closing one DWXS on one thread will force an A/V next time another thread tries to do something like... display a tooltip. Not immediately, but surely soon enough.

This was fixed in Windows 11 by os.2020!5837001. That wasn't backported to Windows 10.

This will cause a ~15MB memory leak PER WINDOW. OBVIOUSLY, this is bad, but it's less bad than crashing.

We're gonna keep using #15384 for other ideas here too.

(cherry picked from commit c589784)
Service-Card-Id: 89283538
Service-Version: 1.18
@zadjii-msft zadjii-msft removed Severity-Crash Crashes are real bad news. Severity-Blocking We won't ship a release like this! No-siree. labels May 23, 2023
@sh-shahrokhi
Copy link

does the leak occur when running on windows 11 too?

@DHowett
Copy link
Member

DHowett commented Jun 8, 2023

does the leak occur when running on windows 11 too?

It shouldn't!

zadjii-msft added a commit that referenced this issue Jul 19, 2023
This is my proposed solution to #15384.

Basically, the issue is that we cannot ever close a
`DesktopWindowXamlSource` ("DWXS"). If we do, then any other thread that
tries to access XAML metadata will explode, which happens frequently. A
DWXS is inextricably linked to an HWND. That means we have to not only
reuse DWXS's, but the HWNDs themselves. XAML also isn't agile, so we've
got to keep the `thread` that the DWXS was started on alive as well.

To do this, we're going to introduce the ability to "refrigerate" and
"reheat" window threads.
* A window thread is "**hot**" if it's actively got a window, and is
pumping window messages, and generally, is a normal thing.
* When a window is closed, we need to "**refrigerate**" it's
`WindowThread` and `IslandWindow`. `WindowEmperor` will take care of
tracking the threads that are refrigerated.
* When a new window is requested, the Emperor first try to
"**reheat**"/"**microwave**" a refrigerated thread. When a thread gets
reheated, we'll create a new AppHost (and `TerminalWindow`/`Page`), and
we'll use the _existing_ `IslandWindow` for that instance.

<sub>The metaphor is obviously ridiculous, but _you get it_ so who
cares.</sub>

In this way, we'll keep all the windows we've ever created around in
memory, for later reuse. This means that the leak goes from (~12MB x
number of windows closed) to (~12MB x maximum number of simultaneously
open Terminal windows). It's still not good.

We won't do this on Windows 11, because the bug that is the fundamental
premise of this issue is fixed already in the OS.




I'm not 100% confident in this yet. 
* [x] There's still a d3d leak of some sort on exit in debug builds.
(maybe #15306 related)
* havent seen this in a while. Must have been a issue in an earlier
revision.
* [x] I need to validate more on Windows 11
* [x] **BAD**: Closing the last tab on Windows 11 doesn't close the
window
* [x] **BAD**: Closing a window on Windows 11 doesn't close the window -
it just closes the one tab item and keeps on choochin'
* [x] **BAD**: Close last tab, open new one, attempt to close window -
ALL windows go \*poof\*. Cause of course. No break into post-mortem
either.
* [x] more comments
* [ ] maybe a diagram
* [x] Restoring windows is at the wrong place entirely? I once reopened
the Terminal with two persisted windows, and it created one at 0,0
* [x] Remaining code TODO!s: 0 (?)
* [ ] "warm-reloading" `useTabsInTitlebar` (change while terminal is
running after closing a window, open a new one) REALLY doesn't work.
Obviously restores the last kind of window. Yike.

is all about #15384

closes #15410 along the way. Might fork that fix off.
@zadjii-msft
Copy link
Member Author

As of #15424 merging, this has been reduced to "On Windows 10, the terminal permanently reserves ~15MB of memory per maximum number of concurrently opened windows". Still not great. But this is probably what we're going to have to live with for 1.18+.

There's possibly... a lot of XAML-side fixes that might need backporting to fix Windows 10 entirely. That's unfortunate, because it makes creating a servicing patch hard-to-impossible to create 😢

@zadjii-msft zadjii-msft removed their assignment Jul 19, 2023
@zadjii-msft zadjii-msft removed this from the Terminal v1.19 milestone Jul 19, 2023
@zadjii-msft zadjii-msft added this to the Backlog milestone Jul 19, 2023
@zadjii-msft zadjii-msft changed the title [1.18+] Closing windows on Windows 10 leaks ~15 MB of memory [1.18+] On Windows 10, Terminal reserves ~15 MB of memory per window opened at the same time Jul 19, 2023
DHowett pushed a commit that referenced this issue Sep 22, 2023
This is my proposed solution to #15384.

Basically, the issue is that we cannot ever close a
`DesktopWindowXamlSource` ("DWXS"). If we do, then any other thread that
tries to access XAML metadata will explode, which happens frequently. A
DWXS is inextricably linked to an HWND. That means we have to not only
reuse DWXS's, but the HWNDs themselves. XAML also isn't agile, so we've
got to keep the `thread` that the DWXS was started on alive as well.

To do this, we're going to introduce the ability to "refrigerate" and
"reheat" window threads.
* A window thread is "**hot**" if it's actively got a window, and is
pumping window messages, and generally, is a normal thing.
* When a window is closed, we need to "**refrigerate**" it's
`WindowThread` and `IslandWindow`. `WindowEmperor` will take care of
tracking the threads that are refrigerated.
* When a new window is requested, the Emperor first try to
"**reheat**"/"**microwave**" a refrigerated thread. When a thread gets
reheated, we'll create a new AppHost (and `TerminalWindow`/`Page`), and
we'll use the _existing_ `IslandWindow` for that instance.

<sub>The metaphor is obviously ridiculous, but _you get it_ so who
cares.</sub>

In this way, we'll keep all the windows we've ever created around in
memory, for later reuse. This means that the leak goes from (~12MB x
number of windows closed) to (~12MB x maximum number of simultaneously
open Terminal windows). It's still not good.

We won't do this on Windows 11, because the bug that is the fundamental
premise of this issue is fixed already in the OS.

I'm not 100% confident in this yet.
* [x] There's still a d3d leak of some sort on exit in debug builds.
(maybe #15306 related)
* havent seen this in a while. Must have been a issue in an earlier
revision.
* [x] I need to validate more on Windows 11
* [x] **BAD**: Closing the last tab on Windows 11 doesn't close the
window
* [x] **BAD**: Closing a window on Windows 11 doesn't close the window -
it just closes the one tab item and keeps on choochin'
* [x] **BAD**: Close last tab, open new one, attempt to close window -
ALL windows go \*poof\*. Cause of course. No break into post-mortem
either.
* [x] more comments
* [ ] maybe a diagram
* [x] Restoring windows is at the wrong place entirely? I once reopened
the Terminal with two persisted windows, and it created one at 0,0
* [x] Remaining code TODO!s: 0 (?)
* [ ] "warm-reloading" `useTabsInTitlebar` (change while terminal is
running after closing a window, open a new one) REALLY doesn't work.
Obviously restores the last kind of window. Yike.

is all about #15384

closes #15410 along the way. Might fork that fix off.

(cherry picked from commit 7010626)
Service-Card-Id: 89927087
Service-Version: 1.18
@driver1998
Copy link

driver1998 commented Dec 18, 2023

While chatting about this with @MouriNaruto offline, he mentioned that it seems only the very first DesktopWindowXamlSource instance (which hosts a CoreWindow for compatibility) will need to be kept, not all of them.

He ended up firing up an empty, hidden XAML window at the very beginning of his app and be done with it:
#6507 (comment)

@zadjii-msft
Copy link
Member Author

Hey guess what! As of Terminal 1.23, we don't need to do the goofy window refrigeration anymore. So this will no longer be an issue.

This was fixed in #18215

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Feb 19, 2025
@zadjii-msft zadjii-msft linked a pull request Feb 19, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants