From 17fcbce32462d5131d655d1b0b02fb1a456794d7 Mon Sep 17 00:00:00 2001
From: stasoid <stasoid@yahoo.com>
Date: Wed, 19 Feb 2025 22:36:05 +0500
Subject: [PATCH] LibCore: Make single-shot timer objects manually reset on
 Windows

This fixes a really nasty EventLoop bug which I debugged for 2 weeks.

The spin_until([&]{return completed_tasks == total_tasks;}) in
TraversableNavigable::check_if_unloading_is_canceled spins forever.

Cause of the bug:

check_if_unloading_is_canceled is called deferred

check_if_unloading_is_canceled creates a task:

        queue_global_task(..., [&] {
            ...
            completed_tasks++;
        }));

This task is never executed.

queue_global_task calls TaskQueue::add

void TaskQueue::add(task)
{
    m_tasks.append(task);
    m_event_loop->schedule();
}

void HTML::EventLoop::schedule()
{
    if (!m_system_event_loop_timer)
        m_system_event_loop_timer = Timer::create_single_shot(
            0, // delay
            [&] { process(); });

    if (!m_system_event_loop_timer->is_active())
        m_system_event_loop_timer->restart();
}

EventLoop::process executes one task from task queue and calls
schedule again if there are more tasks.

So task processing relies on one single-shot zero-delay timer,
m_system_event_loop_timer.

Timers and other notification events are handled by Core::EventLoop
and Core::ThreadEventQueue, these are different from HTML::EventLoop
and HTML::TaskQueue mentioned above.

check_if_unloading_is_canceled is called using deferred_invoke
mechanism, different from m_system_event_loop_timer,
see Navigable::navigate and Core::EventLoop::deferred_invoke.

The core of the problem is that Core::EventLoop::pump is called again
(from spin_until) after timer fired but before its handler is executed.

In ThreadEventQueue::process events are moved into local variable before
executing. The first of those events is check_if_unloading_is_canceled.
One of the rest events is Web::HTML::EventLoop::process sheduled in
EventLoop::schedule using m_system_event_loop_timer.
When check_if_unloading_is_canceled calls queue_global_task its
m_system_event_loop_timer is still active because Timer::timer_event
was not yet called, so the timer is not restarted.
But Timer::timer_event (and hence EventLoop::process) will never execute
because check_if_unloading_is_canceled calls spin_until after
queue_global_task, and EventLoop::process is no longer in
event_queue.m_private->queued_events.

By making a single-shot timer manually-reset we are allowing it to fire
several times. So when spin_until is executed m_system_event_loop_timer
is fired again. Not an ideal solution, but this is the best I could
come up with. This commit makes the behavior match EventLoopImplUnix,
in which single-shot timer can also fire several times.

Adding event_queue.process(); at the start of pump like in EvtLoopImplQt
doesn't fix the problem.

Note: Timer::start calls EventReceiver::start_timer, which calls
EventLoop::register_timer with should_reload always set to true
(single-shot vs periodic are handled in Timer::timer_event instead),
so I use static_cast<Timer&>(object).is_single_shot() instead of
!should_reload.
---
 Libraries/LibCore/EventLoopImplementationWindows.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Libraries/LibCore/EventLoopImplementationWindows.cpp b/Libraries/LibCore/EventLoopImplementationWindows.cpp
index 21e5235e304..2e38479c518 100644
--- a/Libraries/LibCore/EventLoopImplementationWindows.cpp
+++ b/Libraries/LibCore/EventLoopImplementationWindows.cpp
@@ -9,6 +9,7 @@
 #include <LibCore/EventLoopImplementationWindows.h>
 #include <LibCore/Notifier.h>
 #include <LibCore/ThreadEventQueue.h>
+#include <LibCore/Timer.h>
 
 #include <AK/Windows.h>
 
@@ -196,7 +197,9 @@ void EventLoopManagerWindows::unregister_notifier(Notifier& notifier)
 intptr_t EventLoopManagerWindows::register_timer(EventReceiver& object, int milliseconds, bool should_reload, TimerShouldFireWhenNotVisible fire_when_not_visible)
 {
     VERIFY(milliseconds >= 0);
-    HANDLE timer = CreateWaitableTimer(NULL, FALSE, NULL);
+    // FIXME: This is a temporary fix for issue #3641
+    bool manual_reset = static_cast<Timer&>(object).is_single_shot();
+    HANDLE timer = CreateWaitableTimer(NULL, manual_reset, NULL);
     VERIFY(timer);
 
     LARGE_INTEGER first_time = {};