REGRESSION (r243926): [iOS] Release assertion when computing editor state during...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Apr 2019 18:04:29 +0000 (18:04 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Apr 2019 18:04:29 +0000 (18:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197012
<rdar://problem/49908848>

Reviewed by Simon Fraser.

Source/WebKit:

We hit the release assertion due to the following sequence of events:
- Dispatch a queued event (in this case, a scroll event)
- Invoke the scroll event listener, which modifies layout in some way
- This scrolls an overflow scrollable container under the scope of layout
- Overflow scrolling then calls didChangeSelection and triggers an editor state update, which updates layout

In the case where the selection is in the main frame, we bail early due to the check for recursive layout (i.e.
frameView->layoutContext().isInRenderTreeLayout()). However, in the case where the selection is inside a
subframe, we end up skipping past this check, since the subframe's FrameView isn't currently laying out, and so
we end up hitting the release assertion underneath the early return.

To fix this, simply defer editor state updates due to overflow scrolling until the next remote layer tree commit
instead of computing and sending the information immediately. While this only defers editor state updates during
overflow scrolling, <rdar://problem/47258878> tracks making editor state updates deferred in the general case.

Test: editing/selection/overflow-scroll-while-selecting-text.html

* WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:
(WebKit::WebEditorClient::overflowScrollPositionChanged):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::didChangeOverflowScrollPosition):
(WebKit::WebPage::didChangeSelection):
(WebKit::WebPage::didChangeSelectionOrOverflowScrollPosition):
* WebProcess/WebPage/WebPage.h:

LayoutTests:

Adds a new layout test to exercise the crash.

* editing/selection/overflow-scroll-while-selecting-text-expected.txt: Added.
* editing/selection/overflow-scroll-while-selecting-text.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/selection/overflow-scroll-while-selecting-text-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/overflow-scroll-while-selecting-text.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h

index 29cd78a..89307d9 100644 (file)
@@ -1,3 +1,16 @@
+2019-04-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r243926): [iOS] Release assertion when computing editor state during an overflow scroll triggered by layout
+        https://bugs.webkit.org/show_bug.cgi?id=197012
+        <rdar://problem/49908848>
+
+        Reviewed by Simon Fraser.
+
+        Adds a new layout test to exercise the crash.
+
+        * editing/selection/overflow-scroll-while-selecting-text-expected.txt: Added.
+        * editing/selection/overflow-scroll-while-selecting-text.html: Added.
+
 2019-04-17  Alex Christensen  <achristensen@webkit.org>
 
         [Mac iOS WK2] Layout Test http/tests/resourceLoadStatistics/ping-to-prevalent-resource.html is a flaky timeout
diff --git a/LayoutTests/editing/selection/overflow-scroll-while-selecting-text-expected.txt b/LayoutTests/editing/selection/overflow-scroll-while-selecting-text-expected.txt
new file mode 100644 (file)
index 0000000..c1f8415
--- /dev/null
@@ -0,0 +1,11 @@
+This test verifies that we don't crash while scrolling an overflow scrolling container during layout, if the selection is in an editable element in a focused subframe.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Successfully removed the text.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
diff --git a/LayoutTests/editing/selection/overflow-scroll-while-selecting-text.html b/LayoutTests/editing/selection/overflow-scroll-while-selecting-text.html
new file mode 100644 (file)
index 0000000..c6d8dde
--- /dev/null
@@ -0,0 +1,40 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+<script src="../../resources/js-test.js"></script>
+<style>
+iframe, #text {
+    width: 320px;
+    overflow: scroll;
+}
+
+#text {
+    line-height: 2.5;
+    height: 200px;
+    font-size: 40px;
+}
+</style>
+</head>
+<body>
+<iframe srcdoc="
+    <body contenteditable style='font-size: 20px'>Hello world</body>
+    <script>getSelection().selectAllChildren(document.body);</script>
+"></iframe>
+<div id="text">Here's to the crazy ones. The misfits. The rebels. The troublemakers. The round pegs in the square holes. The ones who see things differently. They're not fond of rules. And they have no respect for the status quo.</div>
+</body>
+<script>
+jsTestIsAsync = true;
+document.querySelector("iframe").focus();
+
+description("This test verifies that we don't crash while scrolling an overflow scrolling container during layout, if the selection is in an editable element in a focused subframe.");
+
+const text = document.getElementById("text");
+text.scrollTo(0, 1000);
+addEventListener("load", () => text.scrollTo(0, 500));
+text.addEventListener("scroll", () => {
+    text.textContent = "";
+    testPassed("Successfully removed the text.");
+    finishJSTest();
+});
+</script>
+</html>
index 170a3a1..155a922 100644 (file)
@@ -1,3 +1,36 @@
+2019-04-17  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r243926): [iOS] Release assertion when computing editor state during an overflow scroll triggered by layout
+        https://bugs.webkit.org/show_bug.cgi?id=197012
+        <rdar://problem/49908848>
+
+        Reviewed by Simon Fraser.
+
+        We hit the release assertion due to the following sequence of events:
+        - Dispatch a queued event (in this case, a scroll event)
+        - Invoke the scroll event listener, which modifies layout in some way
+        - This scrolls an overflow scrollable container under the scope of layout
+        - Overflow scrolling then calls didChangeSelection and triggers an editor state update, which updates layout
+
+        In the case where the selection is in the main frame, we bail early due to the check for recursive layout (i.e.
+        frameView->layoutContext().isInRenderTreeLayout()). However, in the case where the selection is inside a
+        subframe, we end up skipping past this check, since the subframe's FrameView isn't currently laying out, and so
+        we end up hitting the release assertion underneath the early return.
+
+        To fix this, simply defer editor state updates due to overflow scrolling until the next remote layer tree commit
+        instead of computing and sending the information immediately. While this only defers editor state updates during
+        overflow scrolling, <rdar://problem/47258878> tracks making editor state updates deferred in the general case.
+
+        Test: editing/selection/overflow-scroll-while-selecting-text.html
+
+        * WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:
+        (WebKit::WebEditorClient::overflowScrollPositionChanged):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::didChangeOverflowScrollPosition):
+        (WebKit::WebPage::didChangeSelection):
+        (WebKit::WebPage::didChangeSelectionOrOverflowScrollPosition):
+        * WebProcess/WebPage/WebPage.h:
+
 2019-04-17  Chris Dumez  <cdumez@apple.com>
 
         Remember device orientation permission for the duration of the browsing session
index 00b871e..2a4a163 100644 (file)
@@ -93,7 +93,7 @@ void WebEditorClient::updateStringForFind(const String& findString)
 
 void WebEditorClient::overflowScrollPositionChanged()
 {
-    m_page->didChangeSelection();
+    m_page->didChangeOverflowScrollPosition();
 }
 
 } // namespace WebKit
index c66ebe6..00f2737 100644 (file)
@@ -5284,8 +5284,18 @@ void WebPage::didChangeContents()
     sendEditorStateUpdate();
 }
 
+void WebPage::didChangeOverflowScrollPosition()
+{
+    didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling::Deferred);
+}
+
 void WebPage::didChangeSelection()
 {
+    didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling::Immediate);
+}
+
+void WebPage::didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling editorStateScheduling)
+{
     Frame& frame = m_page->focusController().focusedOrMainFrame();
     // The act of getting Dictionary Popup info can make selection changes that we should not propagate to the UIProcess.
     // Specifically, if there is a caret selection, it will change to a range selection of the word around the caret. And
@@ -5327,7 +5337,10 @@ void WebPage::didChangeSelection()
     }
 #endif
 
-    sendPartialEditorStateAndSchedulePostLayoutUpdate();
+    if (editorStateScheduling == EditorStateUpdateScheduling::Immediate)
+        sendPartialEditorStateAndSchedulePostLayoutUpdate();
+    else
+        scheduleFullEditorStateUpdate();
 }
 
 void WebPage::resetFocusedElementForFrame(WebFrame* frame)
index 3f5bc3a..2acc328 100644 (file)
@@ -764,6 +764,7 @@ public:
 
     void didApplyStyle();
     void didChangeSelection();
+    void didChangeOverflowScrollPosition();
     void didChangeContents();
     void discardedComposition();
     void canceledComposition();
@@ -1271,6 +1272,9 @@ private:
     void executeEditCommand(const String&, const String&);
     void setEditable(bool);
 
+    enum class EditorStateUpdateScheduling { Deferred, Immediate };
+    void didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling);
+
     void increaseListLevel();
     void decreaseListLevel();
     void changeListType();