Shut down all controls under a tab before we remove it from the list (#…

…4337)

This commit introduces a new recursive pane shutdown that will give all
controls under a tab a chance to clean up their state before beign
detached from the UI. It also reorders the call to LastTabClosed() so
that the application does not exit before the final connections are
terminated.

It also teaches TSFInputControl how to shut down to avoid a dramatic
platform bug.

Fixes #4159.
Fixes #4336.

## PR Checklist
* [x] CLA signed
* [x] I've discussed this with core contributors already.

## Validation Steps Performed
Validated through manual terminal teardown within and without the debugger, given a crazy number of panes and tabs.

src/cascadia/TerminalApp/Pane.cpp

@@ -358,6 +358,25 @@ void Pane::Close()
     _ClosedHandlers(nullptr, nullptr);
 }
 
+// Method Description:
+// - Prepare this pane to be removed from the UI hierarchy by closing all controls
+//   and connections beneath it.
+void Pane::Shutdown()
+{
+    // Lock the create/close lock so that another operation won't concurrently
+    // modify our tree
+    std::unique_lock lock{ _createCloseLock };
+    if (_IsLeaf())
+    {
+        _control.Close();
+    }
+    else
+    {
+        _firstChild->Shutdown();
+        _secondChild->Shutdown();
+    }
+}
+
 // Method Description:
 // - Get the root UIElement of this pane. There may be a single TermControl as a
 //   child, or an entire tree of grids and panes as children of this element.

src/cascadia/TerminalApp/Pane.h

@@ -64,6 +64,7 @@ class Pane : public std::enable_shared_from_this<Pane>
                                                                   const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
     float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const;
 
+    void Shutdown();
     void Close();
 
     WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);

src/cascadia/TerminalApp/Tab.cpp

@@ -298,6 +298,13 @@ void Tab::NavigateFocus(const winrt::TerminalApp::Direction& direction)
     _rootPane->NavigateFocus(direction);
 }
 
+// Method Description:
+// - Prepares this tab for being removed from the UI hierarchy by shutting down all active connections.
+void Tab::Shutdown()
+{
+    _rootPane->Shutdown();
+}
+
 // Method Description:
 // - Closes the currently focused pane in this tab. If it's the last pane in
 //   this tab, our Closed event will be fired (at a later time) for anyone

src/cascadia/TerminalApp/Tab.h

@@ -37,6 +37,7 @@ class Tab : public std::enable_shared_from_this<Tab>
     winrt::hstring GetActiveTitle() const;
     winrt::fire_and_forget SetTabText(const winrt::hstring text);
 
+    void Shutdown();
     void ClosePane();
 
     WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -753,16 +753,20 @@ namespace winrt::TerminalApp::implementation
     // - tabIndex: the index of the tab to be removed
     void TerminalPage::_RemoveTabViewItemByIndex(uint32_t tabIndex)
     {
+        // Removing the tab from the collection should destroy its control and disconnect its connection,
+        // but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction.
+        auto iterator = _tabs.begin() + tabIndex;
+        (*iterator)->Shutdown();
+
+        _tabs.erase(iterator);
+        _tabView.TabItems().RemoveAt(tabIndex);
+
         // To close the window here, we need to close the hosting window.
-        if (_tabs.size() == 1)
+        if (_tabs.size() == 0)
         {
             _lastTabClosedHandlers(*this, nullptr);
         }
 
-        // Removing the tab from the collection will destroy its control and disconnect its connection.
-        _tabs.erase(_tabs.begin() + tabIndex);
-        _tabView.TabItems().RemoveAt(tabIndex);
-
         auto focusedTabIndex = _GetFocusedTabIndex();
         if (gsl::narrow_cast<int>(tabIndex) == focusedTabIndex)
         {

src/cascadia/TerminalControl/TSFInputControl.cpp

@@ -59,23 +59,32 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         // set the input scope to Text because this control is for any text.
         _editContext.InputScope(Core::CoreTextInputScope::Text);
 
-        _editContext.TextRequested({ this, &TSFInputControl::_textRequestedHandler });
+        _textRequestedRevoker = _editContext.TextRequested(winrt::auto_revoke, { this, &TSFInputControl::_textRequestedHandler });
 
-        _editContext.SelectionRequested({ this, &TSFInputControl::_selectionRequestedHandler });
+        _selectionRequestedRevoker = _editContext.SelectionRequested(winrt::auto_revoke, { this, &TSFInputControl::_selectionRequestedHandler });
 
-        _editContext.FocusRemoved({ this, &TSFInputControl::_focusRemovedHandler });
+        _focusRemovedRevoker = _editContext.FocusRemoved(winrt::auto_revoke, { this, &TSFInputControl::_focusRemovedHandler });
 
-        _editContext.TextUpdating({ this, &TSFInputControl::_textUpdatingHandler });
+        _textUpdatingRevoker = _editContext.TextUpdating(winrt::auto_revoke, { this, &TSFInputControl::_textUpdatingHandler });
 
-        _editContext.SelectionUpdating({ this, &TSFInputControl::_selectionUpdatingHandler });
+        _selectionUpdatingRevoker = _editContext.SelectionUpdating(winrt::auto_revoke, { this, &TSFInputControl::_selectionUpdatingHandler });
 
-        _editContext.FormatUpdating({ this, &TSFInputControl::_formatUpdatingHandler });
+        _formatUpdatingRevoker = _editContext.FormatUpdating(winrt::auto_revoke, { this, &TSFInputControl::_formatUpdatingHandler });
 
-        _editContext.LayoutRequested({ this, &TSFInputControl::_layoutRequestedHandler });
+        _layoutRequestedRevoker = _editContext.LayoutRequested(winrt::auto_revoke, { this, &TSFInputControl::_layoutRequestedHandler });
 
-        _editContext.CompositionStarted({ this, &TSFInputControl::_compositionStartedHandler });
+        _compositionStartedRevoker = _editContext.CompositionStarted(winrt::auto_revoke, { this, &TSFInputControl::_compositionStartedHandler });
 
-        _editContext.CompositionCompleted({ this, &TSFInputControl::_compositionCompletedHandler });
+        _compositionCompletedRevoker = _editContext.CompositionCompleted(winrt::auto_revoke, { this, &TSFInputControl::_compositionCompletedHandler });
+    }
+
+    // Method Description:
+    // - Prepares this TSFInputControl to be removed from the UI hierarchy.
+    void TSFInputControl::Close()
+    {
+        // Explicitly disconnect the LayoutRequested handler -- it can cause problems during application teardown.
+        // See GH#4159 for more info.
+        _layoutRequestedRevoker.revoke();
     }
 
     // Method Description:

src/cascadia/TerminalControl/TSFInputControl.h

@@ -37,6 +37,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void NotifyFocusEnter();
         void NotifyFocusLeave();
 
+        void Close();
+
         static void OnCompositionChanged(Windows::UI::Xaml::DependencyObject const&, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const&);
 
         // -------------------------------- WinRT Events ---------------------------------
@@ -55,6 +57,16 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void _textUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, winrt::Windows::UI::Text::Core::CoreTextTextUpdatingEventArgs const& args);
         void _formatUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, winrt::Windows::UI::Text::Core::CoreTextFormatUpdatingEventArgs const& args);
 
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::TextRequested_revoker _textRequestedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionRequested_revoker _selectionRequestedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::FocusRemoved_revoker _focusRemovedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::TextUpdating_revoker _textUpdatingRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionUpdating_revoker _selectionUpdatingRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::FormatUpdating_revoker _formatUpdatingRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::LayoutRequested_revoker _layoutRequestedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionStarted_revoker _compositionStartedRevoker;
+        winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionCompleted_revoker _compositionCompletedRevoker;
+
         Windows::UI::Xaml::Controls::Canvas _canvas;
         Windows::UI::Xaml::Controls::TextBlock _textBlock;
 

src/cascadia/TerminalControl/TSFInputControl.idl

@@ -27,5 +27,7 @@ namespace Microsoft.Terminal.TerminalControl
 
         void NotifyFocusEnter();
         void NotifyFocusLeave();
+
+        void Close();
     }
 }

src/cascadia/TerminalControl/TermControl.cpp

@@ -1734,6 +1734,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             _connection.TerminalOutput(_connectionOutputEventToken);
             _connectionStateChangedRevoker.revoke();
 
+            _tsfInputControl.Close(); // Disconnect the TSF input control so it doesn't receive EditContext events.
+
             if (auto localConnection{ std::exchange(_connection, nullptr) })
             {
                 localConnection.Close();