Bug 270593 – AX: AXObjectCache::performDeferredCacheUpdate can run with dirty sub-frame layout, causing incorrect tree updates

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/124158680>

[Tyler Wilcock]

rdar://103616732

[Tyler Wilcock]

Created attachment 470211 [details]
Patch

[Tyler Wilcock]

Created attachment 470213 [details]
Patch

[Tyler Wilcock]

Created attachment 470214 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> Created attachment 470214 [details]
> Patch

In https://bugs.webkit.org/show_bug.cgi?id=268239, we made AXObjectCache::performDeferredCacheUpdate
detect when layout is dirty, and either wait another layout cycle to trigger a cache update, or
force it if necessary. This is important because processing cache updates with dirty layout
can cause incorrect behavior, like objects being ignored incorrectly.

This change was effective, but did not handle the case where a sub-frame had dirty layout, meaning
we can still run into this bug. This patch addresses the issue by forcing all sub-frames to layout

AG: to layout -> to lay out ?

if necessary.

* LayoutTests/platform/glib/TestExpectations: Skip new test.
* LayoutTests/platform/ios/TestExpectations: Enable new test.
* LayoutTests/platform/ios/accessibility/iframe-tree-update-with-dirty-layout-expected.txt: Added.
* LayoutTests/platform/mac-wk1/accessibility/iframe-tree-update-with-dirty-layout-expected.txt: Added.
* LayoutTests/accessibility/iframe-tree-update-with-dirty-layout-expected.txt: Added.
* LayoutTests/accessibility/iframe-tree-update-with-dirty-layout.html: Added.

* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::performDeferredCacheUpdate):
* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::forceAXObjectCacheUpdate const):
* Source/WebCore/testing/Internals.h:
* Source/WebCore/testing/Internals.idl:
---
 .../WebCore/accessibility/AXObjectCache.cpp   | 11 ++++
 Source/WebCore/accessibility/AXObjectCache.h  |  2 +-
 Source/WebCore/testing/Internals.cpp          |  6 ++
 Source/WebCore/testing/Internals.h            |  1 +
 Source/WebCore/testing/Internals.idl          |  1 +
 ...tree-update-with-dirty-layout-expected.txt | 45 ++++++++++++++
 .../iframe-tree-update-with-dirty-layout.html | 58 +++++++++++++++++++
 LayoutTests/platform/glib/TestExpectations    |  1 +
 LayoutTests/platform/ios/TestExpectations     |  1 +
 ...tree-update-with-dirty-layout-expected.txt | 25 ++++++++
 ...tree-update-with-dirty-layout-expected.txt | 41 +++++++++++++
 11 files changed, 191 insertions(+), 1 deletion(-)
 create mode 100644 LayoutTests/accessibility/iframe-tree-update-with-dirty-layout-expected.txt
 create mode 100644 LayoutTests/accessibility/iframe-tree-update-with-dirty-layout.html
 create mode 100644 LayoutTests/platform/ios/accessibility/iframe-tree-update-with-dirty-layout-expected.txt
 create mode 100644 LayoutTests/platform/mac-wk1/accessibility/iframe-tree-update-with-dirty-layout-expected.txt

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 2b9430fcd819..c29900887c5f 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -4046,6 +4046,17 @@ void AXObjectCache::performDeferredCacheUpdate(ForceLayout forceLayout)
         }
     }

+    RefPtr<Frame> frame = document->frame();

AG: RefPtr frame

+    if (frame && frame->isMainFrame()) {
+        // The layout of subframes must also be clean (assuming we're processing objects from those subframes), so force it here if necessary.
+        for (; frame; frame = frame->tree().traverseNext()) {
+            auto* localFrame = dynamicDowncast<LocalFrame>(frame.get());
+            RefPtr subDocument = localFrame ? localFrame->document() : nullptr;
+            if (subDocument && subDocument->view() && subDocument->view()->needsLayout())
+                subDocument->updateLayoutIgnorePendingStylesheets();

AG: should we do the same for RemoteFrames for site isolation?

+        }
+    }
+
     bool markedRelationsDirty = false;
     auto markRelationsDirty = [&] () {
         if (!markedRelationsDirty) {

[Tyler Wilcock]

Created attachment 470219 [details]
Patch

[Tyler Wilcock]

> we can still run into this bug. This patch addresses the issue by forcing
> all sub-frames to layout
> 
> AG: to layout -> to lay out ?
TW: Fixed.

> +    RefPtr<Frame> frame = document->frame();
>
> AG: RefPtr frame
TW: We need the type specifier unfortunately. Document::frame() returns a LocalFrame, but we need the type to be the superclass Frame because that's what FrameTree::traverseNext() returns. We could eliminate the type specifier if a method like FrameTree::traverseNextLocal() existed.

> subDocument->view()->needsLayout())
> +                subDocument->updateLayoutIgnorePendingStylesheets();
> 
> AG: should we do the same for RemoteFrames for site isolation?
TW: I think we're OK there. Because those RemoteFrames will be their own web content processes, they'll have their own main frames, and prior to this patch we already ensured that the main frame didn't have dirty layout before running this function.

[EWS]

Committed 275791@main (c69e4c0caaf5): <https://commits.webkit.org/275791@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 470219 [details].