From 001d7da25b194adcf2f7dad00407eb3ae5a40736 Mon Sep 17 00:00:00 2001 From: Jelle Raaijmakers Date: Thu, 17 Oct 2024 10:46:27 +0200 Subject: [PATCH 1/6] LibWeb: Implement pending promises in BaseAudioContext Move the pending promises list from AudioContext to BaseAudioContext and deal with all remaining FIXMEs. (cherry picked from commit 2df3488840ccf8e8c560bde4826897c2023dbe29) --- .../LibWeb/WebAudio/AudioContext.cpp | 25 ++++++++++--------- .../Libraries/LibWeb/WebAudio/AudioContext.h | 1 - .../LibWeb/WebAudio/BaseAudioContext.cpp | 15 ++++++++--- .../LibWeb/WebAudio/BaseAudioContext.h | 1 + 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp b/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp index 57c23b0e921f0c..25cf10e5220901 100644 --- a/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp +++ b/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp @@ -92,7 +92,6 @@ void AudioContext::initialize(JS::Realm& realm) void AudioContext::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); - visitor.visit(m_pending_promises); visitor.visit(m_pending_resume_promises); } @@ -107,7 +106,6 @@ AudioTimestamp AudioContext::get_output_timestamp() WebIDL::ExceptionOr> AudioContext::resume() { auto& realm = this->realm(); - auto& vm = realm.vm(); // 1. If this's relevant global object's associated Document is not fully active then return a promise rejected with "InvalidStateError" DOMException. auto const& associated_document = verify_cast(HTML::relevant_global_object(*this)).associated_document(); @@ -128,8 +126,8 @@ WebIDL::ExceptionOr> AudioContext::resume() // 5. If the context is not allowed to start, append promise to [[pending promises]] and [[pending resume promises]] and abort these steps, returning promise. if (m_allowed_to_start) { - TRY_OR_THROW_OOM(vm, m_pending_promises.try_append(promise)); - TRY_OR_THROW_OOM(vm, m_pending_resume_promises.try_append(promise)); + m_pending_promises.append(promise); + m_pending_resume_promises.append(promise); } // 6. Set the [[control thread state]] on the AudioContext to running. @@ -150,25 +148,29 @@ WebIDL::ExceptionOr> AudioContext::resume() // 7.4.1: Reject all promises from [[pending resume promises]] in order, then clear [[pending resume promises]]. for (auto const& promise : m_pending_resume_promises) { WebIDL::reject_promise(realm, promise, JS::js_null()); + + // 7.4.2: Additionally, remove those promises from [[pending promises]]. + m_pending_promises.remove_first_matching([&promise](auto& pending_promise) { + return pending_promise == promise; + }); } m_pending_resume_promises.clear(); - - // FIXME: 7.4.2: Additionally, remove those promises from [[pending promises]]. })); } // 7.5: queue a media element task to execute the following steps: queue_a_media_element_task(JS::create_heap_function(heap(), [&realm, promise, this]() { // 7.5.1: Resolve all promises from [[pending resume promises]] in order. + // 7.5.2: Clear [[pending resume promises]]. Additionally, remove those promises from + // [[pending promises]]. for (auto const& pending_resume_promise : m_pending_resume_promises) { *pending_resume_promise->resolve(); + m_pending_promises.remove_first_matching([&pending_resume_promise](auto& pending_promise) { + return pending_promise == pending_resume_promise; + }); } - - // 7.5.2: Clear [[pending resume promises]]. m_pending_resume_promises.clear(); - // FIXME: Additionally, remove those promises from [[pending promises]]. - // 7.5.3: Resolve promise. *promise->resolve(); @@ -192,7 +194,6 @@ WebIDL::ExceptionOr> AudioContext::resume() WebIDL::ExceptionOr> AudioContext::suspend() { auto& realm = this->realm(); - auto& vm = realm.vm(); // 1. If this's relevant global object's associated Document is not fully active then return a promise rejected with "InvalidStateError" DOMException. auto const& associated_document = verify_cast(HTML::relevant_global_object(*this)).associated_document(); @@ -209,7 +210,7 @@ WebIDL::ExceptionOr> AudioContext::suspend() } // 4. Append promise to [[pending promises]]. - TRY_OR_THROW_OOM(vm, m_pending_promises.try_append(promise)); + m_pending_promises.append(promise); // 5. Set [[suspended by user]] to true. m_suspended_by_user = true; diff --git a/Userland/Libraries/LibWeb/WebAudio/AudioContext.h b/Userland/Libraries/LibWeb/WebAudio/AudioContext.h index a1155f567813c9..daad254029c034 100644 --- a/Userland/Libraries/LibWeb/WebAudio/AudioContext.h +++ b/Userland/Libraries/LibWeb/WebAudio/AudioContext.h @@ -49,7 +49,6 @@ class AudioContext final : public BaseAudioContext { double m_output_latency { 0 }; bool m_allowed_to_start = true; - Vector> m_pending_promises; Vector> m_pending_resume_promises; bool m_suspended_by_user = false; diff --git a/Userland/Libraries/LibWeb/WebAudio/BaseAudioContext.cpp b/Userland/Libraries/LibWeb/WebAudio/BaseAudioContext.cpp index df7ca9064ca92b..7839b56d3b0f9f 100644 --- a/Userland/Libraries/LibWeb/WebAudio/BaseAudioContext.cpp +++ b/Userland/Libraries/LibWeb/WebAudio/BaseAudioContext.cpp @@ -43,6 +43,7 @@ void BaseAudioContext::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); visitor.visit(m_destination); + visitor.visit(m_pending_promises); } void BaseAudioContext::set_onstatechange(WebIDL::CallbackType* event_handler) @@ -144,7 +145,8 @@ JS::NonnullGCPtr BaseAudioContext::decode_audio_data(JS::Handle BaseAudioContext::decode_audio_data(JS::Handle m_destination; + Vector> m_pending_promises; private: void queue_a_decoding_operation(JS::NonnullGCPtr, JS::Handle, JS::GCPtr, JS::GCPtr); From c0e346c577ace39f34cb66b342a1d8cd5f1f0070 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sun, 27 Oct 2024 13:41:28 +1300 Subject: [PATCH 2/6] LibWeb: Add temp execution context for resolving promise in AudioContext Fixes a crash seen on twitter.com, namely from the 'resume' function. (cherry picked from commit 5d7a7a43c40f2413f6936ddd69794f8550172e09) --- Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp b/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp index 25cf10e5220901..80fd4011f0f9a1 100644 --- a/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp +++ b/Userland/Libraries/LibWeb/WebAudio/AudioContext.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -145,6 +146,8 @@ WebIDL::ExceptionOr> AudioContext::resume() if (!start_rendering_audio_graph()) { // 7.4: In case of failure, queue a media element task to execute the following steps: queue_a_media_element_task(JS::create_heap_function(heap(), [&realm, this]() { + HTML::TemporaryExecutionContext context(Bindings::host_defined_environment_settings_object(realm), HTML::TemporaryExecutionContext::CallbacksEnabled::Yes); + // 7.4.1: Reject all promises from [[pending resume promises]] in order, then clear [[pending resume promises]]. for (auto const& promise : m_pending_resume_promises) { WebIDL::reject_promise(realm, promise, JS::js_null()); @@ -160,6 +163,8 @@ WebIDL::ExceptionOr> AudioContext::resume() // 7.5: queue a media element task to execute the following steps: queue_a_media_element_task(JS::create_heap_function(heap(), [&realm, promise, this]() { + HTML::TemporaryExecutionContext context(Bindings::host_defined_environment_settings_object(realm), HTML::TemporaryExecutionContext::CallbacksEnabled::Yes); + // 7.5.1: Resolve all promises from [[pending resume promises]] in order. // 7.5.2: Clear [[pending resume promises]]. Additionally, remove those promises from // [[pending promises]]. @@ -228,6 +233,8 @@ WebIDL::ExceptionOr> AudioContext::suspend() // 7.3: queue a media element task to execute the following steps: queue_a_media_element_task(JS::create_heap_function(heap(), [&realm, promise, this]() { + HTML::TemporaryExecutionContext context(Bindings::host_defined_environment_settings_object(realm), HTML::TemporaryExecutionContext::CallbacksEnabled::Yes); + // 7.3.1: Resolve promise. *promise->resolve(); @@ -281,6 +288,8 @@ WebIDL::ExceptionOr> AudioContext::close() // 5.4: queue a media element task to execute the following steps: queue_a_media_element_task(JS::create_heap_function(heap(), [&realm, promise, this]() { + HTML::TemporaryExecutionContext context(Bindings::host_defined_environment_settings_object(realm), HTML::TemporaryExecutionContext::CallbacksEnabled::Yes); + // 5.4.1: Resolve promise. *promise->resolve(); From 9fd021bf77a4eaafc229eb2d176f7446756fa76b Mon Sep 17 00:00:00 2001 From: Luke Wilde Date: Tue, 12 Nov 2024 18:08:49 +0000 Subject: [PATCH 3/6] LibWeb: Actually traverse the shadow root of the inclusive descendant Previously, the inclusive descendant, which is the node that for_each_shadow_including_inclusive_descendant was called on, would not have it's shadow root traversed if it had one. This is because the shadow root traversal was in the `for` loop, which begins with the node's first child. The fix here is to move the shadow root traversal outside of the loop, and check if the current node is an element instead. (cherry picked from commit 6df4e5f5e75d7d6d439d1365ed7bed4fd2a8a1da) --- ...boundary-of-inserted-node-is-traversed.txt | 1 + ...oundary-of-inserted-node-is-traversed.html | 14 ++++++++ Userland/Libraries/LibWeb/DOM/ShadowRoot.h | 34 ++++++++++++------- 3 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/DOM/shadow-root-boundary-of-inserted-node-is-traversed.txt create mode 100644 Tests/LibWeb/Text/input/DOM/shadow-root-boundary-of-inserted-node-is-traversed.html diff --git a/Tests/LibWeb/Text/expected/DOM/shadow-root-boundary-of-inserted-node-is-traversed.txt b/Tests/LibWeb/Text/expected/DOM/shadow-root-boundary-of-inserted-node-is-traversed.txt new file mode 100644 index 00000000000000..2bfbd60d9c4dd4 --- /dev/null +++ b/Tests/LibWeb/Text/expected/DOM/shadow-root-boundary-of-inserted-node-is-traversed.txt @@ -0,0 +1 @@ +Hello from script in the shadow root of the just inserted div! diff --git a/Tests/LibWeb/Text/input/DOM/shadow-root-boundary-of-inserted-node-is-traversed.html b/Tests/LibWeb/Text/input/DOM/shadow-root-boundary-of-inserted-node-is-traversed.html new file mode 100644 index 00000000000000..b545db3d620b73 --- /dev/null +++ b/Tests/LibWeb/Text/input/DOM/shadow-root-boundary-of-inserted-node-is-traversed.html @@ -0,0 +1,14 @@ + + + diff --git a/Userland/Libraries/LibWeb/DOM/ShadowRoot.h b/Userland/Libraries/LibWeb/DOM/ShadowRoot.h index c76c3b28878a1e..d0a1c01350d913 100644 --- a/Userland/Libraries/LibWeb/DOM/ShadowRoot.h +++ b/Userland/Libraries/LibWeb/DOM/ShadowRoot.h @@ -97,21 +97,37 @@ class ShadowRoot final : public DocumentFragment { template<> inline bool Node::fast_is() const { return node_type() == to_underlying(NodeType::DOCUMENT_FRAGMENT_NODE) && is_shadow_root(); } +// https://dom.spec.whatwg.org/#concept-shadow-including-tree-order +// In shadow-including tree order is shadow-including preorder, depth-first traversal of a node tree. +// Shadow-including preorder, depth-first traversal of a node tree tree is preorder, depth-first traversal +// of tree, with for each shadow host encountered in tree, shadow-including preorder, depth-first traversal +// of that element’s shadow root’s node tree just after it is encountered. + +// https://dom.spec.whatwg.org/#concept-shadow-including-descendant +// An object A is a shadow-including descendant of an object B, if A is a descendant of B, or A’s root is a +// shadow root and A’s root’s host is a shadow-including inclusive descendant of B. + +// https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-descendant +// A shadow-including inclusive descendant is an object or one of its shadow-including descendants. + template inline TraversalDecision Node::for_each_shadow_including_inclusive_descendant(Callback callback) { if (callback(*this) == TraversalDecision::Break) return TraversalDecision::Break; - for (auto* child = first_child(); child; child = child->next_sibling()) { - if (child->is_element()) { - if (auto shadow_root = static_cast(child)->shadow_root()) { - if (shadow_root->for_each_shadow_including_inclusive_descendant(callback) == TraversalDecision::Break) - return TraversalDecision::Break; - } + + if (is_element()) { + if (auto shadow_root = static_cast(this)->shadow_root()) { + if (shadow_root->for_each_shadow_including_inclusive_descendant(callback) == TraversalDecision::Break) + return TraversalDecision::Break; } + } + + for (auto* child = first_child(); child; child = child->next_sibling()) { if (child->for_each_shadow_including_inclusive_descendant(callback) == TraversalDecision::Break) return TraversalDecision::Break; } + return TraversalDecision::Continue; } @@ -119,12 +135,6 @@ template inline TraversalDecision Node::for_each_shadow_including_descendant(Callback callback) { for (auto* child = first_child(); child; child = child->next_sibling()) { - if (child->is_element()) { - if (JS::GCPtr shadow_root = static_cast(child)->shadow_root()) { - if (shadow_root->for_each_shadow_including_inclusive_descendant(callback) == TraversalDecision::Break) - return TraversalDecision::Break; - } - } if (child->for_each_shadow_including_inclusive_descendant(callback) == TraversalDecision::Break) return TraversalDecision::Break; } From 0be1cf35b4a6ee087188c5fa0f139f399d5fc854 Mon Sep 17 00:00:00 2001 From: Luke Wilde Date: Tue, 12 Nov 2024 18:16:08 +0000 Subject: [PATCH 4/6] LibWeb: Make iframe insertion steps check the shadow including root The insertion steps for iframes were following an old version of the spec, where it was checking if the iframe was "in a document tree", which doesn't cross shadow root boundaries. The spec has since been updated to check the shadow including root instead. This is now needed for Cloudflare Turnstile iframe widgets to appear, as they are now inserted into a shadow root. (cherry picked from commit 4dd14d812f73377cb1efa5a8e8a114912a90b5af) --- ...rame-successfully-loads-in-shadow-root.txt | 2 ++ ...ame-successfully-loads-in-shadow-root.html | 25 +++++++++++++++ .../LibWeb/HTML/HTMLIFrameElement.cpp | 31 ++++++++++++------- 3 files changed, 47 insertions(+), 11 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/HTML/iframe-successfully-loads-in-shadow-root.txt create mode 100644 Tests/LibWeb/Text/input/HTML/iframe-successfully-loads-in-shadow-root.html diff --git a/Tests/LibWeb/Text/expected/HTML/iframe-successfully-loads-in-shadow-root.txt b/Tests/LibWeb/Text/expected/HTML/iframe-successfully-loads-in-shadow-root.txt new file mode 100644 index 00000000000000..e3c171e4a7e464 --- /dev/null +++ b/Tests/LibWeb/Text/expected/HTML/iframe-successfully-loads-in-shadow-root.txt @@ -0,0 +1,2 @@ +Received a message: 'Hello from iframe in the shadow root of the just inserted div!' +Was it from the shadow root iframe? true diff --git a/Tests/LibWeb/Text/input/HTML/iframe-successfully-loads-in-shadow-root.html b/Tests/LibWeb/Text/input/HTML/iframe-successfully-loads-in-shadow-root.html new file mode 100644 index 00000000000000..21ba827ee83b4d --- /dev/null +++ b/Tests/LibWeb/Text/input/HTML/iframe-successfully-loads-in-shadow-root.html @@ -0,0 +1,25 @@ + + + diff --git a/Userland/Libraries/LibWeb/HTML/HTMLIFrameElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLIFrameElement.cpp index da0cdad6379dcb..1cf23a8e5b025d 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLIFrameElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLIFrameElement.cpp @@ -58,17 +58,26 @@ void HTMLIFrameElement::inserted() { HTMLElement::inserted(); - // When an iframe element element is inserted into a document whose browsing context is non-null, the user agent must run these steps: - if (in_a_document_tree() && document().browsing_context() && document().is_fully_active()) { - // 1. Create a new child navigable for element. - MUST(create_new_child_navigable(JS::create_heap_function(realm().heap(), [this] { - // 3. Process the iframe attributes for element, with initialInsertion set to true. - process_the_iframe_attributes(true); - set_content_navigable_initialized(); - }))); - - // FIXME: 2. If element has a sandbox attribute, then parse the sandboxing directive given the attribute's value and element's iframe sandboxing flag set. - } + // The iframe HTML element insertion steps, given insertedNode, are: + // 1. If insertedNode's shadow-including root's browsing context is null, then return. + if (!is(shadow_including_root())) + return; + + DOM::Document& document = verify_cast(shadow_including_root()); + + // NOTE: The check for "not fully active" is to prevent a crash on the dom/nodes/node-appendchild-crash.html WPT test. + if (!document.browsing_context() || !document.is_fully_active()) + return; + + // 2. Create a new child navigable for insertedNode. + MUST(create_new_child_navigable(JS::create_heap_function(realm().heap(), [this] { + // FIXME: 3. If insertedNode has a sandbox attribute, then parse the sandboxing directive given the attribute's + // value and insertedNode's iframe sandboxing flag set. + + // 4. Process the iframe attributes for insertedNode, with initialInsertion set to true. + process_the_iframe_attributes(true); + set_content_navigable_initialized(); + }))); } // https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes From 445b8583dd8094c93d0ed2b834457cdf96028269 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 13 Nov 2024 08:16:24 -0500 Subject: [PATCH 5/6] LibWeb: Use GCPtr in MediaQueryList (cherry picked from commit 213155ad7d2cc32271bebce7fecdb35d1e286f22) --- Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp | 4 ++-- Userland/Libraries/LibWeb/CSS/MediaQueryList.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp b/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp index cb8b9de37df90c..7aa76915cd0f11 100644 --- a/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp +++ b/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp @@ -73,7 +73,7 @@ bool MediaQueryList::evaluate() } // https://www.w3.org/TR/cssom-view/#dom-mediaquerylist-addlistener -void MediaQueryList::add_listener(DOM::IDLEventListener* listener) +void MediaQueryList::add_listener(JS::GCPtr listener) { // 1. If listener is null, terminate these steps. if (!listener) @@ -87,7 +87,7 @@ void MediaQueryList::add_listener(DOM::IDLEventListener* listener) } // https://www.w3.org/TR/cssom-view/#dom-mediaquerylist-removelistener -void MediaQueryList::remove_listener(DOM::IDLEventListener* listener) +void MediaQueryList::remove_listener(JS::GCPtr listener) { // 1. Remove an event listener from the associated list of event listeners, whose type is change, callback is listener, and capture is false. // NOTE: While the spec doesn't technically use remove_event_listener and instead manipulates the list directly, every major engine uses remove_event_listener. diff --git a/Userland/Libraries/LibWeb/CSS/MediaQueryList.h b/Userland/Libraries/LibWeb/CSS/MediaQueryList.h index 728f9e7bf63c01..1a6832612d6ec9 100644 --- a/Userland/Libraries/LibWeb/CSS/MediaQueryList.h +++ b/Userland/Libraries/LibWeb/CSS/MediaQueryList.h @@ -26,8 +26,8 @@ class MediaQueryList final : public DOM::EventTarget { bool matches() const; bool evaluate(); - void add_listener(DOM::IDLEventListener*); - void remove_listener(DOM::IDLEventListener*); + void add_listener(JS::GCPtr); + void remove_listener(JS::GCPtr); void set_onchange(WebIDL::CallbackType*); WebIDL::CallbackType* onchange(); From 455b3d5698b33e3f68b30c85e87ff98d728c2532 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 13 Nov 2024 08:19:53 -0500 Subject: [PATCH 6/6] LibWeb: Guard MediaQueryList event listener removal against null A recently imported WPT test has a subtest that effectively does the following: const mql = window.matchMedia(""); mql.removeListener(null); (cherry picked from commit 3e5476c9e09055104fc64d294da39aa1bfccac09) --- Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp b/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp index 7aa76915cd0f11..b83597611c847e 100644 --- a/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp +++ b/Userland/Libraries/LibWeb/CSS/MediaQueryList.cpp @@ -92,7 +92,8 @@ void MediaQueryList::remove_listener(JS::GCPtr listener) // 1. Remove an event listener from the associated list of event listeners, whose type is change, callback is listener, and capture is false. // NOTE: While the spec doesn't technically use remove_event_listener and instead manipulates the list directly, every major engine uses remove_event_listener. // This means if an event listener removes another event listener that comes after it, the removed event listener will not be invoked. - remove_event_listener_without_options(HTML::EventNames::change, *listener); + if (listener) + remove_event_listener_without_options(HTML::EventNames::change, *listener); } void MediaQueryList::set_onchange(WebIDL::CallbackType* event_handler)