From ec05b4bc0a42252c31f8515bd6abfa583c3f11dd Mon Sep 17 00:00:00 2001 From: Andrew Kaster Date: Thu, 14 Sep 2023 15:37:06 -0600 Subject: [PATCH] Ladybird/Android: Post timer events to the correct event loop The FIXME at the bottom of Timer.nativeRun was on the money, and was the cause of some leaked timers. Solve the issue by passing the EventLoopImplementation to the JVM Timer object and retrieving it's thread_local timer list, and posting the events to the Impl rather than trying to invoke the receiver's timer event callback directly in the Timer thread. Because the implementation of ALooperEventLoopManager::did_post_event() reaches for EventLoop::current(), we also need to make sure that the timer thread acutally *has* an EventLoop, even if we don't expect to use it for anything. And we need to make sure to pump it to clear out any of the poke() invocations sending 4 bytes to the wake pipe for that thread. --- .../cpp/ALooperEventLoopImplementation.cpp | 10 +++++-- .../main/cpp/ALooperEventLoopImplementation.h | 28 +++++++++++-------- .../src/main/cpp/TimerExecutorService.cpp | 16 +++++++---- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp index 58b2421b9b..8ab8350e12 100644 --- a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp +++ b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp @@ -18,7 +18,7 @@ namespace Ladybird { EventLoopThreadData& EventLoopThreadData::the() { - static thread_local EventLoopThreadData s_thread_data; + static thread_local EventLoopThreadData s_thread_data { {}, {}, &Core::ThreadEventQueue::current() }; return s_thread_data; } @@ -73,7 +73,7 @@ int ALooperEventLoopManager::register_timer(Core::EventReceiver& receiver, int m JavaEnvironment env(m_vm); auto& thread_data = EventLoopThreadData::the(); - auto timer = env.get()->NewObject(m_timer_class, m_timer_constructor, reinterpret_cast(&thread_data)); + auto timer = env.get()->NewObject(m_timer_class, m_timer_constructor, reinterpret_cast(¤t_impl())); long millis = milliseconds; long timer_id = env.get()->CallLongMethod(m_timer_service, m_register_timer, timer, !should_reload, millis); @@ -112,6 +112,7 @@ void ALooperEventLoopManager::did_post_event() ALooperEventLoopImplementation::ALooperEventLoopImplementation() : m_event_loop(ALooper_prepare(0)) + , m_thread_data(&EventLoopThreadData::the()) { auto ret = pipe2(m_pipe, O_CLOEXEC | O_NONBLOCK); VERIFY(ret == 0); @@ -131,6 +132,11 @@ ALooperEventLoopImplementation::~ALooperEventLoopImplementation() ::close(m_pipe[1]); } +EventLoopThreadData& ALooperEventLoopImplementation::thread_data() +{ + return *m_thread_data; +} + int ALooperEventLoopImplementation::exec() { while (!m_exit_requested.load(MemoryOrder::memory_order_acquire)) diff --git a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h index 6920ee5e47..be0b96bff4 100644 --- a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h +++ b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h @@ -44,6 +44,19 @@ private: jmethodID m_timer_constructor { nullptr }; }; +struct TimerData { + WeakPtr receiver; + Core::TimerShouldFireWhenNotVisible visibility; +}; + +struct EventLoopThreadData { + static EventLoopThreadData& the(); + + HashMap timers; + HashTable notifiers; + Core::ThreadEventQueue* thread_queue = nullptr; +}; + class ALooperEventLoopImplementation : public Core::EventLoopImplementation { public: static NonnullOwnPtr create() { return adopt_own(*new ALooperEventLoopImplementation); } @@ -63,6 +76,8 @@ public: void poke(); + EventLoopThreadData& thread_data(); + private: friend class ALooperEventLoopManager; @@ -77,18 +92,7 @@ private: int m_pipe[2] {}; int m_exit_code { 0 }; Atomic m_exit_requested { false }; -}; - -struct TimerData { - WeakPtr receiver; - Core::TimerShouldFireWhenNotVisible visibility; -}; - -struct EventLoopThreadData { - static EventLoopThreadData& the(); - - HashMap timers; - HashTable notifiers; + EventLoopThreadData* m_thread_data { nullptr }; }; } diff --git a/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp b/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp index 5ae7fb8e02..59a42bd1eb 100644 --- a/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp +++ b/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp @@ -6,11 +6,16 @@ #include "ALooperEventLoopImplementation.h" #include +#include #include -extern "C" JNIEXPORT void JNICALL Java_org_serenityos_ladybird_TimerExecutorService_00024Timer_nativeRun(JNIEnv*, jobject /* thiz */, jlong native_data, jlong id) +extern "C" JNIEXPORT void JNICALL +Java_org_serenityos_ladybird_TimerExecutorService_00024Timer_nativeRun(JNIEnv*, jobject /* thiz */, jlong native_data, jlong id) { - auto& thread_data = *reinterpret_cast(native_data); + static Core::EventLoop s_event_loop; // Here to exist for this thread + + auto& event_loop_impl = *reinterpret_cast(native_data); + auto& thread_data = event_loop_impl.thread_data(); if (auto timer_data = thread_data.timers.get(id); timer_data.has_value()) { auto receiver = timer_data->receiver.strong_ref(); @@ -21,9 +26,8 @@ extern "C" JNIEXPORT void JNICALL Java_org_serenityos_ladybird_TimerExecutorServ if (!receiver->is_visible_for_timer_purposes()) return; - Core::TimerEvent event(id); - - // FIXME: Should the dispatch happen on the thread that registered the timer? - receiver->dispatch_event(event); + event_loop_impl.post_event(*receiver, make(id)); } + // Flush the event loop on this thread to keep any garbage from building up + s_event_loop.pump(Core::EventLoop::WaitMode::PollForEvents); }