Avoid focus loops in ConPTY (#17829)

This fixes a lot of subtle issues:
* Avoid emitting another de-/iconify VT sequence when
  we encounter a (de)iconify VT sequence during parsing.
* Avoid emitting a de-/iconify VT sequence when
  a focus event is received on the signal pipe.
* Avoid emitting such sequences on startup.
* Avoid emitting multiple such sequences
  when rapidly un-/focusing the window.

It's also a minor cleanup, because the `GA_ROOTOWNER` is not security
relevant. It was added because there was concern that someone can just
spawn a ConPTY session, tell it that it's focused, and spawn a child
which is now focused. But why would someone do that, when the console
IOCTLs to do so are not just publicly available but also documented?

I also disabled the IME window.

## Validation Steps Performed
* First:
  ```cpp
  int main() {
      for (bool show = false;; show = !show) {
          printf(show ? "Show in 3s...\n" : "Hide in 3s...\n");
          Sleep(3000);
          ShowWindow(GetConsoleWindow(), show ? SW_SHOW : SW_HIDE);
      }
  }
  ```
* PowerShell 5's `Get-Credential` gains focus ✅
* `sleep 5; Get-Credential` and focus another app. WT should start
  blinking in the taskbar. Restore it. The popup has focus ✅
* Run `:hardcopy` in vim: Window is shown centered at (0,0) ✖️
  But that's okay because it does that already anyway ✅
* `Connect-AzAccount` doesn't crash PowerShell ✅

(cherry picked from commit 4386bf0)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgTs61k
Service-Version: 1.22

.github/actions/spelling/allow/apis.txt

@@ -96,6 +96,7 @@ IGraphics
 IImage
 IInheritable
 IMap
+imm
 IMonarch
 IObject
 iosfwd

src/cascadia/TerminalControl/ControlCore.cpp

@@ -198,6 +198,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                 }
             });
 
+        // If you rapidly show/hide Windows Terminal, something about GotFocus()/LostFocus() gets broken.
+        // We'll then receive easily 10+ such calls from WinUI the next time the application is shown.
+        shared->focusChanged = std::make_unique<til::debounced_func_trailing<bool>>(
+            std::chrono::milliseconds{ 25 },
+            [weakThis = get_weak()](const bool focused) {
+                if (const auto core{ weakThis.get() })
+                {
+                    core->_focusChanged(focused);
+                }
+            });
+
         // Scrollbar updates are also expensive (XAML), so we'll throttle them as well.
         shared->updateScrollBar = std::make_shared<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>>(
             _dispatcher,
@@ -2471,13 +2482,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - This is related to work done for GH#2988.
     void ControlCore::GotFocus()
     {
-        _focusChanged(true);
+        const auto shared = _shared.lock_shared();
+        if (shared->focusChanged)
+        {
+            (*shared->focusChanged)(true);
+        }
     }
 
     // See GotFocus.
     void ControlCore::LostFocus()
     {
-        _focusChanged(false);
+        const auto shared = _shared.lock_shared();
+        if (shared->focusChanged)
+        {
+            (*shared->focusChanged)(false);
+        }
     }
 
     void ControlCore::_focusChanged(bool focused)

src/cascadia/TerminalControl/ControlCore.h

@@ -304,6 +304,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         struct SharedState
         {
             std::unique_ptr<til::debounced_func_trailing<>> outputIdle;
+            std::unique_ptr<til::debounced_func_trailing<bool>> focusChanged;
             std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar;
         };
 

src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj

@@ -57,7 +57,7 @@
       <PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile>
     </ClCompile>
     <Link>
-      <AdditionalDependencies>WindowsApp.lib;WinMM.Lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies>WindowsApp.lib;winmm.Lib;imm32.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
   <!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->

src/host/PtySignalInputThread.cpp

@@ -75,8 +75,6 @@ void PtySignalInputThread::ConnectConsole() noexcept
     {
         _DoShowHide(*_initialShowHide);
     }
-
-    // We should have successfully used the _earlyReparent message in CreatePseudoWindow.
 }
 
 // Method Description:
@@ -88,8 +86,7 @@ void PtySignalInputThread::ConnectConsole() noexcept
 // - Refer to GH#13066 for details.
 void PtySignalInputThread::CreatePseudoWindow()
 {
-    HWND owner = _earlyReparent.has_value() ? reinterpret_cast<HWND>((*_earlyReparent).handle) : HWND_DESKTOP;
-    ServiceLocator::LocatePseudoWindow(owner);
+    ServiceLocator::LocatePseudoWindow();
 }
 
 // Method Description:
@@ -223,7 +220,7 @@ void PtySignalInputThread::_DoShowHide(const ShowHideData& data)
         return;
     }
 
-    _api.ShowWindow(data.show);
+    ServiceLocator::SetPseudoWindowVisibility(data.show);
 }
 
 // Method Description:
@@ -240,44 +237,7 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
     LockConsole();
     auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
 
-    // If the client app hasn't yet connected, stash the new owner.
-    // We'll later (PtySignalInputThread::ConnectConsole) use the value
-    // to set up the owner of the conpty window.
-    if (!_consoleConnected)
-    {
-        _earlyReparent = data;
-        return;
-    }
-
-    const auto owner{ reinterpret_cast<HWND>(data.handle) };
-    // This will initialize s_interactivityFactory for us. It will also
-    // conveniently return 0 when we're on OneCore.
-    //
-    // If the window hasn't been created yet, by some other call to
-    // LocatePseudoWindow, then this will also initialize the owner of the
-    // window.
-    if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(owner) })
-    {
-        // SetWindowLongPtrW may call back into the message handler and wait for it to finish,
-        // similar to SendMessageW(). If the conhost message handler is already processing and
-        // waiting to acquire the console lock, which we're currently holding, we'd deadlock.
-        // --> Release the lock now.
-        Unlock.reset();
-
-        // DO NOT USE SetParent HERE!
-        //
-        // Calling SetParent on a window that is WS_VISIBLE will cause the OS to
-        // hide the window, make it a _child_ window, then call SW_SHOW on the
-        // window to re-show it. SW_SHOW, however, will cause the OS to also set
-        // that window as the _foreground_ window, which would result in the
-        // pty's hwnd stealing the foreground away from the owning terminal
-        // window. That's bad.
-        //
-        // SetWindowLongPtr seems to do the job of changing who the window owner
-        // is, without all the other side effects of reparenting the window.
-        // See #13066
-        ::SetWindowLongPtrW(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
-    }
+    ServiceLocator::SetPseudoWindowOwner(reinterpret_cast<HWND>(data.handle));
 }
 
 // Method Description:

src/host/PtySignalInputThread.hpp

@@ -75,8 +75,5 @@ namespace Microsoft::Console
         std::optional<ResizeWindowData> _earlyResize;
         std::optional<ShowHideData> _initialShowHide;
         ConhostInternalGetSet _api;
-
-    public:
-        std::optional<SetParentData> _earlyReparent;
     };
 }

src/host/exe/Host.EXE.vcxproj

@@ -86,7 +86,7 @@
     </ClCompile>
     <Link>
       <AllowIsolation>true</AllowIsolation>
-      <AdditionalDependencies>WinMM.Lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies>winmm.lib;imm32.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
   <!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->

src/host/ft_fuzzer/Host.FuzzWrapper.vcxproj

@@ -69,15 +69,15 @@
       <AdditionalIncludeDirectories>..;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ClCompile>
     <Link>
-      <AdditionalDependencies>winmm.lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies>winmm.lib;imm32.lib;%(AdditionalDependencies)</AdditionalDependencies>
       <SubSystem>Console</SubSystem>
     </Link>
   </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)'=='Fuzzing'">
     <!-- In theory, we may want to build with a normal main() when Fuzzing is not enabled. -->
     <!-- So, let's only add the fuzzer to the link line when we're building for Fuzzing. -->
     <Link>
-      <AdditionalDependencies>WinMM.Lib;clang_rt.fuzzer_MT-$(OCClangArchitectureName).lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies>winmm.lib;imm32.lib;clang_rt.fuzzer_MT-$(OCClangArchitectureName).lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
   <!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->

src/host/outputStream.cpp

@@ -208,15 +208,22 @@ CursorType ConhostInternalGetSet::GetUserDefaultCursorStyle() const
 // - <none>
 void ConhostInternalGetSet::ShowWindow(bool showOrHide)
 {
-    auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    const auto hwnd = gci.IsInVtIoMode() ? ServiceLocator::LocatePseudoWindow() : ServiceLocator::LocateConsoleWindow()->GetWindowHandle();
+    // ConPTY is supposed to be "transparent" to the VT application. Any VT it processes is given to the terminal.
+    // As such, it must not react to this "CSI 1 t" or "CSI 2 t" sequence. That's the job of the terminal.
+    // If the terminal encounters such a sequence, it can show/hide itself and let ConPTY know via its signal API.
+    const auto window = ServiceLocator::LocateConsoleWindow();
+    if (!window)
+    {
+        return;
+    }
 
     // GH#13301 - When we send this ShowWindow message, if we send it to the
     // conhost HWND, it's going to need to get processed by the window message
     // thread before returning.
     // However, ShowWindowAsync doesn't have this problem. It'll post the
     // message to the window thread, then immediately return, so we don't have
     // to worry about deadlocking.
+    const auto hwnd = window->GetWindowHandle();
     ::ShowWindowAsync(hwnd, showOrHide ? SW_SHOWNOACTIVATE : SW_MINIMIZE);
 }
 

src/host/sources.inc

@@ -132,6 +132,7 @@ TARGETLIBS = \
     $(ONECOREUAP_EXTERNAL_SDK_LIB_PATH)\dxgi.lib \
     $(ONECOREUAP_EXTERNAL_SDK_LIB_PATH)\d3d11.lib \
     $(MODERNCORE_INTERNAL_PRIV_SDK_LIB_VPATH_L)\api-ms-win-mm-playsound-l1.lib \
+    $(MODERNCORE_INTERNAL_PRIV_SDK_LIB_VPATH_L)\ext-ms-win-imm-l1-1-0.lib \
     $(ONECORE_INTERNAL_PRIV_SDK_LIB_VPATH_L)\ext-ms-win-dwmapi-ext-l1.lib \
     $(MINCORE_INTERNAL_PRIV_SDK_LIB_VPATH_L)\ext-ms-win-gdi-dc-l1.lib \
     $(MINCORE_INTERNAL_PRIV_SDK_LIB_VPATH_L)\ext-ms-win-gdi-dc-create-l1.lib \
@@ -195,6 +196,7 @@ DELAYLOAD = \
     api-ms-win-core-com-l1.dll; \
     api-ms-win-core-registry-l2.dll; \
     api-ms-win-mm-playsound-l1.dll; \
+    ext-ms-win-imm-l1-1-0.lib; \
     api-ms-win-shcore-obsolete-l1.dll; \
     api-ms-win-shcore-scaling-l1.dll; \
     api-ms-win-shell-dataobject-l1.dll; \

src/host/ut_host/Host.UnitTests.vcxproj

@@ -87,7 +87,7 @@
       <AdditionalIncludeDirectories>..;$(SolutionDir)src\inc;$(SolutionDir)src\inc\test;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ClCompile>
     <Link>
-      <AdditionalDependencies>WinMM.Lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies>winmm.lib;imm32.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
   <!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->

src/interactivity/base/InteractivityFactory.cpp

@@ -294,7 +294,7 @@ using namespace Microsoft::Console::Interactivity;
 // - owner: the HWND that should be the initial owner of the pseudo window.
 // Return Value:
 // - STATUS_SUCCESS on success, otherwise an appropriate error.
-[[nodiscard]] NTSTATUS InteractivityFactory::CreatePseudoWindow(HWND& hwnd, const HWND owner)
+[[nodiscard]] NTSTATUS InteractivityFactory::CreatePseudoWindow(HWND& hwnd)
 {
     hwnd = nullptr;
     ApiLevel level;
@@ -310,6 +310,11 @@ using namespace Microsoft::Console::Interactivity;
             {
             case ApiLevel::Win32:
             {
+                // We don't need an "Default IME" window for ConPTY. That's the terminal's job.
+                // -1, aka DWORD_MAX, tells the function to disable it for the entire process.
+                // Must be called before creating any window.
+                ImmDisableIME(DWORD_MAX);
+
                 pseudoClass.cbSize = sizeof(WNDCLASSEXW);
                 pseudoClass.lpszClassName = PSEUDO_WINDOW_CLASS;
                 pseudoClass.lpfnWndProc = s_PseudoWindowProc;
@@ -330,19 +335,15 @@ using namespace Microsoft::Console::Interactivity;
                 // will return the console handle again, not the owning
                 // terminal's handle. It's not entirely clear why, but WS_POPUP
                 // is absolutely vital for this to work correctly.
-                const auto windowStyle = WS_OVERLAPPEDWINDOW | WS_POPUP;
-                const auto exStyles = WS_EX_TOOLWINDOW | WS_EX_TRANSPARENT | WS_EX_LAYERED | WS_EX_NOACTIVATE;
-
-                // Attempt to create window.
-                hwnd = CreateWindowExW(exStyles,
+                hwnd = CreateWindowExW(WS_EX_TOOLWINDOW | WS_EX_NOACTIVATE,
                                        reinterpret_cast<LPCWSTR>(windowClassAtom),
                                        nullptr,
-                                       windowStyle,
+                                       WS_POPUP,
                                        0,
                                        0,
                                        0,
                                        0,
-                                       owner,
+                                       _owner.load(std::memory_order_relaxed),
                                        nullptr,
                                        nullptr,
                                        this);
@@ -375,6 +376,38 @@ using namespace Microsoft::Console::Interactivity;
     return status;
 }
 
+void InteractivityFactory::SetOwner(HWND owner) noexcept
+{
+    _owner.store(owner, std::memory_order_relaxed);
+
+    if (_pseudoConsoleWindowHwnd)
+    {
+        // DO NOT USE SetParent HERE!
+        //
+        // Calling SetParent on a window that is WS_VISIBLE will cause the OS to
+        // hide the window, make it a _child_ window, then call SW_SHOW on the
+        // window to re-show it. SW_SHOW, however, will cause the OS to also set
+        // that window as the _foreground_ window, which would result in the
+        // pty's hwnd stealing the foreground away from the owning terminal
+        // window. That's bad.
+        //
+        // SetWindowLongPtr seems to do the job of changing who the window owner
+        // is, without all the other side effects of reparenting the window.
+        // See #13066
+        ::SetWindowLongPtrW(_pseudoConsoleWindowHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
+    }
+}
+
+void InteractivityFactory::SetVisibility(const bool isVisible) noexcept
+{
+    if (_pseudoConsoleWindowHwnd && IsIconic(_pseudoConsoleWindowHwnd) != static_cast<BOOL>(isVisible))
+    {
+        _suppressVisibilityChange.store(true, std::memory_order_relaxed);
+        ShowWindow(_pseudoConsoleWindowHwnd, isVisible ? SW_SHOWNOACTIVATE : SW_MINIMIZE);
+        _suppressVisibilityChange.store(false, std::memory_order_relaxed);
+    }
+}
+
 // Method Description:
 // - Static window procedure for pseudo console windows
 // - Processes set-up on create to stow the "this" pointer to specific instantiations and routes
@@ -446,7 +479,7 @@ using namespace Microsoft::Console::Interactivity;
         {
             _WritePseudoWindowCallback(false);
         }
-        break;
+        return 0;
     }
     // case WM_WINDOWPOSCHANGING:
     //     As long as user32 didn't eat the `ShowWindow` call because the window state requested
@@ -479,12 +512,12 @@ using namespace Microsoft::Console::Interactivity;
     }
     case WM_ACTIVATE:
     {
-        if (const auto ownerHwnd{ ::GetAncestor(hWnd, GA_ROOTOWNER) })
+        if (const auto ownerHwnd = _owner.load(std::memory_order_relaxed))
         {
             SetFocus(ownerHwnd);
             return 0;
         }
-        break;
+        return 0;
     }
     }
     // If we get this far, call the default window proc
@@ -501,6 +534,11 @@ using namespace Microsoft::Console::Interactivity;
 // - <none>
 void InteractivityFactory::_WritePseudoWindowCallback(bool showOrHide)
 {
+    if (_suppressVisibilityChange.load(std::memory_order_relaxed))
+    {
+        return;
+    }
+
     // IMPORTANT!
     //
     // A hosting terminal window should only "restore" itself in response to

src/interactivity/base/InteractivityFactory.hpp

@@ -27,7 +27,10 @@ namespace Microsoft::Console::Interactivity
         [[nodiscard]] NTSTATUS CreateAccessibilityNotifier(_Inout_ std::unique_ptr<IAccessibilityNotifier>& notifier);
         [[nodiscard]] NTSTATUS CreateSystemConfigurationProvider(_Inout_ std::unique_ptr<ISystemConfigurationProvider>& provider);
 
-        [[nodiscard]] NTSTATUS CreatePseudoWindow(HWND& hwnd, const HWND owner);
+        [[nodiscard]] NTSTATUS CreatePseudoWindow(HWND& hwnd);
+
+        void SetOwner(HWND owner) noexcept;
+        void SetVisibility(const bool isVisible) noexcept;
 
         // Wndproc
         [[nodiscard]] static LRESULT CALLBACK s_PseudoWindowProc(_In_ HWND hwnd,
@@ -43,6 +46,8 @@ namespace Microsoft::Console::Interactivity
         void _WritePseudoWindowCallback(bool showOrHide);
 
         HWND _pseudoConsoleWindowHwnd{ nullptr };
+        std::atomic<HWND> _owner{ HWND_DESKTOP };
+        std::atomic<bool> _suppressVisibilityChange{ false };
         WRL::ComPtr<PseudoConsoleWindowAccessibilityProvider> _pPseudoConsoleUiaProvider{ nullptr };
     };
 }