Reinstate active flag for iterators
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Aug 2017 13:04:39 +0000 (13:04 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Aug 2017 13:04:39 +0000 (13:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=175312

Reviewed by Sam Weinig.

LayoutTests/imported/w3c:

Resync WPT tests from upstream to gain test coverage.

* web-platform-tests/dom/traversal/NodeIterator-expected.txt:
* web-platform-tests/dom/traversal/NodeIterator.html:
* web-platform-tests/dom/traversal/TreeWalker-expected.txt:
* web-platform-tests/dom/traversal/TreeWalker.html:

Source/WebCore:

NodeIterator / TreeWalker should no longer allow recursive filters
after the following change to the DOM specification:
- https://github.com/whatwg/dom/pull/359

This patch aligns our behavior with the latest specification.

No new tests, updated existing tests.

* dom/NodeIterator.cpp:
(WebCore::NodeIterator::nextNode):
(WebCore::NodeIterator::previousNode):
Note that we now also call m_candidateNode.clear() before returning an
exception. This was a pre-existing bug that we failed to do so in the
exception case but it became more obvious after this change now that
we throw. This was causing traversal/moz-bug559526.html to fail
otherwise (the filter was called one too many times). The test case
is passing in Firefox (The filter is called 4 times and they throw
each time).

* dom/Traversal.cpp:
(WebCore::NodeIteratorBase::NodeIteratorBase):
(WebCore::NodeIteratorBase::acceptNode):
* dom/Traversal.h:
* dom/TreeWalker.cpp:

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

LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/dom/traversal/NodeIterator-expected.txt
LayoutTests/imported/w3c/web-platform-tests/dom/traversal/NodeIterator.html
LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-expected.txt
LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker.html
LayoutTests/traversal/moz-bug559526-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/dom/NodeIterator.cpp
Source/WebCore/dom/Traversal.cpp
Source/WebCore/dom/Traversal.h
Source/WebCore/dom/TreeWalker.cpp

index 89ae2c6..877b24e 100644 (file)
@@ -1,3 +1,17 @@
+2017-08-09  Chris Dumez  <cdumez@apple.com>
+
+        Reinstate active flag for iterators
+        https://bugs.webkit.org/show_bug.cgi?id=175312
+
+        Reviewed by Sam Weinig.
+
+        Resync WPT tests from upstream to gain test coverage.
+
+        * web-platform-tests/dom/traversal/NodeIterator-expected.txt:
+        * web-platform-tests/dom/traversal/NodeIterator.html:
+        * web-platform-tests/dom/traversal/TreeWalker-expected.txt:
+        * web-platform-tests/dom/traversal/TreeWalker.html:
+
 2017-08-07  Brady Eidson  <beidson@apple.com>
 
         Implement most of ServiceWorkerContainer::addRegistration.
index 26b7c6a..6d79457 100644 (file)
@@ -4,6 +4,7 @@ PASS createNodeIterator() parameter defaults
 PASS createNodeIterator() with null as arguments 
 PASS createNodeIterator() with undefined as arguments 
 PASS Propagate exception from filter function 
+PASS Recursive filters need to throw 
 PASS document.createNodeIterator(paras[0], 0, null) 
 PASS document.createNodeIterator(paras[0], 0, (function(node) { return true })) 
 PASS document.createNodeIterator(paras[0], 0, (function(node) { return false })) 
index 0f618ef..677858d 100644 (file)
@@ -52,6 +52,23 @@ test(function() {
   assert_throws({name: "failed"}, function() { iter.nextNode() });
 }, "Propagate exception from filter function");
 
+test(function() {
+  var depth = 0;
+  var iter = document.createNodeIterator(document, NodeFilter.SHOW_ALL,
+    function() {
+      if (iter.referenceNode != document && depth == 0) {
+        depth++;
+        iter.nextNode();
+      }
+      return NodeFilter.FILTER_ACCEPT;
+    });
+  iter.nextNode();
+  iter.nextNode();
+  assert_throws("InvalidStateError", function() { iter.nextNode() });
+  depth--;
+  assert_throws("InvalidStateError", function() { iter.previousNode() });
+}, "Recursive filters need to throw");
+
 function testIterator(root, whatToShow, filter) {
   var iter = document.createNodeIterator(root, whatToShow, filter);
 
index f26aae8..ddbfd73 100644 (file)
@@ -1,4 +1,5 @@
 
+PASS Recursive filters need to throw 
 PASS document.createTreeWalker(paras[0], 0, null) 
 PASS document.createTreeWalker(paras[0], 0, (function(node) { return true })) 
 PASS document.createTreeWalker(paras[0], 0, (function(node) { return false })) 
index e0e285a..2570c9a 100644 (file)
 
 // TODO .previousNode, .nextNode
 
+test(function() {
+  var depth = 0;
+  var walker = document.createTreeWalker(document, NodeFilter.SHOW_ALL,
+    function() {
+      if (depth == 0) {
+        depth++;
+        walker.firstChild();
+      }
+      return NodeFilter.FILTER_ACCEPT;
+    });
+  walker.currentNode = document.body;
+  assert_throws("InvalidStateError", function() { walker.parentNode() });
+  depth--;
+  assert_throws("InvalidStateError", function() { walker.firstChild() });
+  depth--;
+  assert_throws("InvalidStateError", function() { walker.lastChild() });
+  depth--;
+  assert_throws("InvalidStateError", function() { walker.previousSibling() });
+  depth--;
+  assert_throws("InvalidStateError", function() { walker.nextSibling() });
+  depth--;
+  assert_throws("InvalidStateError", function() { walker.previousNode() });
+  depth--;
+  assert_throws("InvalidStateError", function() { walker.nextNode() });
+}, "Recursive filters need to throw");
+
 function filterNode(node, whatToShow, filter) {
     // "If active flag is set throw an "InvalidStateError"."
     // Ignore active flag for these tests, we aren't calling recursively
index b25d716..20ebdac 100644 (file)
@@ -1,7 +1,7 @@
-CONSOLE MESSAGE: line 49: Should have thrown an exception: null
-CONSOLE MESSAGE: line 49: Should have thrown an exception: null
-CONSOLE MESSAGE: line 49: Should have thrown an exception: null
-CONSOLE MESSAGE: line 49: Should have thrown an exception: null
+CONSOLE MESSAGE: line 49: Should have thrown an exception: InvalidStateError: Recursive filters are not allowed
+CONSOLE MESSAGE: line 49: Should have thrown an exception: InvalidStateError: Recursive filters are not allowed
+CONSOLE MESSAGE: line 49: Should have thrown an exception: InvalidStateError: Recursive filters are not allowed
+CONSOLE MESSAGE: line 49: Should have thrown an exception: InvalidStateError: Recursive filters are not allowed
 CONSOLE MESSAGE: line 94: Should have tests 4 filter calls: 4
 Mozilla Bug 559526
 
index 18f0373..9c6acc7 100644 (file)
@@ -1,3 +1,35 @@
+2017-08-09  Chris Dumez  <cdumez@apple.com>
+
+        Reinstate active flag for iterators
+        https://bugs.webkit.org/show_bug.cgi?id=175312
+
+        Reviewed by Sam Weinig.
+
+        NodeIterator / TreeWalker should no longer allow recursive filters
+        after the following change to the DOM specification:
+        - https://github.com/whatwg/dom/pull/359
+
+        This patch aligns our behavior with the latest specification.
+
+        No new tests, updated existing tests.
+
+        * dom/NodeIterator.cpp:
+        (WebCore::NodeIterator::nextNode):
+        (WebCore::NodeIterator::previousNode):
+        Note that we now also call m_candidateNode.clear() before returning an
+        exception. This was a pre-existing bug that we failed to do so in the
+        exception case but it became more obvious after this change now that
+        we throw. This was causing traversal/moz-bug559526.html to fail
+        otherwise (the filter was called one too many times). The test case
+        is passing in Firefox (The filter is called 4 times and they throw
+        each time).
+
+        * dom/Traversal.cpp:
+        (WebCore::NodeIteratorBase::NodeIteratorBase):
+        (WebCore::NodeIteratorBase::acceptNode):
+        * dom/Traversal.h:
+        * dom/TreeWalker.cpp:
+
 2017-08-09  Antti Koivisto  <antti@apple.com>
 
         RenderQuote should not mutate render tree
index 8fc2151..45d860b 100644 (file)
@@ -97,13 +97,13 @@ ExceptionOr<RefPtr<Node>> NodeIterator::nextNode()
         // of the rejected node. Hence, FILTER_REJECT is the same as FILTER_SKIP.
         RefPtr<Node> provisionalResult = m_candidateNode.node;
 
-        auto callbackResult = acceptNode(*provisionalResult);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*provisionalResult);
+        if (filterResult.hasException()) {
+            m_candidateNode.clear();
+            return filterResult.releaseException();
+        }
 
-        bool nodeWasAccepted = callbackResult.releaseReturnValue() == NodeFilter::FILTER_ACCEPT;
+        bool nodeWasAccepted = filterResult.returnValue() == NodeFilter::FILTER_ACCEPT;
         if (nodeWasAccepted) {
             m_referenceNode = m_candidateNode;
             result = WTFMove(provisionalResult);
@@ -126,13 +126,13 @@ ExceptionOr<RefPtr<Node>> NodeIterator::previousNode()
         // of the rejected node. Hence, FILTER_REJECT is the same as FILTER_SKIP.
         RefPtr<Node> provisionalResult = m_candidateNode.node;
 
-        auto callbackResult = acceptNode(*provisionalResult);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*provisionalResult);
+        if (filterResult.hasException()) {
+            m_candidateNode.clear();
+            return filterResult.releaseException();
+        }
 
-        bool nodeWasAccepted = callbackResult.releaseReturnValue() == NodeFilter::FILTER_ACCEPT;
+        bool nodeWasAccepted = filterResult.returnValue() == NodeFilter::FILTER_ACCEPT;
         if (nodeWasAccepted) {
             m_referenceNode = m_candidateNode;
             result = WTFMove(provisionalResult);
index 3209e52..fdf70c9 100644 (file)
 #include "config.h"
 #include "Traversal.h"
 
+#include "CallbackResult.h"
 #include "Node.h"
 #include "NodeFilter.h"
+#include <wtf/SetForScope.h>
 
 namespace WebCore {
 
 NodeIteratorBase::NodeIteratorBase(Node& rootNode, unsigned whatToShow, RefPtr<NodeFilter>&& nodeFilter)
     : m_root(rootNode)
-    , m_whatToShow(whatToShow)
     , m_filter(WTFMove(nodeFilter))
+    , m_whatToShow(whatToShow)
 {
 }
 
-CallbackResult<unsigned short> NodeIteratorBase::acceptNode(Node& node) const
+// https://dom.spec.whatwg.org/#concept-node-filter
+ExceptionOr<unsigned short> NodeIteratorBase::acceptNode(Node& node)
 {
+    if (m_isActive)
+        return Exception { InvalidStateError, ASCIILiteral("Recursive filters are not allowed") };
+
     // The bit twiddling here is done to map DOM node types, which are given as integers from
     // 1 through 14, to whatToShow bit masks.
     if (!(((1 << (node.nodeType() - 1)) & m_whatToShow)))
         return NodeFilter::FILTER_SKIP;
+
     if (!m_filter)
         return NodeFilter::FILTER_ACCEPT;
 
-    return m_filter->acceptNode(node);
+    SetForScope<bool> isActive(m_isActive, true);
+    auto callbackResult = m_filter->acceptNode(node);
+    if (callbackResult.type() == CallbackResultType::ExceptionThrown)
+        return Exception { ExistingExceptionError };
+    return callbackResult.releaseReturnValue();
 }
 
 } // namespace WebCore
index 96f2fef..65c6ba5 100644 (file)
@@ -24,7 +24,7 @@
 
 #pragma once
 
-#include "CallbackResult.h"
+#include "ExceptionOr.h"
 #include <wtf/RefPtr.h>
 
 namespace WebCore {
@@ -42,12 +42,13 @@ public:
 
 protected:
     NodeIteratorBase(Node&, unsigned whatToShow, RefPtr<NodeFilter>&&);
-    CallbackResult<unsigned short> acceptNode(Node&) const;
+    ExceptionOr<unsigned short> acceptNode(Node&);
 
 private:
     Ref<Node> m_root;
-    unsigned m_whatToShow;
     RefPtr<NodeFilter> m_filter;
+    unsigned m_whatToShow;
+    bool m_isActive { false };
 };
 
 } // namespace WebCore
index e851e87..4bdd85f 100644 (file)
@@ -55,14 +55,11 @@ ExceptionOr<Node*> TreeWalker::parentNode()
         if (!node)
             return nullptr;
 
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
-
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        if (acceptNodeResult == NodeFilter::FILTER_ACCEPT)
+        if (filterResult.returnValue() == NodeFilter::FILTER_ACCEPT)
             return setCurrent(node.releaseNonNull());
     }
     return nullptr;
@@ -71,14 +68,11 @@ ExceptionOr<Node*> TreeWalker::parentNode()
 ExceptionOr<Node*> TreeWalker::firstChild()
 {
     for (RefPtr<Node> node = m_current->firstChild(); node; ) {
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        switch (acceptNodeResult) {
+        switch (filterResult.returnValue()) {
             case NodeFilter::FILTER_ACCEPT:
                 m_current = node.releaseNonNull();
                 return m_current.ptr();
@@ -108,14 +102,11 @@ ExceptionOr<Node*> TreeWalker::firstChild()
 ExceptionOr<Node*> TreeWalker::lastChild()
 {
     for (RefPtr<Node> node = m_current->lastChild(); node; ) {
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        switch (acceptNodeResult) {
+        switch (filterResult.returnValue()) {
             case NodeFilter::FILTER_ACCEPT:
                 m_current = node.releaseNonNull();
                 return m_current.ptr();
@@ -151,34 +142,28 @@ template<TreeWalker::SiblingTraversalType type> ExceptionOr<Node*> TreeWalker::t
     auto isNext = type == SiblingTraversalType::Next;
     while (true) {
         for (RefPtr<Node> sibling = isNext ? node->nextSibling() : node->previousSibling(); sibling; ) {
-            auto callbackResult = acceptNode(*sibling);
-            if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-                return Exception { ExistingExceptionError };
+            auto filterResult = acceptNode(*sibling);
+            if (filterResult.hasException())
+                return filterResult.releaseException();
 
-            ASSERT(callbackResult.type() == CallbackResultType::Success);
-
-            auto acceptNodeResult = callbackResult.releaseReturnValue();
-            if (acceptNodeResult == NodeFilter::FILTER_ACCEPT) {
+            if (filterResult.returnValue() == NodeFilter::FILTER_ACCEPT) {
                 m_current = sibling.releaseNonNull();
                 return m_current.ptr();
             }
             node = sibling;
             sibling = isNext ? sibling->firstChild() : sibling->lastChild();
-            if (acceptNodeResult == NodeFilter::FILTER_REJECT || !sibling)
+            if (filterResult.returnValue() == NodeFilter::FILTER_REJECT || !sibling)
                 sibling = isNext ? node->nextSibling() : node->previousSibling();
         }
         node = node->parentNode();
         if (!node || node == &root())
             return nullptr;
 
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        if (acceptNodeResult == NodeFilter::FILTER_ACCEPT)
+        if (filterResult.returnValue() == NodeFilter::FILTER_ACCEPT)
             return nullptr;
     }
 }
@@ -200,25 +185,21 @@ ExceptionOr<Node*> TreeWalker::previousNode()
         while (Node* previousSibling = node->previousSibling()) {
             node = previousSibling;
 
-            auto callbackResult = acceptNode(*node);
-            if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-                return Exception { ExistingExceptionError };
+            auto filterResult = acceptNode(*node);
+            if (filterResult.hasException())
+                return filterResult.releaseException();
 
-            ASSERT(callbackResult.type() == CallbackResultType::Success);
-
-            auto acceptNodeResult = callbackResult.releaseReturnValue();
+            auto acceptNodeResult = filterResult.returnValue();
             if (acceptNodeResult == NodeFilter::FILTER_REJECT)
                 continue;
             while (Node* lastChild = node->lastChild()) {
                 node = lastChild;
 
-                auto callbackResult = acceptNode(*node);
-                if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-                    return Exception { ExistingExceptionError };
-
-                ASSERT(callbackResult.type() == CallbackResultType::Success);
+                auto filterResult = acceptNode(*node);
+                if (filterResult.hasException())
+                    return filterResult.releaseException();
 
-                acceptNodeResult = callbackResult.releaseReturnValue();
+                acceptNodeResult = filterResult.returnValue();
                 if (acceptNodeResult == NodeFilter::FILTER_REJECT)
                     break;
             }
@@ -234,14 +215,11 @@ ExceptionOr<Node*> TreeWalker::previousNode()
             return nullptr;
         node = parent;
 
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        if (acceptNodeResult == NodeFilter::FILTER_ACCEPT)
+        if (filterResult.returnValue() == NodeFilter::FILTER_ACCEPT)
             return setCurrent(node.releaseNonNull());
     }
     return nullptr;
@@ -254,31 +232,25 @@ Children:
     while (Node* firstChild = node->firstChild()) {
         node = firstChild;
 
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
-
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        if (acceptNodeResult == NodeFilter::FILTER_ACCEPT)
+        if (filterResult.releaseReturnValue() == NodeFilter::FILTER_ACCEPT)
             return setCurrent(node.releaseNonNull());
-        if (acceptNodeResult == NodeFilter::FILTER_REJECT)
+        if (filterResult.returnValue() == NodeFilter::FILTER_REJECT)
             break;
     }
     while (Node* nextSibling = NodeTraversal::nextSkippingChildren(*node, &root())) {
         node = nextSibling;
 
-        auto callbackResult = acceptNode(*node);
-        if (callbackResult.type() == CallbackResultType::ExceptionThrown)
-            return Exception { ExistingExceptionError };
-
-        ASSERT(callbackResult.type() == CallbackResultType::Success);
+        auto filterResult = acceptNode(*node);
+        if (filterResult.hasException())
+            return filterResult.releaseException();
 
-        auto acceptNodeResult = callbackResult.releaseReturnValue();
-        if (acceptNodeResult == NodeFilter::FILTER_ACCEPT)
+        if (filterResult.returnValue() == NodeFilter::FILTER_ACCEPT)
             return setCurrent(node.releaseNonNull());
-        if (acceptNodeResult == NodeFilter::FILTER_SKIP)
+        if (filterResult.returnValue() == NodeFilter::FILTER_SKIP)
             goto Children;
     }
     return nullptr;