<https://webkit.org/b/119886> PseudoElement is abusing parent node pointer
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 17 Aug 2013 15:43:24 +0000 (15:43 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 17 Aug 2013 15:43:24 +0000 (15:43 +0000)
Reviewed by Darin Adler.

PseudoElement sets its host node as its parent. This is confusing and wrong as it breaks
the basic tree consistency (a node is a child node of its parent node).

This patch adds an explicit host pointer PseudoElement and switches the call sites over. Memory
impact is negligible as there are not that many ::befores and ::afters.

* dom/ComposedShadowTreeWalker.cpp:
(WebCore::ComposedShadowTreeWalker::traverseParent):
* dom/EventPathWalker.cpp:
(WebCore::EventPathWalker::moveToParent):
* dom/EventRetargeter.h:
(WebCore::EventRetargeter::eventTargetRespectingTargetRules):
* dom/Node.cpp:
(WebCore::Node::~Node):

    Add consistency assertions. Remove unnecessary clearing of sibling pointers. They should be cleared already.

(WebCore::Node::markAncestorsWithChildNeedsStyleRecalc):
* dom/PseudoElement.cpp:
(WebCore::PseudoElement::PseudoElement):
(WebCore::PseudoElement::customStyleForRenderer):
* dom/PseudoElement.h:
(WebCore::toPseudoElement):

    Add casting functions.

* inspector/InspectorLayerTreeAgent.cpp:
(WebCore::InspectorLayerTreeAgent::buildObjectForLayer):
* rendering/HitTestResult.cpp:
(WebCore::HitTestResult::setInnerNode):
(WebCore::HitTestResult::setInnerNonSharedNode):

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

15 files changed:
Source/WebCore/ChangeLog
Source/WebCore/dom/ComposedShadowTreeWalker.cpp
Source/WebCore/dom/ElementRareData.h
Source/WebCore/dom/EventPathWalker.cpp
Source/WebCore/dom/EventRetargeter.h
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/NodeTraversal.cpp
Source/WebCore/dom/PseudoElement.cpp
Source/WebCore/dom/PseudoElement.h
Source/WebCore/inspector/InspectorLayerTreeAgent.cpp
Source/WebCore/rendering/HitTestResult.cpp
Source/WebCore/rendering/RenderCounter.cpp
Source/WebCore/rendering/RenderListItem.cpp
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h

index f117d94..8fbfced 100644 (file)
@@ -1,3 +1,41 @@
+2013-08-16  Antti Koivisto  <antti@apple.com>
+
+        <https://webkit.org/b/119886> PseudoElement is abusing parent node pointer
+
+        Reviewed by Darin Adler.
+
+        PseudoElement sets its host node as its parent. This is confusing and wrong as it breaks
+        the basic tree consistency (a node is a child node of its parent node).
+        
+        This patch adds an explicit host pointer PseudoElement and switches the call sites over. Memory
+        impact is negligible as there are not that many ::befores and ::afters.
+
+        * dom/ComposedShadowTreeWalker.cpp:
+        (WebCore::ComposedShadowTreeWalker::traverseParent):
+        * dom/EventPathWalker.cpp:
+        (WebCore::EventPathWalker::moveToParent):
+        * dom/EventRetargeter.h:
+        (WebCore::EventRetargeter::eventTargetRespectingTargetRules):
+        * dom/Node.cpp:
+        (WebCore::Node::~Node):
+        
+            Add consistency assertions. Remove unnecessary clearing of sibling pointers. They should be cleared already.
+
+        (WebCore::Node::markAncestorsWithChildNeedsStyleRecalc):
+        * dom/PseudoElement.cpp:
+        (WebCore::PseudoElement::PseudoElement):
+        (WebCore::PseudoElement::customStyleForRenderer):
+        * dom/PseudoElement.h:
+        (WebCore::toPseudoElement):
+        
+            Add casting functions.
+
+        * inspector/InspectorLayerTreeAgent.cpp:
+        (WebCore::InspectorLayerTreeAgent::buildObjectForLayer):
+        * rendering/HitTestResult.cpp:
+        (WebCore::HitTestResult::setInnerNode):
+        (WebCore::HitTestResult::setInnerNonSharedNode):
+
 2013-08-17  Darin Adler  <darin@apple.com>
 
         <https://webkit.org/b/119948> Change drag-specific clipboard writing in DragController to go straight to Pasteboard, not forward through Clipboard
index ec52290..9e1bf7f 100644 (file)
@@ -200,7 +200,7 @@ void ComposedShadowTreeWalker::parent()
 Node* ComposedShadowTreeWalker::traverseParent(const Node* node, ParentTraversalDetails* details) const
 {
     if (node->isPseudoElement())
-        return node->parentOrShadowHostNode();
+        return toPseudoElement(node)->hostElement();
 
     if (!canCrossUpperBoundary() && node->isShadowRoot())
         return 0;
index f5042a7..33cd736 100644 (file)
@@ -251,14 +251,13 @@ inline void ElementRareData::releasePseudoElement(PseudoElement* element)
 {
     if (!element)
         return;
-
     if (element->attached())
         element->detach();
+    element->clearHostElement();
 
     ASSERT(!element->nextSibling());
     ASSERT(!element->previousSibling());
-
-    element->setParentNode(0);
+    ASSERT(!element->parentNode());
 }
 
 inline void ElementRareData::resetComputedStyle()
index 6566756..504a8fb 100644 (file)
@@ -29,6 +29,7 @@
 
 #include "ContentDistributor.h"
 #include "InsertionPoint.h"
+#include "PseudoElement.h"
 #include "ShadowRoot.h"
 
 namespace WebCore {
@@ -59,14 +60,18 @@ void EventPathWalker::moveToParent()
             return;
         }
     }
-    if (!m_node->isShadowRoot()) {
-        m_node = m_node->parentNode();
-        m_isVisitingInsertionPointInReprojection = false;
+    m_isVisitingInsertionPointInReprojection = false;
+    if (m_node->isPseudoElement()) {
+        // FIXME: Pseudo elements should probably not be dispatching events in the first place.
+        m_node = toPseudoElement(m_node)->hostElement();
         return;
     }
-    m_node = toShadowRoot(m_node)->hostElement();
-    m_distributedNode = m_node;
-    m_isVisitingInsertionPointInReprojection = false;
+    if (m_node->isShadowRoot()) {
+        m_node = toShadowRoot(m_node)->hostElement();
+        m_distributedNode = m_node;
+        return;
+    }
+    m_node = m_node->parentNode();
 }
 
 } // namespace
index cc094a6..6ddac00 100644 (file)
@@ -22,6 +22,7 @@
 
 #include "ContainerNode.h"
 #include "EventContext.h"
+#include "PseudoElement.h"
 #include "ShadowRoot.h"
 #include <wtf/HashMap.h>
 #include <wtf/PassRefPtr.h>
@@ -81,7 +82,7 @@ inline EventTarget* EventRetargeter::eventTargetRespectingTargetRules(Node* refe
     ASSERT(referenceNode);
 
     if (referenceNode->isPseudoElement())
-        return referenceNode->parentNode();
+        return toPseudoElement(referenceNode)->hostElement();
 
 #if ENABLE(SVG)
     if (!referenceNode->isSVGElement() || !referenceNode->isInShadowTree())
index 309b612..565ea7e 100644 (file)
@@ -349,6 +349,9 @@ Node::~Node()
 #endif
 
     ASSERT(!renderer());
+    ASSERT(!parentNode());
+    ASSERT(!m_previous);
+    ASSERT(!m_next);
 
     if (hasRareData())
         clearRareData();
@@ -358,11 +361,6 @@ Node::~Node()
             willBeDeletedFrom(document);
     }
 
-    if (m_previous)
-        m_previous->setNextSibling(0);
-    if (m_next)
-        m_next->setPreviousSibling(0);
-
     m_treeScope->guardDeref();
 
     InspectorCounters::decrementCounter(InspectorCounters::NodeCounter);
@@ -707,8 +705,9 @@ LayoutRect Node::renderRect(bool* isReplaced)
 
 void Node::markAncestorsWithChildNeedsStyleRecalc()
 {
-    for (ContainerNode* p = parentOrShadowHostNode(); p && !p->childNeedsStyleRecalc(); p = p->parentOrShadowHostNode())
-        p->setChildNeedsStyleRecalc();
+    ContainerNode* ancestor = isPseudoElement() ? toPseudoElement(this)->hostElement() : parentOrShadowHostNode();
+    for (; ancestor && !ancestor->childNeedsStyleRecalc(); ancestor = ancestor->parentOrShadowHostNode())
+        ancestor->setChildNeedsStyleRecalc();
 
     if (document()->childNeedsStyleRecalc())
         document()->scheduleStyleRecalc();
@@ -891,24 +890,24 @@ bool Node::containsIncludingHostElements(const Node* node) const
 
 Node* Node::pseudoAwarePreviousSibling() const
 {
-    if (parentElement() && !previousSibling()) {
-        Element* parent = parentElement();
-        if (isAfterPseudoElement() && parent->lastChild())
-            return parent->lastChild();
+    Element* parentOrHost = isPseudoElement() ? toPseudoElement(this)->hostElement() : parentElement();
+    if (parentOrHost && !previousSibling()) {
+        if (isAfterPseudoElement() && parentOrHost->lastChild())
+            return parentOrHost->lastChild();
         if (!isBeforePseudoElement())
-            return parent->pseudoElement(BEFORE);
+            return parentOrHost->pseudoElement(BEFORE);
     }
     return previousSibling();
 }
 
 Node* Node::pseudoAwareNextSibling() const
 {
-    if (parentElement() && !nextSibling()) {
-        Element* parent = parentElement();
-        if (isBeforePseudoElement() && parent->firstChild())
-            return parent->firstChild();
+    Element* parentOrHost = isPseudoElement() ? toPseudoElement(this)->hostElement() : parentElement();
+    if (parentOrHost && !nextSibling()) {
+        if (isBeforePseudoElement() && parentOrHost->firstChild())
+            return parentOrHost->firstChild();
         if (!isAfterPseudoElement())
-            return parent->pseudoElement(AFTER);
+            return parentOrHost->pseudoElement(AFTER);
     }
     return nextSibling();
 }
@@ -925,7 +924,6 @@ Node* Node::pseudoAwareFirstChild() const
             first = currentElement->pseudoElement(AFTER);
         return first;
     }
-
     return firstChild();
 }
 
@@ -941,7 +939,6 @@ Node* Node::pseudoAwareLastChild() const
             last = currentElement->pseudoElement(BEFORE);
         return last;
     }
-
     return lastChild();
 }
 
index 216a7a7..82e6c2a 100644 (file)
@@ -42,7 +42,7 @@ Node* previousIncludingPseudo(const Node* current, const Node* stayWithin)
             previous = previous->pseudoAwareLastChild();
         return previous;
     }
-    return current->parentNode();
+    return current->isPseudoElement() ? toPseudoElement(current)->hostElement() : current->parentNode();
 }
 
 Node* nextIncludingPseudo(const Node* current, const Node* stayWithin)
@@ -54,7 +54,8 @@ Node* nextIncludingPseudo(const Node* current, const Node* stayWithin)
         return 0;
     if ((next = current->pseudoAwareNextSibling()))
         return next;
-    for (current = current->parentNode(); current; current = current->parentNode()) {
+    current = current->isPseudoElement() ? toPseudoElement(current)->hostElement() : current->parentNode();
+    for (; current; current = current->parentNode()) {
         if (current == stayWithin)
             return 0;
         if ((next = current->pseudoAwareNextSibling()))
@@ -70,7 +71,8 @@ Node* nextIncludingPseudoSkippingChildren(const Node* current, const Node* stayW
         return 0;
     if ((next = current->pseudoAwareNextSibling()))
         return next;
-    for (current = current->parentNode(); current; current = current->parentNode()) {
+    current = current->isPseudoElement() ? toPseudoElement(current)->hostElement() : current->parentNode();
+    for (; current; current = current->parentNode()) {
         if (current == stayWithin)
             return 0;
         if ((next = current->pseudoAwareNextSibling()))
index d3fc8fb..aa1a545 100644 (file)
@@ -55,18 +55,18 @@ String PseudoElement::pseudoElementNameForEvents(PseudoId pseudoId)
     }
 }
 
-PseudoElement::PseudoElement(Element* parent, PseudoId pseudoId)
-    : Element(pseudoElementTagName(), parent->document(), CreatePseudoElement)
+PseudoElement::PseudoElement(Element* host, PseudoId pseudoId)
+    : Element(pseudoElementTagName(), host->document(), CreatePseudoElement)
+    , m_hostElement(host)
     , m_pseudoId(pseudoId)
 {
-    ASSERT(pseudoId != NOPSEUDO);
-    // FIXME: This is wrong in terms of tree consistency. Pseudo element is now not a child of its parent.
-    setParentNode(parent);
+    ASSERT(pseudoId == BEFORE || pseudoId == AFTER);
     setHasCustomStyleCallbacks();
 }
 
 PseudoElement::~PseudoElement()
 {
+    ASSERT(!m_hostElement);
 #if USE(ACCELERATED_COMPOSITING)
     InspectorInstrumentation::pseudoElementDestroyed(document()->page(), this);
 #endif
@@ -74,7 +74,7 @@ PseudoElement::~PseudoElement()
 
 PassRefPtr<RenderStyle> PseudoElement::customStyleForRenderer()
 {
-    return parentOrShadowHostElement()->renderer()->getCachedPseudoStyle(m_pseudoId);
+    return m_hostElement->renderer()->getCachedPseudoStyle(m_pseudoId);
 }
 
 void PseudoElement::attach(const AttachContext& context)
index 3b34b24..8a6bc84 100644 (file)
@@ -36,12 +36,15 @@ namespace WebCore {
 
 class PseudoElement FINAL : public Element {
 public:
-    static PassRefPtr<PseudoElement> create(Element* parent, PseudoId pseudoId)
+    static PassRefPtr<PseudoElement> create(Element* host, PseudoId pseudoId)
     {
-        return adoptRef(new PseudoElement(parent, pseudoId));
+        return adoptRef(new PseudoElement(host, pseudoId));
     }
     ~PseudoElement();
 
+    Element* hostElement() const { return m_hostElement; }
+    void clearHostElement() { m_hostElement = 0; }
+
     virtual PassRefPtr<RenderStyle> customStyleForRenderer() OVERRIDE;
     virtual void attach(const AttachContext& = AttachContext()) OVERRIDE;
     virtual bool rendererIsNeeded(const NodeRenderingContext&) OVERRIDE;
@@ -63,6 +66,7 @@ private:
     virtual void didRecalcStyle(Style::Change) OVERRIDE;
     virtual PseudoId customPseudoId() const OVERRIDE { return m_pseudoId; }
 
+    Element* m_hostElement;
     PseudoId m_pseudoId;
 };
 
@@ -73,6 +77,18 @@ inline bool pseudoElementRendererIsNeeded(const RenderStyle* style)
     return style && style->display() != NONE && (style->contentData() || !style->regionThread().isEmpty());
 }
 
+inline PseudoElement* toPseudoElement(Node* node)
+{
+    ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isPseudoElement());
+    return static_cast<PseudoElement*>(node);
+}
+
+inline const PseudoElement* toPseudoElement(const Node* node)
+{
+    ASSERT_WITH_SECURITY_IMPLICATION(!node || node->isPseudoElement());
+    return static_cast<const PseudoElement*>(node);
+}
+
 } // namespace
 
 #endif
index 6f01dbf..b6ac135 100644 (file)
@@ -193,7 +193,7 @@ PassRefPtr<TypeBuilder::LayerTree::Layer> InspectorLayerTreeAgent::buildObjectFo
         if (isReflection)
             renderer = renderer->parent();
         layerObject->setIsGeneratedContent(true);
-        layerObject->setPseudoElementId(bindPseudoElement(static_cast<PseudoElement*>(renderer->node())));
+        layerObject->setPseudoElementId(bindPseudoElement(toPseudoElement(renderer->node())));
         if (renderer->isBeforeContent())
             layerObject->setPseudoElement("before");
         else if (renderer->isAfterContent())
index ae2dabe..c5fadbb 100644 (file)
@@ -40,6 +40,7 @@
 #include "HTMLTextAreaElement.h"
 #include "HTMLVideoElement.h"
 #include "HitTestLocation.h"
+#include "PseudoElement.h"
 #include "RenderBlock.h"
 #include "RenderImage.h"
 #include "RenderInline.h"
@@ -133,14 +134,14 @@ void HitTestResult::setToNonShadowAncestor()
 void HitTestResult::setInnerNode(Node* n)
 {
     if (n && n->isPseudoElement())
-        n = n->parentOrShadowHostNode();
+        n = toPseudoElement(n)->hostElement();
     m_innerNode = n;
 }
     
 void HitTestResult::setInnerNonSharedNode(Node* n)
 {
     if (n && n->isPseudoElement())
-        n = n->parentOrShadowHostNode();
+        n = toPseudoElement(n)->hostElement();
     m_innerNonSharedNode = n;
 }
 
index ca059e6..690f624 100644 (file)
@@ -28,6 +28,7 @@
 #include "ElementTraversal.h"
 #include "HTMLNames.h"
 #include "HTMLOListElement.h"
+#include "PseudoElement.h"
 #include "RenderListItem.h"
 #include "RenderListMarker.h"
 #include "RenderStyle.h"
@@ -64,6 +65,13 @@ static RenderObject* previousInPreOrder(const RenderObject* object)
     return previous ? previous->renderer() : 0;
 }
 
+static inline Element* parentOrPseudoHostElement(const RenderObject* object)
+{
+    if (object->node()->isPseudoElement())
+        return toPseudoElement(object->node())->hostElement();
+    return toElement(object->node())->parentElement();
+}
+
 // This function processes the renderer tree in the order of the DOM tree
 // including pseudo elements as defined in CSS 2.1.
 static RenderObject* previousSiblingOrParent(const RenderObject* object)
@@ -74,18 +82,13 @@ static RenderObject* previousSiblingOrParent(const RenderObject* object)
         previous = ElementTraversal::pseudoAwarePreviousSibling(previous);
     if (previous)
         return previous->renderer();
-    previous = self->parentElement();
+    previous = parentOrPseudoHostElement(object);
     return previous ? previous->renderer() : 0;
 }
 
-static inline Element* parentElement(RenderObject* object)
-{
-    return toElement(object->node())->parentElement();
-}
-
 static inline bool areRenderersElementsSiblings(RenderObject* first, RenderObject* second)
 {
-    return parentElement(first) == parentElement(second);
+    return parentOrPseudoHostElement(first) == parentOrPseudoHostElement(second);
 }
 
 // This function processes the renderer tree in the order of the DOM tree
@@ -271,7 +274,7 @@ static bool findPlaceForCounter(RenderObject* counterOwner, const AtomicString&
                         previousSiblingProtector = currentCounter;
                         // We are no longer interested in previous siblings of the currentRenderer or their children
                         // as counters they may have attached cannot be the previous sibling of the counter we are placing.
-                        currentRenderer = parentElement(currentRenderer)->renderer();
+                        currentRenderer = parentOrPseudoHostElement(currentRenderer)->renderer();
                         continue;
                     }
                 } else
@@ -327,7 +330,7 @@ static CounterNode* makeCounterNode(RenderObject* object, const AtomicString& id
     // Checking if some nodes that were previously counter tree root nodes
     // should become children of this node now.
     CounterMaps& maps = counterMaps();
-    Element* stayWithin = parentElement(object);
+    Element* stayWithin = parentOrPseudoHostElement(object);
     bool skipDescendants;
     for (RenderObject* currentRenderer = nextInPreOrder(object, stayWithin); currentRenderer; currentRenderer = nextInPreOrder(currentRenderer, stayWithin, skipDescendants)) {
         skipDescendants = false;
@@ -339,7 +342,7 @@ static CounterNode* makeCounterNode(RenderObject* object, const AtomicString& id
         skipDescendants = true;
         if (currentCounter->parent())
             continue;
-        if (stayWithin == parentElement(currentRenderer) && currentCounter->hasResetType())
+        if (stayWithin == parentOrPseudoHostElement(currentRenderer) && currentCounter->hasResetType())
             break;
         newNode->insertAfter(currentCounter, newNode->lastChild(), identifier);
     }
@@ -554,7 +557,7 @@ void RenderCounter::rendererSubtreeAttached(RenderObject* renderer)
     if (!renderer->view()->hasRenderCounters())
         return;
     Node* node = renderer->node();
-    if (node)
+    if (node && !node->isPseudoElement())
         node = node->parentNode();
     else
         node = renderer->generatingNode();
index bbab226..4510fd9 100644 (file)
@@ -27,6 +27,7 @@
 #include "ElementTraversal.h"
 #include "HTMLNames.h"
 #include "HTMLOListElement.h"
+#include "PseudoElement.h"
 #include "RenderListMarker.h"
 #include "RenderView.h"
 #include "StyleInheritedData.h"
@@ -102,8 +103,9 @@ static Node* enclosingList(const RenderListItem* listItem)
 {
     Node* listItemNode = listItem->node();
     Node* firstNode = 0;
+    Node* parent = listItemNode->isPseudoElement() ? toPseudoElement(listItemNode)->hostElement() : listItemNode->parentNode();
     // We use parentNode because the enclosing list could be a ShadowRoot that's not Element.
-    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
+    for (; parent; parent = parent->parentNode()) {
         if (isList(parent))
             return parent;
         if (!firstNode)
index 2ceabd4..5d170bc 100644 (file)
@@ -46,6 +46,7 @@
 #include "HitTestResult.h"
 #include "LogicalSelectionOffsetCaches.h"
 #include "Page.h"
+#include "PseudoElement.h"
 #include "RenderArena.h"
 #include "RenderCounter.h"
 #include "RenderDeprecatedFlexibleBox.h"
@@ -3227,6 +3228,11 @@ bool RenderObject::canHaveGeneratedChildren() const
     return canHaveChildren();
 }
 
+Node* RenderObject::generatingPseudoHostElement() const
+{
+    return toPseudoElement(node())->hostElement();
+}
+
 bool RenderObject::canBeReplacedWithInlineRunIn() const
 {
     return true;
index 85fcaf8..15a0e80 100644 (file)
@@ -636,7 +636,7 @@ public:
     // Returns the styled node that caused the generation of this renderer.
     // This is the same as node() except for renderers of :before and :after
     // pseudo elements for which their parent node is returned.
-    Node* generatingNode() const { return isPseudoElement() ? node()->parentNode() : node(); }
+    Node* generatingNode() const { return isPseudoElement() ? generatingPseudoHostElement() : node(); }
 
     Document* document() const { return m_node->document(); }
     Frame* frame() const { return document()->frame(); }
@@ -1029,6 +1029,8 @@ private:
 
     Color selectionColor(int colorProperty) const;
 
+    Node* generatingPseudoHostElement() const;
+
 #if ENABLE(CSS_SHAPES)
     void removeShapeImageClient(ShapeValue*);
 #endif