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 <josh@joshmatthews.net>

* script: Make iframe element use post-connection steps when handling initial document insertion.

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

* script: Use a delayed task when running post-connection steps.

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

* script: Add explanatory comment.

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

* Tidy.

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

---------

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

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::<Node>().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::<Node>().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) {

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 {
         }
     }
 
+    /// <https://html.spec.whatwg.org/multipage/#script-processing-model:the-script-element-26>
     fn children_changed(&self, mutation: &ChildrenMutation) {
         if let Some(s) = self.super_type() {
             s.children_changed(mutation);
         }
-        if !self.parser_inserted.get() && self.upcast::<Node>().is_connected() {
-            self.prepare(CanGc::note());
+
+        if self.upcast::<Node>().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) {
+    /// <https://html.spec.whatwg.org/multipage/#script-processing-model:the-script-element-20>
+    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::<Node>().is_connected() && !self.parser_inserted.get() {
+            self.prepare(CanGc::note());
         }
     }
 

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();
     }
 

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.
+    /// <https://dom.spec.whatwg.org/#concept-node-post-connection-ext>
+    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) {

tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts-from-fragment.tentative.html.ini

@@ -1,3 +0,0 @@
-[Node-appendChild-three-scripts-from-fragment.tentative.html]
-  [Node.appendChild: inserting three scripts from a document fragment]
-    expected: FAIL

tests/wpt/meta/dom/nodes/insertion-removing-steps/Node-appendChild-three-scripts.tentative.html.ini

@@ -1,3 +0,0 @@
-[Node-appendChild-three-scripts.tentative.html]
-  [Node.appendChild: inserting three scripts from a div]
-    expected: FAIL

tests/wpt/meta/html/semantics/scripting-1/the-script-element/execution-timing/147.html.ini

@@ -1,3 +0,0 @@
-[147.html]
-  [scheduler: insert multiple inline scripts; first script moves subsequent scripts ]
-    expected: FAIL

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

tests/wpt/mozilla/meta/MANIFEST.json

@@ -13529,6 +13529,13 @@
       {}
      ]
     ],
+    "layout_blocker_operations.html": [
+     "2df68bf1c13179688708c97eab19361608b0de82",
+     [
+      null,
+      {}
+     ]
+    ],
     "lenient_this.html": [
      "960c74613f3c2809bb1f2ee6121bf14f28267051",
      [

tests/wpt/mozilla/tests/mozilla/layout_blocker_operations.html

@@ -0,0 +1,34 @@
+<html>
+<head>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+</head>
+<body>
+<div id="foo"></div>
+
+<script>
+  function verifyLayoutAllowed(t) {
+      t.step(() => {
+          assert_equals(getComputedStyle(document.querySelector('#foo')).display, "block");
+          t.done();
+      })
+  }
+
+  var insertionTest;
+  async_test(function(t) {
+      insertionTest = t;
+      let script = document.createElement('script');
+      document.body.appendChild(script);
+      script.appendChild(document.createTextNode("verifyLayoutAllowed(insertionTest)"));
+  }, "Insertion");
+
+  var replacementTest;
+  async_test(function(t) {
+      replacementTest = t;
+      let script = document.createElement('script');
+      document.body.appendChild(script);
+      script.innerHTML = "verifyLayoutAllowed(replacementTest)";
+  }, "Replace");
+</script>
+</body>
+</html>