[webkit-changes] [WebKit/WebKit] dd872d: Release assert while scrolling subframes under ree...

[Wenson Hsieh]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: dd872d60f50136a1283804ae5e957a8c039e1c96
      https://github.com/WebKit/WebKit/commit/dd872d60f50136a1283804ae5e957a8c039e1c96
  Author: Wenson Hsieh <wenson_hsieh at apple.com>
  Date:   2022-10-25 (Tue, 25 Oct 2022)

  Changed paths:
    A LayoutTests/editing/resources/selection-scrolling-in-multiple-nested-subframes-iframe.html
    A LayoutTests/editing/selection/selection-scrolling-in-multiple-nested-subframes-expected.txt
    A LayoutTests/editing/selection/selection-scrolling-in-multiple-nested-subframes.html
    M Source/WebCore/page/FrameView.cpp

  Log Message:
  -----------
  Release assert while scrolling subframes under reentrant calls to updateAppearanceAfterLayout()
https://bugs.webkit.org/show_bug.cgi?id=246978
rdar://97896551

Reviewed by Simon Fraser.

It's currently possible to induce a release assertion when attempting to update layout underneath
`RenderWidget::updateWidgetPosition()`, by revealing the selection inside of a nested subframe that
contains viewport-constrained elements. The new test case below contains the steps required to set
up this assertion:

-   Load a page that contains a subframe (`s_0`), which in turn contains two or more child frames
    (`s_1`, `s_2`). `s_1` and `s_2` contain editable content, and `s_0` contains a fixed-position
    element.

-   Focus each of `s_1` and `s_2`, and use the keyboard to change the selection. This causes both
    frames' `FrameSelection`s to be in a state where `m_selectionRevealMode` is set to `Reveal`.

-   Scroll `s_0` down to the bottom (see (3) below for more information), and then click the button
    to trigger an event handler that runs the rest of the test.

1.  On click, in `s_1` and `s_2`, we clear the contents of the body, which schedules selection
    revealing as post-layout tasks.

2.  Next, we force a sync layout update by invoking `document.body.offsetHeight;` in the main frame,
    which triggers all the following events in the test.

3.  As a post-layout task, since `s_0` contains a fixed-position element, we invoke
    `FrameView::updateWidgetPositions()`, which triggers a subsequent layout in each of the
    subframes.

4.  In this nested layout pass, we then fire off the additional post-layout tasks scheduled by `s_1`
    and `s_2`, which both attempt to reveal the selection synchronously, one after another.

5.  The selection scrolling causes us to establish a `ScriptDisallowedScope` inside of
    `FrameView::scrollRectToVisibleInChildView`, while attempting scrolling the child frame `s_1` to
    reveal the selection.

6.  This nested `RenderWidget::updateWidgetPosition()` triggers another nested layout pass. Note
    that this, in theory, already reveals the bug — though in practice, we don't crash yet because
    `s_1`'s layout is already up to date. After layout, we fire off the other queued post-layout
    task to reveal the selection, this time for `s_2`.

7.  We then attempt to reveal the selection again, this time for `s_2`. However, due to the fact
    that this is now all happening inside a `ScriptDisallowedScope` established in (5), we now
    crash.

To fix this, we take advantage of some of the prior work done in `commits.webkit.org/250836 at main` to
remove a synchronous post-layout call to `updateAppearanceAfterLayout()` in the case where the
selection is not focused and active. Since we now update the selection appearance in the next
rendering update anyways, this simply defers work that would've otherwise been done as a post-layout
task to the next rendering update instead. Note that we still update eagerly here in the case where
the selection is active, since accessibility notifications still rely on the fact that intermediate
AX notifications are dispatched for selection changes that happen during text editing (see:
accessibility/mac/selection-value-changes-for-aria-textbox.html).

In the future, we could probably queue the accessibility notifications above as well, and eliminate
the post-layout selection appearance update altogether.

Test: editing/selection/selection-scrolling-in-multiple-nested-subframes.html

* LayoutTests/editing/resources/selection-scrolling-in-multiple-nested-subframes-iframe.html: Added.
* LayoutTests/editing/selection/selection-scrolling-in-multiple-nested-subframes-expected.txt: Added.
* LayoutTests/editing/selection/selection-scrolling-in-multiple-nested-subframes.html: Added.
* Source/WebCore/page/FrameView.cpp:
(WebCore::FrameView::performPostLayoutTasks):

Canonical link: https://commits.webkit.org/255998@main