From 642a2570a8637787c36cb301890e0fad02f07849 Mon Sep 17 00:00:00 2001 From: Andrew Kaster Date: Fri, 15 Sep 2023 19:55:59 -0600 Subject: [PATCH] Ladybird/Android: Explicitly schedule Core::EventLoop in main activity Instead of having an annoying loop that constantly reschedules a Core::EventLoop trigger, have the ALooperEventLoopManager do it itself in the did_post_event() function. We cannot simply re-use the Unix implementation directly because that implementation expects to actually be called all the time in order to service timers. If you don't call its' pump() method, timers do not get triggered. So, we do still need the seconary thread for Timers that was added earlier. --- .../cpp/ALooperEventLoopImplementation.cpp | 80 ++++++++++--------- .../main/cpp/ALooperEventLoopImplementation.h | 12 ++- Ladybird/Android/src/main/cpp/JNIHelpers.h | 2 + .../Android/src/main/cpp/LadybirdActivity.cpp | 41 ++++++++-- .../src/main/cpp/LadybirdServiceBase.h | 2 - .../src/main/cpp/TimerExecutorService.cpp | 4 +- .../main/cpp/WebViewImplementationNative.h | 1 - .../cpp/WebViewImplementationNativeJNI.cpp | 5 -- .../serenityos/ladybird/LadybirdActivity.kt | 34 +++----- 9 files changed, 96 insertions(+), 85 deletions(-) diff --git a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp index 8ab8350e12..26943cafb4 100644 --- a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp +++ b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.cpp @@ -27,11 +27,12 @@ static ALooperEventLoopImplementation& current_impl() return verify_cast(Core::EventLoop::current().impl()); } -ALooperEventLoopManager::ALooperEventLoopManager(JavaVM* vm, jobject timer_service) - : m_vm(vm) - , m_timer_service(timer_service) +static int looper_callback(int fd, int events, void* data); + +ALooperEventLoopManager::ALooperEventLoopManager(jobject timer_service) + : m_timer_service(timer_service) { - JavaEnvironment env(m_vm); + JavaEnvironment env(global_vm); jclass timer_class = env.get()->FindClass("org/serenityos/ladybird/TimerExecutorService$Timer"); if (!timer_class) @@ -53,14 +54,30 @@ ALooperEventLoopManager::ALooperEventLoopManager(JavaVM* vm, jobject timer_servi if (!m_unregister_timer) TODO(); env.get()->DeleteLocalRef(timer_service_class); + + auto ret = pipe2(m_pipe, O_CLOEXEC | O_NONBLOCK); + VERIFY(ret == 0); + + m_main_looper = ALooper_forThread(); + VERIFY(m_main_looper); + ALooper_acquire(m_main_looper); + + ret = ALooper_addFd(m_main_looper, m_pipe[0], ALOOPER_POLL_CALLBACK, ALOOPER_EVENT_INPUT, &looper_callback, this); + VERIFY(ret == 1); } ALooperEventLoopManager::~ALooperEventLoopManager() { - JavaEnvironment env(m_vm); + JavaEnvironment env(global_vm); env.get()->DeleteGlobalRef(m_timer_service); env.get()->DeleteGlobalRef(m_timer_class); + + ALooper_removeFd(m_main_looper, m_pipe[0]); + ALooper_release(m_main_looper); + + ::close(m_pipe[0]); + ::close(m_pipe[1]); } NonnullOwnPtr ALooperEventLoopManager::make_implementation() @@ -70,7 +87,7 @@ NonnullOwnPtr ALooperEventLoopManager::make_imple int ALooperEventLoopManager::register_timer(Core::EventReceiver& receiver, int milliseconds, bool should_reload, Core::TimerShouldFireWhenNotVisible visibility) { - JavaEnvironment env(m_vm); + JavaEnvironment env(global_vm); auto& thread_data = EventLoopThreadData::the(); auto timer = env.get()->NewObject(m_timer_class, m_timer_constructor, reinterpret_cast(¤t_impl())); @@ -87,7 +104,7 @@ int ALooperEventLoopManager::register_timer(Core::EventReceiver& receiver, int m bool ALooperEventLoopManager::unregister_timer(int timer_id) { if (auto timer = EventLoopThreadData::the().timers.take(timer_id); timer.has_value()) { - JavaEnvironment env(m_vm); + JavaEnvironment env(global_vm); return env.get()->CallBooleanMethod(m_timer_service, m_unregister_timer, timer_id); } return false; @@ -107,29 +124,34 @@ void ALooperEventLoopManager::unregister_notifier(Core::Notifier& notifier) void ALooperEventLoopManager::did_post_event() { - current_impl().poke(); + int msg = 0xCAFEBABE; + (void)write(m_pipe[1], &msg, sizeof(msg)); +} + +int looper_callback(int fd, int events, void* data) +{ + auto& manager = *static_cast(data); + + if (events & ALOOPER_EVENT_INPUT) { + int msg = 0; + while (read(fd, &msg, sizeof(msg)) == sizeof(msg)) { + // Do nothing, we don't actually care what the message was, just that it was posted + } + manager.on_did_post_event(); + } + return 1; } 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); - ALooper_acquire(m_event_loop); - - ret = ALooper_addFd(m_event_loop, m_pipe[0], ALOOPER_POLL_CALLBACK, ALOOPER_EVENT_INPUT, &ALooperEventLoopImplementation::looper_callback, this); - VERIFY(ret == 1); } ALooperEventLoopImplementation::~ALooperEventLoopImplementation() { - ALooper_removeFd(m_event_loop, m_pipe[0]); ALooper_release(m_event_loop); - - ::close(m_pipe[0]); - ::close(m_pipe[1]); } EventLoopThreadData& ALooperEventLoopImplementation::thread_data() @@ -181,26 +203,6 @@ void ALooperEventLoopImplementation::post_event(Core::EventReceiver& receiver, N wake(); } -int ALooperEventLoopImplementation::looper_callback(int fd, int events, void* data) -{ - auto& impl = *static_cast(data); - (void)impl; // FIXME: Do we need to do anything with the instance here? - - if (events & ALOOPER_EVENT_INPUT) { - int msg = 0; - while (read(fd, &msg, sizeof(msg)) == sizeof(msg)) { - // Do nothing, we don't actually care what the message was, just that it was posted - } - } - return 1; -} - -void ALooperEventLoopImplementation::poke() -{ - int msg = 0xCAFEBABE; - (void)write(m_pipe[1], &msg, sizeof(msg)); -} - static int notifier_callback(int fd, int, void* data) { auto& notifier = *static_cast(data); @@ -211,7 +213,7 @@ static int notifier_callback(int fd, int, void* data) notifier.dispatch_event(event); // Wake up from ALooper_pollAll, and service this event on the event queue - current_impl().poke(); + current_impl().wake(); return 1; } diff --git a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h index be0b96bff4..aa59706dce 100644 --- a/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h +++ b/Ladybird/Android/src/main/cpp/ALooperEventLoopImplementation.h @@ -19,7 +19,7 @@ namespace Ladybird { class ALooperEventLoopManager : public Core::EventLoopManager { public: - ALooperEventLoopManager(JavaVM*, jobject timer_service); + ALooperEventLoopManager(jobject timer_service); virtual ~ALooperEventLoopManager() override; virtual NonnullOwnPtr make_implementation() override; @@ -31,12 +31,15 @@ public: virtual void did_post_event() override; + Function on_did_post_event; + // FIXME: These APIs only exist for obscure use-cases inside SerenityOS. Try to get rid of them. virtual int register_signal(int, Function) override { return 0; } virtual void unregister_signal(int) override { } private: - JavaVM* m_vm { nullptr }; + int m_pipe[2] = {}; + ALooper* m_main_looper { nullptr }; jobject m_timer_service { nullptr }; jmethodID m_register_timer { nullptr }; jmethodID m_unregister_timer { nullptr }; @@ -74,8 +77,6 @@ public: virtual bool was_exit_requested() const override { return false; } virtual void notify_forked_and_in_child() override { } - void poke(); - EventLoopThreadData& thread_data(); private: @@ -83,13 +84,10 @@ private: ALooperEventLoopImplementation(); - static int looper_callback(int fd, int events, void* data); - void register_notifier(Core::Notifier&); void unregister_notifier(Core::Notifier&); ALooper* m_event_loop { nullptr }; - int m_pipe[2] {}; int m_exit_code { 0 }; Atomic m_exit_requested { false }; EventLoopThreadData* m_thread_data { nullptr }; diff --git a/Ladybird/Android/src/main/cpp/JNIHelpers.h b/Ladybird/Android/src/main/cpp/JNIHelpers.h index ae7dc455d0..66887f68d6 100644 --- a/Ladybird/Android/src/main/cpp/JNIHelpers.h +++ b/Ladybird/Android/src/main/cpp/JNIHelpers.h @@ -41,3 +41,5 @@ private: JNIEnv* m_env = nullptr; bool m_did_attach_thread = false; }; + +extern JavaVM* global_vm; diff --git a/Ladybird/Android/src/main/cpp/LadybirdActivity.cpp b/Ladybird/Android/src/main/cpp/LadybirdActivity.cpp index 997ded3838..17f5a879d9 100644 --- a/Ladybird/Android/src/main/cpp/LadybirdActivity.cpp +++ b/Ladybird/Android/src/main/cpp/LadybirdActivity.cpp @@ -5,6 +5,7 @@ */ #include "ALooperEventLoopImplementation.h" +#include "JNIHelpers.h" #include #include #include @@ -22,10 +23,13 @@ static ErrorOr extract_tar_archive(String archive_file, DeprecatedString output_directory); +JavaVM* global_vm; static OwnPtr s_main_event_loop; +static jobject s_java_instance; +static jmethodID s_schedule_event_loop_method; extern "C" JNIEXPORT void JNICALL -Java_org_serenityos_ladybird_LadybirdActivity_initNativeCode(JNIEnv* env, jobject /* thiz */, jstring resource_dir, jstring tag_name, jobject timer_service) +Java_org_serenityos_ladybird_LadybirdActivity_initNativeCode(JNIEnv* env, jobject thiz, jstring resource_dir, jstring tag_name, jobject timer_service) { char const* raw_resource_dir = env->GetStringUTFChars(resource_dir, nullptr); s_serenity_resource_root = raw_resource_dir; @@ -46,18 +50,43 @@ Java_org_serenityos_ladybird_LadybirdActivity_initNativeCode(JNIEnv* env, jobjec dbgln("Hopefully no developer changed the asset files and expected them to be re-extracted!"); } + env->GetJavaVM(&global_vm); + VERIFY(global_vm); + + s_java_instance = env->NewGlobalRef(thiz); + jclass clazz = env->GetObjectClass(s_java_instance); + VERIFY(clazz); + s_schedule_event_loop_method = env->GetMethodID(clazz, "scheduleEventLoop", "()V"); + VERIFY(s_schedule_event_loop_method); + env->DeleteLocalRef(clazz); + jobject timer_service_ref = env->NewGlobalRef(timer_service); - JavaVM* vm = nullptr; - jint ret = env->GetJavaVM(&vm); - VERIFY(ret == 0); - Core::EventLoopManager::install(*new Ladybird::ALooperEventLoopManager(vm, timer_service_ref)); + + auto* event_loop_manager = new Ladybird::ALooperEventLoopManager(timer_service_ref); + event_loop_manager->on_did_post_event = [] { + JavaEnvironment env(global_vm); + env.get()->CallVoidMethod(s_java_instance, s_schedule_event_loop_method); + }; + Core::EventLoopManager::install(*event_loop_manager); s_main_event_loop = make(); } extern "C" JNIEXPORT void JNICALL Java_org_serenityos_ladybird_LadybirdActivity_execMainEventLoop(JNIEnv*, jobject /* thiz */) { - s_main_event_loop->pump(Core::EventLoop::WaitMode::PollForEvents); + if (s_main_event_loop) { + s_main_event_loop->pump(Core::EventLoop::WaitMode::PollForEvents); + } +} + +extern "C" JNIEXPORT void JNICALL +Java_org_serenityos_ladybird_LadybirdActivity_disposeNativeCode(JNIEnv* env, jobject /* thiz */) +{ + s_main_event_loop = nullptr; + s_schedule_event_loop_method = nullptr; + env->DeleteGlobalRef(s_java_instance); + + delete &Core::EventLoopManager::the(); } ErrorOr extract_tar_archive(String archive_file, DeprecatedString output_directory) diff --git a/Ladybird/Android/src/main/cpp/LadybirdServiceBase.h b/Ladybird/Android/src/main/cpp/LadybirdServiceBase.h index a30ab0f09e..48fbd14ef6 100644 --- a/Ladybird/Android/src/main/cpp/LadybirdServiceBase.h +++ b/Ladybird/Android/src/main/cpp/LadybirdServiceBase.h @@ -10,5 +10,3 @@ #include ErrorOr service_main(int ipc_socket, int fd_passing_socket); - -extern JavaVM* global_vm; diff --git a/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp b/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp index 59a42bd1eb..51d0bb6b09 100644 --- a/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp +++ b/Ladybird/Android/src/main/cpp/TimerExecutorService.cpp @@ -29,5 +29,7 @@ Java_org_serenityos_ladybird_TimerExecutorService_00024Timer_nativeRun(JNIEnv*, 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); + if (auto num_events = s_event_loop.pump(Core::EventLoop::WaitMode::PollForEvents); num_events != 0) { + dbgln("BUG: Processed {} events on Timer thread!", num_events); + } } diff --git a/Ladybird/Android/src/main/cpp/WebViewImplementationNative.h b/Ladybird/Android/src/main/cpp/WebViewImplementationNative.h index 364b70c1e5..dd75e36d3a 100644 --- a/Ladybird/Android/src/main/cpp/WebViewImplementationNative.h +++ b/Ladybird/Android/src/main/cpp/WebViewImplementationNative.h @@ -32,7 +32,6 @@ public: static jclass global_class_reference; static jmethodID bind_webcontent_method; static jmethodID invalidate_layout_method; - static JavaVM* global_vm; jobject java_instance() const { return m_java_instance; } diff --git a/Ladybird/Android/src/main/cpp/WebViewImplementationNativeJNI.cpp b/Ladybird/Android/src/main/cpp/WebViewImplementationNativeJNI.cpp index 4dbb7a77c5..b02c172a3a 100644 --- a/Ladybird/Android/src/main/cpp/WebViewImplementationNativeJNI.cpp +++ b/Ladybird/Android/src/main/cpp/WebViewImplementationNativeJNI.cpp @@ -12,15 +12,10 @@ using namespace Ladybird; jclass WebViewImplementationNative::global_class_reference; jmethodID WebViewImplementationNative::bind_webcontent_method; jmethodID WebViewImplementationNative::invalidate_layout_method; -JavaVM* WebViewImplementationNative::global_vm; extern "C" JNIEXPORT void JNICALL Java_org_serenityos_ladybird_WebViewImplementation_00024Companion_nativeClassInit(JNIEnv* env, jobject /* thiz */) { - auto ret = env->GetJavaVM(&WebViewImplementationNative::global_vm); - if (ret != 0) - TODO(); - auto local_class = env->FindClass("org/serenityos/ladybird/WebViewImplementation"); if (!local_class) TODO(); diff --git a/Ladybird/Android/src/main/java/org/serenityos/ladybird/LadybirdActivity.kt b/Ladybird/Android/src/main/java/org/serenityos/ladybird/LadybirdActivity.kt index 84c5762264..c228a8eb9b 100644 --- a/Ladybird/Android/src/main/java/org/serenityos/ladybird/LadybirdActivity.kt +++ b/Ladybird/Android/src/main/java/org/serenityos/ladybird/LadybirdActivity.kt @@ -16,6 +16,8 @@ class LadybirdActivity : AppCompatActivity() { private lateinit var binding: ActivityMainBinding private lateinit var resourceDir: String + private lateinit var view: WebView + private var timerService = TimerExecutorService() override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -28,10 +30,6 @@ class LadybirdActivity : AppCompatActivity() { setSupportActionBar(binding.toolbar) view = binding.webView view.initialize(resourceDir) - - mainExecutor.execute { - callNativeEventLoopForever() - } } override fun onStart() { @@ -44,31 +42,19 @@ class LadybirdActivity : AppCompatActivity() { override fun onDestroy() { view.dispose() + disposeNativeCode() super.onDestroy() } - private lateinit var view: WebView - private var timerService = TimerExecutorService() - - /** - * A native method that is implemented by the 'ladybird' native library, - * which is packaged with this application. - */ - private external fun initNativeCode( - resourceDir: String, - tag: String, - timerService: TimerExecutorService - ) - - // FIXME: Instead of doing this, can we push a message to the message queue of the java Looper - // when an event is pushed to the main thread, and use that to clear out the - // Core::ThreadEventQueues? - private fun callNativeEventLoopForever() { - execMainEventLoop() - mainExecutor.execute { callNativeEventLoopForever() } + private fun scheduleEventLoop() { + mainExecutor.execute { + execMainEventLoop() + } } - private external fun execMainEventLoop(); + private external fun initNativeCode(resourceDir: String, tag: String, timerService: TimerExecutorService) + private external fun disposeNativeCode() + private external fun execMainEventLoop() companion object { // Used to load the 'ladybird' library on application startup.