An assertion failure inside removeChildren
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Feb 2017 09:57:28 +0000 (09:57 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Feb 2017 09:57:28 +0000 (09:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=168069

Reviewed by Brent Fulgham.

Source/WebCore:

The bug was caused by notifyRemovePendingSheet executing scripts synchronously where it shouldn't.

Removed the call to notifyRemovePendingSheetIfNeeded in notifyChildNodeRemoved. Instead, invoke it
in its call sites when they're safe.

Test: http/tests/security/move-iframe-within-focus-handler-inside-removal.html

* dom/ContainerNode.cpp:
(WebCore::ContainerNode::takeAllChildrenFrom):
(WebCore::ContainerNode::notifyChildInserted):
(WebCore::ContainerNode::removeChild):
(WebCore::ContainerNode::parserRemoveChild):
(WebCore::ContainerNode::replaceAllChildren):
(WebCore::ContainerNode::removeChildren):
* dom/ContainerNodeAlgorithms.cpp:
(WebCore::notifyChildNodeRemoved):

LayoutTests:

* http/tests/security/move-iframe-within-focus-handler-inside-removal-expected.txt: Added.
* http/tests/security/move-iframe-within-focus-handler-inside-removal.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@212354 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/security/move-iframe-within-focus-handler-inside-removal-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/security/move-iframe-within-focus-handler-inside-removal.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/ContainerNodeAlgorithms.cpp

index 9b64720..566a7ad 100644 (file)
@@ -1,3 +1,13 @@
+2017-02-14  Ryosuke Niwa  <rniwa@webkit.org>
+
+        An assertion failure inside removeChildren
+        https://bugs.webkit.org/show_bug.cgi?id=168069
+
+        Reviewed by Brent Fulgham.
+
+        * http/tests/security/move-iframe-within-focus-handler-inside-removal-expected.txt: Added.
+        * http/tests/security/move-iframe-within-focus-handler-inside-removal.html: Added.
+
 2017-02-14  Brent Fulgham  <bfulgham@apple.com>
 
         Revalidate URL after events that could trigger navigations
diff --git a/LayoutTests/http/tests/security/move-iframe-within-focus-handler-inside-removal-expected.txt b/LayoutTests/http/tests/security/move-iframe-within-focus-handler-inside-removal-expected.txt
new file mode 100644 (file)
index 0000000..b0a4a2d
--- /dev/null
@@ -0,0 +1,11 @@
+Tests moving an iframe inside a focus event handler while removing a node.
+To manually test, open this page on a new window/tab each time. Reloading would not work.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS newGlobal is not oldGlobal
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/security/move-iframe-within-focus-handler-inside-removal.html b/LayoutTests/http/tests/security/move-iframe-within-focus-handler-inside-removal.html
new file mode 100644 (file)
index 0000000..a9dfe8b
--- /dev/null
@@ -0,0 +1,44 @@
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+
+description(`Tests moving an iframe inside a focus event handler while removing a node.<br>
+<strong>To manually test, open this page on a new window/tab each time. Reloading would not work.</strong>`);
+
+const iframe = document.createElement('iframe');
+document.body.appendChild(iframe);
+const doc = iframe.contentDocument;
+
+doc.body.innerHTML = `<a id="target" href="#"></a><div><link rel="stylesheet" href="data:,d"></div>`;
+let target = doc.querySelector('#target');
+const linkParent = doc.querySelector('link').parentNode;
+
+window.movedFrame = doc.createElement('iframe');
+const helperFrame = doc.createElement('iframe');
+doc.body.appendChild(helperFrame);
+
+target.onfocus = () => {
+    linkParent.appendChild(movedFrame);
+    const helperContent = `<svg xmlns="http://www.w3.org/2000/svg"><script>top.check();<\/script></svg>`;
+    helperFrame.src = URL.createObjectURL(new Blob([helperContent], {type: 'text/xml'}));
+};
+
+function check() {
+    window.oldGlobal = movedFrame.contentWindow;
+    doc.body.appendChild(movedFrame);
+    window.newGlobal = movedFrame.contentWindow;
+    shouldNotBe('newGlobal', 'oldGlobal');
+    iframe.remove();
+    finishJSTest();
+}
+
+iframe.contentWindow.location.hash = target.id;
+linkParent.textContent = '';
+
+var jsTestIsAsync = true;
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index eb6010e..c1e55bb 100644 (file)
@@ -1,3 +1,27 @@
+2017-02-14  Ryosuke Niwa  <rniwa@webkit.org>
+
+        An assertion failure inside removeChildren
+        https://bugs.webkit.org/show_bug.cgi?id=168069
+
+        Reviewed by Brent Fulgham.
+
+        The bug was caused by notifyRemovePendingSheet executing scripts synchronously where it shouldn't.
+
+        Removed the call to notifyRemovePendingSheetIfNeeded in notifyChildNodeRemoved. Instead, invoke it
+        in its call sites when they're safe.
+
+        Test: http/tests/security/move-iframe-within-focus-handler-inside-removal.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::takeAllChildrenFrom):
+        (WebCore::ContainerNode::notifyChildInserted):
+        (WebCore::ContainerNode::removeChild):
+        (WebCore::ContainerNode::parserRemoveChild):
+        (WebCore::ContainerNode::replaceAllChildren):
+        (WebCore::ContainerNode::removeChildren):
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore::notifyChildNodeRemoved):
+
 2017-02-15  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GStreamer] Several tests are timing out after r212349
index 1d999c0..e85fde0 100644 (file)
@@ -145,6 +145,8 @@ void ContainerNode::takeAllChildrenFrom(ContainerNode* oldParent)
         childrenChanged(change);
     }
 
+    document().notifyRemovePendingSheetIfNeeded();
+
     // FIXME: assert that we don't dispatch events here since this container node is still disconnected.
     for (auto& child : children) {
         RELEASE_ASSERT(!child->parentNode() && &child->treeScope() == &treeScope());
@@ -362,6 +364,9 @@ void ContainerNode::notifyChildInserted(Node& child, const ChildChange& change)
 
     childrenChanged(change);
 
+    // Some call sites may have deleted child nodes. Calling it earlier results in bugs like webkit.org/b/168069.
+    document().notifyRemovePendingSheetIfNeeded();
+
     for (auto& target : postInsertionNotificationTargets)
         target->finishedInsertingSubtree();
 }
@@ -549,6 +554,7 @@ ExceptionOr<void> ContainerNode::removeChild(Node& oldChild)
         notifyChildRemoved(child, prev, next, ChildChangeSourceAPI);
     }
 
+    document().notifyRemovePendingSheetIfNeeded();
     rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();
 
@@ -609,6 +615,7 @@ void ContainerNode::parserRemoveChild(Node& oldChild)
     removeBetween(prev, next, oldChild);
 
     notifyChildRemoved(oldChild, prev, next, ChildChangeSourceParser);
+    document().notifyRemovePendingSheetIfNeeded();
 }
 
 // https://dom.spec.whatwg.org/#concept-node-replace-all
@@ -702,6 +709,7 @@ void ContainerNode::removeChildren()
         childrenChanged(change);
     }
 
+    document().notifyRemovePendingSheetIfNeeded();
     rebuildSVGExtensionsElementsIfNecessary();
     dispatchSubtreeModifiedEvent();
 }
index 6cff2aa..d46c4d3 100644 (file)
@@ -159,7 +159,6 @@ void notifyChildNodeRemoved(ContainerNode& insertionPoint, Node& child)
         return;
     }
     notifyNodeRemovedFromDocument(insertionPoint, child);
-    child.document().notifyRemovePendingSheetIfNeeded();
 }
 
 void addChildNodesToDeletionQueue(Node*& head, Node*& tail, ContainerNode& container)