Changeset 220447 in webkit


Ignore:
Timestamp:
Aug 9, 2017 12:36:45 AM (7 years ago)
Author:
Antti Koivisto
Message:

RenderQuote should not mutate render tree
​https://bugs.webkit.org/show_bug.cgi?id=175328

Reviewed by Zalan Bujtas.

RenderQuote text renderers are currently created and deleted in a quirky fashion using a linked list.
This patch moves to a simpler model that guarantees the mutations are always done in controlled fashion
during render tree update.

  • dom/Document.cpp:

(WebCore::Document::updateTextRenderer):

Move text renderer updating to Document so we can set the inRenderTreeUpdate bit for it too.

  • dom/Document.h:
  • dom/Text.cpp:

(WebCore::Text::updateRendererAfterContentChange):

  • rendering/RenderDescendantIterator.h:

(WebCore::RenderDescendantIteratorAdapter<T>::at):
(WebCore::RenderDescendantConstIteratorAdapter<T>::at const):

Add at() function for starting iteration from a specified renderer.

  • rendering/RenderQuote.cpp:

(WebCore::RenderQuote::insertedIntoTree):
(WebCore::RenderQuote::willBeRemovedFromTree):

Register and unregister quotes to RenderView.
Don't do any mutations.

(WebCore::RenderQuote::styleDidChange):

Invalidate the text renderer but don't mutate it.

(WebCore::RenderQuote::updateTextRenderer):
(WebCore::RenderQuote::computeText const):
(WebCore::RenderQuote::updateRenderers):

Compute depth of all render quotes and update the text renderer as needed.

(WebCore::RenderQuote::willBeDestroyed): Deleted.
(WebCore::RenderQuote::attachQuote): Deleted.
(WebCore::RenderQuote::detachQuote): Deleted.
(WebCore::RenderQuote::updateDepth): Deleted.

Get rid of the linked list.

  • rendering/RenderQuote.h:
  • rendering/RenderView.cpp:

(WebCore::RenderView::registerQuote):
(WebCore::RenderView::unregisterQuote):

Maintain a render tree order ListHashSet of RenderQuotes.

(WebCore::RenderView::updateSpecialRenderers):

Add a function for making additional render tree mutations at the end of a render tree update.
Currently this just invokes RenderQuote::updateRenderers.

  • rendering/RenderView.h:
  • style/RenderTreeUpdater.cpp:

(WebCore::RenderTreeUpdater::commit):

Call RenderView::updateSpecialRenderers after committing all other changes.

Location:
trunk/Source/WebCore
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r220444 r220447  
     12017-08-09  Antti Koivisto  <antti@apple.com>
     2
     3        RenderQuote should not mutate render tree
     4        https://bugs.webkit.org/show_bug.cgi?id=175328
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        RenderQuote text renderers are currently created and deleted in a quirky fashion using a linked list.
     9        This patch moves to a simpler model that guarantees the mutations are always done in controlled fashion
     10        during render tree update.
     11
     12        * dom/Document.cpp:
     13        (WebCore::Document::updateTextRenderer):
     14
     15            Move text renderer updating to Document so we can set the inRenderTreeUpdate bit for it too.
     16
     17        * dom/Document.h:
     18        * dom/Text.cpp:
     19        (WebCore::Text::updateRendererAfterContentChange):
     20        * rendering/RenderDescendantIterator.h:
     21        (WebCore::RenderDescendantIteratorAdapter<T>::at):
     22        (WebCore::RenderDescendantConstIteratorAdapter<T>::at const):
     23
     24            Add at() function for starting iteration from a specified renderer.
     25
     26        * rendering/RenderQuote.cpp:
     27        (WebCore::RenderQuote::insertedIntoTree):
     28        (WebCore::RenderQuote::willBeRemovedFromTree):
     29
     30            Register and unregister quotes to RenderView.
     31            Don't do any mutations.
     32
     33        (WebCore::RenderQuote::styleDidChange):
     34
     35            Invalidate the text renderer but don't mutate it.
     36
     37        (WebCore::RenderQuote::updateTextRenderer):
     38        (WebCore::RenderQuote::computeText const):
     39        (WebCore::RenderQuote::updateRenderers):
     40
     41            Compute depth of all render quotes and update the text renderer as needed.
     42
     43        (WebCore::RenderQuote::willBeDestroyed): Deleted.
     44        (WebCore::RenderQuote::attachQuote): Deleted.
     45        (WebCore::RenderQuote::detachQuote): Deleted.
     46        (WebCore::RenderQuote::updateDepth): Deleted.
     47
     48            Get rid of the linked list.
     49
     50        * rendering/RenderQuote.h:
     51        * rendering/RenderView.cpp:
     52        (WebCore::RenderView::registerQuote):
     53        (WebCore::RenderView::unregisterQuote):
     54
     55            Maintain a render tree order ListHashSet of RenderQuotes.
     56
     57        (WebCore::RenderView::updateSpecialRenderers):
     58
     59            Add a function for making additional render tree mutations at the end of a render tree update.
     60            Currently this just invokes RenderQuote::updateRenderers.
     61
     62        * rendering/RenderView.h:
     63        * style/RenderTreeUpdater.cpp:
     64        (WebCore::RenderTreeUpdater::commit):
     65
     66            Call RenderView::updateSpecialRenderers after committing all other changes.
     67
    1682017-08-09  Zan Dobersek  <zdobersek@igalia.com>
    269

  • trunk/Source/WebCore/dom/Document.cpp

    r220427 r220447  
    18691869}
    18701870
     1871void Document::updateTextRenderer(Text& text)
     1872{
     1873    ASSERT(!m_inRenderTreeUpdate);
     1874    SetForScope<bool> inRenderTreeUpdate(m_inRenderTreeUpdate, true);
     1875
     1876    auto textUpdate = std::make_unique<Style::Update>(*this);
     1877    textUpdate->addText(text);
     1878
     1879    RenderTreeUpdater renderTreeUpdater(*this);
     1880    renderTreeUpdater.commit(WTFMove(textUpdate));
     1881}
     1882
    18711883bool Document::needsStyleRecalc() const
    18721884{

  • trunk/Source/WebCore/dom/Document.h

    r220376 r220447  
    12381238    bool inRenderTreeUpdate() const { return m_inRenderTreeUpdate; }
    12391239
     1240    void updateTextRenderer(Text&);
     1241
    12401242    // Return a Locale for the default locale if the argument is null or empty.
    12411243    Locale& getCachedLocale(const AtomicString& locale = nullAtom());

  • trunk/Source/WebCore/dom/Text.cpp

    r219856 r220447  
    2727#include "RenderSVGInlineText.h"
    2828#include "RenderText.h"
    29 #include "RenderTreeUpdater.h"
    3029#include "SVGElement.h"
    3130#include "SVGNames.h"
    … …  
    219218        return;
    220219
    221     auto textUpdate = std::make_unique<Style::Update>(document());
    222     textUpdate->addText(*this);
    223 
    224     RenderTreeUpdater renderTreeUpdater(document());
    225     renderTreeUpdater.commit(WTFMove(textUpdate));
     220    document().updateTextRenderer(*this);
    226221
    227222    if (auto* renderer = this->renderer())

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

    r200994 r220447  
    5252    RenderDescendantIterator<T> begin();
    5353    RenderDescendantIterator<T> end();
     54    RenderDescendantIterator<T> at(T&);
    5455
    5556private:
    … …  
    6364    RenderDescendantConstIterator<T> begin() const;
    6465    RenderDescendantConstIterator<T> end() const;
     66    RenderDescendantConstIterator<T> at(const T&) const;
    6567
    6668private:
    … …  
    131133}
    132134
     135template <typename T>
     136inline RenderDescendantIterator<T> RenderDescendantIteratorAdapter<T>::at(T& current)
     137{
     138    return RenderDescendantIterator<T>(m_root, &current);
     139}
     140
    133141// RenderDescendantConstIteratorAdapter
    134142
    … …  
    151159}
    152160
     161template <typename T>
     162inline RenderDescendantConstIterator<T> RenderDescendantConstIteratorAdapter<T>::at(const T& current) const
     163{
     164    return RenderDescendantConstIterator<T>(m_root, &current);
     165}
     166
    153167// Standalone functions
    154168

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

    r217893 r220447  
    22 * Copyright (C) 2011 Nokia Inc.  All rights reserved.
    33 * Copyright (C) 2012 Google Inc. All rights reserved.
    4  * Copyright (C) 2013 Apple Inc. All rights reserved.
     4 * Copyright (C) 2013, 2017 Apple Inc. All rights reserved.
    55 *
    66 * This library is free software; you can redistribute it and/or
    … …  
    4545}
    4646
    47 void RenderQuote::willBeDestroyed()
    48 {
    49     detachQuote();
    50 
    51     ASSERT(!m_isAttached);
    52     ASSERT(!m_next);
    53     ASSERT(!m_previous);
    54 
    55     RenderInline::willBeDestroyed();
    56 }
    57 
    5847void RenderQuote::insertedIntoTree()
    5948{
    6049    RenderInline::insertedIntoTree();
    61     attachQuote();
     50    view().registerQuote(*this);
    6251}
    6352
    6453void RenderQuote::willBeRemovedFromTree()
    6554{
     55    view().unregisterQuote(*this);
    6656    RenderInline::willBeRemovedFromTree();
    67     detachQuote();
    6857}
    6958
    … …  
    7160{
    7261    RenderInline::styleDidChange(diff, oldStyle);
    73     updateText();
     62    if (diff >= StyleDifferenceLayout) {
     63        m_needsTextUpdate = true;
     64        view().setHasSpecialRendererNeedingUpdate();
     65    }
    7466}
    7567
    … …  
    356348}
    357349
    358 void RenderQuote::updateText()
    359 {
     350void RenderQuote::updateTextRenderer()
     351{
     352    ASSERT_WITH_SECURITY_IMPLICATION(document().inRenderTreeUpdate());
     353    ASSERT_WITH_SECURITY_IMPLICATION(!view().renderTreeIsBeingMutatedInternally());
     354
    360355    String text = computeText();
    361356    if (m_text == text)
    362357        return;
    363358    m_text = text;
    364     // Start from the end of the child list because, if we've had a first-letter
    365     // renderer inserted then the remaining text will be at the end.
    366359    if (auto* renderText = quoteTextRenderer(lastChild())) {
    367360        renderText->setContentString(m_text);
    … …  
    396389}
    397390
    398 void RenderQuote::attachQuote()
    399 {
    400     if (view().renderTreeIsBeingMutatedInternally())
    401         return;
    402 
    403     ASSERT(!m_isAttached);
    404     ASSERT(!m_next);
    405     ASSERT(!m_previous);
    406     ASSERT(isRooted());
    407 
    408     // Optimize case where this is the first quote in a RenderView by not searching for predecessors in that case.
    409     if (view().renderQuoteHead()) {
    410         for (RenderObject* predecessor = previousInPreOrder(); predecessor; predecessor = predecessor->previousInPreOrder()) {
    411             // Skip unattached predecessors to avoid having stale m_previous pointers
    412             // if the previous node is never attached and is then destroyed.
    413             if (!is<RenderQuote>(*predecessor) || !downcast<RenderQuote>(*predecessor).m_isAttached)
    414                 continue;
    415             m_previous = downcast<RenderQuote>(predecessor);
    416             m_next = m_previous->m_next;
    417             m_previous->m_next = this;
    418             if (m_next)
    419                 m_next->m_previous = this;
    420             break;
     391void RenderQuote::updateRenderers(const RenderView& view)
     392{
     393    int depth = -1;
     394    for (auto* quote : view.quotes()) {
     395        bool isOpen = quote->m_type == OPEN_QUOTE || quote->m_type == NO_OPEN_QUOTE;
     396        if (!isOpen)
     397            --depth;
     398        else if (depth < 0)
     399            depth = 0;
     400
     401        if (quote->m_depth != depth || quote->m_needsTextUpdate) {
     402            quote->m_depth = depth;
     403            quote->m_needsTextUpdate = false;
     404            quote->updateTextRenderer();
    421405        }
    422     }
    423 
    424     if (!m_previous) {
    425         m_next = view().renderQuoteHead();
    426         view().setRenderQuoteHead(this);
    427         if (m_next)
    428             m_next->m_previous = this;
    429     }
    430 
    431     m_isAttached = true;
    432 
    433     for (RenderQuote* quote = this; quote; quote = quote->m_next)
    434         quote->updateDepth();
    435 
    436     ASSERT(!m_next || m_next->m_isAttached);
    437     ASSERT(!m_next || m_next->m_previous == this);
    438     ASSERT(!m_previous || m_previous->m_isAttached);
    439     ASSERT(!m_previous || m_previous->m_next == this);
    440 }
    441 
    442 void RenderQuote::detachQuote()
    443 {
    444     if (view().renderTreeIsBeingMutatedInternally())
    445         return;
    446 
    447     ASSERT(!m_next || m_next->m_isAttached);
    448     ASSERT(!m_previous || m_previous->m_isAttached);
    449     if (!m_isAttached)
    450         return;
    451     if (m_previous)
    452         m_previous->m_next = m_next;
    453     else
    454         view().setRenderQuoteHead(m_next);
    455     if (m_next)
    456         m_next->m_previous = m_previous;
    457     if (!renderTreeBeingDestroyed()) {
    458         for (RenderQuote* quote = m_next; quote; quote = quote->m_next)
    459             quote->updateDepth();
    460     }
    461     m_isAttached = false;
    462     m_next = 0;
    463     m_previous = 0;
    464 }
    465 
    466 void RenderQuote::updateDepth()
    467 {
    468     ASSERT(m_isAttached);
    469     int depth = 0;
    470     if (m_previous) {
    471         depth = m_previous->m_depth;
    472         if (depth < 0)
    473             depth = 0;
    474         switch (m_previous->m_type) {
    475         case OPEN_QUOTE:
    476         case NO_OPEN_QUOTE:
    477             depth++;
    478             break;
    479         case CLOSE_QUOTE:
    480         case NO_CLOSE_QUOTE:
    481             break;
    482         }
    483     }
    484     switch (m_type) {
    485     case OPEN_QUOTE:
    486     case NO_OPEN_QUOTE:
    487         break;
    488     case CLOSE_QUOTE:
    489     case NO_CLOSE_QUOTE:
    490         depth--;
    491         break;
    492     }
    493     if (m_depth == depth)
    494         return;
    495     m_depth = depth;
    496     updateText();
     406
     407        if (isOpen)
     408            ++depth;
     409    }
    497410}
    498411

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

    r214173 r220447  
    3232    virtual ~RenderQuote();
    3333
    34     void attachQuote();
     34    static void updateRenderers(const RenderView&);
    3535
    3636private:
    37     void willBeDestroyed() override;
    38     void detachQuote();
    39 
    4037    const char* renderName() const override { return "RenderQuote"; }
    4138    bool isQuote() const override { return true; }
    … …  
    4542
    4643    String computeText() const;
    47     void updateText();
    48     void updateDepth();
     44    void updateTextRenderer();
    4945
    5046    const QuoteType m_type;
    5147    int m_depth { -1 };
    52     RenderQuote* m_next { nullptr };
    53     RenderQuote* m_previous { nullptr };
    54     bool m_isAttached { false };
    5548    String m_text;
     49
     50    bool m_needsTextUpdate { false };
    5651};
    5752

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

    r220333 r220447  
    3939#include "NodeTraversal.h"
    4040#include "Page.h"
     41#include "RenderDescendantIterator.h"
    4142#include "RenderGeometryMap.h"
    4243#include "RenderIterator.h"
    … …  
    4849#include "RenderMultiColumnSpannerPlaceholder.h"
    4950#include "RenderNamedFlowThread.h"
     51#include "RenderQuote.h"
    5052#include "RenderSelectionInfo.h"
    5153#include "RenderWidget.h"
    … …  
    15231525}
    15241526
     1527void RenderView::registerQuote(RenderQuote& quote)
     1528{
     1529    ASSERT(!m_quotes.contains(&quote));
     1530
     1531    setHasSpecialRendererNeedingUpdate();
     1532
     1533    if (m_quotes.isEmpty()) {
     1534        m_quotes.add(&quote);
     1535        return;
     1536    }
     1537    auto quoteRenderers = descendantsOfType<RenderQuote>(*this);
     1538    auto it = quoteRenderers.at(quote);
     1539    if (++it == quoteRenderers.end()) {
     1540        m_quotes.add(&quote);
     1541        return;
     1542    }
     1543    auto& nextQuote = *it;
     1544    ASSERT(m_quotes.contains(&nextQuote));
     1545    m_quotes.insertBefore(&nextQuote, &quote);
     1546}
     1547
     1548void RenderView::unregisterQuote(RenderQuote& quote)
     1549{
     1550    ASSERT(m_quotes.contains(&quote));
     1551
     1552    setHasSpecialRendererNeedingUpdate();
     1553
     1554    m_quotes.remove(&quote);
     1555}
     1556
     1557void RenderView::updateSpecialRenderers()
     1558{
     1559    ASSERT_WITH_SECURITY_IMPLICATION(document().inRenderTreeUpdate());
     1560    ASSERT_WITH_SECURITY_IMPLICATION(!renderTreeIsBeingMutatedInternally());
     1561
     1562    if (!m_hasSpecialRendererNeedingUpdate)
     1563        return;
     1564    m_hasSpecialRendererNeedingUpdate = false;
     1565
     1566    RenderQuote::updateRenderers(*this);
     1567}
     1568
    15251569#if ENABLE(CSS_SCROLL_SNAP)
    15261570void RenderView::registerBoxWithScrollSnapPositions(const RenderBox& box)

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

    r219121 r220447  
    3030#include <memory>
    3131#include <wtf/HashSet.h>
     32#include <wtf/ListHashSet.h>
    3233
    3334#if ENABLE(SERVICE_CONTROLS)
    … …  
    196197    IntSize viewportSizeForCSSViewportUnits() const;
    197198
    198     void setRenderQuoteHead(RenderQuote* head) { m_renderQuoteHead = head; }
    199     RenderQuote* renderQuoteHead() const { return m_renderQuoteHead; }
    200    
     199    void registerQuote(RenderQuote&);
     200    void unregisterQuote(RenderQuote&);
     201    const ListHashSet<RenderQuote*>& quotes() const { return m_quotes; }
     202
     203    void setHasSpecialRendererNeedingUpdate() { m_hasSpecialRendererNeedingUpdate = true; }
     204    void updateSpecialRenderers();
     205
    201206    // FIXME: see class RenderTreeInternalMutation below.
    202207    bool renderTreeIsBeingMutatedInternally() const { return !!m_renderTreeInternalMutationCounter; }
    … …  
    372377    std::unique_ptr<FlowThreadController> m_flowThreadController;
    373378
    374     RenderQuote* m_renderQuoteHead { nullptr };
     379    ListHashSet<RenderQuote*> m_quotes;
     380    bool m_hasSpecialRendererNeedingUpdate { false };
     381
    375382    unsigned m_renderCounterCount { 0 };
    376383    unsigned m_renderTreeInternalMutationCounter { 0 };

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

    r220202 r220447  
    125125        updateRenderTree(*root);
    126126
     127    m_document.renderView()->updateSpecialRenderers();
     128
    127129    m_styleUpdate = nullptr;
    128130}
Note: See TracChangeset for help on using the changeset viewer.