Title: [220447] trunk/Source/WebCore
Revision
220447
Author
an...@apple.com
Date
2017-08-09 00:36:45 -0700 (Wed, 09 Aug 2017)

Log 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.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (220446 => 220447)


--- trunk/Source/WebCore/ChangeLog	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/ChangeLog	2017-08-09 07:36:45 UTC (rev 220447)
@@ -1,3 +1,70 @@
+2017-08-09  Antti Koivisto  <an...@apple.com>
+
+        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.
+
 2017-08-09  Zan Dobersek  <zdober...@igalia.com>
 
         [Soup] Incorrect conversion in msToSoupDate()

Modified: trunk/Source/WebCore/dom/Document.cpp (220446 => 220447)


--- trunk/Source/WebCore/dom/Document.cpp	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/dom/Document.cpp	2017-08-09 07:36:45 UTC (rev 220447)
@@ -1868,6 +1868,18 @@
     // FIXME: Ideally we would ASSERT(!needsStyleRecalc()) here but we have some cases where it is not true.
 }
 
+void Document::updateTextRenderer(Text& text)
+{
+    ASSERT(!m_inRenderTreeUpdate);
+    SetForScope<bool> inRenderTreeUpdate(m_inRenderTreeUpdate, true);
+
+    auto textUpdate = std::make_unique<Style::Update>(*this);
+    textUpdate->addText(text);
+
+    RenderTreeUpdater renderTreeUpdater(*this);
+    renderTreeUpdater.commit(WTFMove(textUpdate));
+}
+
 bool Document::needsStyleRecalc() const
 {
     if (pageCacheState() != NotInPageCache)

Modified: trunk/Source/WebCore/dom/Document.h (220446 => 220447)


--- trunk/Source/WebCore/dom/Document.h	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/dom/Document.h	2017-08-09 07:36:45 UTC (rev 220447)
@@ -1237,6 +1237,8 @@
     bool inStyleRecalc() const { return m_inStyleRecalc; }
     bool inRenderTreeUpdate() const { return m_inRenderTreeUpdate; }
 
+    void updateTextRenderer(Text&);
+
     // Return a Locale for the default locale if the argument is null or empty.
     Locale& getCachedLocale(const AtomicString& locale = nullAtom());
 

Modified: trunk/Source/WebCore/dom/Text.cpp (220446 => 220447)


--- trunk/Source/WebCore/dom/Text.cpp	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/dom/Text.cpp	2017-08-09 07:36:45 UTC (rev 220447)
@@ -26,7 +26,6 @@
 #include "RenderCombineText.h"
 #include "RenderSVGInlineText.h"
 #include "RenderText.h"
-#include "RenderTreeUpdater.h"
 #include "SVGElement.h"
 #include "SVGNames.h"
 #include "ScopedEventQueue.h"
@@ -218,12 +217,8 @@
     if (styleValidity() >= Style::Validity::SubtreeAndRenderersInvalid)
         return;
 
-    auto textUpdate = std::make_unique<Style::Update>(document());
-    textUpdate->addText(*this);
+    document().updateTextRenderer(*this);
 
-    RenderTreeUpdater renderTreeUpdater(document());
-    renderTreeUpdater.commit(WTFMove(textUpdate));
-
     if (auto* renderer = this->renderer())
         renderer->setTextWithOffset(data(), offsetOfReplacedData, lengthOfReplacedData);
 }

Modified: trunk/Source/WebCore/rendering/RenderDescendantIterator.h (220446 => 220447)


--- trunk/Source/WebCore/rendering/RenderDescendantIterator.h	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/rendering/RenderDescendantIterator.h	2017-08-09 07:36:45 UTC (rev 220447)
@@ -51,6 +51,7 @@
     RenderDescendantIteratorAdapter(RenderElement& root);
     RenderDescendantIterator<T> begin();
     RenderDescendantIterator<T> end();
+    RenderDescendantIterator<T> at(T&);
 
 private:
     RenderElement& m_root;
@@ -62,6 +63,7 @@
     RenderDescendantConstIteratorAdapter(const RenderElement& root);
     RenderDescendantConstIterator<T> begin() const;
     RenderDescendantConstIterator<T> end() const;
+    RenderDescendantConstIterator<T> at(const T&) const;
 
 private:
     const RenderElement& m_root;
@@ -130,6 +132,12 @@
     return RenderDescendantIterator<T>(m_root);
 }
 
+template <typename T>
+inline RenderDescendantIterator<T> RenderDescendantIteratorAdapter<T>::at(T& current)
+{
+    return RenderDescendantIterator<T>(m_root, &current);
+}
+
 // RenderDescendantConstIteratorAdapter
 
 template <typename T>
@@ -150,6 +158,12 @@
     return RenderDescendantConstIterator<T>(m_root);
 }
 
+template <typename T>
+inline RenderDescendantConstIterator<T> RenderDescendantConstIteratorAdapter<T>::at(const T& current) const
+{
+    return RenderDescendantConstIterator<T>(m_root, &current);
+}
+
 // Standalone functions
 
 template <typename T>

Modified: trunk/Source/WebCore/rendering/RenderQuote.cpp (220446 => 220447)


--- trunk/Source/WebCore/rendering/RenderQuote.cpp	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/rendering/RenderQuote.cpp	2017-08-09 07:36:45 UTC (rev 220447)
@@ -1,7 +1,7 @@
 /*
  * Copyright (C) 2011 Nokia Inc.  All rights reserved.
  * Copyright (C) 2012 Google Inc. All rights reserved.
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2017 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -44,33 +44,25 @@
     // Do not add any code here. Add it to willBeDestroyed() instead.
 }
 
-void RenderQuote::willBeDestroyed()
-{
-    detachQuote();
-
-    ASSERT(!m_isAttached);
-    ASSERT(!m_next);
-    ASSERT(!m_previous);
-
-    RenderInline::willBeDestroyed();
-}
-
 void RenderQuote::insertedIntoTree()
 {
     RenderInline::insertedIntoTree();
-    attachQuote();
+    view().registerQuote(*this);
 }
 
 void RenderQuote::willBeRemovedFromTree()
 {
+    view().unregisterQuote(*this);
     RenderInline::willBeRemovedFromTree();
-    detachQuote();
 }
 
 void RenderQuote::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
     RenderInline::styleDidChange(diff, oldStyle);
-    updateText();
+    if (diff >= StyleDifferenceLayout) {
+        m_needsTextUpdate = true;
+        view().setHasSpecialRendererNeedingUpdate();
+    }
 }
 
 const unsigned maxDistinctQuoteCharacters = 16;
@@ -355,14 +347,15 @@
     return downcast<RenderTextFragment>(lastChild);
 }
 
-void RenderQuote::updateText()
+void RenderQuote::updateTextRenderer()
 {
+    ASSERT_WITH_SECURITY_IMPLICATION(document().inRenderTreeUpdate());
+    ASSERT_WITH_SECURITY_IMPLICATION(!view().renderTreeIsBeingMutatedInternally());
+
     String text = computeText();
     if (m_text == text)
         return;
     m_text = text;
-    // Start from the end of the child list because, if we've had a first-letter
-    // renderer inserted then the remaining text will be at the end.
     if (auto* renderText = quoteTextRenderer(lastChild())) {
         renderText->setContentString(m_text);
         renderText->dirtyLineBoxes(false);
@@ -395,105 +388,25 @@
     return emptyString();
 }
 
-void RenderQuote::attachQuote()
+void RenderQuote::updateRenderers(const RenderView& view)
 {
-    if (view().renderTreeIsBeingMutatedInternally())
-        return;
+    int depth = -1;
+    for (auto* quote : view.quotes()) {
+        bool isOpen = quote->m_type == OPEN_QUOTE || quote->m_type == NO_OPEN_QUOTE;
+        if (!isOpen)
+            --depth;
+        else if (depth < 0)
+            depth = 0;
 
-    ASSERT(!m_isAttached);
-    ASSERT(!m_next);
-    ASSERT(!m_previous);
-    ASSERT(isRooted());
-
-    // Optimize case where this is the first quote in a RenderView by not searching for predecessors in that case.
-    if (view().renderQuoteHead()) {
-        for (RenderObject* predecessor = previousInPreOrder(); predecessor; predecessor = predecessor->previousInPreOrder()) {
-            // Skip unattached predecessors to avoid having stale m_previous pointers
-            // if the previous node is never attached and is then destroyed.
-            if (!is<RenderQuote>(*predecessor) || !downcast<RenderQuote>(*predecessor).m_isAttached)
-                continue;
-            m_previous = downcast<RenderQuote>(predecessor);
-            m_next = m_previous->m_next;
-            m_previous->m_next = this;
-            if (m_next)
-                m_next->m_previous = this;
-            break;
+        if (quote->m_depth != depth || quote->m_needsTextUpdate) {
+            quote->m_depth = depth;
+            quote->m_needsTextUpdate = false;
+            quote->updateTextRenderer();
         }
-    }
 
-    if (!m_previous) {
-        m_next = view().renderQuoteHead();
-        view().setRenderQuoteHead(this);
-        if (m_next)
-            m_next->m_previous = this;
+        if (isOpen)
+            ++depth;
     }
-
-    m_isAttached = true;
-
-    for (RenderQuote* quote = this; quote; quote = quote->m_next)
-        quote->updateDepth();
-
-    ASSERT(!m_next || m_next->m_isAttached);
-    ASSERT(!m_next || m_next->m_previous == this);
-    ASSERT(!m_previous || m_previous->m_isAttached);
-    ASSERT(!m_previous || m_previous->m_next == this);
 }
 
-void RenderQuote::detachQuote()
-{
-    if (view().renderTreeIsBeingMutatedInternally())
-        return;
-
-    ASSERT(!m_next || m_next->m_isAttached);
-    ASSERT(!m_previous || m_previous->m_isAttached);
-    if (!m_isAttached)
-        return;
-    if (m_previous)
-        m_previous->m_next = m_next;
-    else
-        view().setRenderQuoteHead(m_next);
-    if (m_next)
-        m_next->m_previous = m_previous;
-    if (!renderTreeBeingDestroyed()) {
-        for (RenderQuote* quote = m_next; quote; quote = quote->m_next)
-            quote->updateDepth();
-    }
-    m_isAttached = false;
-    m_next = 0;
-    m_previous = 0;
-}
-
-void RenderQuote::updateDepth()
-{
-    ASSERT(m_isAttached);
-    int depth = 0;
-    if (m_previous) {
-        depth = m_previous->m_depth;
-        if (depth < 0)
-            depth = 0;
-        switch (m_previous->m_type) {
-        case OPEN_QUOTE:
-        case NO_OPEN_QUOTE:
-            depth++;
-            break;
-        case CLOSE_QUOTE:
-        case NO_CLOSE_QUOTE:
-            break;
-        }
-    }
-    switch (m_type) {
-    case OPEN_QUOTE:
-    case NO_OPEN_QUOTE:
-        break;
-    case CLOSE_QUOTE:
-    case NO_CLOSE_QUOTE:
-        depth--;
-        break;
-    }
-    if (m_depth == depth)
-        return;
-    m_depth = depth;
-    updateText();
-}
-
 } // namespace WebCore

Modified: trunk/Source/WebCore/rendering/RenderQuote.h (220446 => 220447)


--- trunk/Source/WebCore/rendering/RenderQuote.h	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/rendering/RenderQuote.h	2017-08-09 07:36:45 UTC (rev 220447)
@@ -31,12 +31,9 @@
     RenderQuote(Document&, RenderStyle&&, QuoteType);
     virtual ~RenderQuote();
 
-    void attachQuote();
+    static void updateRenderers(const RenderView&);
 
 private:
-    void willBeDestroyed() override;
-    void detachQuote();
-
     const char* renderName() const override { return "RenderQuote"; }
     bool isQuote() const override { return true; }
     void styleDidChange(StyleDifference, const RenderStyle*) override;
@@ -44,15 +41,13 @@
     void willBeRemovedFromTree() override;
 
     String computeText() const;
-    void updateText();
-    void updateDepth();
+    void updateTextRenderer();
 
     const QuoteType m_type;
     int m_depth { -1 };
-    RenderQuote* m_next { nullptr };
-    RenderQuote* m_previous { nullptr };
-    bool m_isAttached { false };
     String m_text;
+
+    bool m_needsTextUpdate { false };
 };
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/rendering/RenderView.cpp (220446 => 220447)


--- trunk/Source/WebCore/rendering/RenderView.cpp	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/rendering/RenderView.cpp	2017-08-09 07:36:45 UTC (rev 220447)
@@ -38,6 +38,7 @@
 #include "ImageQualityController.h"
 #include "NodeTraversal.h"
 #include "Page.h"
+#include "RenderDescendantIterator.h"
 #include "RenderGeometryMap.h"
 #include "RenderIterator.h"
 #include "RenderLayer.h"
@@ -47,6 +48,7 @@
 #include "RenderMultiColumnSet.h"
 #include "RenderMultiColumnSpannerPlaceholder.h"
 #include "RenderNamedFlowThread.h"
+#include "RenderQuote.h"
 #include "RenderSelectionInfo.h"
 #include "RenderWidget.h"
 #include "ScrollbarTheme.h"
@@ -1522,6 +1524,48 @@
     return 0;
 }
 
+void RenderView::registerQuote(RenderQuote& quote)
+{
+    ASSERT(!m_quotes.contains(&quote));
+
+    setHasSpecialRendererNeedingUpdate();
+
+    if (m_quotes.isEmpty()) {
+        m_quotes.add(&quote);
+        return;
+    }
+    auto quoteRenderers = descendantsOfType<RenderQuote>(*this);
+    auto it = quoteRenderers.at(quote);
+    if (++it == quoteRenderers.end()) {
+        m_quotes.add(&quote);
+        return;
+    }
+    auto& nextQuote = *it;
+    ASSERT(m_quotes.contains(&nextQuote));
+    m_quotes.insertBefore(&nextQuote, &quote);
+}
+
+void RenderView::unregisterQuote(RenderQuote& quote)
+{
+    ASSERT(m_quotes.contains(&quote));
+
+    setHasSpecialRendererNeedingUpdate();
+
+    m_quotes.remove(&quote);
+}
+
+void RenderView::updateSpecialRenderers()
+{
+    ASSERT_WITH_SECURITY_IMPLICATION(document().inRenderTreeUpdate());
+    ASSERT_WITH_SECURITY_IMPLICATION(!renderTreeIsBeingMutatedInternally());
+
+    if (!m_hasSpecialRendererNeedingUpdate)
+        return;
+    m_hasSpecialRendererNeedingUpdate = false;
+
+    RenderQuote::updateRenderers(*this);
+}
+
 #if ENABLE(CSS_SCROLL_SNAP)
 void RenderView::registerBoxWithScrollSnapPositions(const RenderBox& box)
 {

Modified: trunk/Source/WebCore/rendering/RenderView.h (220446 => 220447)


--- trunk/Source/WebCore/rendering/RenderView.h	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/rendering/RenderView.h	2017-08-09 07:36:45 UTC (rev 220447)
@@ -29,6 +29,7 @@
 #include "SelectionSubtreeRoot.h"
 #include <memory>
 #include <wtf/HashSet.h>
+#include <wtf/ListHashSet.h>
 
 #if ENABLE(SERVICE_CONTROLS)
 #include "SelectionRectGatherer.h"
@@ -195,9 +196,13 @@
 
     IntSize viewportSizeForCSSViewportUnits() const;
 
-    void setRenderQuoteHead(RenderQuote* head) { m_renderQuoteHead = head; }
-    RenderQuote* renderQuoteHead() const { return m_renderQuoteHead; }
-    
+    void registerQuote(RenderQuote&);
+    void unregisterQuote(RenderQuote&);
+    const ListHashSet<RenderQuote*>& quotes() const { return m_quotes; }
+
+    void setHasSpecialRendererNeedingUpdate() { m_hasSpecialRendererNeedingUpdate = true; }
+    void updateSpecialRenderers();
+
     // FIXME: see class RenderTreeInternalMutation below.
     bool renderTreeIsBeingMutatedInternally() const { return !!m_renderTreeInternalMutationCounter; }
 
@@ -371,7 +376,9 @@
     std::unique_ptr<RenderLayerCompositor> m_compositor;
     std::unique_ptr<FlowThreadController> m_flowThreadController;
 
-    RenderQuote* m_renderQuoteHead { nullptr };
+    ListHashSet<RenderQuote*> m_quotes;
+    bool m_hasSpecialRendererNeedingUpdate { false };
+
     unsigned m_renderCounterCount { 0 };
     unsigned m_renderTreeInternalMutationCounter { 0 };
 

Modified: trunk/Source/WebCore/style/RenderTreeUpdater.cpp (220446 => 220447)


--- trunk/Source/WebCore/style/RenderTreeUpdater.cpp	2017-08-09 07:16:56 UTC (rev 220446)
+++ trunk/Source/WebCore/style/RenderTreeUpdater.cpp	2017-08-09 07:36:45 UTC (rev 220447)
@@ -124,6 +124,8 @@
     for (auto* root : findRenderingRoots(*m_styleUpdate))
         updateRenderTree(*root);
 
+    m_document.renderView()->updateSpecialRenderers();
+
     m_styleUpdate = nullptr;
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to