Stop crash on window snap by preventing torn device resources state (#1768)

* Stop crash on window snap by preventing torn device resources state when quick on-the-fly resizing operations happen. #1572

(cherry picked from commit d8485079cd)

src/cascadia/TerminalApp/App.cpp

@@ -262,7 +262,11 @@ namespace winrt::TerminalApp::implementation
 
         const auto buttonText = resourceLoader.GetString(L"Ok");
 
-        _ShowDialog(winrt::box_value(title), winrt::box_value(aboutText), buttonText);
+        Controls::TextBlock aboutTextBlock;
+        aboutTextBlock.Text(aboutText);
+        aboutTextBlock.IsTextSelectionEnabled(true);
+
+        _ShowDialog(winrt::box_value(title), aboutTextBlock, buttonText);
     }
 
     // Method Description:

src/cascadia/TerminalApp/CascadiaSettings.cpp

@@ -37,7 +37,7 @@ CascadiaSettings::~CascadiaSettings()
 ColorScheme _CreateCampbellScheme()
 {
     ColorScheme campbellScheme{ L"Campbell",
-                                RGB(242, 242, 242),
+                                RGB(204, 204, 204),
                                 RGB(12, 12, 12) };
     auto& campbellTable = campbellScheme.GetTable();
     auto campbellSpan = gsl::span<COLORREF>(&campbellTable[0], gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE));
@@ -113,12 +113,12 @@ ColorScheme _CreateOneHalfLightScheme()
 ColorScheme _CreateSolarizedDarkScheme()
 {
     ColorScheme solarizedDarkScheme { L"Solarized Dark",
-                                      RGB(253, 246, 227),
+                                      RGB(131, 148, 150),
-                                      RGB(  7, 54,  66) };
+                                      RGB(  0,  43,  54) };
     auto& solarizedDarkTable = solarizedDarkScheme.GetTable();
     auto solarizedDarkSpan = gsl::span<COLORREF>(&solarizedDarkTable[0], gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE));
     solarizedDarkTable[0]  = RGB(  7, 54, 66);
-    solarizedDarkTable[1]  = RGB(211, 1, 2);
+    solarizedDarkTable[1]  = RGB(220, 50, 47);
     solarizedDarkTable[2]  = RGB(133, 153, 0);
     solarizedDarkTable[3]  = RGB(181, 137, 0);
     solarizedDarkTable[4]  = RGB( 38, 139, 210);
@@ -141,12 +141,12 @@ ColorScheme _CreateSolarizedDarkScheme()
 ColorScheme _CreateSolarizedLightScheme()
 {
     ColorScheme solarizedLightScheme { L"Solarized Light",
-                                       RGB(  7, 54,  66),
+                                       RGB(101, 123, 131),
                                        RGB(253, 246, 227) };
     auto& solarizedLightTable = solarizedLightScheme.GetTable();
     auto solarizedLightSpan = gsl::span<COLORREF>(&solarizedLightTable[0], gsl::narrow<ptrdiff_t>(COLOR_TABLE_SIZE));
     solarizedLightTable[0]  = RGB(  7, 54, 66);
-    solarizedLightTable[1]  = RGB(211, 1, 2);
+    solarizedLightTable[1]  = RGB(220, 50, 47);
     solarizedLightTable[2]  = RGB(133, 153, 0);
     solarizedLightTable[3]  = RGB(181, 137, 0);
     solarizedLightTable[4]  = RGB( 38, 139, 210);

src/cascadia/TerminalApp/Pane.cpp

@@ -331,7 +331,7 @@ void Pane::_CloseChild(const bool closeFirst)
         const auto oldFirstToken = _firstClosedToken;
         const auto oldSecondToken = _secondClosedToken;
         const auto oldFirst = _firstChild;
-        const auto oldSecond = _secondClosedToken;
+        const auto oldSecond = _secondChild;
 
         // Steal all the state from our child
         _splitState = remainingChild->_splitState;
@@ -342,9 +342,14 @@ void Pane::_CloseChild(const bool closeFirst)
         // Set up new close handlers on the children
         _SetupChildCloseHandlers();
 
-        // Revoke the old event handlers.
+        // Revoke the old event handlers on our new children
-        _firstChild->Closed(_firstClosedToken);
+        _firstChild->Closed(remainingChild->_firstClosedToken);
-        _secondChild->Closed(_secondClosedToken);
+        _secondChild->Closed(remainingChild->_secondClosedToken);
+
+        // Revoke event handlers on old panes and controls
+        oldFirst->Closed(oldFirstToken);
+        oldSecond->Closed(oldSecondToken);
+        closedChild->_control.ConnectionClosed(closedChild->_connectionClosedToken);
 
         // Reset our UI:
         _root.Children().Clear();

src/cascadia/TerminalControl/TermControl.cpp

@@ -627,15 +627,18 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             return;
         }
 
-        auto modifiers = _GetPressedModifierKeys();
+        const auto modifiers = _GetPressedModifierKeys();
-
         const auto vkey = static_cast<WORD>(e.OriginalKey());
 
         bool handled = false;
         auto bindings = _settings.KeyBindings();
         if (bindings)
         {
-            KeyChord chord(modifiers, vkey);
+            KeyChord chord(
+                WI_IsAnyFlagSet(modifiers, CTRL_PRESSED),
+                WI_IsAnyFlagSet(modifiers, ALT_PRESSED),
+                WI_IsFlagSet(modifiers, SHIFT_PRESSED),
+                vkey);
             handled = bindings.TryKeyChord(chord);
         }
 
@@ -645,10 +648,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
             // If the terminal translated the key, mark the event as handled.
             // This will prevent the system from trying to get the character out
             // of it and sending us a CharacterRecieved event.
-            handled = _terminal->SendKeyEvent(vkey,
+            handled = _terminal->SendKeyEvent(vkey, modifiers);
-                                              WI_IsFlagSet(modifiers, KeyModifiers::Ctrl),
-                                              WI_IsFlagSet(modifiers, KeyModifiers::Alt),
-                                              WI_IsFlagSet(modifiers, KeyModifiers::Shift));
 
             if (_cursorTimer.has_value())
             {
@@ -1417,8 +1417,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
     //   find out which modifiers (ctrl, alt, shift) are pressed in events that
     //   don't necessarily include that state.
     // Return Value:
-    // - a KeyModifiers value with flags set for each key that's pressed.
+    // - The combined ControlKeyState flags as a bitfield.
-    Settings::KeyModifiers TermControl::_GetPressedModifierKeys() const
+    DWORD TermControl::_GetPressedModifierKeys() const
     {
         CoreWindow window = CoreWindow::GetForCurrentThread();
         // DONT USE
@@ -1428,17 +1428,31 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         // Sometimes with the key down, the state is Down | Locked.
         // Sometimes with the key up, the state is Locked.
         // IsFlagSet(Down) is the only correct solution.
-        const auto ctrlKeyState = window.GetKeyState(VirtualKey::Control);
-        const auto shiftKeyState = window.GetKeyState(VirtualKey::Shift);
-        const auto altKeyState = window.GetKeyState(VirtualKey::Menu);
 
-        const auto ctrl = WI_IsFlagSet(ctrlKeyState, CoreVirtualKeyStates::Down);
+        struct KeyModifier
-        const auto shift = WI_IsFlagSet(shiftKeyState, CoreVirtualKeyStates::Down);
+        {
-        const auto alt = WI_IsFlagSet(altKeyState, CoreVirtualKeyStates::Down);
+            VirtualKey vkey;
+            DWORD flag;
+        };
 
-        return KeyModifiers{ (ctrl ? Settings::KeyModifiers::Ctrl : Settings::KeyModifiers::None) |
+        constexpr std::array<KeyModifier, 5> modifiers{ {
-                             (alt ? Settings::KeyModifiers::Alt : Settings::KeyModifiers::None) |
+            { VirtualKey::RightMenu, RIGHT_ALT_PRESSED },
-                             (shift ? Settings::KeyModifiers::Shift : Settings::KeyModifiers::None) };
+            { VirtualKey::LeftMenu, LEFT_ALT_PRESSED },
+            { VirtualKey::RightControl, RIGHT_CTRL_PRESSED },
+            { VirtualKey::LeftControl, LEFT_CTRL_PRESSED },
+            { VirtualKey::Shift, SHIFT_PRESSED },
+        } };
+
+        DWORD flags = 0;
+
+        for (const auto& mod : modifiers)
+        {
+            const auto state = window.GetKeyState(mod.vkey);
+            const auto isDown = WI_IsFlagSet(state, CoreVirtualKeyStates::Down);
+            flags |= isDown ? mod.flag : 0;
+        }
+
+        return flags;
     }
 
     // Method Description:

src/cascadia/TerminalControl/TermControl.h

@@ -140,7 +140,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
         void _ScrollbarUpdater(Windows::UI::Xaml::Controls::Primitives::ScrollBar scrollbar, const int viewTop, const int viewHeight, const int bufferSize);
         static Windows::UI::Xaml::Thickness _ParseThicknessFromPadding(const hstring padding);
 
-        Settings::KeyModifiers _GetPressedModifierKeys() const;
+        DWORD _GetPressedModifierKeys() const;
 
         const COORD _GetTerminalPosition(winrt::Windows::Foundation::Point cursorPosition);
     };

src/cascadia/TerminalCore/ITerminalInput.hpp

@@ -9,7 +9,7 @@ namespace Microsoft::Terminal::Core
     public:
         virtual ~ITerminalInput() {}
 
-        virtual bool SendKeyEvent(const WORD vkey, const bool ctrlPressed, const bool altPressed, const bool shiftPressed) = 0;
+        virtual bool SendKeyEvent(const WORD vkey, const DWORD modifiers) = 0;
 
         // void SendMouseEvent(uint row, uint col, KeyModifiers modifiers);
         [[nodiscard]] virtual HRESULT UserResize(const COORD size) noexcept = 0;

src/cascadia/TerminalCore/Terminal.cpp

@@ -195,16 +195,11 @@ void Terminal::Write(std::wstring_view stringView)
 //   real character out of the event.
 // Arguments:
 // - vkey: The vkey of the key pressed.
-// - ctrlPressed: true iff either ctrl key is pressed.
+// - modifiers: The current ControlKeyState flags.
-// - altPressed: true iff either alt key is pressed.
-// - shiftPressed: true iff either shift key is pressed.
 // Return Value:
 // - true if we translated the key event, and it should not be processed any further.
 // - false if we did not translate the key, and it should be processed into a character.
-bool Terminal::SendKeyEvent(const WORD vkey,
+bool Terminal::SendKeyEvent(const WORD vkey, const DWORD modifiers)
-                            const bool ctrlPressed,
-                            const bool altPressed,
-                            const bool shiftPressed)
 {
     if (_snapOnInput && _scrollOffset != 0)
     {
@@ -213,10 +208,23 @@ bool Terminal::SendKeyEvent(const WORD vkey,
         _NotifyScrollEvent();
     }
 
-    DWORD modifiers = 0 |
+    KeyEvent keyEv{ true, 0, vkey, 0, UNICODE_NULL, modifiers };
-                      (ctrlPressed ? LEFT_CTRL_PRESSED : 0) |
+
-                      (altPressed ? LEFT_ALT_PRESSED : 0) |
+    // AltGr key combinations don't always contain any meaningful,
-                      (shiftPressed ? SHIFT_PRESSED : 0);
+    // pretranslated unicode character during WM_KEYDOWN.
+    // E.g. on a German keyboard AltGr+Q should result in a "@" character,
+    // but actually results in "Q" with Alt and Ctrl modifier states.
+    // By returning false though, we can abort handling this WM_KEYDOWN
+    // event and let the WM_CHAR handler kick in, which will be
+    // provided with an appropriate unicode character.
+    if (keyEv.IsAltGrPressed())
+    {
+        return false;
+    }
+
+    const auto ctrlPressed = keyEv.IsCtrlPressed();
+    const auto altPressed = keyEv.IsAltPressed();
+    const auto shiftPressed = keyEv.IsShiftPressed();
 
     // Alt key sequences _require_ the char to be in the keyevent. If alt is
     // pressed, manually get the character that's being typed, and put it in the
@@ -250,9 +258,9 @@ bool Terminal::SendKeyEvent(const WORD vkey,
         ch = UNICODE_SPACE;
     }
 
-    const bool manuallyHandled = ch != UNICODE_NULL;
+    keyEv.SetCharData(ch);
 
-    KeyEvent keyEv{ true, 0, vkey, 0, ch, modifiers };
+    const bool manuallyHandled = ch != UNICODE_NULL;
     const bool translated = _terminalInput->HandleKey(&keyEv);
 
     return translated && manuallyHandled;

src/cascadia/TerminalCore/Terminal.hpp

@@ -76,10 +76,7 @@ public:
 
 #pragma region ITerminalInput
     // These methods are defined in Terminal.cpp
-    bool SendKeyEvent(const WORD vkey,
+    bool SendKeyEvent(const WORD vkey, const DWORD modifiers) override;
-                      const bool ctrlPressed,
-                      const bool altPressed,
-                      const bool shiftPressed) override;
     [[nodiscard]] HRESULT UserResize(const COORD viewportSize) noexcept override;
     void UserScrollViewport(const int viewTop) override;
     int GetScrollOffset() override;

src/cascadia/UnitTests_TerminalCore/InputTest.cpp

@@ -48,12 +48,12 @@ namespace TerminalCoreUnitTests
         // Verify that Alt+a generates a lowercase a on the input
         expectedinput = L"\x1b"
                         "a";
-        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', false, true, false));
+        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', LEFT_ALT_PRESSED));
 
         // Verify that Alt+shift+a generates a uppercase a on the input
         expectedinput = L"\x1b"
                         "A";
-        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', false, true, true));
+        VERIFY_IS_TRUE(term.SendKeyEvent(L'A', LEFT_ALT_PRESSED | SHIFT_PRESSED));
     }
 
     void InputTest::AltSpace()
@@ -61,6 +61,6 @@ namespace TerminalCoreUnitTests
         // Make sure we don't handle Alt+Space. The system will use this to
         // bring up the system menu for restore, min/maximimize, size, move,
         // close
-        VERIFY_IS_FALSE(term.SendKeyEvent(L' ', false, true, false));
+        VERIFY_IS_FALSE(term.SendKeyEvent(L' ', LEFT_ALT_PRESSED));
     }
 }

src/cppwinrt.build.pre.props

@@ -128,6 +128,7 @@
     </ClCompile>
     <Link>
       <AdditionalDependencies>WindowsApp.lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AppContainer>false</AppContainer>
     </Link>
   </ItemDefinitionGroup>
 

src/renderer/dx/DxRenderer.cpp

@@ -130,8 +130,6 @@ DxEngine::~DxEngine()
 
     RETURN_IF_FAILED(CreateDXGIFactory1(IID_PPV_ARGS(&_dxgiFactory2)));
 
-    RETURN_IF_FAILED(_dxgiFactory2->EnumAdapters1(0, &_dxgiAdapter1));
-
     const DWORD DeviceFlags = D3D11_CREATE_DEVICE_BGRA_SUPPORT |
                               // clang-format off
         // This causes problems for folks who do not have the whole DirectX SDK installed
@@ -154,8 +152,24 @@ DxEngine::~DxEngine()
         D3D_FEATURE_LEVEL_9_1,
     };
 
-    RETURN_IF_FAILED(D3D11CreateDevice(_dxgiAdapter1.Get(),
+    // Trying hardware first for maximum performance, then trying WARP (software) renderer second
-                                       D3D_DRIVER_TYPE_UNKNOWN,
+    // in case we're running inside a downlevel VM where hardware passthrough isn't enabled like
+    // for Windows 7 in a VM.
+    const auto hardwareResult = D3D11CreateDevice(NULL,
+                                                  D3D_DRIVER_TYPE_HARDWARE,
+                                                  NULL,
+                                                  DeviceFlags,
+                                                  FeatureLevels,
+                                                  ARRAYSIZE(FeatureLevels),
+                                                  D3D11_SDK_VERSION,
+                                                  &_d3dDevice,
+                                                  NULL,
+                                                  &_d3dDeviceContext);
+
+    if (FAILED(hardwareResult))
+    {
+        RETURN_IF_FAILED(D3D11CreateDevice(NULL,
+                                           D3D_DRIVER_TYPE_WARP,
                                            NULL,
                                            DeviceFlags,
                                            FeatureLevels,
@@ -164,8 +178,7 @@ DxEngine::~DxEngine()
                                            &_d3dDevice,
                                            NULL,
                                            &_d3dDeviceContext));
-
+    }
-    RETURN_IF_FAILED(_dxgiAdapter1->EnumOutputs(0, &_dxgiOutput));
 
     _displaySizePixels = _GetClientSize();
 
@@ -309,7 +322,6 @@ void DxEngine::_ReleaseDeviceResources() noexcept
 
     _dxgiSurface.Reset();
     _dxgiSwapChain.Reset();
-    _dxgiOutput.Reset();
 
     if (nullptr != _d3dDeviceContext.Get())
     {
@@ -321,7 +333,6 @@ void DxEngine::_ReleaseDeviceResources() noexcept
 
     _d3dDevice.Reset();
 
-    _dxgiAdapter1.Reset();
     _dxgiFactory2.Reset();
 }
 
@@ -679,10 +690,24 @@ void DxEngine::_InvalidOr(RECT rc) noexcept
         else if (_displaySizePixels.cy != clientSize.cy ||
                  _displaySizePixels.cx != clientSize.cx)
         {
+            // OK, we're going to play a dangerous game here for the sake of optimizing resize
+            // First, set up a complete clear of all device resources if something goes terribly wrong.
+            auto resetDeviceResourcesOnFailure = wil::scope_exit([&] {
+                _ReleaseDeviceResources();
+            });
+
+            // Now let go of a few of the device resources that get in the way of resizing buffers in the swap chain
             _dxgiSurface.Reset();
             _d2dRenderTarget.Reset();
-            _dxgiSwapChain->ResizeBuffers(2, clientSize.cx, clientSize.cy, DXGI_FORMAT_B8G8R8A8_UNORM, 0);
+
+            // Change the buffer size and recreate the render target (and surface)
+            RETURN_IF_FAILED(_dxgiSwapChain->ResizeBuffers(2, clientSize.cx, clientSize.cy, DXGI_FORMAT_B8G8R8A8_UNORM, 0));
             RETURN_IF_FAILED(_PrepareRenderTarget());
+
+            // OK we made it past the parts that can cause errors. We can release our failure handler.
+            resetDeviceResourcesOnFailure.release();
+
+            // And persist the new size.
             _displaySizePixels = clientSize;
         }
 

src/renderer/dx/DxRenderer.hpp

@@ -154,9 +154,7 @@ namespace Microsoft::Console::Render
         bool _haveDeviceResources;
         ::Microsoft::WRL::ComPtr<ID3D11Device> _d3dDevice;
         ::Microsoft::WRL::ComPtr<ID3D11DeviceContext> _d3dDeviceContext;
-        ::Microsoft::WRL::ComPtr<IDXGIAdapter1> _dxgiAdapter1;
         ::Microsoft::WRL::ComPtr<IDXGIFactory2> _dxgiFactory2;
-        ::Microsoft::WRL::ComPtr<IDXGIOutput> _dxgiOutput;
         ::Microsoft::WRL::ComPtr<IDXGISurface> _dxgiSurface;
         ::Microsoft::WRL::ComPtr<ID2D1RenderTarget> _d2dRenderTarget;
         ::Microsoft::WRL::ComPtr<ID2D1SolidColorBrush> _d2dBrushForeground;

src/terminal/adapter/ut_adapter/inputTest.cpp

@@ -339,6 +339,7 @@ void InputTest::TerminalInputModifierKeyTests()
     unsigned int uiKeystate = uiActualKeystate;
 
     const TerminalInput* const pInput = new TerminalInput(s_TerminalInputTestCallback);
+    const BYTE slashVkey = LOBYTE(VkKeyScanW(L'/'));
 
     Log::Comment(L"Sending every possible VKEY at the input stream for interception during key DOWN.");
     for (BYTE vkey = 0; vkey < BYTE_MAX; vkey++)
@@ -356,6 +357,12 @@ void InputTest::TerminalInputModifierKeyTests()
         irTest.Event.KeyEvent.wVirtualKeyCode = vkey;
         irTest.Event.KeyEvent.bKeyDown = TRUE;
 
+        // Ctrl-/ is handled in another test, because it's weird.
+        if (ControlPressed(uiKeystate) && (vkey == VK_DIVIDE || vkey == slashVkey))
+        {
+            continue;
+        }
+
         // Set up expected result
         switch (vkey)
         {
@@ -373,9 +380,7 @@ void InputTest::TerminalInputModifierKeyTests()
             continue;
         case VK_BACK:
             // Backspace is kinda different from other keys - we'll handle in another test.
-        case VK_DIVIDE:
         case VK_OEM_2:
-            // Ctrl-/ is also handled in another test, because it's weird.
             // VK_OEM_2 is typically the '/?' key
             continue;
             // wcscpy_s(s_pwsInputBuffer, L"\x7f");