[iOS] Web content process occasionally crashes under VisibleSelection…

…::adjustPositionForEnd https://bugs.webkit.org/show_bug.cgi?id=225072 rdar://77159489 Reviewed by Darin Adler. Source/WebCore: Add a helper method to check whether or not the given `Position` is in a tree scope. See WebKit/ChangeLog for more details. Test: editing/selection/ios/clear-selection-while-moving-selection-handles.html * dom/Position.h: (WebCore::Position::isInTreeScope const): Source/WebKit: Make `rangeForPointInRootViewCoordinates` robust in the case where the original selection may have become orphaned or null, by checking that the `targetNode` and existing `selectionStart` or `selectionEnd` are in a tree scope before accessing `treeScope()`. * WebProcess/WebPage/ios/WebPageIOS.mm: (WebKit::rangeForPointInRootViewCoordinates): LayoutTests: Add a layout test that programmatically clears the selection in the middle of changing it by moving selection handles. * editing/selection/ios/clear-selection-while-moving-selection-handles-expected.txt: Added. * editing/selection/ios/clear-selection-while-moving-selection-handles.html: Added. Canonical link: https://commits.webkit.org/237102@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@276688 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Apr 28, 2021 · bc255263d426b651b9f4c0da8d2bb006a0fc658a · bc25526
1 parent 094c7c8
commit bc255263d426b651b9f4c0da8d2bb006a0fc658a
Show file tree

Hide file tree

Showing 7 changed files with 103 additions and 2 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2021-04-27  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Web content process occasionally crashes under VisibleSelection::adjustPositionForEnd
+        https://bugs.webkit.org/show_bug.cgi?id=225072
+        rdar://77159489
+
+        Reviewed by Darin Adler.
+
+        Add a layout test that programmatically clears the selection in the middle of changing it by moving selection
+        handles.
+
+        * editing/selection/ios/clear-selection-while-moving-selection-handles-expected.txt: Added.
+        * editing/selection/ios/clear-selection-while-moving-selection-handles.html: Added.
+
 2021-04-27  Diego Pino Garcia  <dpino@igalia.com>
 
         [GLIB] Unreviewed test gardening. fast/replaced/pdf-as-embed-with-no-mime-type-is-not-blank.html is failing after r276635.

diff --git a/...utTests/editing/selection/ios/clear-selection-while-moving-selection-handles-expected.txt b/...utTests/editing/selection/ios/clear-selection-while-moving-selection-handles-expected.txt
@@ -0,0 +1,6 @@
+PASS Did not crash
+PASS successfullyParsed is true
+
+TEST COMPLETE
+Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumyeirmodtempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.
+This test verifies that programmatically clearing the text selection while interacting with selection handles does not cause a crash.
diff --git a/LayoutTests/editing/selection/ios/clear-selection-while-moving-selection-handles.html b/LayoutTests/editing/selection/ios/clear-selection-while-moving-selection-handles.html
@@ -0,0 +1,48 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ] -->
+<html>
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<head>
+<script src="../../../resources/js-test.js"></script>
+<script src="../../../resources/ui-helper.js"></script>
+<style>
+body {
+    margin: 0;
+    font-size: 18px;
+}
+</style>
+<script>
+jsTestIsAsync = true;
+
+addEventListener("load", async () => {
+    let target = document.getElementById("target");
+    await UIHelper.longPressElement(target);
+    await UIHelper.waitForSelectionToAppear();
+    let grabberRect = await UIHelper.getSelectionEndGrabberViewRect();
+    let grabberLocationX = grabberRect.left + (grabberRect.width / 2);
+    let grabberLocationY = grabberRect.top + (grabberRect.height / 2);
+
+    selectionChangeCount = 0;
+    document.addEventListener("selectionchange", () => {
+        ++selectionChangeCount;
+        if (selectionChangeCount > 5 && getSelection().rangeCount)
+            getSelection().removeAllRanges();
+    });
+
+    await UIHelper.sendEventStream(new UIHelper.EventStreamBuilder()
+        .begin(grabberLocationX, grabberLocationY)
+        .move(10, grabberLocationY, 1)
+        .end(10, grabberLocationY)
+        .takeResult());
+
+    testPassed("Did not crash");
+    finishJSTest();
+});
+</script>
+</head>
+<body>
+    Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
+    <span id="target">nonumyeirmodtempor</span> invidunt ut labore et dolore magna aliquyam erat,
+    sed diam voluptua.
+    <p>This test verifies that programmatically clearing the text selection while interacting with selection handles does not cause a crash.</p>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,19 @@
+2021-04-27  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Web content process occasionally crashes under VisibleSelection::adjustPositionForEnd
+        https://bugs.webkit.org/show_bug.cgi?id=225072
+        rdar://77159489
+
+        Reviewed by Darin Adler.
+
+        Add a helper method to check whether or not the given `Position` is in a tree scope. See WebKit/ChangeLog for
+        more details.
+
+        Test: editing/selection/ios/clear-selection-while-moving-selection-handles.html
+
+        * dom/Position.h:
+        (WebCore::Position::isInTreeScope const):
+
 2021-04-27  Cameron McCormack  <heycam@apple.com>
 
         Associate a picture element with an img only if the img is a direct child.

diff --git a/Source/WebCore/dom/Position.h b/Source/WebCore/dom/Position.h
@@ -124,6 +124,8 @@ class Position {
         return container ? container->rootEditableElement() : nullptr;
     }
 
+    bool isInTreeScope() const { return m_anchorNode && m_anchorNode->isInTreeScope(); }
+
     // These should only be used for PositionIsOffsetInAnchor positions, unless
     // the position is a legacy editing position.
     void moveToPosition(Node* anchorNode, unsigned offset);

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,18 @@
+2021-04-27  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [iOS] Web content process occasionally crashes under VisibleSelection::adjustPositionForEnd
+        https://bugs.webkit.org/show_bug.cgi?id=225072
+        rdar://77159489
+
+        Reviewed by Darin Adler.
+
+        Make `rangeForPointInRootViewCoordinates` robust in the case where the original selection may have become
+        orphaned or null, by checking that the `targetNode` and existing `selectionStart` or `selectionEnd` are in a
+        tree scope before accessing `treeScope()`.
+
+        * WebProcess/WebPage/ios/WebPageIOS.mm:
+        (WebKit::rangeForPointInRootViewCoordinates):
+
 2021-04-27  Chris Dumez  <cdumez@apple.com>
 
         IPC::decoder::decode() should mark decoder as invalid when failing

diff --git a/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm b/Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm
@@ -1524,14 +1524,14 @@ static IntPoint constrainPoint(const IntPoint& point, const Frame& frame, const
     if (baseIsStart) {
         if (result <= selectionStart)
             result = selectionStart.next();
-        else if (targetNode && selectionStart.deepEquivalent().treeScope() != &targetNode->treeScope())
+        else if (targetNode && targetNode->isInTreeScope() && selectionStart.deepEquivalent().isInTreeScope() && selectionStart.deepEquivalent().treeScope() != &targetNode->treeScope())
             result = VisibleSelection::adjustPositionForEnd(result.deepEquivalent(), selectionStart.deepEquivalent().containerNode());
 
         range = makeSimpleRange(selectionStart, result);
     } else {
         if (selectionEnd <= result)
             result = selectionEnd.previous();
-        else if (targetNode && selectionEnd.deepEquivalent().treeScope() != &targetNode->treeScope())
+        else if (targetNode && targetNode->isInTreeScope() && selectionEnd.deepEquivalent().isInTreeScope() && selectionEnd.deepEquivalent().treeScope() != &targetNode->treeScope())
             result = VisibleSelection::adjustPositionForStart(result.deepEquivalent(), selectionEnd.deepEquivalent().containerNode());
 
         range = makeSimpleRange(result, selectionEnd);