From 981616f91868de481f55e793a7632e6e133a743e Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Thu, 26 Dec 2024 01:06:09 -0500 Subject: [PATCH] Don't run scripts while DOM tree is undergoing mutations (#34505) * script: Implement node insertion post-connection hook. Ensure script elements only run scripts when the DOM has stabilized. Signed-off-by: Josh Matthews * script: Make iframe element use post-connection steps when handling initial document insertion. Signed-off-by: Josh Matthews * script: Use a delayed task when running post-connection steps. Signed-off-by: Josh Matthews * script: Add explanatory comment. Signed-off-by: Josh Matthews * Tidy. Signed-off-by: Josh Matthews --------- Signed-off-by: Josh Matthews --- components/script/dom/htmliframeelement.rs | 37 +++++++------------ components/script/dom/htmlscriptelement.rs | 27 +++++++++----- components/script/dom/node.rs | 33 +++++++++++++++++ components/script/dom/virtualmethods.rs | 9 +++++ ...e-scripts-from-fragment.tentative.html.ini | 3 -- ...pendChild-three-scripts.tentative.html.ini | 3 -- .../execution-timing/147.html.ini | 3 -- .../Document-prototype-currentScript.html.ini | 12 ++---- tests/wpt/mozilla/meta/MANIFEST.json | 7 ++++ .../mozilla/layout_blocker_operations.html | 34 +++++++++++++++++ 10 files changed, 117 insertions(+), 51 deletions(-) delete mode 100644 tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts-from-fragment.tentative.html.ini delete mode 100644 tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts.tentative.html.ini delete mode 100644 tests/wpt/meta/html/semantics/scripting-1/the-script-element/execution-timing/147.html.ini create mode 100644 tests/wpt/mozilla/tests/mozilla/layout_blocker_operations.html diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs index 9b4ad049c6e..c0b43296f22 100644 --- a/components/script/dom/htmliframeelement.rs +++ b/components/script/dom/htmliframeelement.rs @@ -26,7 +26,6 @@ use crate::dom::bindings::cell::DomRefCell; use crate::dom::bindings::codegen::Bindings::HTMLIFrameElementBinding::HTMLIFrameElementMethods; use crate::dom::bindings::codegen::Bindings::WindowBinding::Window_Binding::WindowMethods; use crate::dom::bindings::inheritance::Castable; -use crate::dom::bindings::refcounted::Trusted; use crate::dom::bindings::reflector::DomObject; use crate::dom::bindings::root::{DomRoot, LayoutDom, MutNullableDom}; use crate::dom::bindings::str::{DOMString, USVString}; @@ -38,9 +37,7 @@ use crate::dom::element::{ use crate::dom::eventtarget::EventTarget; use crate::dom::globalscope::GlobalScope; use crate::dom::htmlelement::HTMLElement; -use crate::dom::node::{ - document_from_node, window_from_node, BindContext, Node, NodeDamage, UnbindContext, -}; +use crate::dom::node::{document_from_node, window_from_node, Node, NodeDamage, UnbindContext}; use crate::dom::virtualmethods::VirtualMethods; use crate::dom::windowproxy::WindowProxy; use crate::script_runtime::CanGc; @@ -741,28 +738,22 @@ impl VirtualMethods for HTMLIFrameElement { } } - fn bind_to_tree(&self, context: &BindContext) { + fn post_connection_steps(&self) { if let Some(s) = self.super_type() { - s.bind_to_tree(context); + s.post_connection_steps(); } - let tree_connected = context.tree_connected; - let iframe = Trusted::new(self); - document_from_node(self).add_delayed_task(task!(IFrameDelayedInitialize: move || { - let this = iframe.root(); - // https://html.spec.whatwg.org/multipage/#the-iframe-element - // "When an iframe element is inserted into a document that has - // a browsing context, the user agent must create a new - // browsing context, set the element's nested browsing context - // to the newly-created browsing context, and then process the - // iframe attributes for the "first time"." - if this.upcast::().is_connected_with_browsing_context() { - debug!("iframe bound to browsing context."); - debug_assert!(tree_connected, "is_connected_with_bc, but not tree_connected"); - this.create_nested_browsing_context(CanGc::note()); - this.process_the_iframe_attributes(ProcessingMode::FirstTime, CanGc::note()); - } - })); + // https://html.spec.whatwg.org/multipage/#the-iframe-element + // "When an iframe element is inserted into a document that has + // a browsing context, the user agent must create a new + // browsing context, set the element's nested browsing context + // to the newly-created browsing context, and then process the + // iframe attributes for the "first time"." + if self.upcast::().is_connected_with_browsing_context() { + debug!("iframe bound to browsing context."); + self.create_nested_browsing_context(CanGc::note()); + self.process_the_iframe_attributes(ProcessingMode::FirstTime, CanGc::note()); + } } fn unbind_from_tree(&self, context: &UnbindContext) { diff --git a/components/script/dom/htmlscriptelement.rs b/components/script/dom/htmlscriptelement.rs index 4ab12c60df1..244a18568c4 100644 --- a/components/script/dom/htmlscriptelement.rs +++ b/components/script/dom/htmlscriptelement.rs @@ -55,7 +55,7 @@ use crate::dom::event::{Event, EventBubbles, EventCancelable, EventStatus}; use crate::dom::globalscope::GlobalScope; use crate::dom::htmlelement::HTMLElement; use crate::dom::node::{ - document_from_node, window_from_node, BindContext, ChildrenMutation, CloneChildrenFlag, Node, + document_from_node, window_from_node, ChildrenMutation, CloneChildrenFlag, Node, }; use crate::dom::performanceresourcetiming::InitiatorType; use crate::dom::virtualmethods::VirtualMethods; @@ -1188,25 +1188,32 @@ impl VirtualMethods for HTMLScriptElement { } } + /// fn children_changed(&self, mutation: &ChildrenMutation) { if let Some(s) = self.super_type() { s.children_changed(mutation); } - if !self.parser_inserted.get() && self.upcast::().is_connected() { - self.prepare(CanGc::note()); + + if self.upcast::().is_connected() && !self.parser_inserted.get() { + let script = Trusted::new(self); + // This method can be invoked while there are script/layout blockers present + // as DOM mutations have not yet settled. We use a delayed task to avoid + // running any scripts until the DOM tree is safe for interactions. + document_from_node(self).add_delayed_task(task!(ScriptPrepare: move || { + let this = script.root(); + this.prepare(CanGc::note()); + })); } } - fn bind_to_tree(&self, context: &BindContext) { + /// + fn post_connection_steps(&self) { if let Some(s) = self.super_type() { - s.bind_to_tree(context); + s.post_connection_steps(); } - if context.tree_connected && !self.parser_inserted.get() { - let script = Trusted::new(self); - document_from_node(self).add_delayed_task(task!(ScriptDelayedInitialize: move || { - script.root().prepare(CanGc::note()); - })); + if self.upcast::().is_connected() && !self.parser_inserted.get() { + self.prepare(CanGc::note()); } } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 496468d2f8c..792812acfbc 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -68,6 +68,7 @@ use crate::dom::bindings::inheritance::{ Castable, CharacterDataTypeId, ElementTypeId, EventTargetTypeId, HTMLElementTypeId, NodeTypeId, SVGElementTypeId, SVGGraphicsElementTypeId, TextTypeId, }; +use crate::dom::bindings::refcounted::Trusted; use crate::dom::bindings::reflector::{reflect_dom_object_with_proto, DomObject, DomObjectWrap}; use crate::dom::bindings::root::{Dom, DomRoot, DomSlice, LayoutDom, MutNullableDom}; use crate::dom::bindings::str::{DOMString, USVString}; @@ -2128,6 +2129,38 @@ impl Node { }; MutationObserver::queue_a_mutation_record(parent, mutation); } + + // Step 10. Let staticNodeList be a list of nodes, initially « ». + let mut static_node_list = vec![]; + + // Step 11. For each node of nodes, in tree order: + for node in new_nodes { + // Step 11.1 For each shadow-including inclusive descendant inclusiveDescendant of node, + // in shadow-including tree order, append inclusiveDescendant to staticNodeList. + static_node_list.extend( + node.traverse_preorder(ShadowIncluding::Yes) + .map(|n| Trusted::new(&*n)), + ); + } + + // We use a delayed task for this step to work around an awkward interaction between + // script/layout blockers, Node::replace_all, and the children_changed vtable method. + // Any node with a post connection step that triggers layout (such as iframes) needs + // to be marked as dirty before doing so. This is handled by Node's children_changed + // callback, but when Node::insert is called as part of Node::replace_all then the + // callback is suppressed until we return to Node::replace_all. To ensure the sequence: + // 1) children_changed in Node::replace_all, + // 2) post_connection_steps from Node::insert, + // we use a delayed task that will run as soon as Node::insert removes its + // script/layout blocker. + node.owner_doc().add_delayed_task(task!(PostConnectionSteps: move || { + // Step 12. For each node of staticNodeList, if node is connected, then run the + // post-connection steps with node. + for node in static_node_list.iter().map(Trusted::root).filter(|n| n.is_connected()) { + vtable_for(&node).post_connection_steps(); + } + })); + node.owner_doc().remove_script_and_layout_blocker(); } diff --git a/components/script/dom/virtualmethods.rs b/components/script/dom/virtualmethods.rs index 16516356e83..2c329fa92a9 100644 --- a/components/script/dom/virtualmethods.rs +++ b/components/script/dom/virtualmethods.rs @@ -93,6 +93,15 @@ pub trait VirtualMethods { } } + /// Invoked during a DOM tree mutation after a node becomes connected, once all + /// related DOM tree mutations have been applied. + /// + fn post_connection_steps(&self) { + if let Some(s) = self.super_type() { + s.post_connection_steps(); + } + } + /// Called when a Node is appended to a tree, where 'tree_connected' indicates /// whether the tree is part of a Document. fn bind_to_tree(&self, context: &BindContext) { diff --git a/tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts-from-fragment.tentative.html.ini b/tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts-from-fragment.tentative.html.ini deleted file mode 100644 index 329de683b8c..00000000000 --- a/tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts-from-fragment.tentative.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[Node-appendChild-three-scripts-from-fragment.tentative.html] - [Node.appendChild: inserting three scripts from a document fragment] - expected: FAIL diff --git a/tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts.tentative.html.ini b/tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts.tentative.html.ini deleted file mode 100644 index 996233b6e71..00000000000 --- a/tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts.tentative.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[Node-appendChild-three-scripts.tentative.html] - [Node.appendChild: inserting three scripts from a div] - expected: FAIL diff --git a/tests/wpt/meta/html/semantics/scripting-1/the-script-element/execution-timing/147.html.ini b/tests/wpt/meta/html/semantics/scripting-1/the-script-element/execution-timing/147.html.ini deleted file mode 100644 index f1f32db7bb4..00000000000 --- a/tests/wpt/meta/html/semantics/scripting-1/the-script-element/execution-timing/147.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[147.html] - [scheduler: insert multiple inline scripts; first script moves subsequent scripts ] - expected: FAIL diff --git a/tests/wpt/meta/shadow-dom/Document-prototype-currentScript.html.ini b/tests/wpt/meta/shadow-dom/Document-prototype-currentScript.html.ini index 2d3f25d0767..b831fefb3be 100644 --- a/tests/wpt/meta/shadow-dom/Document-prototype-currentScript.html.ini +++ b/tests/wpt/meta/shadow-dom/Document-prototype-currentScript.html.ini @@ -1,13 +1,7 @@ [Document-prototype-currentScript.html] - expected: TIMEOUT + expected: ERROR [document.currentScript must not be set to a script element that loads an external script in an open shadow tree] - expected: TIMEOUT + expected: FAIL [document.currentScript must not be set to a script element that loads an external script in a closed shadow tree] - expected: NOTRUN - - [document.currentScript must be set to a script element that loads an external script that was in an open shadow tree and then removed] - expected: NOTRUN - - [document.currentScript must be set to a script element that loads an external script that was in a closed shadow tree and then removed] - expected: NOTRUN + expected: FAIL diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index f002e4ad775..cda69f9407c 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -13529,6 +13529,13 @@ {} ] ], + "layout_blocker_operations.html": [ + "2df68bf1c13179688708c97eab19361608b0de82", + [ + null, + {} + ] + ], "lenient_this.html": [ "960c74613f3c2809bb1f2ee6121bf14f28267051", [ diff --git a/tests/wpt/mozilla/tests/mozilla/layout_blocker_operations.html b/tests/wpt/mozilla/tests/mozilla/layout_blocker_operations.html new file mode 100644 index 00000000000..2df68bf1c13 --- /dev/null +++ b/tests/wpt/mozilla/tests/mozilla/layout_blocker_operations.html @@ -0,0 +1,34 @@ + + + + + + +
+ + + +