Do not update selection rect on dirty lineboxes.

https://bugs.webkit.org/show_bug.cgi?id=163862 <rdar://problem/28813156> Reviewed by Simon Fraser. Source/WebCore: In certain cases RenderBlock::updateFirstLetter() triggers unwanted render tree mutation while the caller assumes intact renderers. This patch ensures that no renderers gets destroyed while computing the preferred widths when we are outside of layout context. Test: fast/css-generated-content/dynamic-first-letter-selection-clear-crash.html * rendering/RenderBlock.cpp: (WebCore::RenderBlock::computePreferredLogicalWidths): (WebCore::RenderBlock::updateFirstLetter): * rendering/RenderBlock.h: * rendering/RenderListItem.cpp: (WebCore::RenderListItem::insertOrMoveMarkerRendererIfNeeded): * rendering/RenderRubyRun.cpp: (WebCore::RenderRubyRun::updateFirstLetter): * rendering/RenderRubyRun.h: * rendering/RenderTable.cpp: (WebCore::RenderTable::updateFirstLetter): * rendering/RenderTable.h: * rendering/svg/RenderSVGText.cpp: (WebCore::RenderSVGText::updateFirstLetter): * rendering/svg/RenderSVGText.h: LayoutTests: * fast/css-generated-content/dynamic-first-letter-selection-clear-crash-expected.txt: Added. * fast/css-generated-content/dynamic-first-letter-selection-clear-crash.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@207804 268f45cc-cd09-0410-ab3c-d52691b4dbfc
paul99 · Oct 25, 2016 · 6fe0acd · 6fe0acd
1 parent 102bf8e
commit 6fe0acd
Show file tree

Hide file tree

Showing 13 changed files with 91 additions and 10 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2016-10-24  Zalan Bujtas  <[email protected]>
+
+        Do not update selection rect on dirty lineboxes.
+        https://bugs.webkit.org/show_bug.cgi?id=163862
+        <rdar://problem/28813156>
+
+        Reviewed by Simon Fraser.
+
+        * fast/css-generated-content/dynamic-first-letter-selection-clear-crash-expected.txt: Added.
+        * fast/css-generated-content/dynamic-first-letter-selection-clear-crash.html: Added.
+
 2016-10-24  Ryan Haddad  <[email protected]>
 
         Unreviewed, rolling out r207795.

diff --git a/...tTests/fast/css-generated-content/dynamic-first-letter-selection-clear-crash-expected.txt b/...tTests/fast/css-generated-content/dynamic-first-letter-selection-clear-crash-expected.txt
@@ -0,0 +1,2 @@
+Pass if
+no crash.
diff --git a/LayoutTests/fast/css-generated-content/dynamic-first-letter-selection-clear-crash.html b/LayoutTests/fast/css-generated-content/dynamic-first-letter-selection-clear-crash.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>This tests that we can clear selection properly on dynamic content with first letter.</title>
+<style>
+.floatClass::first-letter {
+  float: right;
+}
+
+#innerBody {
+ column-count: 2;
+}
+
+</style>
+</head>
+<body>
+<li id=li style="float: right;">Pass if<span style="float: right;"></span></li><body id=innerBody style="height: 100px"><span id=span style="display: none;"></span>no crash.</body>
+<script>
+  if (window.testRunner)
+    testRunner.dumpAsText();
+  innerBody.style.webkitWritingMode = "vertical-rl";
+  innerBody.className = "floatClass";
+
+  document.getSelection().setBaseAndExtent(span, 0, innerBody, innerBody.childNodes.length);
+  window.getSelection().modify("extend", "left", "documentboundary");
+  innerBody.scrollIntoViewIfNeeded(true);
+  li.style.cssText = "float: right; height: 40px; width: 40px;";
+  document.body.offsetHeight;
+  li.style.cssText = "";
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2016-10-24  Zalan Bujtas  <[email protected]>
+
+        Do not update selection rect on dirty lineboxes.
+        https://bugs.webkit.org/show_bug.cgi?id=163862
+        <rdar://problem/28813156>
+
+        Reviewed by Simon Fraser.
+
+        In certain cases RenderBlock::updateFirstLetter() triggers
+        unwanted render tree mutation while the caller assumes intact renderers.
+        This patch ensures that no renderers gets destroyed while computing the preferred widths
+        when we are outside of layout context.
+
+        Test: fast/css-generated-content/dynamic-first-letter-selection-clear-crash.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::computePreferredLogicalWidths):
+        (WebCore::RenderBlock::updateFirstLetter):
+        * rendering/RenderBlock.h:
+        * rendering/RenderListItem.cpp:
+        (WebCore::RenderListItem::insertOrMoveMarkerRendererIfNeeded):
+        * rendering/RenderRubyRun.cpp:
+        (WebCore::RenderRubyRun::updateFirstLetter):
+        * rendering/RenderRubyRun.h:
+        * rendering/RenderTable.cpp:
+        (WebCore::RenderTable::updateFirstLetter):
+        * rendering/RenderTable.h:
+        * rendering/svg/RenderSVGText.cpp:
+        (WebCore::RenderSVGText::updateFirstLetter):
+        * rendering/svg/RenderSVGText.h:
+
 2016-10-24  Ryan Haddad  <[email protected]>
 
         Unreviewed, rolling out r207795.

diff --git a/Source/WebCore/rendering/RenderBlock.cpp b/Source/WebCore/rendering/RenderBlock.cpp
@@ -2755,7 +2755,9 @@ void RenderBlock::computePreferredLogicalWidths()
 {
     ASSERT(preferredLogicalWidthsDirty());
 
-    updateFirstLetter();
+    // FIXME: Do not even try to reshuffle first letter renderers when we are not in layout
+    // until after webkit.org/b/163848 is fixed.
+    updateFirstLetter(view().frameView().isInRenderTreeLayout() ? RenderTreeMutationIsAllowed::Yes : RenderTreeMutationIsAllowed::No);
 
     m_minPreferredLogicalWidth = 0;
     m_maxPreferredLogicalWidth = 0;
@@ -3305,7 +3307,7 @@ void RenderBlock::getFirstLetter(RenderObject*& firstLetter, RenderElement*& fir
         firstLetterContainer = nullptr;
 }
 
-void RenderBlock::updateFirstLetter()
+void RenderBlock::updateFirstLetter(RenderTreeMutationIsAllowed mutationAllowedOrNot)
 {
     RenderObject* firstLetterObj;
     RenderElement* firstLetterContainer;
@@ -3326,6 +3328,8 @@ void RenderBlock::updateFirstLetter()
     if (!is<RenderText>(*firstLetterObj))
         return;
 
+    if (mutationAllowedOrNot != RenderTreeMutationIsAllowed::Yes)
+        return;
     // Our layout state is not valid for the repaints we are going to trigger by
     // adding and removing children of firstLetterContainer.
     LayoutStateDisabler layoutStateDisabler(view());

diff --git a/Source/WebCore/rendering/RenderBlock.h b/Source/WebCore/rendering/RenderBlock.h
@@ -245,7 +245,8 @@ class RenderBlock : public RenderBox {
     LayoutUnit collapsedMarginBeforeForChild(const RenderBox& child) const;
     LayoutUnit collapsedMarginAfterForChild(const RenderBox& child) const;
 
-    virtual void updateFirstLetter();
+    enum class RenderTreeMutationIsAllowed { Yes, No };
+    virtual void updateFirstLetter(RenderTreeMutationIsAllowed = RenderTreeMutationIsAllowed::Yes);
     void getFirstLetter(RenderObject*& firstLetter, RenderElement*& firstLetterContainer, RenderObject* skipObject = nullptr);
 
     virtual void scrollbarsChanged(bool /*horizontalScrollbarChanged*/, bool /*verticalScrollbarChanged*/) { }

diff --git a/Source/WebCore/rendering/RenderListItem.cpp b/Source/WebCore/rendering/RenderListItem.cpp
@@ -268,7 +268,7 @@ void RenderListItem::insertOrMoveMarkerRendererIfNeeded()
     if (!m_marker)
         return;
 
-    // FIXME: Do not even try reposition the marker when we are not in layout
+    // FIXME: Do not even try to reposition the marker when we are not in layout
     // until after we fixed webkit.org/b/163789.
     if (!view().frameView().isInRenderTreeLayout())
         return;

diff --git a/Source/WebCore/rendering/RenderRubyRun.cpp b/Source/WebCore/rendering/RenderRubyRun.cpp
@@ -101,7 +101,7 @@ RenderBlock* RenderRubyRun::firstLineBlock() const
     return 0;
 }
 
-void RenderRubyRun::updateFirstLetter()
+void RenderRubyRun::updateFirstLetter(RenderTreeMutationIsAllowed)
 {
 }
 

diff --git a/Source/WebCore/rendering/RenderRubyRun.h b/Source/WebCore/rendering/RenderRubyRun.h
@@ -61,7 +61,7 @@ class RenderRubyRun final : public RenderBlockFlow {
     void removeChild(RenderObject&) override;
 
     RenderBlock* firstLineBlock() const override;
-    void updateFirstLetter() override;
+    void updateFirstLetter(RenderTreeMutationIsAllowed = RenderTreeMutationIsAllowed::Yes) override;
 
     void getOverhang(bool firstLine, RenderObject* startRenderer, RenderObject* endRenderer, float& startOverhang, float& endOverhang) const;
 

diff --git a/Source/WebCore/rendering/RenderTable.cpp b/Source/WebCore/rendering/RenderTable.cpp
@@ -1458,7 +1458,7 @@ RenderBlock* RenderTable::firstLineBlock() const
     return nullptr;
 }
 
-void RenderTable::updateFirstLetter()
+void RenderTable::updateFirstLetter(RenderTreeMutationIsAllowed)
 {
 }
 

diff --git a/Source/WebCore/rendering/RenderTable.h b/Source/WebCore/rendering/RenderTable.h
@@ -303,7 +303,7 @@ class RenderTable : public RenderBlock {
     void invalidateCachedColumnOffsets();
 
     RenderBlock* firstLineBlock() const final;
-    void updateFirstLetter() final;
+    void updateFirstLetter(RenderTreeMutationIsAllowed = RenderTreeMutationIsAllowed::Yes) final;
 
     void updateLogicalWidth() final;
 

diff --git a/Source/WebCore/rendering/svg/RenderSVGText.cpp b/Source/WebCore/rendering/svg/RenderSVGText.cpp
@@ -545,7 +545,7 @@ RenderBlock* RenderSVGText::firstLineBlock() const
 
 // Fix for <rdar://problem/8048875>. We should not render :first-letter CSS Style
 // in a SVG text element context.
-void RenderSVGText::updateFirstLetter()
+void RenderSVGText::updateFirstLetter(RenderTreeMutationIsAllowed)
 {
 }
 

diff --git a/Source/WebCore/rendering/svg/RenderSVGText.h b/Source/WebCore/rendering/svg/RenderSVGText.h
@@ -92,7 +92,7 @@ class RenderSVGText final : public RenderSVGBlock {
     std::unique_ptr<RootInlineBox> createRootInlineBox() override;
 
     RenderBlock* firstLineBlock() const override;
-    void updateFirstLetter() override;
+    void updateFirstLetter(RenderTreeMutationIsAllowed = RenderTreeMutationIsAllowed::Yes) override;
 
     bool shouldHandleSubtreeMutations() const;