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.

Sign up for GitHub

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

Jump to bottom

Tabs are not released by TabSwitcher #8651

Closed
Don-Vito opened this issue Dec 24, 2020 · 3 comments · Fixed by #8653
Closed

Tabs are not released by TabSwitcher #8651

Don-Vito opened this issue Dec 24, 2020 · 3 comments · Fixed by #8653
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.

Comments

@Don-Vito
Copy link
Contributor

Don-Vito commented Dec 24, 2020 •
edited
Loading

Environment

Dev Latest

Steps to reproduce

  1. Measure memory
  2. Create 10 tabs
  3. Open Tab Switcher
  4. Close 10 tabs

Expected behavior

Memory is back to normal

Actual behavior

Memory remains high until Command Palette (action mode) is open.

Same test without opening tab switcher returns memory usage back to normal.

Observations

  1. Heap analysis shows that TerminalTab objects are still alive. Most probably due to CommandPalette still holding a reference to them somehow (I believe this semi-leak exists in 1.5 or earlier, but became painful since introduction of TabPaletteItem, as now we hold a reference to a tab rather than SwitchToTabCommand)
  2. We do not not reset tabs in Palette upon tab removal. This is something we need to fix. But it is insufficient to explain this "leak" - we do call SetTabs on the following invocations of TabSwitcher and yet it does not release the memory
  3. I tried to track when the actual memory release happens, and it looks that it is insufficient to switch to the action mode in the code, we really need to make the palette visible before the memory is released. This led me to the thought that the view is somehow holding the ref-count, although the model has already changed.
  4. I blamed x:Bind not handling PropertyChanged ref-counting correctly (like here: Navigation Memory Leak microsoft-ui-xaml#934). But removing the INotifyPropertyChanged completely and even converting the bindings to OneTime didn't help. So now I am not sure if this is the right direction
@ghost ghost 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 Dec 24, 2020
@Don-Vito
Copy link
Contributor Author

@DHowett, @zadjii-msft - I am still trying to understand what is going on, but please consider this as potential release blocker.

@Don-Vito Don-Vito mentioned this issue Dec 25, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Dec 25, 2020
@DHowett DHowett added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Severity-Blocking We won't ship a release like this! No-siree. Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 27, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 27, 2020
@DHowett
Copy link
Member

DHowett commented Dec 27, 2020

Good find! Thank you :)

@ghost ghost closed this as completed in #8653 Jan 4, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 4, 2021
ghost pushed a commit that referenced this issue Jan 4, 2021
@Don-Vito
Prevent Tab Switcher from increasing Tab refcount (#8653)
68e0af4 
Fix `TabPaletteItem` to hold only a weak reference to a tab.
This way we guarantee that the refcount of the closed tab 
gets to 0 immediately
(and that command palette cannot "raise it from the dead").

While this seems a correct thing to do, 
it is still not clear why the `FilteredCommand` itself 
(the one holding the `TabPaletteItem`) doesn't get released
until the UI is refreshed.

There is an impact of not registering to PropertyChanged event:
if the tab title changes during Tab Switcher navigation
the Tab Switcher item won't be updated immediately
(the change will apply next time the Tab Switcher is open).

Due to this change we need to make sure that the tabs binding
in #8427
doesn't break the title / icon update.

## Validation Steps Performed
* Manual testing

Closes #8651
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8653, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@Don-Vito @mpela81
Prevent Tab Switcher from increasing Tab refcount (microsoft#8653)
4d80fe4 
Fix `TabPaletteItem` to hold only a weak reference to a tab.
This way we guarantee that the refcount of the closed tab 
gets to 0 immediately
(and that command palette cannot "raise it from the dead").

While this seems a correct thing to do, 
it is still not clear why the `FilteredCommand` itself 
(the one holding the `TabPaletteItem`) doesn't get released
until the UI is refreshed.

There is an impact of not registering to PropertyChanged event:
if the tab title changes during Tab Switcher navigation
the Tab Switcher item won't be updated immediately
(the change will apply next time the Tab Switcher is open).

Due to this change we need to make sure that the tabs binding
in microsoft#8427
doesn't break the title / icon update.

## Validation Steps Performed
* Manual testing

Closes microsoft#8651
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Prevent Tab Switcher from increasing Tab refcount Don-Vito/terminal
2 participants
@DHowett @Don-Vito