Attachment 342553 Details for Bug 186555 – Patch for landing

[patch] Patch for landing

bug-186555-20180612094606.patch (text/plain), 5.89 KB, created by Wenson Hsieh on 2018-06-12 09:46:07 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-06-12 09:46:07 PDT

Size: 5.89 KB

patch

obsolete

>Subversion Revision: 232663
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 89e3e47648da71efd92e46979962f6d39b9534fb..86a27a462612293cbca471006df0889dbfe1ac3d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2018-06-11  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand at the end of the document
>+        https://bugs.webkit.org/show_bug.cgi?id=186555
>+        <rdar://problem/39703004>
>+
>+        Reviewed by Ryosuke Niwa.
>+
>+        r228724 removed a null check for m_lastNodeInserted in InsertedNodes::pastLastLeaf(). This null check was
>+        guarding against the case where m_firstNodeInserted might exist, but m_lastNodeInserted is null, which may
>+        happen when inserting content at the end of the document, since InsertedNodes::willRemoveNodePreservingChildren
>+        may cause m_lastNodeInserted to be null. This is because the removed node may be at the very end of the document
>+        _and also_ not have any children, which means that both `node->lastChild()` as well as
>+        `NodeTraversal::nextSkippingChildren(*node)` will be null.
>+
>+        After getting into this state, we subsequently crash when attempting to compute InsertedNodes::pastLastLeaf().
>+        To fix this, avoid accidentally clearing out m_lastNodeInserted; if the last inserted node has neither a child
>+        nor a next node, seek backwards to the previous node in the DOM instead, and clamp to the first inserted node,
>+        such that the last inserted node's document position is at or after the first inserted node's position.
>+
>+        Test: editing/execCommand/insert-apple-style-span-at-document-end.html
>+
>+        * editing/ReplaceSelectionCommand.cpp:
>+        (WebCore::ReplaceSelectionCommand::InsertedNodes::willRemoveNodePreservingChildren):
>+
> 2018-06-09  Zalan Bujtas  <zalan@apple.com>
> 
>         REGRESSION (r230480): Cannot adjust photo position on LinkedIn's profile page
>diff --git a/Source/WebCore/editing/ReplaceSelectionCommand.cpp b/Source/WebCore/editing/ReplaceSelectionCommand.cpp
>index 8029333e59f481224a0537d2e6e12ca70b8fce1e..bd7c57670b4dbd5f952f039b94e933fedbfba5da 100644
>--- a/Source/WebCore/editing/ReplaceSelectionCommand.cpp
>+++ b/Source/WebCore/editing/ReplaceSelectionCommand.cpp
>@@ -345,8 +345,17 @@ inline void ReplaceSelectionCommand::InsertedNodes::willRemoveNodePreservingChil
> {
>     if (m_firstNodeInserted == node)
>         m_firstNodeInserted = NodeTraversal::next(*node);
>-    if (m_lastNodeInserted == node)
>-        m_lastNodeInserted = node->lastChild() ? node->lastChild() : NodeTraversal::nextSkippingChildren(*node);
>+    if (m_lastNodeInserted == node) {
>+        m_lastNodeInserted = node->lastChild() ?: NodeTraversal::nextSkippingChildren(*node);
>+        if (!m_lastNodeInserted) {
>+            // If the last inserted node is at the end of the document and doesn't have any children, look backwards for the
>+            // previous node as the last inserted node, clamping to the first inserted node if needed to ensure that the
>+            // document position of the last inserted node is not behind the first inserted node.
>+            auto* previousNode = NodeTraversal::previousSkippingChildren(*node);
>+            ASSERT(previousNode);
>+            m_lastNodeInserted = m_firstNodeInserted->compareDocumentPosition(*previousNode) & Node::DOCUMENT_POSITION_FOLLOWING ? previousNode : m_firstNodeInserted;
>+        }
>+    }
> }
> 
> inline void ReplaceSelectionCommand::InsertedNodes::willRemoveNode(Node* node)
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ac9c72c1bfa75c2df90bd15ad945868281ca4321..847278e3d7a4fef0fe94d6f7e024970b208a843a 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-11  Wenson Hsieh  <wenson_hsieh@apple.com>
>+
>+        REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand at the end of the document
>+        https://bugs.webkit.org/show_bug.cgi?id=186555
>+        <rdar://problem/39703004>
>+
>+        Reviewed by Ryosuke Niwa.
>+
>+        Add a new layout test demonstrating the crash. The test passes if the right text is inserted into the DOM, and
>+        we don't hit a crash.
>+
>+        * editing/execCommand/insert-apple-style-span-at-document-end-expected.txt: Added.
>+        * editing/execCommand/insert-apple-style-span-at-document-end.html: Added.
>+
> 2018-06-09  Zalan Bujtas  <zalan@apple.com>
> 
>         REGRESSION (r230480): Cannot adjust photo position on LinkedIn's profile page
>diff --git a/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end-expected.txt b/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..9d74a782ffe913c624320e32f05980c538644fec
>--- /dev/null
>+++ b/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end-expected.txt
>@@ -0,0 +1,3 @@
>+| "This test passes if we do not crash...Apple"
>+| <br>
>+| "WebKit<#selection-caret>"
>diff --git a/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end.html b/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..a3eb902b0f7692510041e37630dc81cb20fb7073
>--- /dev/null
>+++ b/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end.html
>@@ -0,0 +1,18 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../../resources/dump-as-markup.js"></script>
>+<script>
>+Markup.noAutoDump();
>+Markup.waitUntilDone();
>+
>+addEventListener("load", () => {
>+    getSelection().setPosition(document.body, 1);
>+    document.execCommand("InsertHTML", false, `<div>Apple</div>WebKit<span class="Apple-style-span"></span>`);
>+    Markup.dump(document.body);
>+    Markup.notifyDone();
>+});
>+</script>
>+</head>
>+<body contenteditable autofocus>This test passes if we do not crash...</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 186555: 342510 | 342511 | 342514 | 342553