bugfix: the animation system no longer crashes due to bad accesses on…

… the miral::Window (#352)
miracle-wm-org · Jan 22, 2025 · 16871f9 · 16871f9
1 parent 4ef1b06
commit 16871f9
Show file tree

Hide file tree

Showing 12 changed files with 123 additions and 49 deletions.
diff --git a/src/animator.cpp b/src/animator.cpp
@@ -15,13 +15,14 @@ You should have received a copy of the GNU General Public License
 along with this program.  If not, see <http://www.gnu.org/licenses/>.
 **/
 
-#include "animator.h"
-#include <chrono>
-#include <mir/server_action_queue.h>
 #define MIR_LOG_COMPONENT "animator"
-#include <mir/log.h>
 #define GLM_ENABLE_EXPERIMENTAL
+
+#include "animator.h"
+#include <chrono>
 #include <glm/gtx/transform.hpp>
+#include <mir/log.h>
+#include <mir/server_action_queue.h>
 #include <utility>
 
 using namespace miracle;
@@ -408,12 +409,12 @@ AnimationHandle Animator::register_animateable()
 void Animator::append(std::shared_ptr<Animation> const& animation)
 {
     std::lock_guard<std::mutex> lock(processing_lock);
-    for (auto& other : queued_animations)
+    for (auto& other : active)
     {
         if (other->get_handle() == animation->get_handle())
             other->mark_for_great_animator_in_the_sky();
     }
-    queued_animations.push_back(animation);
+    active.push_back(animation);
     animation->on_tick(animation->init());
     cv.notify_one();
 }
@@ -422,7 +423,7 @@ void Animator::tick(float dt)
 {
     std::lock_guard<std::mutex> lock(processing_lock);
 
-    for (auto& item : queued_animations)
+    for (auto& item : active)
     {
         if (item->is_going_to_great_animator_in_the_sky())
             continue;
@@ -435,18 +436,18 @@ void Animator::tick(float dt)
             item->mark_for_great_animator_in_the_sky();
     }
 
-    queued_animations.erase(std::remove_if(queued_animations.begin(), queued_animations.end(), [](std::shared_ptr<Animation> const& animation)
+    active.erase(std::remove_if(active.begin(), active.end(), [](std::shared_ptr<Animation> const& animation)
     {
         return animation->is_going_to_great_animator_in_the_sky();
     }),
-        queued_animations.end());
+        active.end());
 }
 
 void Animator::set_size_hack(AnimationHandle handle, mir::geometry::Size const& size)
 {
     std::lock_guard<std::mutex> lock(processing_lock);
 
-    for (auto& animation : queued_animations)
+    for (auto& animation : active)
     {
         if (animation->get_handle() == handle)
             animation->set_current_size(size);
@@ -456,9 +457,9 @@ void Animator::set_size_hack(AnimationHandle handle, mir::geometry::Size const&
 void Animator::remove_by_animation_handle(miracle::AnimationHandle handle)
 {
     std::lock_guard<std::mutex> lock(processing_lock);
-    for (auto& animation : queued_animations)
+    for (auto& animation : active)
     {
         if (animation->get_handle() == handle)
             animation->mark_for_great_animator_in_the_sky();
     }
-}
+}
diff --git a/src/animator.h b/src/animator.h
@@ -102,12 +102,12 @@ class Animator
     void append(std::shared_ptr<Animation> const&);
     void set_size_hack(AnimationHandle handle, mir::geometry::Size const& size);
     void remove_by_animation_handle(AnimationHandle handle);
-    bool has_animations() const { return !queued_animations.empty(); }
+    bool has_animations() const { return !active.empty(); }
     std::condition_variable& get_cv() { return cv; }
     std::mutex& get_lock() { return processing_lock; }
 
 private:
-    std::vector<std::shared_ptr<Animation>> queued_animations;
+    std::vector<std::shared_ptr<Animation>> active;
     std::thread run_thread;
     std::condition_variable cv;
     std::mutex processing_lock;

diff --git a/src/output.cpp b/src/output.cpp
@@ -59,6 +59,11 @@ MiralWrapperOutput::MiralWrapperOutput(
 {
 }
 
+MiralWrapperOutput::~MiralWrapperOutput()
+{
+    animator.remove_by_animation_handle(handle);
+}
+
 Workspace* MiralWrapperOutput::active() const
 {
     if (active_workspace.expired())

diff --git a/src/output.h b/src/output.h
@@ -106,7 +106,7 @@ class MiralWrapperOutput : public Output
         std::shared_ptr<Config> const& options,
         WindowController&,
         Animator&);
-    ~MiralWrapperOutput() = default;
+    ~MiralWrapperOutput();
 
     std::shared_ptr<Container> intersect(float x, float y) override;
     AllocationHint allocate_position(

diff --git a/src/parent_container.cpp b/src/parent_container.cpp
@@ -228,8 +228,8 @@ std::shared_ptr<LeafContainer> ParentContainer::confirm_window(miral::Window con
 {
     if (pending_node == nullptr)
     {
-        mir::fatal_error("Unable to add the window to the scene. Was create_space_for_window called?");
-        return nullptr;
+        mir::log_error("confirm_window: create_space_for_window wasn't called, so we will call it, but this is odd!");
+        pending_node = create_space_for_window(-1);
     }
 
     auto retval = pending_node;

diff --git a/src/policy.cpp b/src/policy.cpp
@@ -117,7 +117,7 @@ Policy::Policy(
     floating_window_manager(std::make_unique<MinimalWindowManager>(tools, config)),
     animator_loop(std::make_unique<ThreadedAnimatorLoop>(animator)),
     workspace_manager(workspace_observer_registrar, config, state),
-    window_controller(tools, *animator, state, config, server.the_main_loop()),
+    window_controller(tools, *animator, state, config, server.the_main_loop(), this),
     scratchpad_(window_controller, state),
     self(std::make_shared<Self>(*this)),
     command_controller(
@@ -687,6 +687,21 @@ void Policy::handle_request_resize(
     container->handle_request_resize(input_event, edge);
 }
 
+void Policy::handle_animation(
+    AnimationStepResult const& asr,
+    std::weak_ptr<Container> const& container)
+{
+    std::lock_guard lock(self->mutex);
+    auto sh_container = container.lock();
+    if (!sh_container)
+    {
+        mir::log_error("handle_animation: container is invalid");
+        return;
+    }
+
+    window_controller.process_animation(asr, sh_container);
+}
+
 mir::geometry::Rectangle Policy::confirm_inherited_move(
     const miral::WindowInfo& window_info,
     mir::geometry::Displacement movement)

diff --git a/src/policy.h b/src/policy.h
@@ -91,6 +91,9 @@ class Policy : public miral::WindowManagementPolicy
         miral::WindowInfo& window_info,
         const MirInputEvent* input_event,
         MirResizeEdge edge) override;
+    void handle_animation(
+        AnimationStepResult const& asr,
+        std::weak_ptr<Container> const& container);
     auto confirm_inherited_move(
         const miral::WindowInfo& window_info,
         mir::geometry::Displacement movement) -> mir::geometry::Rectangle override;

diff --git a/src/window_controller.h b/src/window_controller.h
@@ -58,6 +58,7 @@ class WindowController
     virtual void move_cursor_to(float x, float y) = 0;
     virtual void set_size_hack(AnimationHandle handle, geom::Size const& size) = 0;
     virtual miral::Window window_at(float x, float y) = 0;
+    virtual void process_animation(AnimationStepResult const&, std::shared_ptr<Container> const&) = 0;
 };
 
 }

diff --git a/src/window_manager_tools_window_controller.cpp b/src/window_manager_tools_window_controller.cpp
@@ -22,6 +22,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "compositor_state.h"
 #include "config.h"
 #include "leaf_container.h"
+#include "policy.h"
 #include "window_helpers.h"
 #include <mir/log.h>
 #include <mir/scene/surface.h>
@@ -34,12 +35,14 @@ WindowManagerToolsWindowController::WindowManagerToolsWindowController(
     Animator& animator,
     CompositorState& state,
     std::shared_ptr<Config> const& config,
-    std::shared_ptr<mir::ServerActionQueue> const& server_action_queue) :
+    std::shared_ptr<mir::ServerActionQueue> const& server_action_queue,
+    Policy* policy) :
     tools { tools },
     animator { animator },
     state { state },
     config { config },
-    server_action_queue { server_action_queue }
+    server_action_queue { server_action_queue },
+    policy { policy }
 {
 }
 
@@ -56,13 +59,13 @@ void WindowManagerToolsWindowController::open(miral::Window const& window)
     geom::Rectangle rect { window.top_left(), window.size() };
     if (info.parent())
     {
-        on_animation({ container->animation_handle(), true, rect }, container);
+        policy->handle_animation({ container->animation_handle(), true, rect }, container);
         return;
     }
 
     if (!config->are_animations_enabled())
     {
-        on_animation(AnimationStepResult { container->animation_handle(), true, rect }, container);
+        policy->handle_animation(AnimationStepResult { container->animation_handle(), true, rect }, container);
         return;
     }
 
@@ -97,13 +100,13 @@ void WindowManagerToolsWindowController::set_rectangle(
     auto const& info = info_for(window);
     if (info.parent())
     {
-        on_animation({ container->animation_handle(), true, to }, container);
+        policy->handle_animation({ container->animation_handle(), true, to }, container);
         return;
     }
 
     if (!config->are_animations_enabled())
     {
-        on_animation(
+        policy->handle_animation(
             AnimationStepResult { container->animation_handle(),
                 true,
                 to,
@@ -200,14 +203,16 @@ WindowManagerToolsWindowController::WindowAnimation::WindowAnimation(
 {
 }
 
-void WindowManagerToolsWindowController::WindowAnimation::on_tick(miracle::AnimationStepResult const& asr)
+void WindowManagerToolsWindowController::WindowAnimation::on_tick(AnimationStepResult const& asr)
 {
-    if (auto shared_container = container.lock())
-        controller->on_animation(asr, shared_container);
+    controller->server_action_queue->enqueue(controller, [controller = controller, asr = asr, container = container]()
+    {
+        controller->policy->handle_animation(asr, container);
+    });
 }
 
-void WindowManagerToolsWindowController::on_animation(
-    miracle::AnimationStepResult const& result,
+void WindowManagerToolsWindowController::process_animation(
+    AnimationStepResult const& result,
     std::shared_ptr<Container> const& container)
 {
     bool needs_modify = false;
@@ -234,26 +239,23 @@ void WindowManagerToolsWindowController::on_animation(
 
     if (needs_modify)
     {
-        server_action_queue->enqueue(this, [this, container, spec, result]()
-        {
-            if (!container->window())
-                return;
+        if (!container->window())
+            return;
 
-            auto window = container->window().value();
-            if (!window)
-                return;
-            tools.modify_window(window, spec);
+        auto window = container->window().value();
+        if (!window)
+            return;
+        tools.modify_window(window, spec);
 
-            if (result.is_complete)
-                container->constrain();
+        if (result.is_complete)
+            container->constrain();
+        else
+        {
+            if (container->get_type() == ContainerType::leaf)
+                clip(window, result.clip_area);
             else
-            {
-                if (container->get_type() == ContainerType::leaf)
-                    clip(window, result.clip_area);
-                else
-                    noclip(window);
-            }
-        });
+                noclip(window);
+        }
     }
 }
 

diff --git a/src/window_manager_tools_window_controller.h b/src/window_manager_tools_window_controller.h
@@ -30,6 +30,7 @@ namespace miracle
 {
 class CompositorState;
 class Config;
+class Policy;
 
 class WindowManagerToolsWindowController : public WindowController
 {
@@ -39,7 +40,8 @@ class WindowManagerToolsWindowController : public WindowController
         Animator& animator,
         CompositorState& state,
         std::shared_ptr<Config> const& config,
-        std::shared_ptr<mir::ServerActionQueue> const& server_action_queue);
+        std::shared_ptr<mir::ServerActionQueue> const& server_action_queue,
+        Policy* policy);
     void open(miral::Window const&) override;
     bool is_fullscreen(miral::Window const&) override;
     void set_rectangle(miral::Window const&, geom::Rectangle const&, geom::Rectangle const&) override;
@@ -60,13 +62,15 @@ class WindowManagerToolsWindowController : public WindowController
     void move_cursor_to(float x, float y) override;
     void set_size_hack(AnimationHandle handle, mir::geometry::Size const& size) override;
     miral::Window window_at(float x, float y) override;
+    void process_animation(AnimationStepResult const&, std::shared_ptr<Container> const&) override;
 
 private:
     miral::WindowManagerTools tools;
     Animator& animator;
     CompositorState& state;
     std::shared_ptr<Config> config;
     std::shared_ptr<mir::ServerActionQueue> server_action_queue;
+    Policy* policy;
 
     class WindowAnimation : public Animation
     {
@@ -85,8 +89,6 @@ class WindowManagerToolsWindowController : public WindowController
         WindowManagerToolsWindowController* controller;
         std::weak_ptr<Container> container;
     };
-
-    void on_animation(AnimationStepResult const& result, std::shared_ptr<Container> const&);
 };
 }
 

diff --git a/tests/ipc/stress.py b/tests/ipc/stress.py
@@ -0,0 +1,41 @@
+'''
+This script stress tests miracle by opening a number of instances of
+foot and then performing ipc commands on them until the test is quit.
+
+Do NOT run this in CI.
+'''
+
+from i3ipc import Connection
+import os
+import subprocess
+from conftest import Server
+import time
+
+def main():
+    num_terminals: int = 100
+    server = Server(os.environ["SWAYSOCK"], os.environ["WAYLAND_DISPLAY"])
+    conn = Connection(server.ipc)
+    for i in range(0, num_terminals):
+        p = server.open_app("foot")
+
+        if i % 2 != 0:
+            conn.command("layout toggle split")
+
+        if i % 10 == 0:
+            conn.command(f"workspace {i / 10 + 1}")
+        time.sleep(0.5)
+
+    max_workspaces = num_terminals / 10 + 1
+    i: int = 1
+    while True:
+        conn.command(f"workspace {i}")
+        i += 1;
+        if i == max_workspaces:
+            i = 1
+        time.sleep(1)
+
+
+# Using the special variable 
+# __name__
+if __name__=="__main__":
+    main()
diff --git a/tests/stub_window_controller.h b/tests/stub_window_controller.h
@@ -138,6 +138,10 @@ class StubWindowController : public miracle::WindowController
         return miral::Window();
     }
 
+    void process_animation(AnimationStepResult const&, std::shared_ptr<Container> const&)
+    {
+    }
+
 private:
     std::vector<StubWindowData>& pairs;
     miral::WindowInfo stub_win_info;