Avoid creating first-letter RenderTextFragments for unsuitable text nodes

Avoid creating first-letter RenderTextFragments for unsuitable text nodes

First-letter code would formerly find the first RenderText in a RenderBlock
and pass it off to be split into RenderTextFragments. If no suitable letter
was found in that RenderText, we'd use its entire contents (be it whitespace
or punctuation) in the RenderTextFragment. When the RenderText is whitespace
only and was combined with column handling code that splits blocks, this
could lead to creating first-letter renderers out of pre-existing first-
letter renderers, which could cause crashes.

Adding some assertions and changing the search algorithm for the first
text node to also check for a valid first-letter character. This avoids
the crashes and leads to results that are closer to my interpretation of
what the spec suggests (i.e. we won't apply first-letter to only punctuation).

BUG=264574
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159205

[eseidel, 2013-10-07 22:27:25]

https://codereview.chromium.org/26315006/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/26315006/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:5745: if (text.length() && length ==
text.length())
So when you just have <div>F<div> and a first-letter style, are we still going
to correctly style the F?

[leviw_travelin_and_unemployed, 2013-10-07 22:33:19]

https://codereview.chromium.org/26315006/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/26315006/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlock.cpp:5745: if (text.length() && length ==
text.length())
On 2013/10/07 22:27:25, eseidel wrote:
> So when you just have <div>F<div> and a first-letter style, are we still going
> to correctly style the F?

Yup :)

In that case length will simply be 0 here. We only increment the length pointer
when the characters return true for shouldSkipForFirstLetter.

[eseidel, 2013-10-07 23:23:53]

lgtm

[commit-bot: I haz the power, 2013-10-07 23:25:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/26315006/1

[commit-bot: I haz the power, 2013-10-07 23:25:22]

Failed to apply patch for Source/core/rendering/RenderBlock.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBlock.cpp
  Hunk #1 succeeded at 5740 (offset -1 lines).
  Hunk #2 succeeded at 5762 (offset -1 lines).
  Hunk #3 succeeded at 5785 (offset -1 lines).
  Hunk #4 succeeded at 5823 (offset 1 line).
  Hunk #5 FAILED at 5859.
  Hunk #6 succeeded at 5867 (offset -14 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderBlock.cpp.rej

Patch:       Source/core/rendering/RenderBlock.cpp
Index: Source/core/rendering/RenderBlock.cpp
diff --git a/Source/core/rendering/RenderBlock.cpp
b/Source/core/rendering/RenderBlock.cpp
index
fbd1d494c7b239d82916e9157fdf4fbe33c34109..68f9dd836b69c8c30bac6af85947faedd2760367
100644
--- a/Source/core/rendering/RenderBlock.cpp
+++ b/Source/core/rendering/RenderBlock.cpp
@@ -5741,6 +5741,10 @@ static inline unsigned firstLetterLength(const String&
text)
     while (length < text.length() && shouldSkipForFirstLetter((text)[length]))
         length++;
 
+    // Bail if we didn't find a letter
+    if (text.length() && length == text.length())
+        return 0;
+
     // Account for first letter.
     length++;
 
@@ -5759,8 +5763,11 @@ static inline unsigned firstLetterLength(const String&
text)
     return length;
 }
 
-void RenderBlock::createFirstLetterRenderer(RenderObject* firstLetterBlock,
RenderObject* currentChild)
+void RenderBlock::createFirstLetterRenderer(RenderObject* firstLetterBlock,
RenderObject* currentChild, unsigned length)
 {
+    ASSERT(length);
+    ASSERT(currentChild->isText() &&
!toRenderText(currentChild)->isTextFragment());
+
     RenderObject* firstLetterContainer = currentChild->parent();
     RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock,
firstLetterContainer);
     RenderObject* firstLetter = 0;
@@ -5779,11 +5786,6 @@ void RenderBlock::createFirstLetterRenderer(RenderObject*
firstLetterBlock, Rend
     String oldText = textObj->originalText();
     ASSERT(oldText.impl());
 
-    unsigned length = firstLetterLength(oldText);
-
-    if (!length)
-        return;
-
     // Construct a text fragment for the text after the first letter.
     // This text fragment might be empty.
     RenderTextFragment* remainingText =
@@ -5820,12 +5822,18 @@ void RenderBlock::updateFirstLetter()
 
     // Drill into inlines looking for our first text child.
     RenderObject* currChild = firstLetterBlock->firstChild();
+    unsigned length = 0;
     while (currChild) {
-        if (currChild->isText())
-            break;
-        if (currChild->isListMarker())
+        if (currChild->isText()) {
+            // FIXME: If there is leading punctuation in a different RenderText
than
+            // the first letter, we'll not apply the correct style to it.
+            length =
firstLetterLength(toRenderText(currChild)->originalText());
+            if (length)
+                break;
+            currChild = currChild->nextSibling();
+        } else if (currChild->isListMarker()) {
             currChild = currChild->nextSibling();
-        else if (currChild->isFloatingOrOutOfFlowPositioned()) {
+        } else if (currChild->isFloatingOrOutOfFlowPositioned()) {
             if (currChild->style()->styleType() == FIRST_LETTER) {
                 currChild = currChild->firstChild();
                 break;
@@ -5851,12 +5859,18 @@ void RenderBlock::updateFirstLetter()
             if (!remainingText->isText() || remainingText->isBR())
                 return;
 
+            RenderText* newFirstText = toRenderText(remainingText);
+            length = firstLetterLength(newFirstText->originalText());
+
+            if (!length)
+                return;
+
             LayoutStateDisabler layoutStateDisabler(view());
 
             if (RenderObject* oldRemainingText =
toRenderBoxModelObject(currChild->parent())->firstLetterRemainingText())
                
toRenderText(oldRemainingText)->setText(toText(oldRemainingText->node())->data().impl());
 
-            createFirstLetterRenderer(firstLetterBlock, remainingText);
+            createFirstLetterRenderer(firstLetterBlock, newFirstText, length);
             return;
         }
 
@@ -5873,7 +5887,7 @@ void RenderBlock::updateFirstLetter()
     // adding and removing children of firstLetterContainer.
     LayoutStateDisabler layoutStateDisabler(view());
 
-    createFirstLetterRenderer(firstLetterBlock, currChild);
+    createFirstLetterRenderer(firstLetterBlock, currChild, length);
 }
 
 // Helper methods for obtaining the last line, computing line counts and
heights for line counts

[commit-bot: I haz the power, 2013-10-08 00:01:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/26315006/11001

[commit-bot: I haz the power, 2013-10-08 04:08:17]

Failed to apply patch for
LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index:
LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
  |diff --git
a/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
b/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
  |index
3fb3b57a509b90a1374dd7e4399ed3b3357d2cba..7aa0b30d379e008b168385552bd7ecf8e2c854cf
100644
  |---
a/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
  |+++
b/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:      
LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
Index:
LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
diff --git
a/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
b/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
index
3fb3b57a509b90a1374dd7e4399ed3b3357d2cba..7aa0b30d379e008b168385552bd7ecf8e2c854cf
100644
--- a/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
+++ b/LayoutTests/platform/win/editing/selection/extend-by-word-002-expected.txt
@@ -16,59 +16,65 @@ layer at (0,0) size 800x600
         RenderBlock {UL} at (14,14) size 120x138
           RenderListItem {LI} at (0,0) size 120x21
             RenderListMarker at (-14,6) size 5x12: bullet
-            RenderInline (generated) at (0,0) size 14x25
-              RenderText {#text} at (5,-5) size 14x25
-                text run at (5,-5) width 14: "\x{B7} "
-            RenderText {#text} at (0,0) size 0x0
-            RenderInline {A} at (0,0) size 58x12 [color=#0000EE]
-              RenderText {#text} at (19,6) size 58x12
-                text run at (19,6) width 58: "Appetizers"
+            RenderText {#text} at (5,6) size 8x12
+              text run at (5,6) width 8: "\x{B7} "
+            RenderInline {A} at (0,0) size 65x12 [color=#0000EE]
+              RenderInline (generated) at (0,0) size 14x25
+                RenderText {#text} at (13,-5) size 14x25
+                  text run at (13,-5) width 14: "A"
+              RenderText {#text} at (27,6) size 51x12
+                text run at (27,6) width 51: "ppetizers"
           RenderListItem {LI} at (0,21) size 120x21
             RenderListMarker at (-14,6) size 5x12: bullet
-            RenderInline (generated) at (0,0) size 14x25
-              RenderText {#text} at (5,-5) size 14x25
-                text run at (5,-5) width 14: "\x{B7} "
-            RenderText {#text} at (0,0) size 0x0
-            RenderInline {A} at (0,0) size 86x12 [color=#0000EE]
-              RenderText {#text} at (19,6) size 86x12
-                text run at (19,6) width 86: "Soups & Salads"
+            RenderText {#text} at (5,6) size 8x12
+              text run at (5,6) width 8: "\x{B7} "
+            RenderInline {A} at (0,0) size 93x12 [color=#0000EE]
+              RenderInline (generated) at (0,0) size 14x25
+                RenderText {#text} at (13,-5) size 14x25
+                  text run at (13,-5) width 14: "S"
+              RenderText {#text} at (27,6) size 79x12
+                text run at (27,6) width 79: "oups & Salads"
           RenderListItem {LI} at (0,42) size 120x33
             RenderListMarker at (-14,6) size 5x12: bullet
-            RenderInline (generated) at (0,0) size 14x25
-              RenderText {#text} at (5,-5) size 14x25
-                text run at (5,-5) width 14: "\x{B7} "
-            RenderText {#text} at (0,0) size 0x0
-            RenderInline {A} at (0,0) size 81x24 [color=#0000EE]
-              RenderText {#text} at (19,6) size 81x24
-                text run at (19,6) width 74: "Sandwiches &"
-                text run at (93,6) width 4: " "
+            RenderText {#text} at (5,6) size 8x12
+              text run at (5,6) width 8: "\x{B7} "
+            RenderInline {A} at (0,0) size 85x24 [color=#0000EE]
+              RenderInline (generated) at (0,0) size 14x25
+                RenderText {#text} at (13,-5) size 14x25
+                  text run at (13,-5) width 14: "S"
+              RenderText {#text} at (27,6) size 82x24
+                text run at (27,6) width 67: "andwiches &"
+                text run at (94,6) width 4: " "
                 text run at (16,18) width 42: "Burgers"
           RenderListItem {LI} at (0,75) size 120x21
             RenderListMarker at (-14,6) size 5x12: bullet
-            RenderInline (generated) at (0,0) size 14x25
-              RenderText {#text} at (5,-5) size 14x25
-                text run at (5,-5) width 14: "\x{B7} "
-            RenderText {#text} at (0,0) size 0x0
-            RenderInline {A} at (0,0) size 71x12 [color=#0000EE]
-              RenderText {#text} at (19,6) size 71x12
-                text run at (19,6) width 71: "Steak & Ribs"
+            RenderText {#text} at (5,6) size 8x12
+              text run at (5,6) width 8: "\x{B7} "
+            RenderInline {A} at (0,0) size 78x12 [color=#0000EE]
+              RenderInline (generated) at (0,0) size 14x25
+                RenderText {#text} at (13,-5) size 14x25
+                  text run at (13,-5) width 14: "S"
+              RenderText {#text} at (27,6) size 64x12
+                text run at (27,6) width 64: "teak & Ribs"
           RenderListItem {LI} at (0,96) size 120x21
             RenderListMarker at (-14,6) size 5x12: bullet
-            RenderInline (generated) at (0,0) size 14x25
-              RenderText {#text} at (5,-5) size 14x25
-                text run at (5,-5) width 14: "\x{B7} "
-            RenderText {#text} at (0,0) size 0x0
-            RenderInline {A} at (0,0) size 46x12 [color=#0000EE]
-              RenderText {#text} at (19,6) size 46x12
-                text run at (19,6) width 46: "Seafood"
+            RenderText {#text} at (5,6) size 8x12
+              text run at (5,6) width 8: "\x{B7} "
+            RenderInline {A} at (0,0) size 53x12 [color=#0000EE]
+              RenderInline (generated) at (0,0) size 14x25
+                RenderText {#text} at (13,-5) size 14x25
+                  text run at (13,-5) width 14: "S"
+              RenderText {#text} at (27,6) size 39x12
+                text run at (27,6) width 39: "eafood"
           RenderListItem {LI} at (0,117) size 120x21
             RenderListMarker at (-14,6) size 5x12: bullet
-            RenderInline (generated) at (0,0) size 14x25
-              RenderText {#text} at (5,-5) size 14x25
-                text run at (5,-5) width 14: "\x{B7} "
-            RenderText {#text} at (0,0) size 0x0
-            RenderInline {A} at (0,0) size 46x12 [color=#0000EE]
-              RenderText {#text} at (19,6) size 46x12
-                text run at (19,6) width 46: "Combos"
-selection start: position 0 of child 0 {#text} of child 1 {A} of child 1 {LI}
of child 1 {UL} of child 1 {DIV} of body
-selection end:   position 5 of child 0 {#text} of child 1 {A} of child 7 {LI}
of child 1 {UL} of child 1 {DIV} of body
+            RenderText {#text} at (5,6) size 8x12
+              text run at (5,6) width 8: "\x{B7} "
+            RenderInline {A} at (0,0) size 52x12 [color=#0000EE]
+              RenderInline (generated) at (0,0) size 14x25
+                RenderText {#text} at (13,-5) size 14x25
+                  text run at (13,-5) width 14: "C"
+              RenderText {#text} at (27,6) size 38x12
+                text run at (27,6) width 38: "ombos"
+selection start: position 0 of child 0 {#text} of child 1 {LI} of child 1 {UL}
of child 1 {DIV} of body
+selection end:   position 4 of child 0 {#text} of child 1 {A} of child 7 {LI}
of child 1 {UL} of child 1 {DIV} of body

[commit-bot: I haz the power, 2013-10-08 19:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/26315006/27001

[commit-bot: I haz the power, 2013-10-08 23:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/26315006/41001

[commit-bot: I haz the power, 2013-10-09 05:47:21]

Change committed as 159205