Attachment 357452 Details for Bug 192762 – Patch

[patch] Patch

bug-192762-20181217110147.patch (text/plain), 12.94 KB, created by zalan on 2018-12-17 11:01:47 PST

(hide)

Description:

Filename:

MIME Type:

Creator: zalan

Created: 2018-12-17 11:01:47 PST

Size: 12.94 KB

patch

obsolete

>Subversion Revision: 239232
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 2ec8741f5c7f638a91c877d25770acd7ccc8d4d9..d74806cd85db8031d283a7ed12892ff8cb258509 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-14  David Kilzer  <ddkilzer@apple.com>
> 
>         clang-tidy: Fix unnecessary object copies in WebCore/platform/graphics/avfoundation/objc/
>diff --git a/Source/WebCore/dom/DocumentMarkerController.cpp b/Source/WebCore/dom/DocumentMarkerController.cpp
>index 39177632d37cb0a6419d22f61cc0df9fd96fd8c0..d55c56f13693258b72dfcbda546bbc7c6f89a302 100644
>--- a/Source/WebCore/dom/DocumentMarkerController.cpp
>+++ b/Source/WebCore/dom/DocumentMarkerController.cpp
>@@ -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()))
>diff --git a/Source/WebCore/dom/DocumentMarkerController.h b/Source/WebCore/dom/DocumentMarkerController.h
>index f24103f455bd9f5d9f9e4aba9601db7df8a468b9..dd7d64f086596ac7f0efe014c154028a2a5a9608 100644
>--- a/Source/WebCore/dom/DocumentMarkerController.h
>+++ b/Source/WebCore/dom/DocumentMarkerController.h
>@@ -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>);
> 
>diff --git a/Source/WebCore/editing/AlternativeTextController.cpp b/Source/WebCore/editing/AlternativeTextController.cpp
>index e64f9478a249b1b5e2289fc809ee038978342541..582be0de5932c5cbe49c51f0c81b8dc980717d1e 100644
>--- a/Source/WebCore/editing/AlternativeTextController.cpp
>+++ b/Source/WebCore/editing/AlternativeTextController.cpp
>@@ -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;
>diff --git a/Source/WebCore/editing/Editor.cpp b/Source/WebCore/editing/Editor.cpp
>index 69c16a94dd9dbd83a09d0e6df33ca83d8b32f954..e5ae71336fb1fadf1f7e69e8becdfc3c97d8b3d3 100644
>--- a/Source/WebCore/editing/Editor.cpp
>+++ b/Source/WebCore/editing/Editor.cpp
>@@ -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;
>diff --git a/Source/WebCore/rendering/InlineTextBox.cpp b/Source/WebCore/rendering/InlineTextBox.cpp
>index 1d01090b11fba959f1d09acf63c355616c1c2ef3..4c33c59bdc57bb4a46ab63d5ce58136605226a32 100644
>--- a/Source/WebCore/rendering/InlineTextBox.cpp
>+++ b/Source/WebCore/rendering/InlineTextBox.cpp
>@@ -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) {
>diff --git a/Source/WebCore/rendering/RenderReplaced.cpp b/Source/WebCore/rendering/RenderReplaced.cpp
>index a0f08398dbbb5b89e1a766db5a821db1a4ccf24b..d58edfeb03c68546b8ae3ef6c40218ba123a5a23 100644
>--- a/Source/WebCore/rendering/RenderReplaced.cpp
>+++ b/Source/WebCore/rendering/RenderReplaced.cpp
>@@ -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);
>         }
>diff --git a/Source/WebCore/rendering/RenderText.cpp b/Source/WebCore/rendering/RenderText.cpp
>index 42257f725b503c61e03195045eec949a9135430c..c15ee978be53a8ca30402b9d24a836ffd67544b9 100644
>--- a/Source/WebCore/rendering/RenderText.cpp
>+++ b/Source/WebCore/rendering/RenderText.cpp
>@@ -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 { };
> 
>diff --git a/Source/WebCore/rendering/SimpleLineLayout.cpp b/Source/WebCore/rendering/SimpleLineLayout.cpp
>index 112a7fa819bb1eefa85d0e09953cf6e52bf5df4e..616f0a61b703a984838014ec73c29f15ef2866fb 100644
>--- a/Source/WebCore/rendering/SimpleLineLayout.cpp
>+++ b/Source/WebCore/rendering/SimpleLineLayout.cpp
>@@ -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;
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index c3227214ae82a344ea5c63abb13749ff2a8aa1c5..c75e26bc7a3a09660f084b4fba4805b60932045d 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -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];
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 8642cbfca317aa207b825ec1f75912645f8559a6..dbc1ed32af52a53daaad5c46853ccc19f91a48e3 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * editing/document-marker-null-check-expected.txt: Added.
>+        * editing/document-marker-null-check.html: Added.
>+
> 2018-12-14  Matt Baker  <mattbaker@apple.com>
> 
>         Web Inspector: Cookies view should use model objects instead of raw payload data
>diff --git a/LayoutTests/editing/document-marker-null-check-expected.txt b/LayoutTests/editing/document-marker-null-check-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..166765e5d20797c47ba4f1f3b5a607d27113b05d
>--- /dev/null
>+++ b/LayoutTests/editing/document-marker-null-check-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..18e0ace4bfbb3103594bb13da3977ba692d418ea
>--- /dev/null
>+++ b/LayoutTests/editing/document-marker-null-check.html
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 192762: 357452