RenderObject::destroy() should only be invoked after renderer has bee…

…n removed from the tree

https://bugs.webkit.org/show_bug.cgi?id=178075

Reviewed by Zalan Bujtas.

Source/WebCore:

This patch fixes the remaining cases where the renderer is still in the tree while destroy()
is called and adds the assert.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::removeLeftoverAnonymousBlock):
(WebCore::RenderBlock::takeChild):
* rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::willBeDestroyed):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::~RenderLayer):

    Null the parent pointers for m_scrollCorner/m_resizer.

(WebCore::RenderLayer::calculateClipRects const):
* rendering/RenderLayer.h:
* rendering/RenderObject.cpp:
(WebCore::RenderObject::willBeDestroyed):
(WebCore::RenderObject::removeFromParentAndDestroyCleaningUpAnonymousWrappers):
(WebCore::RenderObject::destroy):

    Use RELEASE_ASSERT as these are cheap and important checks.
    Also turn isBeingDestroyed test into RELEASE_ASSERT.
    Remove AX call that no longer does anything.

(WebCore::RenderObject::destroyAndCleanupAnonymousWrappers): Deleted.
* rendering/RenderObject.h:
* rendering/RenderRubyBase.cpp:
(WebCore::RenderRubyBase::moveBlockChildren):
* rendering/RenderTableRow.cpp:
(WebCore::RenderTableRow::collapseAndDestroyAnonymousSiblingRows):
(WebCore::RenderTableRow::destroyAndCollapseAnonymousSiblingRows): Deleted.

    Renamed and made this no longer destroy itself. The caller now takes care of that.
    Removed an unnecessary lambda.

* rendering/RenderTableRow.h:
* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::tearDownRenderers):
(WebCore::RenderTreeUpdater::tearDownRenderer):
* style/RenderTreeUpdaterListItem.cpp:
(WebCore::RenderTreeUpdater::ListItem::updateMarker):

LayoutTests:

* accessibility/mac/textbox-role-reports-notifications.html:

This passed because spurious AXValueChanged notifications. Force layout to prevent coalescing between mutations.


Canonical link: https://commits.webkit.org/194374@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223131 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,14 @@
+2017-10-10  Antti Koivisto  <antti@apple.com>
+
+        RenderObject::destroy() should only be invoked after renderer has been removed from the tree
+        https://bugs.webkit.org/show_bug.cgi?id=178075
+
+        Reviewed by Zalan Bujtas.
+
+        * accessibility/mac/textbox-role-reports-notifications.html:
+
+        This passed because spurious AXValueChanged notifications. Force layout to prevent coalescing between mutations.
+
 2017-10-10  Joanmarie Diggs  <jdiggs@igalia.com>
 
         AX: [ATK] STATE_CHECKABLE should be removed from radio buttons in radiogroups with aria-readonly="true"

LayoutTests/accessibility/mac/textbox-role-reports-notifications.html

@@ -19,7 +19,9 @@
         textboxAxElement.addNotificationListener(logNotification);
         pendingNotifications = 3;
         ariaTextBox.firstChild.deleteData(0, 5);
+        ariaTextBox.offsetWidth;
         ariaTextBox.textContent = "changed textContent";
+        ariaTextBox.offsetWidth;
         ariaTextBox.innerText = "changed innerText";
     }
 

Source/WebCore/ChangeLog

@@ -1,3 +1,52 @@
+2017-10-10  Antti Koivisto  <antti@apple.com>
+
+        RenderObject::destroy() should only be invoked after renderer has been removed from the tree
+        https://bugs.webkit.org/show_bug.cgi?id=178075
+
+        Reviewed by Zalan Bujtas.
+
+        This patch fixes the remaining cases where the renderer is still in the tree while destroy()
+        is called and adds the assert.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::removeLeftoverAnonymousBlock):
+        (WebCore::RenderBlock::takeChild):
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::willBeDestroyed):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::~RenderLayer):
+
+            Null the parent pointers for m_scrollCorner/m_resizer.
+
+        (WebCore::RenderLayer::calculateClipRects const):
+        * rendering/RenderLayer.h:
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::willBeDestroyed):
+        (WebCore::RenderObject::removeFromParentAndDestroyCleaningUpAnonymousWrappers):
+        (WebCore::RenderObject::destroy):
+
+            Use RELEASE_ASSERT as these are cheap and important checks.
+            Also turn isBeingDestroyed test into RELEASE_ASSERT.
+            Remove AX call that no longer does anything.
+
+        (WebCore::RenderObject::destroyAndCleanupAnonymousWrappers): Deleted.
+        * rendering/RenderObject.h:
+        * rendering/RenderRubyBase.cpp:
+        (WebCore::RenderRubyBase::moveBlockChildren):
+        * rendering/RenderTableRow.cpp:
+        (WebCore::RenderTableRow::collapseAndDestroyAnonymousSiblingRows):
+        (WebCore::RenderTableRow::destroyAndCollapseAnonymousSiblingRows): Deleted.
+
+            Renamed and made this no longer destroy itself. The caller now takes care of that.
+            Removed an unnecessary lambda.
+
+        * rendering/RenderTableRow.h:
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::tearDownRenderers):
+        (WebCore::RenderTreeUpdater::tearDownRenderer):
+        * style/RenderTreeUpdaterListItem.cpp:
+        (WebCore::RenderTreeUpdater::ListItem::updateMarker):
+
 2017-10-09  Antti Koivisto  <antti@apple.com>
 
         Add isContinuation bit

Source/WebCore/rendering/RenderBlock.cpp

@@ -764,15 +764,15 @@ void RenderBlock::removeLeftoverAnonymousBlock(RenderBlock* child)
             child->nextSibling()->setPreviousSibling(child->previousSibling());
     }
 
-    child->setFirstChild(0);
-    child->m_next = 0;
+    child->setFirstChild(nullptr);
+    child->m_next = nullptr;
 
     // Remove all the information in the flow thread associated with the leftover anonymous block.
     child->resetFragmentedFlowStateOnRemoval();
 
-    child->setParent(0);
-    child->setPreviousSibling(0);
-    child->setNextSibling(0);
+    child->setParent(nullptr);
+    child->setPreviousSibling(nullptr);
+    child->setNextSibling(nullptr);
 
     child->destroy();
 }
@@ -875,7 +875,7 @@ RenderPtr<RenderObject> RenderBlock::takeChild(RenderObject& oldChild)
 
             // Delete the now-empty block's lines and nuke it.
             nextBlock.deleteLines();
-            nextBlock.destroy();
+            nextBlock.removeFromParentAndDestroy();
             next = nullptr;
         }
     }
@@ -934,7 +934,8 @@ RenderPtr<RenderObject> RenderBlock::takeChild(RenderObject& oldChild)
                 break;
             }
             setContinuation(nullptr);
-            destroy();
+            // FIXME: This is dangerous.
+            removeFromParentAndDestroy();
         }
     }
     return takenChild;

Source/WebCore/rendering/RenderBoxModelObject.cpp

@@ -188,7 +188,7 @@ RenderBoxModelObject::~RenderBoxModelObject()
 void RenderBoxModelObject::willBeDestroyed()
 {
     if (hasContinuation()) {
-        continuation()->destroy();
+        continuation()->removeFromParentAndDestroy();
         setContinuation(nullptr);
     }
 

Source/WebCore/rendering/RenderLayer.cpp

@@ -369,6 +369,9 @@ RenderLayer::~RenderLayer()
     if (m_reflection)
         removeReflection();
 
+    clearScrollCorner();
+    clearResizer();
+
     FilterInfo::remove(*this);
 
     // Child layers will be deleted by their corresponding render objects, so
@@ -6663,36 +6666,54 @@ void RenderLayer::updateScrollCornerStyle()
     auto corner = renderer().hasOverflowClip() ? actualRenderer->getUncachedPseudoStyle(PseudoStyleRequest(SCROLLBAR_CORNER), &actualRenderer->style()) : nullptr;
 
     if (!corner) {
-        m_scrollCorner = nullptr;
+        clearScrollCorner();
         return;
     }
 
     if (!m_scrollCorner) {
         m_scrollCorner = createRenderer<RenderScrollbarPart>(renderer().document(), WTFMove(*corner));
+        // FIXME: A renderer should be a child of its parent!
         m_scrollCorner->setParent(&renderer());
         m_scrollCorner->initializeStyle();
     } else
         m_scrollCorner->setStyle(WTFMove(*corner));
 }
 
+void RenderLayer::clearScrollCorner()
+{
+    if (!m_scrollCorner)
+        return;
+    m_scrollCorner->setParent(nullptr);
+    m_scrollCorner = nullptr;
+}
+
 void RenderLayer::updateResizerStyle()
 {
     RenderElement* actualRenderer = rendererForScrollbar(renderer());
     auto resizer = renderer().hasOverflowClip() ? actualRenderer->getUncachedPseudoStyle(PseudoStyleRequest(RESIZER), &actualRenderer->style()) : nullptr;
 
     if (!resizer) {
-        m_resizer = nullptr;
+        clearResizer();
         return;
     }
 
     if (!m_resizer) {
         m_resizer = createRenderer<RenderScrollbarPart>(renderer().document(), WTFMove(*resizer));
+        // FIXME: A renderer should be a child of its parent!
         m_resizer->setParent(&renderer());
         m_resizer->initializeStyle();
     } else
         m_resizer->setStyle(WTFMove(*resizer));
 }
 
+void RenderLayer::clearResizer()
+{
+    if (!m_resizer)
+        return;
+    m_resizer->setParent(nullptr);
+    m_resizer = nullptr;
+}
+
 RenderLayer* RenderLayer::reflectionLayer() const
 {
     return m_reflection ? m_reflection->layer() : nullptr;
@@ -6702,6 +6723,7 @@ void RenderLayer::createReflection()
 {
     ASSERT(!m_reflection);
     m_reflection = createRenderer<RenderReplica>(renderer().document(), createReflectionStyle());
+    // FIXME: A renderer should be a child of its parent!
     m_reflection->setParent(&renderer()); // We create a 1-way connection.
     m_reflection->initializeStyle();
 }

Source/WebCore/rendering/RenderLayer.h

@@ -978,7 +978,9 @@ class RenderLayer final : public ScrollableArea {
 
     void positionOverflowControls(const IntSize&);
     void updateScrollCornerStyle();
+    void clearScrollCorner();
     void updateResizerStyle();
+    void clearResizer();
 
     void drawPlatformResizerImage(GraphicsContext&, const LayoutRect& resizerCornerRect);
 

Source/WebCore/rendering/RenderObject.cpp

@@ -1428,22 +1428,9 @@ bool RenderObject::isSelectionBorder() const
 
 void RenderObject::willBeDestroyed()
 {
-    // For accessibility management, notify the parent of the imminent change to its child set.
-    // We do it now, before remove(), while the parent pointer is still available.
-    if (AXObjectCache* cache = document().existingAXObjectCache())
-        cache->childrenChanged(this->parent());
-
-    if (m_parent) {
-        // FIXME: We should have always been removed from the parent before being destroyed.
-        auto takenThis = m_parent->takeChild(*this);
-        auto* leakedPtr = takenThis.release();
-        UNUSED_PARAM(leakedPtr);
-    }
-
+    ASSERT(!m_parent);
     ASSERT(renderTreeBeingDestroyed() || !is<RenderElement>(*this) || !view().frameView().hasSlowRepaintObject(downcast<RenderElement>(*this)));
 
-    // The remove() call above may invoke axObjectCache()->childrenChanged() on the parent, which may require the AX render
-    // object for this renderer. So we remove the AX render object now, after the renderer is removed.
     if (AXObjectCache* cache = document().existingAXObjectCache())
         cache->remove(this);
 
@@ -1476,11 +1463,11 @@ void RenderObject::willBeRemovedFromTree()
     parent()->setNeedsBoundariesUpdate();
 }
 
-void RenderObject::destroyAndCleanupAnonymousWrappers()
+void RenderObject::removeFromParentAndDestroyCleaningUpAnonymousWrappers()
 {
     // If the tree is destroyed, there is no need for a clean-up phase.
     if (renderTreeBeingDestroyed()) {
-        destroy();
+        removeFromParentAndDestroy();
         return;
     }
 
@@ -1497,18 +1484,20 @@ void RenderObject::destroyAndCleanupAnonymousWrappers()
         destroyRootParent = destroyRootParent->parent();
     }
 
-    if (is<RenderTableRow>(*destroyRoot)) {
-        downcast<RenderTableRow>(*destroyRoot).destroyAndCollapseAnonymousSiblingRows();
-        return;
-    }
+    if (is<RenderTableRow>(*destroyRoot))
+        downcast<RenderTableRow>(*destroyRoot).collapseAndDestroyAnonymousSiblingRows();
 
-    destroyRoot->destroy();
+    destroyRoot->removeFromParentAndDestroy();
     // WARNING: |this| is deleted here.
 }
 
 void RenderObject::destroy()
 {
-    ASSERT(!m_bitfields.beingDestroyed());
+    RELEASE_ASSERT(!m_parent);
+    RELEASE_ASSERT(!m_next);
+    RELEASE_ASSERT(!m_previous);
+    RELEASE_ASSERT(!m_bitfields.beingDestroyed());
+
     m_bitfields.setBeingDestroyed(true);
 
 #if PLATFORM(IOS)

Source/WebCore/rendering/RenderObject.h

@@ -721,7 +721,6 @@ class RenderObject : public CachedImageClient {
     // When performing a global document tear-down, or when going into the page cache, the renderer of the document is cleared.
     bool renderTreeBeingDestroyed() const;
 
-    void destroyAndCleanupAnonymousWrappers();
     void destroy();
 
     // Virtual function helpers for the deprecated Flexible Box Layout (display: -webkit-box).
@@ -748,6 +747,7 @@ class RenderObject : public CachedImageClient {
     virtual void imageChanged(WrappedImagePtr, const IntRect* = nullptr) { }
 
     void removeFromParentAndDestroy();
+    void removeFromParentAndDestroyCleaningUpAnonymousWrappers();
 
     CSSAnimationController& animation() const;
 

Source/WebCore/rendering/RenderRubyBase.cpp

@@ -125,7 +125,7 @@ void RenderRubyBase::moveBlockChildren(RenderRubyBase* toBase, RenderObject* bef
         RenderBlock* anonBlockThere = downcast<RenderBlock>(lastChildThere);
         anonBlockHere->moveAllChildrenTo(anonBlockThere, true);
         anonBlockHere->deleteLines();
-        anonBlockHere->destroy();
+        anonBlockHere->removeFromParentAndDestroy();
     }
     // Move all remaining children normally.
     moveChildrenTo(toBase, firstChild(), beforeChild);

Source/WebCore/rendering/RenderTableRow.cpp

@@ -284,44 +284,38 @@ RenderPtr<RenderTableRow> RenderTableRow::createAnonymousWithParentRenderer(cons
     return RenderTableRow::createTableRowWithStyle(parent.document(), parent.style());
 }
 
-void RenderTableRow::destroyAndCollapseAnonymousSiblingRows()
+void RenderTableRow::collapseAndDestroyAnonymousSiblingRows()
 {
-    auto collapseAnonymousSiblingRows = [&] {
-        auto* section = this->section();
-        if (!section)
+    auto* section = this->section();
+    if (!section)
+        return;
+
+    // All siblings generated?
+    for (auto* current = section->firstRow(); current; current = current->nextRow()) {
+        if (current == this)
+            continue;
+        if (!current->isAnonymous())
             return;
+    }
 
-        // All siblings generated?
-        for (auto* current = section->firstRow(); current; current = current->nextRow()) {
-            if (current == this)
-                continue;
-            if (!current->isAnonymous())
-                return;
+    RenderTableRow* rowToInsertInto = nullptr;
+    auto* currentRow = section->firstRow();
+    while (currentRow) {
+        if (currentRow == this) {
+            currentRow = currentRow->nextRow();
+            continue;
         }
-
-        RenderTableRow* rowToInsertInto = nullptr;
-        auto* currentRow = section->firstRow();
-        while (currentRow) {
-            if (currentRow == this) {
-                currentRow = currentRow->nextRow();
-                continue;
-            }
-            if (!rowToInsertInto) {
-                rowToInsertInto = currentRow;
-                currentRow = currentRow->nextRow();
-                continue;
-            }
-            currentRow->moveAllChildrenTo(rowToInsertInto);
-            auto* destroyThis = currentRow;
+        if (!rowToInsertInto) {
+            rowToInsertInto = currentRow;
             currentRow = currentRow->nextRow();
-            destroyThis->destroy();
+            continue;
         }
-        if (rowToInsertInto)
-            rowToInsertInto->setNeedsLayout();
-    };
-
-    collapseAnonymousSiblingRows();
-    destroy();
+        currentRow->moveAllChildrenTo(rowToInsertInto);
+        auto toDestroy = section->takeChild(*currentRow);
+        currentRow = currentRow->nextRow();
+    }
+    if (rowToInsertInto)
+        rowToInsertInto->setNeedsLayout();
 }
 
 } // namespace WebCore

Source/WebCore/rendering/RenderTableRow.h

@@ -62,7 +62,7 @@ class RenderTableRow final : public RenderBox {
 
     bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, HitTestAction) override;
 
-    void destroyAndCollapseAnonymousSiblingRows();
+    void collapseAndDestroyAnonymousSiblingRows();
 
 private:
     static RenderPtr<RenderTableRow> createTableRowWithStyle(Document&, const RenderStyle&);

Source/WebCore/style/RenderTreeUpdater.cpp

@@ -520,7 +520,7 @@ void RenderTreeUpdater::tearDownRenderers(Element& root, TeardownType teardownTy
             element.clearStyleDerivedDataBeforeDetachingRenderer();
 
             if (auto* renderer = element.renderer()) {
-                renderer->destroyAndCleanupAnonymousWrappers();
+                renderer->removeFromParentAndDestroyCleaningUpAnonymousWrappers();
                 element.setRenderer(nullptr);
             }
             if (element.hasCustomStyleResolveCallbacks())
@@ -550,7 +550,7 @@ void RenderTreeUpdater::tearDownRenderer(Text& text)
     auto* renderer = text.renderer();
     if (!renderer)
         return;
-    renderer->destroyAndCleanupAnonymousWrappers();
+    renderer->removeFromParentAndDestroyCleaningUpAnonymousWrappers();
     text.setRenderer(nullptr);
 }