From e09816c37c50fc222831041c9ad2e844fe7545b0 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Wed, 20 Mar 2024 15:41:45 +0100 Subject: [PATCH] LibWeb: Run only tasks with navigation source in "apply history step" In our implementation of the "apply the history step" algorithm, we have to spin-wait for the completion of tasks queued on the event loop. Before this change, we allowed tasks from any source to be executed while we were waiting. It should not be possible because it allows to interrupt history step application by anything, including another history step application. Fixes https://github.com/SerenityOS/serenity/issues/23598 --- .../remove-iframe-from-timeout-callback.txt | 1 + .../remove-iframe-from-timeout-callback.html | 12 ++++++ .../LibWeb/HTML/EventLoop/EventLoop.cpp | 40 +++++++++++++++++++ .../LibWeb/HTML/EventLoop/EventLoop.h | 3 ++ .../LibWeb/HTML/TraversableNavigable.cpp | 4 +- 5 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/navigation/remove-iframe-from-timeout-callback.txt create mode 100644 Tests/LibWeb/Text/input/navigation/remove-iframe-from-timeout-callback.html diff --git a/Tests/LibWeb/Text/expected/navigation/remove-iframe-from-timeout-callback.txt b/Tests/LibWeb/Text/expected/navigation/remove-iframe-from-timeout-callback.txt new file mode 100644 index 00000000000..136d06384a4 --- /dev/null +++ b/Tests/LibWeb/Text/expected/navigation/remove-iframe-from-timeout-callback.txt @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/Tests/LibWeb/Text/input/navigation/remove-iframe-from-timeout-callback.html b/Tests/LibWeb/Text/input/navigation/remove-iframe-from-timeout-callback.html new file mode 100644 index 00000000000..e6de8c2e329 --- /dev/null +++ b/Tests/LibWeb/Text/input/navigation/remove-iframe-from-timeout-callback.html @@ -0,0 +1,12 @@ + +
+ + +
+ diff --git a/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp b/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp index b3138ec1f81..bebef552a14 100644 --- a/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp +++ b/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp @@ -92,9 +92,49 @@ void EventLoop::spin_until(JS::SafeFunction goal_condition) // NOTE: This is achieved by returning from the function. } +void EventLoop::spin_processing_tasks_with_source_until(Task::Source source, JS::SafeFunction goal_condition) +{ + m_vm->save_execution_context_stack(); + m_vm->clear_execution_context_stack(); + + perform_a_microtask_checkpoint(); + + // NOTE: HTML event loop processing steps could run a task with arbitrary source + m_skip_event_loop_processing_steps = true; + + Platform::EventLoopPlugin::the().spin_until([&] { + if (goal_condition()) + return true; + if (m_task_queue.has_runnable_tasks()) { + auto tasks = m_task_queue.take_tasks_matching([&](auto& task) { + return task.source() == source; + }); + + for (auto& task : tasks.value()) { + m_currently_running_task = task.ptr(); + task->execute(); + m_currently_running_task = nullptr; + } + } + + // FIXME: Remove the platform event loop plugin so that this doesn't look out of place + Core::EventLoop::current().wake(); + return goal_condition(); + }); + + m_skip_event_loop_processing_steps = false; + + schedule(); + + m_vm->restore_execution_context_stack(); +} + // https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model void EventLoop::process() { + if (m_skip_event_loop_processing_steps) + return; + // An event loop must continually run through the following steps for as long as it exists: // 1. Let oldestTask be null. diff --git a/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.h b/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.h index c186a520553..019c9af1187 100644 --- a/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.h +++ b/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.h @@ -38,6 +38,7 @@ public: TaskQueue const& microtask_queue() const { return m_microtask_queue; } void spin_until(JS::SafeFunction goal_condition); + void spin_processing_tasks_with_source_until(Task::Source, JS::SafeFunction goal_condition); void process(); // https://html.spec.whatwg.org/multipage/browsing-the-web.html#termination-nesting-level @@ -110,6 +111,8 @@ private: size_t m_termination_nesting_level { 0 }; bool m_execution_paused { false }; + + bool m_skip_event_loop_processing_steps { false }; }; EventLoop& main_thread_event_loop(); diff --git a/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp b/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp index e43a617e673..dbc3e750538 100644 --- a/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp +++ b/Userland/Libraries/LibWeb/HTML/TraversableNavigable.cpp @@ -543,7 +543,7 @@ TraversableNavigable::HistoryStepResult TraversableNavigable::apply_the_history_ // AD-HOC: Since currently populate_session_history_entry_document does not run in parallel // we call spin_until to interrupt execution of this function and let document population // to complete. - Platform::EventLoopPlugin::the().spin_until([&] { + main_thread_event_loop().spin_processing_tasks_with_source_until(Task::Source::NavigationAndTraversal, [&] { return !changing_navigable_continuations.is_empty() || completed_change_jobs == total_change_jobs; }); @@ -679,7 +679,7 @@ TraversableNavigable::HistoryStepResult TraversableNavigable::apply_the_history_ // AD-HOC: Since currently populate_session_history_entry_document does not run in parallel // we call spin_until to interrupt execution of this function and let document population // to complete. - Platform::EventLoopPlugin::the().spin_until([&] { + main_thread_event_loop().spin_processing_tasks_with_source_until(Task::Source::NavigationAndTraversal, [&] { return completed_non_changing_jobs == total_non_changing_jobs; });