Changeset 223131 in webkit


Ignore:
Timestamp:
Oct 10, 2017 6:02:47 AM (7 years ago)
Author:
Antti Koivisto
Message:

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.

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.

Location:
trunk
Files:
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r223126 r223131  
     12017-10-10  Antti Koivisto  <antti@apple.com>
     2
     3        RenderObject::destroy() should only be invoked after renderer has been removed from the tree
     4        https://bugs.webkit.org/show_bug.cgi?id=178075
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        * accessibility/mac/textbox-role-reports-notifications.html:
     9
     10        This passed because spurious AXValueChanged notifications. Force layout to prevent coalescing between mutations.
     11
    1122017-10-10  Joanmarie Diggs  <jdiggs@igalia.com>
    213

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

    r217171 r223131  
    2020        pendingNotifications = 3;
    2121        ariaTextBox.firstChild.deleteData(0, 5);
     22        ariaTextBox.offsetWidth;
    2223        ariaTextBox.textContent = "changed textContent";
     24        ariaTextBox.offsetWidth;
    2325        ariaTextBox.innerText = "changed innerText";
    2426    }

  • trunk/Source/WebCore/ChangeLog

    r223127 r223131  
     12017-10-10  Antti Koivisto  <antti@apple.com>
     2
     3        RenderObject::destroy() should only be invoked after renderer has been removed from the tree
     4        https://bugs.webkit.org/show_bug.cgi?id=178075
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        This patch fixes the remaining cases where the renderer is still in the tree while destroy()
     9        is called and adds the assert.
     10
     11        * rendering/RenderBlock.cpp:
     12        (WebCore::RenderBlock::removeLeftoverAnonymousBlock):
     13        (WebCore::RenderBlock::takeChild):
     14        * rendering/RenderBoxModelObject.cpp:
     15        (WebCore::RenderBoxModelObject::willBeDestroyed):
     16        * rendering/RenderLayer.cpp:
     17        (WebCore::RenderLayer::~RenderLayer):
     18
     19            Null the parent pointers for m_scrollCorner/m_resizer.
     20
     21        (WebCore::RenderLayer::calculateClipRects const):
     22        * rendering/RenderLayer.h:
     23        * rendering/RenderObject.cpp:
     24        (WebCore::RenderObject::willBeDestroyed):
     25        (WebCore::RenderObject::removeFromParentAndDestroyCleaningUpAnonymousWrappers):
     26        (WebCore::RenderObject::destroy):
     27
     28            Use RELEASE_ASSERT as these are cheap and important checks.
     29            Also turn isBeingDestroyed test into RELEASE_ASSERT.
     30            Remove AX call that no longer does anything.
     31
     32        (WebCore::RenderObject::destroyAndCleanupAnonymousWrappers): Deleted.
     33        * rendering/RenderObject.h:
     34        * rendering/RenderRubyBase.cpp:
     35        (WebCore::RenderRubyBase::moveBlockChildren):
     36        * rendering/RenderTableRow.cpp:
     37        (WebCore::RenderTableRow::collapseAndDestroyAnonymousSiblingRows):
     38        (WebCore::RenderTableRow::destroyAndCollapseAnonymousSiblingRows): Deleted.
     39
     40            Renamed and made this no longer destroy itself. The caller now takes care of that.
     41            Removed an unnecessary lambda.
     42
     43        * rendering/RenderTableRow.h:
     44        * style/RenderTreeUpdater.cpp:
     45        (WebCore::RenderTreeUpdater::tearDownRenderers):
     46        (WebCore::RenderTreeUpdater::tearDownRenderer):
     47        * style/RenderTreeUpdaterListItem.cpp:
     48        (WebCore::RenderTreeUpdater::ListItem::updateMarker):
     49
    1502017-10-09  Antti Koivisto  <antti@apple.com>
    251

  • trunk/Source/WebCore/rendering/RenderBlock.cpp

    r223072 r223131  
    765765    }
    766766
    767     child->setFirstChild(0);
    768     child->m_next = 0;
     767    child->setFirstChild(nullptr);
     768    child->m_next = nullptr;
    769769
    770770    // Remove all the information in the flow thread associated with the leftover anonymous block.
    771771    child->resetFragmentedFlowStateOnRemoval();
    772772
    773     child->setParent(0);
    774     child->setPreviousSibling(0);
    775     child->setNextSibling(0);
     773    child->setParent(nullptr);
     774    child->setPreviousSibling(nullptr);
     775    child->setNextSibling(nullptr);
    776776
    777777    child->destroy();
    … …  
    876876            // Delete the now-empty block's lines and nuke it.
    877877            nextBlock.deleteLines();
    878             nextBlock.destroy();
     878            nextBlock.removeFromParentAndDestroy();
    879879            next = nullptr;
    880880        }
    … …  
    935935            }
    936936            setContinuation(nullptr);
    937             destroy();
     937            // FIXME: This is dangerous.
     938            removeFromParentAndDestroy();
    938939        }
    939940    }

  • trunk/Source/WebCore/rendering/RenderBoxModelObject.cpp

    r223127 r223131  
    189189{
    190190    if (hasContinuation()) {
    191         continuation()->destroy();
     191        continuation()->removeFromParentAndDestroy();
    192192        setContinuation(nullptr);
    193193    }

  • trunk/Source/WebCore/rendering/RenderLayer.cpp

    r222575 r223131  
    370370        removeReflection();
    371371
     372    clearScrollCorner();
     373    clearResizer();
     374
    372375    FilterInfo::remove(*this);
    373376
    … …  
    66646667
    66656668    if (!corner) {
    6666         m_scrollCorner = nullptr;
     6669        clearScrollCorner();
    66676670        return;
    66686671    }
    … …  
    66706673    if (!m_scrollCorner) {
    66716674        m_scrollCorner = createRenderer<RenderScrollbarPart>(renderer().document(), WTFMove(*corner));
     6675        // FIXME: A renderer should be a child of its parent!
    66726676        m_scrollCorner->setParent(&renderer());
    66736677        m_scrollCorner->initializeStyle();
    … …  
    66766680}
    66776681
     6682void RenderLayer::clearScrollCorner()
     6683{
     6684    if (!m_scrollCorner)
     6685        return;
     6686    m_scrollCorner->setParent(nullptr);
     6687    m_scrollCorner = nullptr;
     6688}
     6689
    66786690void RenderLayer::updateResizerStyle()
    66796691{
    … …  
    66826694
    66836695    if (!resizer) {
    6684         m_resizer = nullptr;
     6696        clearResizer();
    66856697        return;
    66866698    }
    … …  
    66886700    if (!m_resizer) {
    66896701        m_resizer = createRenderer<RenderScrollbarPart>(renderer().document(), WTFMove(*resizer));
     6702        // FIXME: A renderer should be a child of its parent!
    66906703        m_resizer->setParent(&renderer());
    66916704        m_resizer->initializeStyle();
    … …  
    66946707}
    66956708
     6709void RenderLayer::clearResizer()
     6710{
     6711    if (!m_resizer)
     6712        return;
     6713    m_resizer->setParent(nullptr);
     6714    m_resizer = nullptr;
     6715}
     6716
    66966717RenderLayer* RenderLayer::reflectionLayer() const
    66976718{
    … …  
    67036724    ASSERT(!m_reflection);
    67046725    m_reflection = createRenderer<RenderReplica>(renderer().document(), createReflectionStyle());
     6726    // FIXME: A renderer should be a child of its parent!
    67056727    m_reflection->setParent(&renderer()); // We create a 1-way connection.
    67066728    m_reflection->initializeStyle();

  • trunk/Source/WebCore/rendering/RenderLayer.h

    r222575 r223131  
    979979    void positionOverflowControls(const IntSize&);
    980980    void updateScrollCornerStyle();
     981    void clearScrollCorner();
    981982    void updateResizerStyle();
     983    void clearResizer();
    982984
    983985    void drawPlatformResizerImage(GraphicsContext&, const LayoutRect& resizerCornerRect);

  • trunk/Source/WebCore/rendering/RenderObject.cpp

    r223047 r223131  
    14291429void RenderObject::willBeDestroyed()
    14301430{
    1431     // For accessibility management, notify the parent of the imminent change to its child set.
    1432     // We do it now, before remove(), while the parent pointer is still available.
    1433     if (AXObjectCache* cache = document().existingAXObjectCache())
    1434         cache->childrenChanged(this->parent());
    1435 
    1436     if (m_parent) {
    1437         // FIXME: We should have always been removed from the parent before being destroyed.
    1438         auto takenThis = m_parent->takeChild(*this);
    1439         auto* leakedPtr = takenThis.release();
    1440         UNUSED_PARAM(leakedPtr);
    1441     }
    1442 
     1431    ASSERT(!m_parent);
    14431432    ASSERT(renderTreeBeingDestroyed() || !is<RenderElement>(*this) || !view().frameView().hasSlowRepaintObject(downcast<RenderElement>(*this)));
    14441433
    1445     // The remove() call above may invoke axObjectCache()->childrenChanged() on the parent, which may require the AX render
    1446     // object for this renderer. So we remove the AX render object now, after the renderer is removed.
    14471434    if (AXObjectCache* cache = document().existingAXObjectCache())
    14481435        cache->remove(this);
    … …  
    14771464}
    14781465
    1479 void RenderObject::destroyAndCleanupAnonymousWrappers()
     1466void RenderObject::removeFromParentAndDestroyCleaningUpAnonymousWrappers()
    14801467{
    14811468    // If the tree is destroyed, there is no need for a clean-up phase.
    14821469    if (renderTreeBeingDestroyed()) {
    1483         destroy();
     1470        removeFromParentAndDestroy();
    14841471        return;
    14851472    }
    … …  
    14981485    }
    14991486
    1500     if (is<RenderTableRow>(*destroyRoot)) {
    1501         downcast<RenderTableRow>(*destroyRoot).destroyAndCollapseAnonymousSiblingRows();
    1502         return;
    1503     }
    1504 
    1505     destroyRoot->destroy();
     1487    if (is<RenderTableRow>(*destroyRoot))
     1488        downcast<RenderTableRow>(*destroyRoot).collapseAndDestroyAnonymousSiblingRows();
     1489
     1490    destroyRoot->removeFromParentAndDestroy();
    15061491    // WARNING: |this| is deleted here.
    15071492}
    … …  
    15091494void RenderObject::destroy()
    15101495{
    1511     ASSERT(!m_bitfields.beingDestroyed());
     1496    RELEASE_ASSERT(!m_parent);
     1497    RELEASE_ASSERT(!m_next);
     1498    RELEASE_ASSERT(!m_previous);
     1499    RELEASE_ASSERT(!m_bitfields.beingDestroyed());
     1500
    15121501    m_bitfields.setBeingDestroyed(true);
    15131502

  • trunk/Source/WebCore/rendering/RenderObject.h

    r223127 r223131  
    722722    bool renderTreeBeingDestroyed() const;
    723723
    724     void destroyAndCleanupAnonymousWrappers();
    725724    void destroy();
    726725
    … …  
    749748
    750749    void removeFromParentAndDestroy();
     750    void removeFromParentAndDestroyCleaningUpAnonymousWrappers();
    751751
    752752    CSSAnimationController& animation() const;

  • trunk/Source/WebCore/rendering/RenderRubyBase.cpp

    r222679 r223131  
    126126        anonBlockHere->moveAllChildrenTo(anonBlockThere, true);
    127127        anonBlockHere->deleteLines();
    128         anonBlockHere->destroy();
     128        anonBlockHere->removeFromParentAndDestroy();
    129129    }
    130130    // Move all remaining children normally.

  • trunk/Source/WebCore/rendering/RenderTableRow.cpp

    r222679 r223131  
    285285}
    286286
    287 void RenderTableRow::destroyAndCollapseAnonymousSiblingRows()
    288 {
    289     auto collapseAnonymousSiblingRows = [&] {
    290         auto* section = this->section();
    291         if (!section)
     287void RenderTableRow::collapseAndDestroyAnonymousSiblingRows()
     288{
     289    auto* section = this->section();
     290    if (!section)
     291        return;
     292
     293    // All siblings generated?
     294    for (auto* current = section->firstRow(); current; current = current->nextRow()) {
     295        if (current == this)
     296            continue;
     297        if (!current->isAnonymous())
    292298            return;
    293 
    294         // All siblings generated?
    295         for (auto* current = section->firstRow(); current; current = current->nextRow()) {
    296             if (current == this)
    297                 continue;
    298             if (!current->isAnonymous())
    299                 return;
    300         }
    301 
    302         RenderTableRow* rowToInsertInto = nullptr;
    303         auto* currentRow = section->firstRow();
    304         while (currentRow) {
    305             if (currentRow == this) {
    306                 currentRow = currentRow->nextRow();
    307                 continue;
    308             }
    309             if (!rowToInsertInto) {
    310                 rowToInsertInto = currentRow;
    311                 currentRow = currentRow->nextRow();
    312                 continue;
    313             }
    314             currentRow->moveAllChildrenTo(rowToInsertInto);
    315             auto* destroyThis = currentRow;
     299    }
     300
     301    RenderTableRow* rowToInsertInto = nullptr;
     302    auto* currentRow = section->firstRow();
     303    while (currentRow) {
     304        if (currentRow == this) {
    316305            currentRow = currentRow->nextRow();
    317             destroyThis->destroy();
    318         }
    319         if (rowToInsertInto)
    320             rowToInsertInto->setNeedsLayout();
    321     };
    322 
    323     collapseAnonymousSiblingRows();
    324     destroy();
     306            continue;
     307        }
     308        if (!rowToInsertInto) {
     309            rowToInsertInto = currentRow;
     310            currentRow = currentRow->nextRow();
     311            continue;
     312        }
     313        currentRow->moveAllChildrenTo(rowToInsertInto);
     314        auto toDestroy = section->takeChild(*currentRow);
     315        currentRow = currentRow->nextRow();
     316    }
     317    if (rowToInsertInto)
     318        rowToInsertInto->setNeedsLayout();
    325319}
    326320

  • trunk/Source/WebCore/rendering/RenderTableRow.h

    r222679 r223131  
    6363    bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, HitTestAction) override;
    6464
    65     void destroyAndCollapseAnonymousSiblingRows();
     65    void collapseAndDestroyAnonymousSiblingRows();
    6666
    6767private:

  • trunk/Source/WebCore/style/RenderTreeUpdater.cpp

    r222679 r223131  
    521521
    522522            if (auto* renderer = element.renderer()) {
    523                 renderer->destroyAndCleanupAnonymousWrappers();
     523                renderer->removeFromParentAndDestroyCleaningUpAnonymousWrappers();
    524524                element.setRenderer(nullptr);
    525525            }
    … …  
    551551    if (!renderer)
    552552        return;
    553     renderer->destroyAndCleanupAnonymousWrappers();
     553    renderer->removeFromParentAndDestroyCleaningUpAnonymousWrappers();
    554554    text.setRenderer(nullptr);
    555555}

  • trunk/Source/WebCore/style/RenderTreeUpdaterListItem.cpp

    r222936 r223131  
    117117        // If current parent is an anonymous block that has lost all its children, destroy it.
    118118        if (currentParent && currentParent->isAnonymousBlock() && !currentParent->firstChild() && !downcast<RenderBlock>(*currentParent).continuation())
    119             currentParent->destroy();
     119            currentParent->removeFromParentAndDestroy();
    120120    }
    121121}
Note: See TracChangeset for help on using the changeset viewer.