Avoid focus loops in ConPTY #17829

lhecker · 2024-08-29T23:16:03Z

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:

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 ✅

lhecker · 2024-08-29T23:16:22Z

src/cascadia/TerminalControl/ControlCore.cpp

+        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);
+                }
+            });


Avoid emitting multiple such sequences when rapidly un-/focusing the window.

lhecker · 2024-08-29T23:20:07Z

src/host/PtySignalInputThread.cpp

-        // See #13066
-        ::SetWindowLongPtrW(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
-    }
+    ServiceLocator::SetPseudoWindowOwner(reinterpret_cast<HWND>(data.handle));


The Azure PowerShell extension crashes the PowerShell process hard (not even an exception or anything) if you give it a HWND_MESSAGE. But my entire concept was based around that! That's why I moved all of the parent/owner code to InteractivityFactory too (below in this PR): A HWND_MESSAGE window cannot have a custom parent like this and so I had to store the owner in a member. I felt like InteractivityFactory should store it.

I left these changes in, because it IMO still makes the code easier to maintain: Now all of the parent/owner code is centralized in a single file.

lhecker · 2024-08-29T23:20:34Z

src/host/outputStream.cpp

+    // 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;
+    }


Avoid emitting another de-/iconify VT sequence when we encounter a (de)iconify VT sequence during parsing.

lhecker · 2024-08-29T23:22:19Z

src/interactivity/base/InteractivityFactory.cpp

                                       reinterpret_cast<LPCWSTR>(windowClassAtom),
                                       nullptr,
-                                       windowStyle,
+                                       WS_POPUP,


No WS_OVERLAPPEDWINDOW = no DWM, no border, no size. Makes it cheaper and easier. Seems to still work too.

Wonder if this fixes the "openconsole has a 0x0 window at 0x0" problem

lhecker · 2024-08-29T23:22:35Z

src/interactivity/base/InteractivityFactory.cpp

+                // 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);


This avoids spawning the entire IME machinery inside ConPTY.

Oh, this answers my question about imm32.

For sources, can you instead link ext-ms-win-imm-l1-1-0? It is an extension APIset and can be delay-loaded.

That should be sufficient.
No need to change the link line for the vcxproj.

lhecker · 2024-08-29T23:26:17Z

src/interactivity/base/InteractivityFactory.cpp

+    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);
+    }


Avoid emitting a de-/iconify VT sequence when a focus event is received on the signal pipe.

lhecker · 2024-08-29T23:33:09Z

src/terminal/adapter/InteractDispatch.cpp

+        WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, focused);
+        gci.ProcessHandleList.ModifyConsoleProcessFocus(focused);


This is the security related change I mentioned.

lhecker · 2024-08-29T23:33:59Z

src/interactivity/base/InteractivityFactory.cpp

@@ -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))


The benefit of this change is that we now assign focus to the exact HWND that we were told is our owning window and not any window up the GA_ROOTOWNER chain.

Oh, this is great

carlos-zamora

Nice work!

DHowett · 2024-09-04T22:44:35Z

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>


Which API do we need from this? If it is part of an APIset, we should prefer apiset linkage. We may also need to update sources.

DHowett · 2024-09-04T22:50:22Z

src/interactivity/base/InteractivityFactory.cpp

+                // 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);


Oh, this answers my question about imm32.

For sources, can you instead link ext-ms-win-imm-l1-1-0? It is an extension APIset and can be delay-loaded.

That should be sufficient.
No need to change the link line for the vcxproj.

DHowett · 2024-09-04T22:51:26Z

src/interactivity/base/InteractivityFactory.cpp

                                       reinterpret_cast<LPCWSTR>(windowClassAtom),
                                       nullptr,
-                                       windowStyle,
+                                       WS_POPUP,


Wonder if this fixes the "openconsole has a 0x0 window at 0x0" problem

DHowett · 2024-09-04T22:52:33Z

src/interactivity/base/InteractivityFactory.cpp

+    if (_pseudoConsoleWindowHwnd && IsIconic(_pseudoConsoleWindowHwnd) != static_cast<BOOL>(isVisible))
+    {
+        _suppressVisibilityChange.store(true, std::memory_order_relaxed);
+        ShowWindow(_pseudoConsoleWindowHwnd, isVisible ? SW_SHOWNOACTIVATE : SW_MINIMIZE);


Are we certain that this is processed in order on all versions of Windows we care about?

Alternatively... should we do the management of suppress in a custom window message?

DHowett · 2024-09-04T22:52:52Z

src/interactivity/base/InteractivityFactory.cpp

@@ -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))


Oh, this is great

lhecker added Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. labels Aug 29, 2024

Avoid focus loops in ConPTY

a3c12bf

lhecker force-pushed the dev/lhecker/conpty-message-window branch from 16e54e9 to a3c12bf Compare August 29, 2024 23:18

lhecker commented Aug 29, 2024

View reviewed changes

lhecker mentioned this pull request Aug 30, 2024

Cannot type in new pane or tab until focus is manually activated with mouse click #13388

Open

carlos-zamora approved these changes Sep 3, 2024

View reviewed changes

DHowett approved these changes Sep 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid focus loops in ConPTY #17829

Avoid focus loops in ConPTY #17829

lhecker commented Aug 29, 2024 •

edited

Loading

lhecker Aug 29, 2024

lhecker Aug 29, 2024

lhecker Aug 29, 2024

lhecker Aug 29, 2024

DHowett Sep 4, 2024

lhecker Aug 29, 2024

DHowett Sep 4, 2024

lhecker Aug 29, 2024

lhecker Aug 29, 2024

lhecker Aug 29, 2024

DHowett Sep 4, 2024

carlos-zamora left a comment

DHowett Sep 4, 2024

DHowett Sep 4, 2024

DHowett Sep 4, 2024

DHowett Sep 4, 2024

DHowett Sep 4, 2024

		WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, focused);
		gci.ProcessHandleList.ModifyConsoleProcessFocus(focused);

Avoid focus loops in ConPTY #17829

Are you sure you want to change the base?

Avoid focus loops in ConPTY #17829

Conversation

lhecker commented Aug 29, 2024 • edited Loading

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Aug 29, 2024 •

edited

Loading