Title: [283311] trunk
Revision
283311
Author
an...@apple.com
Date
2021-09-30 07:53:33 -0700 (Thu, 30 Sep 2021)

Log Message

Source/WebCore:
Regression (283158): Having unused TextIterator instance shouldn't crash if document is mutated
https://bugs.webkit.org/show_bug.cgi?id=231013
rdar://83690985

Reviewed by Alan Bujtas.

By using bundle APIs it is possible to create a retained TextIterator and hit CheckedPtr assertion because layout
is torn down while iterator still exists. This is not dangerous in itself so can be supported.

This patch ensures LineLayout disconnects cleanly from (refcounted) InlineContent.

Test: editing/text-iterator/text-iterator-document-mutation.html

* layout/integration/LayoutIntegrationInlineContent.cpp:
(WebCore::LayoutIntegration::InlineContent::clearAndDetach):

Remove all content and clear the LineLayout pointer.

This is safe since iterators refer to content via indexes and any attempt to access
via them will hit nullptrs or vector asserts.

* layout/integration/LayoutIntegrationInlineContent.h:
(WebCore::LayoutIntegration::InlineContent::lineLayout const):
* layout/integration/LayoutIntegrationLineLayout.cpp:
(WebCore::LayoutIntegration::LineLayout::~LineLayout):
(WebCore::LayoutIntegration::LineLayout::layout):
(WebCore::LayoutIntegration::LineLayout::hitTest):
(WebCore::LayoutIntegration::LineLayout::clearInlineContent):

Clear InlineContent before nulling it as it can still be kept alive by iterators.

* layout/integration/LayoutIntegrationLineLayout.h:
* testing/Internals.cpp:
(WebCore::Internals::retainTextIteratorForDocumentContent):
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:
Having unused TextIterator instance shouldn't crash if document is mutated
https://bugs.webkit.org/show_bug.cgi?id=231013
rdar://83690985

Reviewed by Alan Bujtas.

* editing/text-iterator/text-iterator-document-mutation-expected.txt: Added.
* editing/text-iterator/text-iterator-document-mutation.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (283310 => 283311)


--- trunk/LayoutTests/ChangeLog	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/LayoutTests/ChangeLog	2021-09-30 14:53:33 UTC (rev 283311)
@@ -1,3 +1,14 @@
+2021-09-30  Antti Koivisto  <an...@apple.com>
+
+        Having unused TextIterator instance shouldn't crash if document is mutated
+        https://bugs.webkit.org/show_bug.cgi?id=231013
+        rdar://83690985
+
+        Reviewed by Alan Bujtas.
+
+        * editing/text-iterator/text-iterator-document-mutation-expected.txt: Added.
+        * editing/text-iterator/text-iterator-document-mutation.html: Added.
+
 2021-09-30  Lauro Moura  <lmo...@igalia.com>
 
         [WPE] Rebaseline a number of text-only text-related failures

Added: trunk/LayoutTests/editing/text-iterator/text-iterator-document-mutation-expected.txt (0 => 283311)


--- trunk/LayoutTests/editing/text-iterator/text-iterator-document-mutation-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/editing/text-iterator/text-iterator-document-mutation-expected.txt	2021-09-30 14:53:33 UTC (rev 283311)
@@ -0,0 +1 @@
+This test passes if it doesn't crash

Added: trunk/LayoutTests/editing/text-iterator/text-iterator-document-mutation.html (0 => 283311)


--- trunk/LayoutTests/editing/text-iterator/text-iterator-document-mutation.html	                        (rev 0)
+++ trunk/LayoutTests/editing/text-iterator/text-iterator-document-mutation.html	2021-09-30 14:53:33 UTC (rev 283311)
@@ -0,0 +1,10 @@
+<div id=div>text</div>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+div.offsetLeft;
+if (window.internals)
+    internals.retainTextIteratorForDocumentContent();
+div.textContent = "This test passes if it doesn't crash";
+</script>

Modified: trunk/Source/WebCore/ChangeLog (283310 => 283311)


--- trunk/Source/WebCore/ChangeLog	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/ChangeLog	2021-09-30 14:53:33 UTC (rev 283311)
@@ -1,3 +1,42 @@
+2021-09-30  Antti Koivisto  <an...@apple.com>
+
+        Regression (283158): Having unused TextIterator instance shouldn't crash if document is mutated
+        https://bugs.webkit.org/show_bug.cgi?id=231013
+        rdar://83690985
+
+        Reviewed by Alan Bujtas.
+
+        By using bundle APIs it is possible to create a retained TextIterator and hit CheckedPtr assertion because layout
+        is torn down while iterator still exists. This is not dangerous in itself so can be supported.
+
+        This patch ensures LineLayout disconnects cleanly from (refcounted) InlineContent.
+
+        Test: editing/text-iterator/text-iterator-document-mutation.html
+
+        * layout/integration/LayoutIntegrationInlineContent.cpp:
+        (WebCore::LayoutIntegration::InlineContent::clearAndDetach):
+
+        Remove all content and clear the LineLayout pointer.
+
+        This is safe since iterators refer to content via indexes and any attempt to access
+        via them will hit nullptrs or vector asserts.
+
+        * layout/integration/LayoutIntegrationInlineContent.h:
+        (WebCore::LayoutIntegration::InlineContent::lineLayout const):
+        * layout/integration/LayoutIntegrationLineLayout.cpp:
+        (WebCore::LayoutIntegration::LineLayout::~LineLayout):
+        (WebCore::LayoutIntegration::LineLayout::layout):
+        (WebCore::LayoutIntegration::LineLayout::hitTest):
+        (WebCore::LayoutIntegration::LineLayout::clearInlineContent):
+
+        Clear InlineContent before nulling it as it can still be kept alive by iterators.
+
+        * layout/integration/LayoutIntegrationLineLayout.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::retainTextIteratorForDocumentContent):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2021-09-30  Enrique Ocaña González  <eoca...@igalia.com>
 
         [MSE][GStreamer] Allow infinite duration on MSE

Modified: trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.cpp (283310 => 283311)


--- trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.cpp	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.cpp	2021-09-30 14:53:33 UTC (rev 283311)
@@ -143,6 +143,14 @@
     return it->value;
 }
 
+void InlineContent::clearAndDetach()
+{
+    releaseCaches();
+    boxes.clear();
+    lines.clear();
+    m_lineLayout = nullptr;
+}
+
 void InlineContent::releaseCaches()
 {
     m_firstBoxIndexCache = { };

Modified: trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.h (283310 => 283311)


--- trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.h	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.h	2021-09-30 14:53:33 UTC (rev 283311)
@@ -68,7 +68,7 @@
     WTF::IteratorRange<const InlineDisplay::Box*> boxesForRect(const LayoutRect&) const;
     void shrinkToFit();
 
-    const LineLayout& lineLayout() const { return m_lineLayout; }
+    const LineLayout& lineLayout() const { return *m_lineLayout; }
     const RenderObject& rendererForLayoutBox(const Layout::Box&) const;
     const RenderBlockFlow& containingBlock() const;
 
@@ -80,12 +80,13 @@
     std::optional<size_t> firstBoxIndexForLayoutBox(const Layout::Box&) const;
     const Vector<size_t>& nonRootInlineBoxIndexesForLayoutBox(const Layout::Box&) const;
 
+    void clearAndDetach();
     void releaseCaches();
 
 private:
     InlineContent(const LineLayout&);
 
-    CheckedRef<const LineLayout> m_lineLayout;
+    CheckedPtr<const LineLayout> m_lineLayout;
 
     using FirstBoxIndexCache = HashMap<CheckedRef<const Layout::Box>, size_t>;
     mutable std::unique_ptr<FirstBoxIndexCache> m_firstBoxIndexCache;

Modified: trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp (283310 => 283311)


--- trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp	2021-09-30 14:53:33 UTC (rev 283311)
@@ -67,7 +67,10 @@
     m_layoutState.setIsIntegratedRootBoxFirstChild(flow.parent()->firstChild() == &flow);
 }
 
-LineLayout::~LineLayout() = default;
+LineLayout::~LineLayout()
+{
+    clearInlineContent();
+}
 
 RenderBlockFlow* LineLayout::blockContainer(RenderObject& renderer)
 {
@@ -218,7 +221,8 @@
     prepareFloatingState();
 
     // FIXME: Do not clear the lines and boxes here unconditionally, but consult with the damage object instead.
-    m_inlineContent = nullptr;
+    clearInlineContent();
+
     auto& rootGeometry = m_layoutState.geometryForBox(rootLayoutBox);
     auto inlineFormattingContext = Layout::InlineFormattingContext { rootLayoutBox, m_inlineFormattingState, m_lineDamage.get() };
 
@@ -572,6 +576,14 @@
         m_inlineContent->releaseCaches();
 }
 
+void LineLayout::clearInlineContent()
+{
+    if (!m_inlineContent)
+        return;
+    m_inlineContent->clearAndDetach();
+    m_inlineContent = nullptr;
+}
+
 void LineLayout::paintTextBoxUsingPhysicalCoordinates(PaintInfo& paintInfo, const LayoutPoint& paintOffset, const InlineDisplay::Box& textBox)
 {
     auto& style = textBox.style();

Modified: trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.h (283310 => 283311)


--- trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.h	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.h	2021-09-30 14:53:33 UTC (rev 283311)
@@ -129,6 +129,7 @@
 
     const Layout::ContainerBox& rootLayoutBox() const;
     Layout::ContainerBox& rootLayoutBox();
+    void clearInlineContent();
     void releaseCaches();
 
     BoxTree m_boxTree;

Modified: trunk/Source/WebCore/testing/Internals.cpp (283310 => 283311)


--- trunk/Source/WebCore/testing/Internals.cpp	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/testing/Internals.cpp	2021-09-30 14:53:33 UTC (rev 283311)
@@ -6537,4 +6537,15 @@
 }
 #endif
 
+void Internals::retainTextIteratorForDocumentContent()
+{
+    auto* document = contextDocument();
+    if (!document)
+        return;
+
+    auto range = makeRangeSelectingNodeContents(*document);
+    m_textIterator = makeUnique<TextIterator>(range);
+}
+
+
 } // namespace WebCore

Modified: trunk/Source/WebCore/testing/Internals.h (283310 => 283311)


--- trunk/Source/WebCore/testing/Internals.h	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/testing/Internals.h	2021-09-30 14:53:33 UTC (rev 283311)
@@ -102,6 +102,7 @@
 class SourceBuffer;
 class StringCallback;
 class StyleSheet;
+class TextIterator;
 class TextTrack;
 class TimeRanges;
 class TypeConversions;
@@ -1190,6 +1191,8 @@
     };
     ExceptionOr<void> setDocumentAutoplayPolicy(Document&, AutoplayPolicy);
 
+    void retainTextIteratorForDocumentContent();
+
 private:
     explicit Internals(Document&);
     Document* contextDocument() const;
@@ -1219,6 +1222,8 @@
 
     HashMap<unsigned, std::unique_ptr<WebCore::SleepDisabler>> m_sleepDisablers;
 
+    std::unique_ptr<TextIterator> m_textIterator;
+
 #if ENABLE(WEBXR)
     RefPtr<WebXRTest> m_xrTest;
 #endif

Modified: trunk/Source/WebCore/testing/Internals.idl (283310 => 283311)


--- trunk/Source/WebCore/testing/Internals.idl	2021-09-30 13:41:34 UTC (rev 283310)
+++ trunk/Source/WebCore/testing/Internals.idl	2021-09-30 14:53:33 UTC (rev 283311)
@@ -1063,4 +1063,6 @@
     DOMString dumpStyleResolvers();
 
     undefined setDocumentAutoplayPolicy(Document document, AutoplayPolicy policy);
+
+    undefined retainTextIteratorForDocumentContent();
 };
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to