mirror of
https://github.com/microsoft/terminal.git
synced 2026-02-07 21:25:41 +00:00
Fix Peasant::ActivateWindow being called with an all 0 GUID (#15187)
`WM_ACTIVATE` is sent on window creation, whereas `WM_SHOWWINDOW` is
sent when the window is shown. Before we call `Peasant::ActivateWindow`
in the `WM_ACTIVATE` handler, we try to get the virtual desktop GUID of
our window, but since it's not shown yet during startup, there's also
no GUID that can be retrieved. This results in an error log message and
an all 0 GUID to be sent via `Peasant::ActivateWindow`.
The GUID of the window that actually spawned on the other hand is never
reported until the first time you reactivate it again, leading to a
number of subtle bugs around window activity.
Additionally, this commit fixes a race condition and pointer unsafety,
by pulling all relevant member variables onto the coroutine's stack,
before it yields itself to a background thread.
## Validation Steps Performed
- Set a trace breakpoint on `_peasantNotifyActivateWindow`
- GUID is non-zero ✅
This commit is contained in:
@@ -32,11 +32,11 @@ AppHost::AppHost(const winrt::TerminalApp::AppLogic& logic,
|
||||
winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
|
||||
const Remoting::WindowManager& manager,
|
||||
const Remoting::Peasant& peasant) noexcept :
|
||||
_appLogic{ logic },
|
||||
_windowLogic{ nullptr }, // don't make one, we're going to take a ref on app's
|
||||
_windowManager{ manager },
|
||||
_peasant{ peasant },
|
||||
_appLogic{ logic }, // don't make one, we're going to take a ref on app's
|
||||
_windowLogic{ nullptr },
|
||||
_window{ nullptr }
|
||||
_desktopManager{ winrt::try_create_instance<IVirtualDesktopManager>(__uuidof(VirtualDesktopManager)) }
|
||||
{
|
||||
_HandleCommandlineArgs(args);
|
||||
|
||||
@@ -851,30 +851,39 @@ void AppHost::_DispatchCommandline(winrt::Windows::Foundation::IInspectable send
|
||||
_windowLogic.ExecuteCommandline(args.Commandline(), args.CurrentDirectory());
|
||||
}
|
||||
|
||||
winrt::fire_and_forget AppHost::_WindowActivated(bool activated)
|
||||
void AppHost::_WindowActivated(bool activated)
|
||||
{
|
||||
_windowLogic.WindowActivated(activated);
|
||||
|
||||
if (!activated)
|
||||
if (activated && _isWindowInitialized)
|
||||
{
|
||||
_peasantNotifyActivateWindow();
|
||||
}
|
||||
}
|
||||
|
||||
winrt::fire_and_forget AppHost::_peasantNotifyActivateWindow()
|
||||
{
|
||||
const auto desktopManager = _desktopManager;
|
||||
const auto peasant = _peasant;
|
||||
const auto hwnd = _window->GetHandle();
|
||||
|
||||
co_await winrt::resume_background();
|
||||
|
||||
GUID currentDesktopGuid{};
|
||||
if (FAILED_LOG(desktopManager->GetWindowDesktopId(hwnd, ¤tDesktopGuid)))
|
||||
{
|
||||
co_return;
|
||||
}
|
||||
|
||||
co_await winrt::resume_background();
|
||||
|
||||
if (_peasant)
|
||||
{
|
||||
const auto currentDesktopGuid{ _CurrentDesktopGuid() };
|
||||
|
||||
// TODO: projects/5 - in the future, we'll want to actually get the
|
||||
// desktop GUID in IslandWindow, and bubble that up here, then down to
|
||||
// the Peasant. For now, we're just leaving space for it.
|
||||
Remoting::WindowActivatedArgs args{ _peasant.GetID(),
|
||||
(uint64_t)_window->GetHandle(),
|
||||
currentDesktopGuid,
|
||||
winrt::clock().now() };
|
||||
_peasant.ActivateWindow(args);
|
||||
}
|
||||
// TODO: projects/5 - in the future, we'll want to actually get the
|
||||
// desktop GUID in IslandWindow, and bubble that up here, then down to
|
||||
// the Peasant. For now, we're just leaving space for it.
|
||||
peasant.ActivateWindow({
|
||||
peasant.GetID(),
|
||||
reinterpret_cast<uint64_t>(hwnd),
|
||||
currentDesktopGuid,
|
||||
winrt::clock().now(),
|
||||
});
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
@@ -907,30 +916,6 @@ winrt::Windows::Foundation::IAsyncOperation<winrt::hstring> AppHost::_GetWindowL
|
||||
co_return layoutJson;
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
// - Helper to initialize our instance of IVirtualDesktopManager. If we already
|
||||
// got one, then this will just return true. Otherwise, we'll try and init a
|
||||
// new instance of one, and store that.
|
||||
// - This will return false if we weren't able to initialize one, which I'm not
|
||||
// sure is actually possible.
|
||||
// Arguments:
|
||||
// - <none>
|
||||
// Return Value:
|
||||
// - true iff _desktopManager points to a non-null instance of IVirtualDesktopManager
|
||||
bool AppHost::_LazyLoadDesktopManager()
|
||||
{
|
||||
if (_desktopManager == nullptr)
|
||||
{
|
||||
try
|
||||
{
|
||||
_desktopManager = winrt::create_instance<IVirtualDesktopManager>(__uuidof(VirtualDesktopManager));
|
||||
}
|
||||
CATCH_LOG();
|
||||
}
|
||||
|
||||
return _desktopManager != nullptr;
|
||||
}
|
||||
|
||||
void AppHost::_HandleSummon(const winrt::Windows::Foundation::IInspectable& /*sender*/,
|
||||
const Remoting::SummonWindowBehavior& args)
|
||||
{
|
||||
@@ -938,7 +923,7 @@ void AppHost::_HandleSummon(const winrt::Windows::Foundation::IInspectable& /*se
|
||||
|
||||
if (args != nullptr && args.MoveToCurrentDesktop())
|
||||
{
|
||||
if (_LazyLoadDesktopManager())
|
||||
if (_desktopManager)
|
||||
{
|
||||
// First thing - make sure that we're not on the current desktop. If
|
||||
// we are, then don't call MoveWindowToDesktop. This is to mitigate
|
||||
@@ -966,23 +951,6 @@ void AppHost::_HandleSummon(const winrt::Windows::Foundation::IInspectable& /*se
|
||||
}
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
// - This gets the GUID of the desktop our window is currently on. It does NOT
|
||||
// get the GUID of the desktop that's currently active.
|
||||
// Arguments:
|
||||
// - <none>
|
||||
// Return Value:
|
||||
// - the GUID of the desktop our window is currently on
|
||||
GUID AppHost::_CurrentDesktopGuid()
|
||||
{
|
||||
GUID currentDesktopGuid{ 0 };
|
||||
if (_LazyLoadDesktopManager())
|
||||
{
|
||||
LOG_IF_FAILED(_desktopManager->GetWindowDesktopId(_window->GetHandle(), ¤tDesktopGuid));
|
||||
}
|
||||
return currentDesktopGuid;
|
||||
}
|
||||
|
||||
// Method Description:
|
||||
// - Called when this window wants _all_ windows to display their
|
||||
// identification. We'll hop to the BG thread, and raise an event (eventually
|
||||
@@ -1264,8 +1232,9 @@ winrt::fire_and_forget AppHost::_WindowInitializedHandler(const winrt::Windows::
|
||||
// UI thread. This is shockingly load bearing - without this, then
|
||||
// sometimes, we'll _still_ show the HWND before the XAML island actually
|
||||
// paints.
|
||||
co_await winrt::resume_background();
|
||||
co_await wil::resume_foreground(_windowLogic.GetRoot().Dispatcher(), winrt::Windows::UI::Core::CoreDispatcherPriority::Low);
|
||||
|
||||
_isWindowInitialized = true;
|
||||
ShowWindow(_window->GetHandle(), nCmdShow);
|
||||
|
||||
// If we didn't start the window hidden (in one way or another), then try to
|
||||
@@ -1280,6 +1249,7 @@ winrt::fire_and_forget AppHost::_WindowInitializedHandler(const winrt::Windows::
|
||||
if (!noForeground)
|
||||
{
|
||||
SetForegroundWindow(_window->GetHandle());
|
||||
_peasantNotifyActivateWindow();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -13,7 +13,7 @@ public:
|
||||
winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs args,
|
||||
const winrt::Microsoft::Terminal::Remoting::WindowManager& manager,
|
||||
const winrt::Microsoft::Terminal::Remoting::Peasant& peasant) noexcept;
|
||||
virtual ~AppHost();
|
||||
~AppHost();
|
||||
|
||||
void AppTitleChanged(const winrt::Windows::Foundation::IInspectable& sender, winrt::hstring newTitle);
|
||||
void LastTabClosed(const winrt::Windows::Foundation::IInspectable& sender, const winrt::TerminalApp::LastTabClosedEventArgs& args);
|
||||
@@ -37,7 +37,7 @@ private:
|
||||
winrt::Microsoft::Terminal::Remoting::Peasant _peasant{ nullptr };
|
||||
|
||||
winrt::com_ptr<IVirtualDesktopManager> _desktopManager{ nullptr };
|
||||
|
||||
bool _isWindowInitialized = false;
|
||||
bool _useNonClientArea{ false };
|
||||
winrt::Microsoft::Terminal::Settings::Model::LaunchMode _launchMode{};
|
||||
|
||||
@@ -70,7 +70,8 @@ private:
|
||||
void _RaiseVisualBell(const winrt::Windows::Foundation::IInspectable& sender,
|
||||
const winrt::Windows::Foundation::IInspectable& arg);
|
||||
void _WindowMouseWheeled(const til::point coord, const int32_t delta);
|
||||
winrt::fire_and_forget _WindowActivated(bool activated);
|
||||
void _WindowActivated(bool activated);
|
||||
winrt::fire_and_forget _peasantNotifyActivateWindow();
|
||||
void _WindowMoved();
|
||||
|
||||
void _DispatchCommandline(winrt::Windows::Foundation::IInspectable sender,
|
||||
|
||||
Reference in New Issue
Block a user