Attachment 342511 Details for Bug 186555 – Minor tweak

[patch] Minor tweak

bug-186555-20180611204956.patch (text/plain), 5.75 KB, created by Wenson Hsieh on 2018-06-11 20:49:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Wenson Hsieh

Created: 2018-06-11 20:49:56 PDT

Size: 5.75 KB

patch

obsolete

>Subversion Revision: 232663
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 89e3e47648da71efd92e46979962f6d39b9534fb..8a8252a5c279218084e1e0b1d107b21b2c804122 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 NOBODY (OOPS!).
>+
>+        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..87b538d8264ad2c76ecbd6675d771347e73be3af 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
>+            // deepest 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..bbc700ee10ad59474b3113ded56a766dbef2bd42 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 NOBODY (OOPS!).
>+
>+        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..9a0d72af5eca0b65b4aad19f9cff7a2a82adc761
>--- /dev/null
>+++ b/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end-expected.txt
>@@ -0,0 +1,2 @@
>+This test passes if we do not crash...Apple
>+WebKit
>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..bd2950a29bbba65d607fdaafad8b732a89346ea6
>--- /dev/null
>+++ b/LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end.html
>@@ -0,0 +1,14 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script>
>+onload = () => {
>+    getSelection().setPosition(document.body, 1);
>+    document.execCommand("InsertHTML", false, `<div>Apple</div>WebKit<span class="Apple-style-span"></span>`);
>+    if (window.testRunner)
>+        testRunner.dumpAsText();
>+}
>+</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