[FancyZones] Fix deadlocks in ZoneWindowDrawing (#10461)

* Fixed deadlocks in ZoneWindowDrawing

Moved all possibly reentrant or blocking calls to ShowWindow out of critical sections.

* Initialize bools

* Tune flashing visuals

* Address PR comments

* Use  = true; to initialize bools
* Remove tracing from GetAnimationAlpha
* Use member initialization when constructing AnimationInfo

* Refactor rendering

* Whitespace

* Hide window on render failure

src/modules/fancyzones/lib/FancyZones.cpp

@@ -647,7 +647,7 @@ void FancyZones::ToggleEditor() noexcept
     wil::unique_cotaskmem_string virtualDesktopId;
     if (!SUCCEEDED(StringFromCLSID(m_currentDesktopId, &virtualDesktopId)))
     {
-        return;   
+        return;
     }
 
     /*
@@ -677,7 +677,7 @@ void FancyZones::ToggleEditor() noexcept
     {
         params += FancyZonesUtils::GenerateUniqueIdAllMonitorsArea(virtualDesktopId.get()) + divider; /* Monitor id where the Editor should be opened */
     }
-   
+
     // device id map
     std::unordered_map<std::wstring, DWORD> displayDeviceIdxMap;
 

src/modules/fancyzones/lib/ZoneWindow.cpp

@@ -163,8 +163,6 @@ private:
     std::vector<size_t> m_highlightZone;
     WPARAM m_keyLast{};
     size_t m_keyCycle{};
-    static const UINT m_showAnimationDuration = 200; // ms
-    static const UINT m_flashDuration = 1000; // ms
     std::unique_ptr<ZoneWindowDrawing> m_zoneWindowDrawing;
 };
 
@@ -384,7 +382,7 @@ ZoneWindow::ShowZoneWindow() noexcept
     {
         SetAsTopmostWindow();
         m_zoneWindowDrawing->DrawActiveZoneSet(m_activeZoneSet->GetZones(), m_highlightZone, m_host);
-        m_zoneWindowDrawing->Show(m_showAnimationDuration);
+        m_zoneWindowDrawing->Show();
     }
 }
 
@@ -428,7 +426,7 @@ ZoneWindow::FlashZones() noexcept
     {
         SetAsTopmostWindow();
         m_zoneWindowDrawing->DrawActiveZoneSet(m_activeZoneSet->GetZones(), {}, m_host);
-        m_zoneWindowDrawing->Flash(m_flashDuration);
+        m_zoneWindowDrawing->Flash();
     }
 }
 

src/modules/fancyzones/lib/ZoneWindowDrawing.cpp

@@ -9,6 +9,12 @@
 
 #include <common/logger/logger.h>
 
+namespace
+{
+    const int FadeInDurationMillis = 200;
+    const int FlashZonesDurationMillis = 700;
+}
+
 namespace NonLocalizable
 {
     const wchar_t SegoeUiFont[] = L"Segoe ui";
@@ -16,25 +22,23 @@ namespace NonLocalizable
 
 float ZoneWindowDrawing::GetAnimationAlpha()
 {
-    // Lock is being held
+    // Lock is held by the caller
+
     if (!m_animation)
     {
         return 0.f;
     }
 
     auto tNow = std::chrono::steady_clock().now();
-    auto alpha = (tNow - m_animation->tStart).count() / (1e6f * m_animation->duration);
+    auto millis = (tNow - m_animation->tStart).count() / 1e6f;
 
-    if (m_animation->fadeIn)
+    if (m_animation->autoHide && millis > FlashZonesDurationMillis)
     {
-        // Return a positive value to avoid hiding
+        return 0.f;
-        return std::clamp(alpha, 0.001f, 1.f);
-    }
-    else
-    {
-        // Quadratic function looks better
-        return std::clamp(1.f - alpha * alpha, 0.f, 1.f);
     }
+
+    // Return a positive value to avoid hiding
+    return std::clamp(millis / FadeInDurationMillis, 0.001f, 1.f);
 }
 
 ID2D1Factory* ZoneWindowDrawing::GetD2DFactory()
@@ -91,7 +95,7 @@ ZoneWindowDrawing::ZoneWindowDrawing(HWND window)
         D2D1::PixelFormat(DXGI_FORMAT_UNKNOWN, D2D1_ALPHA_MODE_PREMULTIPLIED),
         96.f,
         96.f);
-    
+
     auto renderTargetSize = D2D1::SizeU(m_clientRect.right - m_clientRect.left, m_clientRect.bottom - m_clientRect.top);
     auto hwndRenderTargetProperties = D2D1::HwndRenderTargetProperties(window, renderTargetSize);
 
@@ -106,20 +110,20 @@ ZoneWindowDrawing::ZoneWindowDrawing(HWND window)
     m_renderThread = std::thread([this]() { RenderLoop(); });
 }
 
-void ZoneWindowDrawing::Render()
+ZoneWindowDrawing::RenderResult ZoneWindowDrawing::Render()
 {
     std::unique_lock lock(m_mutex);
 
     if (!m_renderTarget)
     {
-        return;
+        return RenderResult::Failed;
     }
 
     float animationAlpha = GetAnimationAlpha();
 
     if (animationAlpha <= 0.f)
     {
-        return;
+        return RenderResult::AnimationEnded;
     }
 
     m_renderTarget->BeginDraw();
@@ -186,91 +190,96 @@ void ZoneWindowDrawing::Render()
     lock.unlock();
 
     m_renderTarget->EndDraw();
+    return RenderResult::Ok;
 }
 
 void ZoneWindowDrawing::RenderLoop()
 {
     while (!m_abortThread)
     {
-        float animationAlpha;
-        {
-            // The lock must be held by the caller when calling GetAnimationAlpha
-            std::unique_lock lock(m_mutex);
-            animationAlpha = GetAnimationAlpha();
-        }
-
-        // Check whether the animation expired and we need to hide the window
-        if (animationAlpha <= 0.f)
-        {
-            Hide();
-        }
-
         {
             // Wait here while rendering is disabled
             std::unique_lock lock(m_mutex);
             m_cv.wait(lock, [this]() { return (bool)m_shouldRender; });
         }
-        
+
-        Render();
+        auto result = Render();
+
+        if (result == RenderResult::AnimationEnded || result == RenderResult::Failed)
+        {
+            Hide();
+        }
     }
 }
 
 void ZoneWindowDrawing::Hide()
 {
     _TRACER_;
-    std::unique_lock lock(m_mutex);
+    bool shouldHideWindow = true;
-
-    m_animation.reset();
-
-    if (m_shouldRender)
     {
+        std::unique_lock lock(m_mutex);
+        m_animation.reset();
+        shouldHideWindow = m_shouldRender;
         m_shouldRender = false;
+    }
+
+    if (shouldHideWindow)
+    {
         ShowWindow(m_window, SW_HIDE);
     }
 }
 
-void ZoneWindowDrawing::Show(unsigned animationMillis)
+void ZoneWindowDrawing::Show()
 {
     _TRACER_;
-    std::unique_lock lock(m_mutex);
+    bool shouldShowWindow = true;
-    
+    {
-    if (!m_shouldRender)
+        std::unique_lock lock(m_mutex);
+        shouldShowWindow = !m_shouldRender;
+        m_shouldRender = true;
+
+        if (!m_animation)
+        {
+            m_animation.emplace(AnimationInfo{ .tStart = std::chrono::steady_clock().now(), .autoHide = false });
+        }
+        else if (m_animation->autoHide)
+        {
+            // Do not change the starting time of the animation, just reset autoHide
+            m_animation->autoHide = false;
+        }
+    }
+
+    if (shouldShowWindow)
     {
         ShowWindow(m_window, SW_SHOWNA);
     }
 
-    animationMillis = max(animationMillis, 1);
-
-    if (!m_animation || !m_animation->fadeIn)
-    {
-        m_animation.emplace(AnimationInfo{ std::chrono::steady_clock().now(), animationMillis, true });
-    }
-
-    m_shouldRender = true;
     m_cv.notify_all();
 }
 
-void ZoneWindowDrawing::Flash(unsigned animationMillis)
+void ZoneWindowDrawing::Flash()
 {
     _TRACER_;
-    std::unique_lock lock(m_mutex);
+    bool shouldShowWindow = true;
+    {
+        std::unique_lock lock(m_mutex);
+        shouldShowWindow = !m_shouldRender;
+        m_shouldRender = true;
 
-    if (!m_shouldRender)
+        m_animation.emplace(AnimationInfo{ .tStart = std::chrono::steady_clock().now(), .autoHide = true });
+    }
+
+    if (shouldShowWindow)
     {
         ShowWindow(m_window, SW_SHOWNA);
     }
 
-    animationMillis = max(animationMillis, 1);
-
-    m_animation.emplace(AnimationInfo{ std::chrono::steady_clock().now(), animationMillis, false });
-
-    m_shouldRender = true;
     m_cv.notify_all();
 }
 
 void ZoneWindowDrawing::DrawActiveZoneSet(const IZoneSet::ZonesMap& zones,
-                       const std::vector<size_t>& highlightZones,
+                                          const std::vector<size_t>& highlightZones,
-                       winrt::com_ptr<IZoneWindowHost> host)
+                                          winrt::com_ptr<IZoneWindowHost> host)
 {
     _TRACER_;
     std::unique_lock lock(m_mutex);

src/modules/fancyzones/lib/ZoneWindowDrawing.h

@@ -25,8 +25,14 @@ class ZoneWindowDrawing
     struct AnimationInfo
     {
         std::chrono::steady_clock::time_point tStart;
-        unsigned duration;
+        bool autoHide;
-        bool fadeIn;
+    };
+
+    enum struct RenderResult
+    {
+        Ok,
+        AnimationEnded,
+        Failed,
     };
 
     HWND m_window = nullptr;
@@ -42,7 +48,7 @@ class ZoneWindowDrawing
     static IDWriteFactory* GetWriteFactory();
     static D2D1_COLOR_F ConvertColor(COLORREF color);
     static D2D1_RECT_F ConvertRect(RECT rect);
-    void Render();
+    RenderResult Render();
     void RenderLoop();
 
     std::atomic<bool> m_shouldRender = false;
@@ -55,8 +61,8 @@ public:
     ~ZoneWindowDrawing();
     ZoneWindowDrawing(HWND window);
     void Hide();
-    void Show(unsigned animationMillis);
+    void Show();
-    void Flash(unsigned animationMillis);
+    void Flash();
     void DrawActiveZoneSet(const IZoneSet::ZonesMap& zones,
                            const std::vector<size_t>& highlightZones,
                            winrt::com_ptr<IZoneWindowHost> host);