Changeset 179691 in webkit

Timestamp:

Feb 5, 2015, 8:50:44 AM (12 years ago)

Author:

Alan Bujtas

Message:

Do not destroy RenderQuote's text fragment child when quotation mark string is changing.
​https://bugs.webkit.org/show_bug.cgi?id=141271
rdar://problem/18169375

Reviewed by Antti Koivisto.

Similar approach as ​https://codereview.chromium.org/679593004/

This patch ensures that laying out a RenderQuote does not force a sibling RenderQuote's
child renderer(RenderText) to be destroyed.
BreakingContext holds a pointer to the next renderer on the line (BreakingContext::m_nextObject).
While laying out the line, initiated by BreakingContext, placing the current renderer could end up destroying the "next" renderer.
This happens when the pseudo after quotation mark(RenderQuote) becomes floated, the sibling <q>'s pseudo
before text needs to be changed (from " to ') so that we don't end up with 2 sets of the same opening
strings.
The fix is to reuse the RenderTextFragment object instead of destroy/recreate it.

Source/WebCore:

Test: fast/css/content/quote-crash-when-floating.html

• rendering/RenderQuote.cpp:

(WebCore::RenderQuote::RenderQuote):
(WebCore::fragmentChild):
(WebCore::RenderQuote::updateText):

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

(WebCore::RenderTextFragment::setText):
(WebCore::RenderTextFragment::setContentString):

• rendering/RenderTextFragment.h:

LayoutTests:

• fast/css/content/quote-crash-when-floating-expected.txt: Added.
• fast/css/content/quote-crash-when-floating.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/css/content/quote-crash-when-floating-expected.txt (added)
• LayoutTests/fast/css/content/quote-crash-when-floating.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderQuote.cpp (modified) (3 diffs)
• Source/WebCore/rendering/RenderQuote.h (modified) (1 diff)
• Source/WebCore/rendering/RenderTextFragment.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderTextFragment.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2015-02-05  Zalan Bujtas  <zalan@apple.com>
+
+        Do not destroy RenderQuote's text fragment child when quotation mark string is changing.
+        https://bugs.webkit.org/show_bug.cgi?id=141271
+        rdar://problem/18169375
+
+        Reviewed by Antti Koivisto.
+
+        Similar approach as https://codereview.chromium.org/679593004/
+
+        This patch ensures that laying out a RenderQuote does not force a sibling RenderQuote's
+        child renderer(RenderText) to be destroyed.
+        BreakingContext holds a pointer to the next renderer on the line (BreakingContext::m_nextObject).
+        While laying out the line, initiated by BreakingContext, placing the current renderer could end up destroying the "next" renderer.
+        This happens when the pseudo after quotation mark(RenderQuote) becomes floated, the sibling <q>'s pseudo
+        before text needs to be changed (from " to ') so that we don't end up with 2 sets of the same opening
+        strings.
+        The fix is to reuse the RenderTextFragment object instead of destroy/recreate it.
+
+        * fast/css/content/quote-crash-when-floating-expected.txt: Added.
+        * fast/css/content/quote-crash-when-floating.html: Added.
+
 2015-02-05  Gyuyoung Kim  <gyuyoung.kim@samsung.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-02-05  Zalan Bujtas  <zalan@apple.com>
+
+        Do not destroy RenderQuote's text fragment child when quotation mark string is changing.
+        https://bugs.webkit.org/show_bug.cgi?id=141271
+        rdar://problem/18169375
+
+        Reviewed by Antti Koivisto.
+
+        Similar approach as https://codereview.chromium.org/679593004/
+
+        This patch ensures that laying out a RenderQuote does not force a sibling RenderQuote's
+        child renderer(RenderText) to be destroyed.
+        BreakingContext holds a pointer to the next renderer on the line (BreakingContext::m_nextObject).
+        While laying out the line, initiated by BreakingContext, placing the current renderer could end up destroying the "next" renderer.
+        This happens when the pseudo after quotation mark(RenderQuote) becomes floated, the sibling <q>'s pseudo
+        before text needs to be changed (from " to ') so that we don't end up with 2 sets of the same opening
+        strings.
+        The fix is to reuse the RenderTextFragment object instead of destroy/recreate it.
+
+        Test: fast/css/content/quote-crash-when-floating.html
+
+        * rendering/RenderQuote.cpp:
+        (WebCore::RenderQuote::RenderQuote):
+        (WebCore::fragmentChild):
+        (WebCore::RenderQuote::updateText):
+        * rendering/RenderQuote.h:
+        * rendering/RenderTextFragment.cpp:
+        (WebCore::RenderTextFragment::setText):
+        (WebCore::RenderTextFragment::setContentString):
+        * rendering/RenderTextFragment.h:
+
 2015-02-04  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebCore/rendering/RenderQuote.cpp

@@ -35 +35 @@
     : RenderInline(document, WTF::move(style))
     , m_type(quote)
-    , m_depth(-1)
-    , m_next(0)
-    , m_previous(0)
-    , m_isAttached(false)
+    , m_text(emptyString())
 {
 }
@@ -334 +331 @@
 }
 
+static RenderTextFragment* fragmentChild(RenderObject* lastChild)
+{
+    if (!lastChild)
+        return nullptr;
+
+    if (!is<RenderTextFragment>(lastChild))
+        return nullptr;
+
+    return downcast<RenderTextFragment>(lastChild);
+}
+
 void RenderQuote::updateText()
 {
@@ -339 +347 @@
     if (m_text == text)
         return;
-
-    while (RenderObject* child = lastChild())
-        child->destroy();
-
-    if (text == emptyString() || text == String()) {
-        m_text = String();
+    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* fragment = fragmentChild(lastChild())) {
+        fragment->setContentString(m_text);
         return;
     }
-
-    m_text = text;
-
-    RenderTextFragment* fragment = new RenderTextFragment(document(), m_text.impl());
-    addChild(fragment);
+    addChild(new RenderTextFragment(document(), m_text));
 }
 

trunk/Source/WebCore/rendering/RenderQuote.h

@@ -47 +47 @@
     void updateDepth();
 
-    QuoteType m_type;
-    int m_depth;
-    RenderQuote* m_next;
-    RenderQuote* m_previous;
-    bool m_isAttached;
+    const QuoteType m_type;
+    int m_depth { -1 };
+    RenderQuote* m_next { nullptr };
+    RenderQuote* m_previous { nullptr };
+    bool m_isAttached { false };
     String m_text;
 };

trunk/Source/WebCore/rendering/RenderTextFragment.cpp

@@ -89 +89 @@
     if (!m_firstLetter)
         return;
-    ASSERT(!m_contentString);
     m_firstLetter->destroy();
     m_firstLetter = 0;
@@ -120 +119 @@
 }
 
+void RenderTextFragment::setContentString(const String& text)
+{
+    m_contentString = text;
+    setText(text);
+}
+
 } // namespace WebCore

trunk/Source/WebCore/rendering/RenderTextFragment.h

@@ -50 +50 @@
     RenderBlock* blockForAccompanyingFirstLetter();
 
+    void setContentString(const String& text);
     StringImpl* contentString() const { return m_contentString.impl(); }