Reproducible ASSERTion failure when toggling layer borders with find-in-page up
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Dec 2018 19:42:55 +0000 (19:42 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Dec 2018 19:42:55 +0000 (19:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192762
<rdar://problem/46676873>

Reviewed by Simon Fraser.

Source/WebCore:

DocumentMarkerController::markersFor() should take a reference instead of a Node*.

Test: editing/document-marker-null-check.html

* dom/DocumentMarkerController.cpp:
(DocumentMarkerController::hasMarkers):
* dom/DocumentMarkerController.h:
* editing/AlternativeTextController.cpp:
(WebCore::AlternativeTextController::respondToChangedSelection):
* editing/Editor.cpp:
(WebCore::Editor::selectionStartHasMarkerFor const):
* rendering/InlineTextBox.cpp:
(WebCore::InlineTextBox::collectMarkedTextsForDocumentMarkers const):
* rendering/RenderReplaced.cpp:
(WebCore::RenderReplaced::paint):
* rendering/RenderText.cpp:
(WebCore::RenderText::draggedContentRangesBetweenOffsets const):
* rendering/SimpleLineLayout.cpp:
(WebCore::SimpleLineLayout::canUseForWithReason):
* testing/Internals.cpp:
(WebCore::Internals::markerCountForNode):

LayoutTests:

* editing/document-marker-null-check-expected.txt: Added.
* editing/document-marker-null-check.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239279 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/document-marker-null-check-expected.txt [new file with mode: 0644]
LayoutTests/editing/document-marker-null-check.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/DocumentMarkerController.cpp
Source/WebCore/dom/DocumentMarkerController.h
Source/WebCore/editing/AlternativeTextController.cpp
Source/WebCore/editing/Editor.cpp
Source/WebCore/rendering/InlineTextBox.cpp
Source/WebCore/rendering/RenderReplaced.cpp
Source/WebCore/rendering/RenderText.cpp
Source/WebCore/rendering/SimpleLineLayout.cpp
Source/WebCore/testing/Internals.cpp

index b82d0cd..e57880b 100644 (file)
@@ -1,3 +1,14 @@
+2018-12-17  Zalan Bujtas  <zalan@apple.com>
+
+        Reproducible ASSERTion failure when toggling layer borders with find-in-page up
+        https://bugs.webkit.org/show_bug.cgi?id=192762
+        <rdar://problem/46676873>
+
+        Reviewed by Simon Fraser.
+
+        * editing/document-marker-null-check-expected.txt: Added.
+        * editing/document-marker-null-check.html: Added.
+
 2018-12-17  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r239265 and r239274.
diff --git a/LayoutTests/editing/document-marker-null-check-expected.txt b/LayoutTests/editing/document-marker-null-check-expected.txt
new file mode 100644 (file)
index 0000000..166765e
--- /dev/null
@@ -0,0 +1 @@
+Pass if no crash or assert.
diff --git a/LayoutTests/editing/document-marker-null-check.html b/LayoutTests/editing/document-marker-null-check.html
new file mode 100644 (file)
index 0000000..18e0ace
--- /dev/null
@@ -0,0 +1,29 @@
+<html>
+<head>
+<title>This test that we don't crash on a null text node with marker on it.</title>
+<style>
+div:before {
+       content: "foobar";
+       display: table;
+}
+</style>
+</head>
+<body>
+<div>Pass if no crash or assert.</div>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+if (window.internals) {
+       internals.settings.setSimpleLineLayoutEnabled(false);   
+       var findOptions = ['CaseInsensitive', 'AtWordStarts', 'TreatMedialCapitalAsWordStart', 'WrapAround'];
+       internals.countMatchesForText('assert', findOptions, 'mark');
+}
+
+document.body.offsetHeight;
+if (window.internals)
+       internals.settings.setSimpleLineLayoutEnabled(true);
+document.body.offsetHeight;
+</script>
+</body>
+</html>
index a10573f..ab9f585 100644 (file)
@@ -1,3 +1,33 @@
+2018-12-17  Zalan Bujtas  <zalan@apple.com>
+
+        Reproducible ASSERTion failure when toggling layer borders with find-in-page up
+        https://bugs.webkit.org/show_bug.cgi?id=192762
+        <rdar://problem/46676873>
+
+        Reviewed by Simon Fraser.
+
+        DocumentMarkerController::markersFor() should take a reference instead of a Node*.
+
+        Test: editing/document-marker-null-check.html
+
+        * dom/DocumentMarkerController.cpp:
+        (DocumentMarkerController::hasMarkers):
+        * dom/DocumentMarkerController.h:
+        * editing/AlternativeTextController.cpp:
+        (WebCore::AlternativeTextController::respondToChangedSelection):
+        * editing/Editor.cpp:
+        (WebCore::Editor::selectionStartHasMarkerFor const):
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::collectMarkedTextsForDocumentMarkers const):
+        * rendering/RenderReplaced.cpp:
+        (WebCore::RenderReplaced::paint):
+        * rendering/RenderText.cpp:
+        (WebCore::RenderText::draggedContentRangesBetweenOffsets const):
+        * rendering/SimpleLineLayout.cpp:
+        (WebCore::SimpleLineLayout::canUseForWithReason):
+        * testing/Internals.cpp:
+        (WebCore::Internals::markerCountForNode):
+
 2018-12-17  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r239265 and r239274.
index 3917763..d55c56f 100644 (file)
@@ -521,13 +521,13 @@ DocumentMarker* DocumentMarkerController::markerContainingPoint(const LayoutPoin
     return nullptr;
 }
 
-Vector<RenderedDocumentMarker*> DocumentMarkerController::markersFor(Node* node, OptionSet<DocumentMarker::MarkerType> markerTypes)
+Vector<RenderedDocumentMarker*> DocumentMarkerController::markersFor(Node& node, OptionSet<DocumentMarker::MarkerType> markerTypes)
 {
     if (!possiblyHasMarkers(markerTypes))
         return { };
 
     Vector<RenderedDocumentMarker*> result;
-    MarkerList* list = m_markers.get(node);
+    MarkerList* list = m_markers.get(&node);
     if (!list)
         return result;
 
@@ -551,7 +551,8 @@ Vector<RenderedDocumentMarker*> DocumentMarkerController::markersInRange(Range&
 
     Node* pastLastNode = range.pastLastNode();
     for (Node* node = range.firstNode(); node != pastLastNode; node = NodeTraversal::next(*node)) {
-        for (auto* marker : markersFor(node)) {
+        ASSERT(node);
+        for (auto* marker : markersFor(*node)) {
             if (!markerTypes.contains(marker->type()))
                 continue;
             if (node == &startContainer && marker->endOffset() <= range.startOffset())
@@ -763,7 +764,8 @@ bool DocumentMarkerController::hasMarkers(Range& range, OptionSet<DocumentMarker
 
     Node* pastLastNode = range.pastLastNode();
     for (Node* node = range.firstNode(); node != pastLastNode; node = NodeTraversal::next(*node)) {
-        for (auto* marker : markersFor(node)) {
+        ASSERT(node);
+        for (auto* marker : markersFor(*node)) {
             if (!markerTypes.contains(marker->type()))
                 continue;
             if (node == &startContainer && marker->endOffset() <= static_cast<unsigned>(range.startOffset()))
index f24103f..dd7d64f 100644 (file)
@@ -82,7 +82,7 @@ public:
     void setMarkersActive(Range*, bool);
     void setMarkersActive(Node*, unsigned startOffset, unsigned endOffset, bool);
 
-    WEBCORE_EXPORT Vector<RenderedDocumentMarker*> markersFor(Node*, OptionSet<DocumentMarker::MarkerType> = DocumentMarker::allMarkers());
+    WEBCORE_EXPORT Vector<RenderedDocumentMarker*> markersFor(Node&, OptionSet<DocumentMarker::MarkerType> = DocumentMarker::allMarkers());
     WEBCORE_EXPORT Vector<RenderedDocumentMarker*> markersInRange(Range&, OptionSet<DocumentMarker::MarkerType>);
     void clearDescriptionOnMarkersIntersectingRange(Range&, OptionSet<DocumentMarker::MarkerType>);
 
index e64f947..582be0d 100644 (file)
@@ -420,7 +420,8 @@ void AlternativeTextController::respondToChangedSelection(const VisibleSelection
         return;
 
     Node* node = position.containerNode();
-    for (auto* marker : node->document().markers().markersFor(node)) {
+    ASSERT(node);
+    for (auto* marker : node->document().markers().markersFor(*node)) {
         ASSERT(marker);
         if (respondToMarkerAtEndOfWord(*marker, position))
             break;
index 69c16a9..e5ae713 100644 (file)
@@ -3768,7 +3768,7 @@ bool Editor::selectionStartHasMarkerFor(DocumentMarker::MarkerType markerType, i
 
     unsigned int startOffset = static_cast<unsigned int>(from);
     unsigned int endOffset = static_cast<unsigned int>(from + length);
-    Vector<RenderedDocumentMarker*> markers = document().markers().markersFor(node);
+    Vector<RenderedDocumentMarker*> markers = document().markers().markersFor(*node);
     for (auto* marker : markers) {
         if (marker->startOffset() <= startOffset && endOffset <= marker->endOffset() && marker->type() == markerType)
             return true;
index 1d01090..4c33c59 100644 (file)
@@ -862,7 +862,7 @@ Vector<MarkedText> InlineTextBox::collectMarkedTextsForDocumentMarkers(TextPaint
     if (!renderer().textNode())
         return { };
 
-    Vector<RenderedDocumentMarker*> markers = renderer().document().markers().markersFor(renderer().textNode());
+    Vector<RenderedDocumentMarker*> markers = renderer().document().markers().markersFor(*renderer().textNode());
 
     auto markedTextTypeForMarkerType = [] (DocumentMarker::MarkerType type) {
         switch (type) {
index a0f0839..d58edfe 100644 (file)
@@ -162,7 +162,8 @@ void RenderReplaced::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
     GraphicsContextStateSaver savedGraphicsContext(paintInfo.context(), false);
     if (element() && element()->parentOrShadowHostElement()) {
         auto* parentContainer = element()->parentOrShadowHostElement();
-        if (draggedContentContainsReplacedElement(document().markers().markersFor(parentContainer, DocumentMarker::DraggedContent), *element())) {
+        ASSERT(parentContainer);
+        if (draggedContentContainsReplacedElement(document().markers().markersFor(*parentContainer, DocumentMarker::DraggedContent), *element())) {
             savedGraphicsContext.save();
             paintInfo.context().setAlpha(0.25);
         }
index 42257f7..c15ee97 100644 (file)
@@ -1063,7 +1063,7 @@ Vector<std::pair<unsigned, unsigned>> RenderText::draggedContentRangesBetweenOff
     if (!textNode())
         return { };
 
-    auto markers = document().markers().markersFor(textNode(), DocumentMarker::DraggedContent);
+    auto markers = document().markers().markersFor(*textNode(), DocumentMarker::DraggedContent);
     if (markers.isEmpty())
         return { };
 
index 112a7fa..616f0a6 100644 (file)
@@ -315,7 +315,7 @@ AvoidanceReasonFlags canUseForWithReason(const RenderBlockFlow& flow, IncludeRea
             SET_REASON_AND_RETURN_IF_NEEDED(FlowChildIsSelected, reasons, includeReasons);
         if (is<RenderText>(*child)) {
             const auto& renderText = downcast<RenderText>(*child);
-            if (!renderText.document().markers().markersFor(renderText.textNode()).isEmpty())
+            if (renderText.textNode() && !renderText.document().markers().markersFor(*renderText.textNode()).isEmpty())
                 SET_REASON_AND_RETURN_IF_NEEDED(FlowIncludesDocumentMarkers, reasons, includeReasons);
             child = child->nextSibling();
             continue;
index 889c138..999e5dd 100644 (file)
@@ -1541,7 +1541,7 @@ ExceptionOr<unsigned> Internals::markerCountForNode(Node& node, const String& ma
         return Exception { SyntaxError };
 
     node.document().frame()->editor().updateEditorUINowIfScheduled();
-    return node.document().markers().markersFor(&node, markerTypes).size();
+    return node.document().markers().markersFor(node, markerTypes).size();
 }
 
 ExceptionOr<RenderedDocumentMarker*> Internals::markerAt(Node& node, const String& markerType, unsigned index)
@@ -1554,7 +1554,7 @@ ExceptionOr<RenderedDocumentMarker*> Internals::markerAt(Node& node, const Strin
 
     node.document().frame()->editor().updateEditorUINowIfScheduled();
 
-    Vector<RenderedDocumentMarker*> markers = node.document().markers().markersFor(&node, markerTypes);
+    Vector<RenderedDocumentMarker*> markers = node.document().markers().markersFor(node, markerTypes);
     if (markers.size() <= index)
         return nullptr;
     return markers[index];