Changeset 283311 in webkit

Timestamp:

Sep 30, 2021, 7:53:33 AM (5 years ago)

Author:

Antti Koivisto

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/text-iterator/text-iterator-document-mutation-expected.txt (added)
• LayoutTests/editing/text-iterator/text-iterator-document-mutation.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/layout/integration/LayoutIntegrationInlineContent.cpp (modified) (1 diff)
• Source/WebCore/layout/integration/LayoutIntegrationInlineContent.h (modified) (3 diffs)
• Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp (modified) (3 diffs)
• Source/WebCore/layout/integration/LayoutIntegrationLineLayout.h (modified) (1 diff)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (3 diffs)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-09-30  Antti Koivisto  <antti@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  <lmoura@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-09-30  Antti Koivisto  <antti@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  <eocanha@igalia.com>
 

trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.cpp

@@ -144 +144 @@
 }
 
+void InlineContent::clearAndDetach()
+{
+    releaseCaches();
+    boxes.clear();
+    lines.clear();
+    m_lineLayout = nullptr;
+}
+
 void InlineContent::releaseCaches()
 {

trunk/Source/WebCore/layout/integration/LayoutIntegrationInlineContent.h

@@ -69 +69 @@
     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;
@@ -81 +81 @@
     const Vector<size_t>& nonRootInlineBoxIndexesForLayoutBox(const Layout::Box&) const;
 
+    void clearAndDetach();
     void releaseCaches();
 
@@ -86 +87 @@
     InlineContent(const LineLayout&);
 
-    CheckedRef<const LineLayout> m_lineLayout;
+    CheckedPtr<const LineLayout> m_lineLayout;
 
     using FirstBoxIndexCache = HashMap<CheckedRef<const Layout::Box>, size_t>;

trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp

@@ -68 +68 @@
 }
 
-LineLayout::~LineLayout() = default;
+LineLayout::~LineLayout()
+{
+    clearInlineContent();
+}
 
 RenderBlockFlow* LineLayout::blockContainer(RenderObject& renderer)
@@ -219 +222 @@
 
     // 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() };
@@ -573 +577 @@
 }
 
+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)
 {

trunk/Source/WebCore/layout/integration/LayoutIntegrationLineLayout.h

@@ -130 +130 @@
     const Layout::ContainerBox& rootLayoutBox() const;
     Layout::ContainerBox& rootLayoutBox();
+    void clearInlineContent();
     void releaseCaches();
 

trunk/Source/WebCore/testing/Internals.cpp

@@ -6538 +6538 @@
 #endif
 
+void Internals::retainTextIteratorForDocumentContent()
+{
+    auto* document = contextDocument();
+    if (!document)
+        return;
+
+    auto range = makeRangeSelectingNodeContents(*document);
+    m_textIterator = makeUnique<TextIterator>(range);
+}
+
+
 } // namespace WebCore

trunk/Source/WebCore/testing/Internals.h

@@ -103 +103 @@
 class StringCallback;
 class StyleSheet;
+class TextIterator;
 class TextTrack;
 class TimeRanges;
@@ -1191 +1192 @@
     ExceptionOr<void> setDocumentAutoplayPolicy(Document&, AutoplayPolicy);
 
+    void retainTextIteratorForDocumentContent();
+
 private:
     explicit Internals(Document&);
@@ -1220 +1223 @@
     HashMap<unsigned, std::unique_ptr<WebCore::SleepDisabler>> m_sleepDisablers;
 
+    std::unique_ptr<TextIterator> m_textIterator;
+
 #if ENABLE(WEBXR)
     RefPtr<WebXRTest> m_xrTest;

trunk/Source/WebCore/testing/Internals.idl

@@ -1064 +1064 @@
 
     undefined setDocumentAutoplayPolicy(Document document, AutoplayPolicy policy);
-};
+
+    undefined retainTextIteratorForDocumentContent();
+};