Changeset 143060 in webkit

Timestamp:

Feb 15, 2013 3:11:47 PM (11 years ago)

Author:

esprehn@chromium.org

Message:

RenderQuote should not mark renderers as needing layout during layout
​https://bugs.webkit.org/show_bug.cgi?id=109876

Reviewed by Ojan Vafai.

Source/WebCore:

Marking RenderQuotes as needing pref width recalcs and layouts during a
layout is dangerous since an ancestor may mark itself as having completed
layout, but then some subtree still thinks it needs layout.

Instead, since the only time we create RenderQuote instances is inside
PseudoElement, we can call attachQuote inside PseudoElement::attach during
the regular tree mutating cycle. We can then use RenderQuote::styleDidChange
to update the kind of quotes on normal style changes.

This makes RenderQuote behave much more similarly to DOM nodes and means
we no longer need to set dirty bits during layout.

Test: fast/css-generated-content/quote-layout-focus-crash.html

• dom/PseudoElement.cpp:

(WebCore::PseudoElement::attach): Now call attachQuote().

• rendering/RenderQuote.cpp:

(WebCore::RenderQuote::~RenderQuote):
(WebCore::RenderQuote::willBeRemovedFromTree):
(WebCore::RenderQuote::styleDidChange):
(WebCore::RenderQuote::updateText):
(WebCore::RenderQuote::attachQuote):
(WebCore::RenderQuote::detachQuote):
(WebCore::RenderQuote::updateDepth):

• rendering/RenderQuote.h:

(RenderQuote):

LayoutTests:

• fast/block/float/float-not-removed-from-pre-block-expected.txt:
• fast/css-generated-content/quote-layout-focus-crash-expected.txt: Added.
• fast/css-generated-content/quote-layout-focus-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt (modified) (1 diff)
• LayoutTests/fast/css-generated-content/quote-layout-focus-crash-expected.txt (added)
• LayoutTests/fast/css-generated-content/quote-layout-focus-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/PseudoElement.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderQuote.cpp (modified) (4 diffs)
• Source/WebCore/rendering/RenderQuote.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-02-15  Elliott Sprehn  <esprehn@chromium.org>
+
+        RenderQuote should not mark renderers as needing layout during layout
+        https://bugs.webkit.org/show_bug.cgi?id=109876
+
+        Reviewed by Ojan Vafai.
+
+        * fast/block/float/float-not-removed-from-pre-block-expected.txt:
+        * fast/css-generated-content/quote-layout-focus-crash-expected.txt: Added.
+        * fast/css-generated-content/quote-layout-focus-crash.html: Added.
+
 2013-02-15  Rik Cabanier  <cabanier@adobe.com>
 

trunk/LayoutTests/fast/block/float/float-not-removed-from-pre-block-expected.txt

@@ -1 +1 @@
 Bug 101970: Heap-use-after-free in WebCore::RenderLayerModelObject::hasSelfPaintingLayer
 Test passes if it does not crash.
-
+  

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-15  Elliott Sprehn  <esprehn@chromium.org>
+
+        RenderQuote should not mark renderers as needing layout during layout
+        https://bugs.webkit.org/show_bug.cgi?id=109876
+
+        Reviewed by Ojan Vafai.
+
+        Marking RenderQuotes as needing pref width recalcs and layouts during a
+        layout is dangerous since an ancestor may mark itself as having completed
+        layout, but then some subtree still thinks it needs layout.
+
+        Instead, since the only time we create RenderQuote instances is inside
+        PseudoElement, we can call attachQuote inside PseudoElement::attach during
+        the regular tree mutating cycle. We can then use RenderQuote::styleDidChange
+        to update the kind of quotes on normal style changes.
+
+        This makes RenderQuote behave much more similarly to DOM nodes and means
+        we no longer need to set dirty bits during layout.
+
+        Test: fast/css-generated-content/quote-layout-focus-crash.html
+
+        * dom/PseudoElement.cpp:
+        (WebCore::PseudoElement::attach): Now call attachQuote().
+        * rendering/RenderQuote.cpp:
+        (WebCore::RenderQuote::~RenderQuote):
+        (WebCore::RenderQuote::willBeRemovedFromTree):
+        (WebCore::RenderQuote::styleDidChange):
+        (WebCore::RenderQuote::updateText):
+        (WebCore::RenderQuote::attachQuote):
+        (WebCore::RenderQuote::detachQuote):
+        (WebCore::RenderQuote::updateDepth):
+        * rendering/RenderQuote.h:
+        (RenderQuote):
+
 2013-02-15  Sheriff Bot  <webkit.review.bot@gmail.com>
 

trunk/Source/WebCore/dom/PseudoElement.cpp

@@ -31 +31 @@
 #include "NodeRenderingContext.h"
 #include "RenderObject.h"
+#include "RenderQuote.h"
 
 namespace WebCore {
@@ -83 +84 @@
     for (const ContentData* content = style->contentData(); content; content = content->next()) {
         RenderObject* child = content->createRenderer(document(), style);
-        if (renderer->isChildAllowed(child, style))
+        if (renderer->isChildAllowed(child, style)) {
             renderer->addChild(child);
-        else
+            if (child->isQuote())
+                toRenderQuote(child)->attachQuote();
+        } else
             child->destroy();
     }

trunk/Source/WebCore/rendering/RenderQuote.cpp

@@ -55 +55 @@
 {
     RenderText::willBeRemovedFromTree();
-
     detachQuote();
+}
+
+void RenderQuote::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
+{
+    RenderText::styleDidChange(diff, oldStyle);
+    updateText();
 }
 
@@ -246 +251 @@
 void RenderQuote::updateText()
 {
-    computePreferredLogicalWidths(0);
-}
-
-void RenderQuote::computePreferredLogicalWidths(float lead)
-{
-#ifndef NDEBUG
-    // FIXME: We shouldn't be modifying the tree in computePreferredLogicalWidths.
-    // Instead, we should properly hook the appropriate changes in the DOM and modify
-    // the render tree then. When that's done, we also won't need to override
-    // computePreferredLogicalWidths at all.
-    // https://bugs.webkit.org/show_bug.cgi?id=104829
-    SetLayoutNeededForbiddenScope layoutForbiddenScope(this, false);
-#endif
-
-    if (!m_attached)
-        attachQuote();
-    setTextInternal(originalText());
-
-    RenderText::computePreferredLogicalWidths(lead);
+    setText(originalText());
 }
 
@@ -286 +273 @@
     ASSERT(!m_attached);
     ASSERT(!m_next && !m_previous);
-
-    // FIXME: Don't set pref widths dirty during layout. See updateDepth() for
-    // more detail.
-    if (!isRooted()) {
-        setNeedsLayoutAndPrefWidthsRecalc();
-        return;
-    }
+    ASSERT(isRooted());
 
     if (!view()->renderQuoteHead()) {
@@ -371 +352 @@
         }
     }
-    // FIXME: Don't call setNeedsLayout or dirty our preferred widths during layout.
-    // This is likely to fail anyway as one of our ancestor will call setNeedsLayout(false),
-    // preventing the future layout to occur on |this|. The solution is to move that to a
-    // pre-layout phase.
     if (oldDepth != m_depth)
-        setNeedsLayoutAndPrefWidthsRecalc();
+        updateText();
 }
 

trunk/Source/WebCore/rendering/RenderQuote.h

@@ -37 +37 @@
     virtual ~RenderQuote();
     void attachQuote();
+
+    virtual void updateText() OVERRIDE;
+
+private:
     void detachQuote();
 
-private:
     virtual void willBeDestroyed() OVERRIDE;
     virtual const char* renderName() const OVERRIDE { return "RenderQuote"; };
     virtual bool isQuote() const OVERRIDE { return true; };
     virtual PassRefPtr<StringImpl> originalText() const OVERRIDE;
-
-    virtual void updateText() OVERRIDE;
-    virtual void computePreferredLogicalWidths(float leadWidth) OVERRIDE;
+    virtual void styleDidChange(StyleDifference, const RenderStyle*) OVERRIDE;
 
     // We don't override insertedIntoTree to call attachQuote() as it would be attached