From de34143a4efc13abd7f815ebec6103bdbd7fe99e Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Thu, 12 Jun 2025 08:51:45 -0400 Subject: [PATCH] LibWeb: Do not spin the event loop awaiting text track state changes The text track processing model would previously spin forever waiting for the track URL to change. It would then recursively invoke itself to handle the new URL, again entering the spin loop. This meant that tracks could easily cause event loop hangs. We now have an observer system to be notified when the track state changes instead. This lets us exit the processing model and move on. --- Libraries/LibWeb/CMakeLists.txt | 1 + Libraries/LibWeb/Forward.h | 1 + Libraries/LibWeb/HTML/HTMLTrackElement.cpp | 49 +++++++++++++------ Libraries/LibWeb/HTML/HTMLTrackElement.h | 7 ++- Libraries/LibWeb/HTML/TextTrack.cpp | 29 +++++++++++ Libraries/LibWeb/HTML/TextTrack.h | 8 ++- Libraries/LibWeb/HTML/TextTrackObserver.cpp | 43 ++++++++++++++++ Libraries/LibWeb/HTML/TextTrackObserver.h | 35 +++++++++++++ Tests/LibWeb/Assets/track.vtt | 4 ++ .../HTMLTrackElement-processing-model.txt | 1 + .../HTMLTrackElement-processing-model.html | 21 ++++++++ 11 files changed, 182 insertions(+), 17 deletions(-) create mode 100644 Libraries/LibWeb/HTML/TextTrackObserver.cpp create mode 100644 Libraries/LibWeb/HTML/TextTrackObserver.h create mode 100644 Tests/LibWeb/Assets/track.vtt create mode 100644 Tests/LibWeb/Text/expected/HTML/HTMLTrackElement-processing-model.txt create mode 100644 Tests/LibWeb/Text/input/HTML/HTMLTrackElement-processing-model.html diff --git a/Libraries/LibWeb/CMakeLists.txt b/Libraries/LibWeb/CMakeLists.txt index 2c60a5d0070..3a5a3cb14e3 100644 --- a/Libraries/LibWeb/CMakeLists.txt +++ b/Libraries/LibWeb/CMakeLists.txt @@ -543,6 +543,7 @@ set(SOURCES HTML/TextTrackCue.cpp HTML/TextTrackCueList.cpp HTML/TextTrackList.cpp + HTML/TextTrackObserver.cpp HTML/Timer.cpp HTML/TimeRanges.cpp HTML/ToggleEvent.cpp diff --git a/Libraries/LibWeb/Forward.h b/Libraries/LibWeb/Forward.h index 1542d4cece5..1eeed6c6340 100644 --- a/Libraries/LibWeb/Forward.h +++ b/Libraries/LibWeb/Forward.h @@ -626,6 +626,7 @@ class TextTrack; class TextTrackCue; class TextTrackCueList; class TextTrackList; +class TextTrackObserver; class Timer; class TimeRanges; class ToggleEvent; diff --git a/Libraries/LibWeb/HTML/HTMLTrackElement.cpp b/Libraries/LibWeb/HTML/HTMLTrackElement.cpp index c7239eefed3..f0902ca9131 100644 --- a/Libraries/LibWeb/HTML/HTMLTrackElement.cpp +++ b/Libraries/LibWeb/HTML/HTMLTrackElement.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include namespace Web::HTML { @@ -37,12 +38,14 @@ void HTMLTrackElement::initialize(JS::Realm& realm) Base::initialize(realm); m_track = TextTrack::create(realm); + m_track_observer = realm.create(realm, *m_track); } void HTMLTrackElement::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); visitor.visit(m_track); + visitor.visit(m_track_observer); visitor.visit(m_fetch_algorithms); visitor.visit(m_fetch_controller); } @@ -132,16 +135,28 @@ void HTMLTrackElement::set_track_url(String track_url) m_track_url = move(track_url); + auto track_is_hidden_or_showing = first_is_one_of(m_track->mode(), Bindings::TextTrackMode::Hidden, Bindings::TextTrackMode::Showing); + // https://html.spec.whatwg.org/multipage/media.html#start-the-track-processing-model - if (m_loading && m_fetch_controller && first_is_one_of(m_track->mode(), Bindings::TextTrackMode::Hidden, Bindings::TextTrackMode::Showing)) { + if (m_loading && m_fetch_controller && track_is_hidden_or_showing) { m_loading = false; m_fetch_controller->abort(realm(), {}); } + + // https://html.spec.whatwg.org/multipage/media.html#start-the-track-processing-model + if (m_awaiting_track_url_change && track_is_hidden_or_showing) { + m_awaiting_track_url_change = false; + + // 13. Jump to the step labeled top. + start_the_track_processing_model_parallel_steps(); + } } // https://html.spec.whatwg.org/multipage/media.html#start-the-track-processing-model void HTMLTrackElement::start_the_track_processing_model() { + auto& realm = this->realm(); + // 1. If another occurrence of this algorithm is already running for this text track and its track element, return, // letting that other algorithm take care of this element. if (m_loading) @@ -158,14 +173,15 @@ void HTMLTrackElement::start_the_track_processing_model() m_loading = true; // 4. Run the remainder of these steps in parallel, allowing whatever caused these steps to run to continue. - auto& realm = this->realm(); - Platform::EventLoopPlugin::the().deferred_invoke(GC::create_function(realm.heap(), [this, &realm] { - start_the_track_processing_model_parallel_steps(realm); + Platform::EventLoopPlugin::the().deferred_invoke(GC::create_function(realm.heap(), [this]() { + start_the_track_processing_model_parallel_steps(); })); } -void HTMLTrackElement::start_the_track_processing_model_parallel_steps(JS::Realm& realm) +void HTMLTrackElement::start_the_track_processing_model_parallel_steps() { + auto& realm = this->realm(); + // 5. Top: Await a stable state. The synchronous section consists of the following steps. // 6. ⌛ Set the text track readiness state to loading. @@ -236,17 +252,22 @@ void HTMLTrackElement::start_the_track_processing_model_parallel_steps(JS::Realm } // 11. Wait until the text track readiness state is no longer set to loading. - HTML::main_thread_event_loop().spin_until(GC::create_function(realm.heap(), [this] { - return m_track->readiness_state() != TextTrack::ReadinessState::Loading; - })); + if (m_track->readiness_state() == TextTrack::ReadinessState::Loading) { + m_track_observer->set_track_readiness_observer([this, url = move(url)](TextTrack::ReadinessState) mutable { + if (m_track->readiness_state() != TextTrack::ReadinessState::Loading) + track_became_ready(); + }); + } else { + track_became_ready(); + } +} + +void HTMLTrackElement::track_became_ready() +{ + m_track_observer->set_track_readiness_observer({}); // 12. Wait until the track URL is no longer equal to URL, at the same time as the text track mode is set to hidden or showing. - HTML::main_thread_event_loop().spin_until(GC::create_function(realm.heap(), [this, url = move(url)] { - return track_url() != url && first_is_one_of(m_track->mode(), Bindings::TextTrackMode::Hidden, Bindings::TextTrackMode::Showing); - })); - - // 13. Jump to the step labeled top. - start_the_track_processing_model_parallel_steps(realm); + m_awaiting_track_url_change = true; } void HTMLTrackElement::track_failed_to_load() diff --git a/Libraries/LibWeb/HTML/HTMLTrackElement.h b/Libraries/LibWeb/HTML/HTMLTrackElement.h index bd8e4c97220..410cf7809b7 100644 --- a/Libraries/LibWeb/HTML/HTMLTrackElement.h +++ b/Libraries/LibWeb/HTML/HTMLTrackElement.h @@ -7,8 +7,8 @@ #pragma once +#include #include -#include namespace Web::HTML { @@ -33,8 +33,9 @@ private: void set_track_url(String); void start_the_track_processing_model(); - void start_the_track_processing_model_parallel_steps(JS::Realm& realm); + void start_the_track_processing_model_parallel_steps(); + void track_became_ready(); void track_failed_to_load(); // ^DOM::Element @@ -42,6 +43,7 @@ private: virtual void inserted() override; GC::Ptr m_track; + GC::Ptr m_track_observer; // https://html.spec.whatwg.org/multipage/media.html#track-url String m_track_url {}; @@ -50,6 +52,7 @@ private: GC::Ptr m_fetch_controller; bool m_loading { false }; + bool m_awaiting_track_url_change { false }; }; } diff --git a/Libraries/LibWeb/HTML/TextTrack.cpp b/Libraries/LibWeb/HTML/TextTrack.cpp index 8ec2df5eeee..4c4bc2493d0 100644 --- a/Libraries/LibWeb/HTML/TextTrack.cpp +++ b/Libraries/LibWeb/HTML/TextTrack.cpp @@ -8,6 +8,7 @@ #include #include #include +#include namespace Web::HTML { @@ -31,6 +32,12 @@ void TextTrack::initialize(JS::Realm& realm) Base::initialize(realm); } +void TextTrack::visit_edges(Cell::Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_observers); +} + // https://html.spec.whatwg.org/multipage/media.html#dom-texttrack-kind Bindings::TextTrackKind TextTrack::kind() { @@ -98,6 +105,28 @@ WebIDL::CallbackType* TextTrack::oncuechange() return event_handler_attribute(HTML::EventNames::cuechange); } +void TextTrack::set_readiness_state(ReadinessState readiness_state) +{ + m_readiness_state = readiness_state; + + for (auto observer : m_observers) { + if (auto callback = observer->track_readiness_observer()) + callback->function()(m_readiness_state); + } +} + +void TextTrack::register_observer(Badge, TextTrackObserver& observer) +{ + auto result = m_observers.set(observer); + VERIFY(result == AK::HashSetResult::InsertedNewEntry); +} + +void TextTrack::unregister_observer(Badge, TextTrackObserver& observer) +{ + bool was_removed = m_observers.remove(observer); + VERIFY(was_removed); +} + Bindings::TextTrackKind text_track_kind_from_string(String value) { // https://html.spec.whatwg.org/multipage/media.html#attr-track-kind diff --git a/Libraries/LibWeb/HTML/TextTrack.h b/Libraries/LibWeb/HTML/TextTrack.h index 4b7f656f5d8..bf4b4dd5ca3 100644 --- a/Libraries/LibWeb/HTML/TextTrack.h +++ b/Libraries/LibWeb/HTML/TextTrack.h @@ -51,12 +51,16 @@ public: WebIDL::CallbackType* oncuechange(); ReadinessState readiness_state() { return m_readiness_state; } - void set_readiness_state(ReadinessState readiness_state) { m_readiness_state = readiness_state; } + void set_readiness_state(ReadinessState readiness_state); + + void register_observer(Badge, TextTrackObserver&); + void unregister_observer(Badge, TextTrackObserver&); private: TextTrack(JS::Realm&); virtual void initialize(JS::Realm&) override; + virtual void visit_edges(Cell::Visitor&) override; Bindings::TextTrackKind m_kind { Bindings::TextTrackKind::Subtitles }; String m_label {}; @@ -67,6 +71,8 @@ private: Bindings::TextTrackMode m_mode { Bindings::TextTrackMode::Disabled }; ReadinessState m_readiness_state { ReadinessState::NotLoaded }; + + HashTable> m_observers; }; Bindings::TextTrackKind text_track_kind_from_string(String); diff --git a/Libraries/LibWeb/HTML/TextTrackObserver.cpp b/Libraries/LibWeb/HTML/TextTrackObserver.cpp new file mode 100644 index 00000000000..209fbc17750 --- /dev/null +++ b/Libraries/LibWeb/HTML/TextTrackObserver.cpp @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2025, Tim Flynn + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include + +namespace Web::HTML { + +GC_DEFINE_ALLOCATOR(TextTrackObserver); + +TextTrackObserver::TextTrackObserver(JS::Realm& realm, TextTrack& text_track) + : Bindings::PlatformObject(realm) + , m_text_track(text_track) +{ + m_text_track->register_observer({}, *this); +} + +void TextTrackObserver::visit_edges(Cell::Visitor& visitor) +{ + Base::visit_edges(visitor); + visitor.visit(m_text_track); + visitor.visit(m_track_readiness_observer); +} + +void TextTrackObserver::finalize() +{ + Base::finalize(); + m_text_track->unregister_observer({}, *this); +} + +void TextTrackObserver::set_track_readiness_observer(Function callback) +{ + if (callback) + m_track_readiness_observer = GC::create_function(vm().heap(), move(callback)); + else + m_track_readiness_observer = nullptr; +} + +} diff --git a/Libraries/LibWeb/HTML/TextTrackObserver.h b/Libraries/LibWeb/HTML/TextTrackObserver.h new file mode 100644 index 00000000000..afc628ec3a0 --- /dev/null +++ b/Libraries/LibWeb/HTML/TextTrackObserver.h @@ -0,0 +1,35 @@ +/* + * Copyright (c) 2025, Tim Flynn + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#pragma once + +#include +#include +#include +#include +#include + +namespace Web::HTML { + +class TextTrackObserver final : public Bindings::PlatformObject { + WEB_PLATFORM_OBJECT(TextTrackObserver, Bindings::PlatformObject); + GC_DECLARE_ALLOCATOR(TextTrackObserver); + +public: + [[nodiscard]] GC::Ptr> track_readiness_observer() const { return m_track_readiness_observer; } + void set_track_readiness_observer(Function); + +private: + explicit TextTrackObserver(JS::Realm&, TextTrack&); + + virtual void visit_edges(Cell::Visitor&) override; + virtual void finalize() override; + + GC::Ref m_text_track; + GC::Ptr> m_track_readiness_observer; +}; + +} diff --git a/Tests/LibWeb/Assets/track.vtt b/Tests/LibWeb/Assets/track.vtt new file mode 100644 index 00000000000..e35588a1026 --- /dev/null +++ b/Tests/LibWeb/Assets/track.vtt @@ -0,0 +1,4 @@ +WEBVTT + +00:00:00.000 --> 00:00:04.299 line:80% +WFH! diff --git a/Tests/LibWeb/Text/expected/HTML/HTMLTrackElement-processing-model.txt b/Tests/LibWeb/Text/expected/HTML/HTMLTrackElement-processing-model.txt new file mode 100644 index 00000000000..1853caa7e73 --- /dev/null +++ b/Tests/LibWeb/Text/expected/HTML/HTMLTrackElement-processing-model.txt @@ -0,0 +1 @@ +PASS! diff --git a/Tests/LibWeb/Text/input/HTML/HTMLTrackElement-processing-model.html b/Tests/LibWeb/Text/input/HTML/HTMLTrackElement-processing-model.html new file mode 100644 index 00000000000..e7b46876745 --- /dev/null +++ b/Tests/LibWeb/Text/input/HTML/HTMLTrackElement-processing-model.html @@ -0,0 +1,21 @@ + + + + + +