From 7b3ddd5e155dc64b9db7d03bd61ae348ef713aee Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 22 Mar 2024 15:28:41 -0400 Subject: [PATCH] LibWeb: Track fetching-related tasks in FetchController for cancellation The HTMLMediaElement, for example, contains spec text which states any ongoing fetch process must be "stopped". The spec does not indicate how to do this, so our implementation is rather ad-hoc. Our current implementation may cause a crash in places that assume one of the fetch algorithms that we set to null is *not* null. For example: if (fetch_params.process_response) { queue_fetch_task([]() { fetch_params.process_response(); }; } If the fetch process is stopped after queuing the fetch task, but not before the fetch task is run, we will crash when running this fetch algorithm. We now track queued fetch tasks on the fetch controller. When the fetch process is stopped, we cancel any such pending task. It is a little bit awkward maintaining a fetch task ID. Ideally, we could use the underlying task ID throughout. But we do not have access to the underlying task nor its ID when the task is running, at which point we need some ID to remove from the pending task list. --- .../Text/expected/video-canceled-load.txt | 1 + .../Text/input/video-canceled-load.html | 13 ++++++++++++ .../LibWeb/Fetch/Fetching/Fetching.cpp | 6 +++--- .../Fetch/Infrastructure/FetchController.cpp | 21 ++++++++++++++++++- .../Fetch/Infrastructure/FetchController.h | 8 +++++++ .../LibWeb/Fetch/Infrastructure/Task.cpp | 20 ++++++++++++++++-- .../LibWeb/Fetch/Infrastructure/Task.h | 4 +++- 7 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/video-canceled-load.txt create mode 100644 Tests/LibWeb/Text/input/video-canceled-load.html diff --git a/Tests/LibWeb/Text/expected/video-canceled-load.txt b/Tests/LibWeb/Text/expected/video-canceled-load.txt new file mode 100644 index 0000000000..6d15bb5498 --- /dev/null +++ b/Tests/LibWeb/Text/expected/video-canceled-load.txt @@ -0,0 +1 @@ + wfh diff --git a/Tests/LibWeb/Text/input/video-canceled-load.html b/Tests/LibWeb/Text/input/video-canceled-load.html new file mode 100644 index 0000000000..2c1d94ee49 --- /dev/null +++ b/Tests/LibWeb/Text/input/video-canceled-load.html @@ -0,0 +1,13 @@ + + + diff --git a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp index 83b0390508..2201b763ff 100644 --- a/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Fetching/Fetching.cpp @@ -627,7 +627,7 @@ WebIDL::ExceptionOr fetch_response_handover(JS::Realm& realm, Infrastructu auto task_destination = fetch_params.task_destination().get>(); // 5. Queue a fetch task to run processResponseEndOfBodyTask with fetchParams’s task destination. - Infrastructure::queue_fetch_task(task_destination, move(process_response_end_of_body_task)); + Infrastructure::queue_fetch_task(fetch_params.controller(), task_destination, move(process_response_end_of_body_task)); }; // FIXME: Handle 'parallel queue' task destination @@ -636,7 +636,7 @@ WebIDL::ExceptionOr fetch_response_handover(JS::Realm& realm, Infrastructu // 4. If fetchParams’s process response is non-null, then queue a fetch task to run fetchParams’s process response // given response, with fetchParams’s task destination. if (fetch_params.algorithms()->process_response()) { - Infrastructure::queue_fetch_task(task_destination, [&fetch_params, &response]() { + Infrastructure::queue_fetch_task(fetch_params.controller(), task_destination, [&fetch_params, &response]() { fetch_params.algorithms()->process_response()(response); }); } @@ -674,7 +674,7 @@ WebIDL::ExceptionOr fetch_response_handover(JS::Realm& realm, Infrastructu // 3. If internalResponse's body is null, then queue a fetch task to run processBody given null, with // fetchParams’s task destination. if (!internal_response->body()) { - Infrastructure::queue_fetch_task(task_destination, [process_body = move(process_body)]() { + Infrastructure::queue_fetch_task(fetch_params.controller(), task_destination, [process_body = move(process_body)]() { process_body({}); }); } diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.cpp b/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.cpp index b3805800d8..047d77efeb 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include namespace Web::Fetch::Infrastructure { @@ -105,11 +106,29 @@ void FetchController::stop_fetch() // AD-HOC: Some HTML elements need to stop an ongoing fetching process without causing any network error to be raised // (which abort() and terminate() will both do). This is tricky because the fetch process runs across several // nested Platform::EventLoopPlugin::deferred_invoke() invocations. For now, we "stop" the fetch process by - // ignoring any callbacks. + // cancelling any queued fetch tasks and then ignoring any callbacks. + auto ongoing_fetch_tasks = move(m_ongoing_fetch_tasks); + + HTML::main_thread_event_loop().task_queue().remove_tasks_matching([&](auto const& task) { + return ongoing_fetch_tasks.remove_all_matching([&](u64, int task_id) { + return task.id() == task_id; + }); + }); + if (m_fetch_params) { auto fetch_algorithms = FetchAlgorithms::create(vm, {}); m_fetch_params->set_algorithms(fetch_algorithms); } } +void FetchController::fetch_task_queued(u64 fetch_task_id, int event_id) +{ + m_ongoing_fetch_tasks.set(fetch_task_id, event_id); +} + +void FetchController::fetch_task_complete(u64 fetch_task_id) +{ + m_ongoing_fetch_tasks.remove(fetch_task_id); +} + } diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.h b/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.h index 7828660d3c..879bd7f387 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.h +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/FetchController.h @@ -7,6 +7,7 @@ #pragma once #include +#include #include #include #include @@ -48,6 +49,10 @@ public: void stop_fetch(); + u64 next_fetch_task_id() { return m_next_fetch_task_id++; } + void fetch_task_queued(u64 fetch_task_id, int event_id); + void fetch_task_complete(u64 fetch_task_id); + private: FetchController(); @@ -78,6 +83,9 @@ private: JS::GCPtr> m_next_manual_redirect_steps; JS::GCPtr m_fetch_params; + + HashMap m_ongoing_fetch_tasks; + u64 m_next_fetch_task_id { 0 }; }; } diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.cpp b/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.cpp index 3718f91acd..54e0a733c7 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.cpp +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.cpp @@ -4,18 +4,34 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include namespace Web::Fetch::Infrastructure { // https://fetch.spec.whatwg.org/#queue-a-fetch-task -void queue_fetch_task(JS::Object& task_destination, JS::SafeFunction algorithm) +int queue_fetch_task(JS::Object& task_destination, JS::SafeFunction algorithm) { // FIXME: 1. If taskDestination is a parallel queue, then enqueue algorithm to taskDestination. // 2. Otherwise, queue a global task on the networking task source with taskDestination and algorithm. - HTML::queue_global_task(HTML::Task::Source::Networking, task_destination, move(algorithm)); + return HTML::queue_global_task(HTML::Task::Source::Networking, task_destination, move(algorithm)); +} + +// AD-HOC: This overload allows tracking the queued task within the fetch controller so that we may cancel queued tasks +// when the spec indicates that we must stop an ongoing fetch. +int queue_fetch_task(JS::NonnullGCPtr fetch_controller, JS::Object& task_destination, JS::SafeFunction algorithm) +{ + auto fetch_task_id = fetch_controller->next_fetch_task_id(); + + int event_id = queue_fetch_task(task_destination, [fetch_controller, fetch_task_id, algorithm = move(algorithm)]() { + fetch_controller->fetch_task_complete(fetch_task_id); + algorithm(); + }); + + fetch_controller->fetch_task_queued(fetch_task_id, event_id); + return event_id; } } diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.h b/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.h index b026e412c2..c305accd79 100644 --- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.h +++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/Task.h @@ -10,12 +10,14 @@ #include #include #include +#include namespace Web::Fetch::Infrastructure { // FIXME: 'or a parallel queue' using TaskDestination = Variant>; -void queue_fetch_task(JS::Object&, JS::SafeFunction); +int queue_fetch_task(JS::Object&, JS::SafeFunction); +int queue_fetch_task(JS::NonnullGCPtr, JS::Object&, JS::SafeFunction); }