Changeset 243331 in webkit

Timestamp:

Mar 21, 2019, 2:43:14 PM (7 years ago)

Author:

Alan Bujtas

Message:

Do not insert the first-letter anonymous container until after we've constructed the first-letter renderer.
​https://bugs.webkit.org/show_bug.cgi?id=195919
<rdar://problem/48573434>

Reviewed by Brent Fulgham.

Source/WebCore:

When the container is injected too early, we might end up removing it as part of the collapsing logic
while the text renderer is being removed (replaced with the first letter + remaining text).

Test: fast/css/first-letter-and-float-crash.html

• rendering/updating/RenderTreeBuilderFirstLetter.cpp:

(WebCore::RenderTreeBuilder::FirstLetter::createRenderers):

LayoutTests:

• fast/css/first-letter-and-float-crash-expected.txt: Added.
• fast/css/first-letter-and-float-crash.html: Added.
• platform/mac/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/fast/css/first-letter-and-float-crash-expected.txt (added)
• LayoutTests/fast/css/first-letter-and-float-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-03-21  Zalan Bujtas  <zalan@apple.com>
+
+        Do not insert the first-letter anonymous container until after we've constructed the first-letter renderer.
+        https://bugs.webkit.org/show_bug.cgi?id=195919
+        <rdar://problem/48573434>
+
+        Reviewed by Brent Fulgham.
+
+        * fast/css/first-letter-and-float-crash-expected.txt: Added.
+        * fast/css/first-letter-and-float-crash.html: Added.
+        * platform/mac/TestExpectations:
+
 2019-03-21  Eric Carlson  <eric.carlson@apple.com>
 

trunk/LayoutTests/TestExpectations

@@ -3060 +3060 @@
 imported/w3c/web-platform-tests/css/css-lists/list-marker-with-lineheight-and-overflow-hidden-001.html [ ImageOnlyFailure ]
 imported/w3c/web-platform-tests/css/css-lists/list-with-image-display-changed-001.html [ ImageOnlyFailure ]
+
+[ Debug ] fast/css/first-letter-and-float-crash.html [ Skip ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-03-21  Zalan Bujtas  <zalan@apple.com>
+
+        Do not insert the first-letter anonymous container until after we've constructed the first-letter renderer.
+        https://bugs.webkit.org/show_bug.cgi?id=195919
+        <rdar://problem/48573434>
+
+        Reviewed by Brent Fulgham.
+
+        When the container is injected too early, we might end up removing it as part of the collapsing logic
+        while the text renderer is being removed (replaced with the first letter + remaining text).
+
+        Test: fast/css/first-letter-and-float-crash.html
+
+        * rendering/updating/RenderTreeBuilderFirstLetter.cpp:
+        (WebCore::RenderTreeBuilder::FirstLetter::createRenderers):
+
 2019-03-21  Eric Carlson  <eric.carlson@apple.com>
 

trunk/Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp

@@ -218 +218 @@
     newFirstLetter->setIsFirstLetter();
 
-    auto& firstLetter = *newFirstLetter;
-    m_builder.attach(*firstLetterContainer, WTFMove(newFirstLetter), &currentTextChild);
-
     // The original string is going to be either a generated content string or a DOM node's
     // string. We want the original string before it got transformed in case first-letter has
@@ -254 +251 @@
         auto* textNode = currentTextChild.textNode();
         auto* beforeChild = currentTextChild.nextSibling();
+        auto inlineWrapperForDisplayContents = makeWeakPtr(currentTextChild.inlineWrapperForDisplayContents());
+        auto hasInlineWrapperForDisplayContents = inlineWrapperForDisplayContents.get();
         m_builder.destroy(currentTextChild);
 
@@ -266 +265 @@
 
         RenderTextFragment& remainingText = *newRemainingText;
+        ASSERT_UNUSED(hasInlineWrapperForDisplayContents, hasInlineWrapperForDisplayContents == inlineWrapperForDisplayContents.get());
+        remainingText.setInlineWrapperForDisplayContents(inlineWrapperForDisplayContents.get());
         m_builder.attach(*textContentParent, WTFMove(newRemainingText), beforeChild);
+
+        // FIXME: Make attach the final step so that we don't need to keep firstLetter around.
+        auto& firstLetter = *newFirstLetter;
         remainingText.setFirstLetter(firstLetter);
         firstLetter.setFirstLetterRemainingText(remainingText);
-
-        // construct text fragment for the first letter
+        m_builder.attach(*firstLetterContainer, WTFMove(newFirstLetter), &remainingText);
+
+        // Construct text fragment for the first letter.
         auto letter = createRenderer<RenderTextFragment>(firstLetterBlock.document(), oldText, 0, length);
-
         m_builder.attach(firstLetter, WTFMove(letter));
     }