Assert failure from capitalized RenderTextFragment

https://bugs.webkit.org/show_bug.cgi?id=82096

Patch by Ken Buchanan <kenrb@chromium.org> on 2012-03-26
Reviewed by Ryosuke Niwa.

Source/WebCore:

The cause of this bug was the call to RenderTextFragment::setTextInternal
resulting from a style change from RenderObject::addChild. The idea here
is to better separate the code path for transforming existing text from
the code path for replacing the underlying text of a node. For
RenderTextFragment this has to be clear because only in the latter case
does the first-letter get reset.

* rendering/RenderObject.cpp:
(WebCore::RenderObject::addChild): Added call to transformText
* rendering/RenderText.cpp:
(WebCore::RenderText::styleDidChange): Added call to transformText
(WebCore::RenderText::transformText): Added
* rendering/RenderText.h:
(WebCore::RenderText::setText): Changed to virtual so RenderTextFragment can override
(WebCore::RenderText::transformText): Added
* rendering/RenderTextFragment.cpp:
(WebCore::RenderTextFragment::styleDidChange): Removed references to
m_allowFragmentReset which was the previous approach to separating the
code paths
(WebCore::RenderTextFragment::setTextInternal): Deleted
(WebCore::RenderTextFragment::setText): Added with most of logic that was
previously in setTextInternal
(WebCore::RenderTextFragment::transformText): Added
* rendering/RenderTextFragment.h:
(WebCore::RenderTextFragment::setText): Added
(WebCore::RenderTextFragment::transformText): Added
(WebCore::RenderTextFragment::setTextInternal): Deleted

LayoutTests:

Test that exercises failure condition in bug 82096.

* fast/css/first-letter-capitalized-edit-select-crash-expected.txt: Added
* fast/css/first-letter-capitalized-edit-select-crash.html: Added

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@112200 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 247f0b4..ece37bb 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2012-03-26  Ken Buchanan  <kenrb@chromium.org>
+
+        Assert failure from capitalized RenderTextFragment
+        https://bugs.webkit.org/show_bug.cgi?id=82096
+
+        Reviewed by Ryosuke Niwa.
+
+        Test that exercises failure condition in bug 82096.
+
+        * fast/css/first-letter-capitalized-edit-select-crash-expected.txt: Added
+        * fast/css/first-letter-capitalized-edit-select-crash.html: Added
+
 2012-03-26  Shinya Kawanaka  <shinyak@chromium.org>
 
         Triggers assertion if dragging from outside of <meter> in a shadow tree to inside of it.

diff --git a/LayoutTests/fast/css/first-letter-capitalized-edit-select-crash-expected.txt b/LayoutTests/fast/css/first-letter-capitalized-edit-select-crash-expected.txt
new file mode 100644 (file)
index 0000000..7d25813
--- /dev/null
+++ b/LayoutTests/fast/css/first-letter-capitalized-edit-select-crash-expected.txt
@@ -0,0 +1,3 @@
+Pass if no crash or assert in debug
+
+

diff --git a/LayoutTests/fast/css/first-letter-capitalized-edit-select-crash.html b/LayoutTests/fast/css/first-letter-capitalized-edit-select-crash.html
new file mode 100644 (file)
index 0000000..4175bb6
--- /dev/null
+++ b/LayoutTests/fast/css/first-letter-capitalized-edit-select-crash.html
@@ -0,0 +1,20 @@
+<style>
+.writable{-webkit-user-modify:read-write;}
+.list{content:normal;display:list-item;}
+*:first-letter{stroke-linecap:round;}
+*:only-child{-webkit-font-variant-ligatures:common-ligatures;text-transform:capitalize;</style>
+</style>
+Pass if no crash or assert in debug
+<span class="writable" id="span">
+<label class="list">
+<code>
+<object></object>
+</code></label></span>
+<script>
+window.onload=function()
+{
+    document.getElementById('span').focus();
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+};
+</script>

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index fda8eaa..ceab0c9 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,38 @@
+2012-03-26  Ken Buchanan  <kenrb@chromium.org>
+
+        Assert failure from capitalized RenderTextFragment
+        https://bugs.webkit.org/show_bug.cgi?id=82096
+
+        Reviewed by Ryosuke Niwa.
+
+        The cause of this bug was the call to RenderTextFragment::setTextInternal
+        resulting from a style change from RenderObject::addChild. The idea here
+        is to better separate the code path for transforming existing text from
+        the code path for replacing the underlying text of a node. For
+        RenderTextFragment this has to be clear because only in the latter case
+        does the first-letter get reset.
+
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::addChild): Added call to transformText
+        * rendering/RenderText.cpp:
+        (WebCore::RenderText::styleDidChange): Added call to transformText
+        (WebCore::RenderText::transformText): Added
+        * rendering/RenderText.h:
+        (WebCore::RenderText::setText): Changed to virtual so RenderTextFragment can override
+        (WebCore::RenderText::transformText): Added
+        * rendering/RenderTextFragment.cpp:
+        (WebCore::RenderTextFragment::styleDidChange): Removed references to
+        m_allowFragmentReset which was the previous approach to separating the
+        code paths
+        (WebCore::RenderTextFragment::setTextInternal): Deleted
+        (WebCore::RenderTextFragment::setText): Added with most of logic that was
+        previously in setTextInternal
+        (WebCore::RenderTextFragment::transformText): Added
+        * rendering/RenderTextFragment.h:
+        (WebCore::RenderTextFragment::setText): Added
+        (WebCore::RenderTextFragment::transformText): Added
+        (WebCore::RenderTextFragment::setTextInternal): Deleted
+
 2012-03-26  Adam Klein  <adamk@chromium.org>
 
         Always set V8 wrappers via V8DOMWrapper::setJSWrapperFor* instead of WeakReferenceMap::set()

diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
index b3239ad..e4c5a95 100755 (executable)
--- a/Source/WebCore/rendering/RenderObject.cpp
+++ b/Source/WebCore/rendering/RenderObject.cpp
@@ -332,11 +332,8 @@ void RenderObject::addChild(RenderObject* newChild, RenderObject* beforeChild)
         children->insertChildNode(this, newChild, beforeChild);
     }
 
-    if (newChild->isText() && newChild->style()->textTransform() == CAPITALIZE) {
-        RefPtr<StringImpl> textToTransform = toRenderText(newChild)->originalText();
-        if (textToTransform)
-            toRenderText(newChild)->setText(textToTransform.release(), true);
-    }
+    if (newChild->isText() && newChild->style()->textTransform() == CAPITALIZE)
+        toRenderText(newChild)->transformText();
 
     // SVG creates renderers for <g display="none">, as SVG requires children of hidden
     // <g>s to have renderers - at least that's how our implementation works. Consider:

diff --git a/Source/WebCore/rendering/RenderText.cpp b/Source/WebCore/rendering/RenderText.cpp
index 00e9960..82d543c 100644 (file)
--- a/Source/WebCore/rendering/RenderText.cpp
+++ b/Source/WebCore/rendering/RenderText.cpp
@@ -201,10 +201,8 @@ void RenderText::styleDidChange(StyleDifference diff, const RenderStyle* oldStyl
 
     ETextTransform oldTransform = oldStyle ? oldStyle->textTransform() : TTNONE;
     ETextSecurity oldSecurity = oldStyle ? oldStyle->textSecurity() : TSNONE;
-    if (needsResetText || oldTransform != newStyle->textTransform() || oldSecurity != newStyle->textSecurity()) {
-        if (RefPtr<StringImpl> textToTransform = originalText())
-            setText(textToTransform.release(), true);
-    }
+    if (needsResetText || oldTransform != newStyle->textTransform() || oldSecurity != newStyle->textSecurity()) 
+        transformText();
 }
 
 void RenderText::removeAndDestroyTextBoxes()
@@ -1238,6 +1236,12 @@ void RenderText::setTextWithOffset(PassRefPtr<StringImpl> text, unsigned offset,
     setText(text, force);
 }
 
+void RenderText::transformText()
+{
+    if (RefPtr<StringImpl> textToTransform = originalText())
+        setText(textToTransform.release(), true);
+}
+
 static inline bool isInlineFlowOrEmptyText(const RenderObject* o)
 {
     if (o->isRenderInline())

diff --git a/Source/WebCore/rendering/RenderText.h b/Source/WebCore/rendering/RenderText.h
index c444dce..5705eb9 100644 (file)
--- a/Source/WebCore/rendering/RenderText.h
+++ b/Source/WebCore/rendering/RenderText.h
@@ -89,9 +89,11 @@ public:
     float firstRunX() const;
     float firstRunY() const;
 
-    void setText(PassRefPtr<StringImpl>, bool force = false);
+    virtual void setText(PassRefPtr<StringImpl>, bool force = false);
     void setTextWithOffset(PassRefPtr<StringImpl>, unsigned offset, unsigned len, bool force = false);
 
+    virtual void transformText();
+
     virtual bool canBeSelectionLeaf() const { return true; }
     virtual void setSelectionState(SelectionState s);
     virtual LayoutRect selectionRectForRepaint(RenderBoxModelObject* repaintContainer, bool clipToVisibleContent = true);
@@ -160,7 +162,6 @@ private:
     bool isAllASCII() const { return m_isAllASCII; }
     void updateNeedsTranscoding();
 
-    inline void transformText(String&) const;
     void secureText(UChar mask);
 
     float m_minWidth; // here to minimize padding in 64-bit.

diff --git a/Source/WebCore/rendering/RenderTextFragment.cpp b/Source/WebCore/rendering/RenderTextFragment.cpp
index 1f267cd..9f497d3 100644 (file)
--- a/Source/WebCore/rendering/RenderTextFragment.cpp
+++ b/Source/WebCore/rendering/RenderTextFragment.cpp
@@ -33,7 +33,6 @@ RenderTextFragment::RenderTextFragment(Node* node, StringImpl* str, int startOff
     , m_start(startOffset)
     , m_end(length)
     , m_firstLetter(0)
-    , m_allowFragmentReset(true)
 {
 }
 
@@ -43,7 +42,6 @@ RenderTextFragment::RenderTextFragment(Node* node, StringImpl* str)
     , m_end(str ? str->length() : 0)
     , m_contentString(str)
     , m_firstLetter(0)
-    , m_allowFragmentReset(true)
 {
 }
 
@@ -62,9 +60,7 @@ PassRefPtr<StringImpl> RenderTextFragment::originalText() const
 
 void RenderTextFragment::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
-    m_allowFragmentReset = false;
     RenderText::styleDidChange(diff, oldStyle);
-    m_allowFragmentReset = true;
 
     if (RenderBlock* block = blockForAccompanyingFirstLetter()) {
         block->style()->removeCachedPseudoStyle(FIRST_LETTER);
@@ -79,25 +75,30 @@ void RenderTextFragment::willBeDestroyed()
     RenderText::willBeDestroyed();
 }
 
-void RenderTextFragment::setTextInternal(PassRefPtr<StringImpl> text)
+void RenderTextFragment::setText(PassRefPtr<StringImpl> text, bool force)
 {
-    RenderText::setTextInternal(text);
-
-    if (m_allowFragmentReset) {
-        m_start = 0;
-        m_end = textLength();
-        if (m_firstLetter) {
-            ASSERT(!m_contentString);
-            m_firstLetter->destroy();
-            m_firstLetter = 0;
-            if (Node* t = node()) {
-                ASSERT(!t->renderer());
-                t->setRenderer(this);
-            }
+    RenderText::setText(text, force);
+
+    m_start = 0;
+    m_end = textLength();
+    if (m_firstLetter) {
+        ASSERT(!m_contentString);
+        m_firstLetter->destroy();
+        m_firstLetter = 0;
+        if (Node* t = node()) {
+            ASSERT(!t->renderer());
+            t->setRenderer(this);
         }
     }
 }
 
+void RenderTextFragment::transformText()
+{
+    // Don't reset first-letter here because we are only transforming the truncated fragment.
+    if (RefPtr<StringImpl> textToTransform = originalText())
+        RenderText::setText(textToTransform.release(), true);
+}
+
 UChar RenderTextFragment::previousCharacter() const
 {
     if (start()) {

diff --git a/Source/WebCore/rendering/RenderTextFragment.h b/Source/WebCore/rendering/RenderTextFragment.h
index 5ba15bc..b7f3b0e 100644 (file)
--- a/Source/WebCore/rendering/RenderTextFragment.h
+++ b/Source/WebCore/rendering/RenderTextFragment.h
@@ -50,13 +50,16 @@ public:
     StringImpl* contentString() const { return m_contentString.get(); }
     virtual PassRefPtr<StringImpl> originalText() const;
 
+    virtual void setText(PassRefPtr<StringImpl>, bool force = false) OVERRIDE;
+
+    virtual void transformText() OVERRIDE;
+
 protected:
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
 
 private:
     virtual void willBeDestroyed();
 
-    virtual void setTextInternal(PassRefPtr<StringImpl>);
     virtual UChar previousCharacter() const;
     RenderBlock* blockForAccompanyingFirstLetter() const;
 
@@ -64,7 +67,6 @@ private:
     unsigned m_end;
     RefPtr<StringImpl> m_contentString;
     RenderObject* m_firstLetter;
-    bool m_allowFragmentReset;
 };
 
 inline RenderTextFragment* toRenderTextFragment(RenderObject* object)