-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Memory leak and other issues when saving settings #10609
Comments
Typically, yea, we'd prefer separate issues, but since you've already got #10619 open for the overflow bit, we can just leave this open for the memory leak. I've got a strong suspicion that the memory leak and the "failed to load settings" dialog will end up with the same root cause |
When navigating the settings (or saving/discarding) the animation of the main content overflows the bar with the save and discard buttons. If the main content is encapsulated in a ScrollView the issue goes away. Fixes one of the issues in #10609 ## Validation Steps Performed Clicked around a whole bunch and have not seen the overflow happen again. Verified that on tabs where scroll is necessary it can still be scrolled, and reflow of elements still functions.
Unfortunately the question here seems to be what doesn't leak.
Besides that pretty much any interaction the
I'm stopping my investigation here. Perhaps it is easier to debug with access to the Windows.UI.* source if the terminal team has access? |
When navigating the settings (or saving/discarding) the animation of the main content overflows the bar with the save and discard buttons. If the main content is encapsulated in a ScrollView the issue goes away. Fixes one of the issues in #10609 ## Validation Steps Performed Clicked around a whole bunch and have not seen the overflow happen again. Verified that on tabs where scroll is necessary it can still be scrolled, and reflow of elements still functions. (cherry picked from commit 19f8b9c)
I couldn't resist taking another look. It seems the main (or only) issue is navigating the pages. (The Interestingly, some pages leak way more than others. For example, on 1.8.1521.0, the "color schemes" page leaks hardly anything while the "actions" page seems to leak more than a megabyte per click on "discard". A debug build on main shows "color schemes" also leaking, and makes the "actions" page leak about 5MB per navigation. I'll file a separate issue for the settings file not loading, as that seems less related now. |
## Summary of the Pull Request Add an explicit background color to part of the settings UI to prevent animation overflow. The previous solution (adding a ScrollViewer) caused problems. ## References #10619 adds a ScrollViewer for one of the issues in #10609 ## PR Checklist * [x] Closes #10664 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed Visually confirmed the animation doesn't overflow, changed the theme and confirmed the colors are responsive. Confirmed the extra scrollbar is gone.
## Summary of the Pull Request Add an explicit background color to part of the settings UI to prevent animation overflow. The previous solution (adding a ScrollViewer) caused problems. ## References #10619 adds a ScrollViewer for one of the issues in #10609 ## PR Checklist * [x] Closes #10664 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed Visually confirmed the animation doesn't overflow, changed the theme and confirmed the colors are responsive. Confirmed the extra scrollbar is gone.
Maybe related:
I could have sworn I saw another related issue yesterday, but I'm having a hard time finding it now |
Okay, I've got a VS diag session in Looks like half of the 8MB that got allocated by saving the menu a few times were random |
1005e0d makes this a bit better, but there's still some leakage. Here's the deal though - there's leakage even in the XAML Controls Gallery with the nav view. |
|
Seems like this is still present on 1.22 canary builds. Even after closing the SUI, the XAML objects remain leaked. Amazingly enough, I think this might be fixed in WinUI 3. The WinUI 2 gallery repros this pretty readily, just switching pages in the NavView page. But the WinUI 3 gallery didn't seem to. |
Windows Terminal version (or Windows build number)
1.8.1521.0 (also observed on source build 2021-07-11)
Other Software
No response
Steps to reproduce
Open the settings dialog and spam click save. No option needs to be changed.
Video: 13 seconds in, the settings could not be reloaded from file (not sure if it is not loading or file corruption).
https://user-images.githubusercontent.com/2277504/125179830-a31f3a00-e1f2-11eb-8b6f-6f6aa07896c5.mp4
Expected Behavior
Memory stays roughly the same, settings file can be loaded after saving, main UI does not extend past its borders.
Actual Behavior
Memory grows and is not reclaimed, settings file can sometimes not be loaded after saving, UI grows past its borders during reloading.
(Second and third only discovered after deciding to record a video for the memory leak)
Not sure if you want separate tracking issues for these, let me know.
The text was updated successfully, but these errors were encountered: