Changeset 244388 in webkit

Timestamp:

Apr 17, 2019 11:04:29 AM (5 years ago)

Author:

Wenson Hsieh

Message:

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.

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/selection/overflow-scroll-while-selecting-text-expected.txt (added)
• LayoutTests/editing/selection/overflow-scroll-while-selecting-text.html (added)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm (modified) (1 diff)
• Source/WebKit/WebProcess/WebPage/WebPage.cpp (modified) (2 diffs)
• Source/WebKit/WebProcess/WebPage/WebPage.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm

@@ -94 +94 @@
 void WebEditorClient::overflowScrollPositionChanged()
 {
-    m_page->didChangeSelection();
+    m_page->didChangeOverflowScrollPosition();
 }
 

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -5285 +5285 @@
 }
 
+void WebPage::didChangeOverflowScrollPosition()
+{
+    didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling::Deferred);
+}
+
 void WebPage::didChangeSelection()
+{
+    didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling::Immediate);
+}
+
+void WebPage::didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling editorStateScheduling)
 {
     Frame& frame = m_page->focusController().focusedOrMainFrame();
@@ -5328 +5338 @@
 #endif
 
-    sendPartialEditorStateAndSchedulePostLayoutUpdate();
+    if (editorStateScheduling == EditorStateUpdateScheduling::Immediate)
+        sendPartialEditorStateAndSchedulePostLayoutUpdate();
+    else
+        scheduleFullEditorStateUpdate();
 }
 

trunk/Source/WebKit/WebProcess/WebPage/WebPage.h

@@ -765 +765 @@
     void didApplyStyle();
     void didChangeSelection();
+    void didChangeOverflowScrollPosition();
     void didChangeContents();
     void discardedComposition();
@@ -1272 +1273 @@
     void setEditable(bool);
 
+    enum class EditorStateUpdateScheduling { Deferred, Immediate };
+    void didChangeSelectionOrOverflowScrollPosition(EditorStateUpdateScheduling);
+
     void increaseListLevel();
     void decreaseListLevel();