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

Fix a number of shutdown crashes in TermControl #10115

Merged
merged 1 commit into from
May 17, 2021

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented May 17, 2021

  1. The TSFInputControl may get a layout event after it has been removed
    from service (and no longer has a XAML tree)
    • Two fixes:
      • first, guard the layour updater from accessing detached xaml
        objects
      • second, shut down all pending throttled functions during close
        (not destruction!¹)
  2. The TermControlAutomationPeer may be destructed before its events
    fire.
  3. The TermControlAutomationPeer may receive a notification after it has
    been detached from XAML (and therefore has no dispatcher).

¹ Close happens before the control is removed from the XAML tree;
destruction happens some time later. We must detach all UI-bound events
in Close so that they don't fire between when we detach and when we
destruct.

Fixes MSFT-32496693
Fixes MSFT-32496158
Fixes MSFT-32509759
Fixes MSFT-32871913

1. The TSFInputControl may get a layout event after it has been removed
   from service (and no longer has a XAML tree)
   * Two fixes:
      * first, guard the layour updater from accessing detached xaml
	objects
      * second, shut down all pending throttled functions during close
	(not destruction!¹)
2. The TermControlAutomationPeer may be destructed before its events
   fire.
3. The TermControlAutomationPeer may receive a notification after it has
   been detached from XAML (and therefore has no dispatcher).

¹ Close happens before the control is removed from the XAML tree;
destruction happens some time later. We must detach all UI-bound events
in Close so that they don't fire between when we detach and when we
destruct.
@DHowett DHowett requested review from zadjii-msft and lhecker May 17, 2021 17:50
@DHowett
Copy link
Member Author

DHowett commented May 17, 2021

I will port this forward to 1.8, but I will need some assistance in porting it for 1.9.

@DHowett
Copy link
Member Author

DHowett commented May 17, 2021

In general (/cc @zadjii-msft), we need to be very crisp on what constitutes "teardown" for UI objects and teardown for the control. All of these crashes are caused by the UI being in a torn down state, but the control itself not being a torn down state and trying to manipulate the UI.

@DHowett
Copy link
Member Author

DHowett commented May 17, 2021

These account for 65% of all the WT Stable crashes in Watson (!)

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof merging this with #10051 and the rest of the branches in #5000 is gonna be fun

@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels May 17, 2021
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand how there cannot be a Dispatcher sometimes... I always assumed that it's valid until after the class was deallocated...

@DHowett DHowett merged commit 661fde5 into release-1.7 May 17, 2021
@DHowett DHowett deleted the dev/duhowett/1.7-stab branch May 17, 2021 19:22
DHowett added a commit that referenced this pull request May 17, 2021
1. The TSFInputControl may get a layout event after it has been removed
   from service (and no longer has a XAML tree)
   * Two fixes:
      * first, guard the layour updater from accessing detached xaml
	objects
      * second, shut down all pending throttled functions during close
	(not destruction!¹)
2. The TermControlAutomationPeer may be destructed before its events
   fire.
3. The TermControlAutomationPeer may receive a notification after it has
   been detached from XAML (and therefore has no dispatcher).

¹ Close happens before the control is removed from the XAML tree;
destruction happens some time later. We must detach all UI-bound events
in Close so that they don't fire between when we detach and when we
destruct.

Fixes MSFT-32496693
Fixes MSFT-32496158
Fixes MSFT-32509759
Fixes MSFT-32871913

(cherry picked from commit 661fde5)
@ghost
Copy link

ghost commented May 25, 2021

🎉Windows Terminal v1.8.1444.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented May 25, 2021

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants