Bug 268239 – AX: Depending on the timing of Document::flushDeferredAXObjectCacheUpdate(), AXObjectCache::performDeferredCacheUpdate can be run with dirty layout

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

This causes incorrect accessibility tree updates.

[Radar WebKit Bug Importer]

<rdar://problem/121760427>

[Tyler Wilcock]

Created attachment 469575 [details]
Patch

[Tyler Wilcock]

Created attachment 469577 [details]
Patch

[chris fleizach]

Comment on attachment 469577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=469577&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:3980
> +        if (m_cacheUpdateDeferredCount >= 3 && !Accessibility::inRenderTreeOrStyleUpdate(document())) {

is it also an assertion condition that we wouldn't be rendering anything right now?

[Tyler Wilcock]

Created attachment 469580 [details]
Patch

[Tyler Wilcock]

Created attachment 469597 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXCoreObject.cpp b/Source/WebCore/accessibility/AXCoreObject.cpp
index 8f2b1827e33e..8749e3e775d8 100644
--- a/Source/WebCore/accessibility/AXCoreObject.cpp
+++ b/Source/WebCore/accessibility/AXCoreObject.cpp
@@ -28,6 +28,7 @@

 #include "config.h"
 #include "AXCoreObject.h"
+#include "LocalFrameView.h"

 namespace WebCore {

@@ -595,4 +596,16 @@ void AXCoreObject::appendRadioButtonGroupMembers(AccessibilityChildrenVector& li
     }
 }

+namespace Accessibility {
+
+bool inRenderTreeOrStyleUpdate(const Document& document)
+{
+    if (document.inStyleRecalc() || document.inRenderTreeUpdate())
+        return true;
+    auto* view = document.view();
+    return view && view->layoutContext().isInRenderTreeLayout();
+}
+
+} // namespace Accessibility
+
 } // namespace WebCore
diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h
index c95c64fcb2a6..763c064fa104 100644
--- a/Source/WebCore/accessibility/AXCoreObject.h
+++ b/Source/WebCore/accessibility/AXCoreObject.h
@@ -1662,6 +1662,8 @@ template<typename T, typename U> inline T retrieveAutoreleasedValueFromMainThrea
 }
 #endif

+bool inRenderTreeOrStyleUpdate(const Document&);
+
 } // namespace Accessibility

 inline bool AXCoreObject::isDescendantOfObject(const AXCoreObject* axObject) const
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 78a330da4c5c..bd3127dcf1d4 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -3965,15 +3965,6 @@ bool AXObjectCache::nodeIsTextControl(const Node* node)
     return axObject && axObject->isTextControl();
 }

-void AXObjectCache::performCacheUpdateTimerFired()
-{
-    // If there's a pending layout, let the layout trigger the AX update.
-    if (!document().view() || document().view()->needsLayout())
-        return;
-
-    performDeferredCacheUpdate();
-}
-
 void AXObjectCache::performDeferredCacheUpdate()
 {
     AXTRACE(makeString("AXObjectCache::performDeferredCacheUpdate 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
@@ -3983,6 +3974,23 @@ void AXObjectCache::performDeferredCacheUpdate()
     }
     SetForScope performingDeferredCacheUpdate(m_performingDeferredCacheUpdate, true);

+    // It's unexpected for this function to run in the middle of a render tree or style update.
+    ASSERT(!Accessibility::inRenderTreeOrStyleUpdate(document()));

AG: is Accessibility::inRenderTreeOrStyleUpdate equivalent to layout dirty? If so, we know we'll hit this assert in some cases, because that's the bug we are dealing with.

+
+    if (!document().view() || document().view()->needsLayout()) {
+        // Layout became dirty while waiting to performDeferredCacheUpdate, and we require clean layout
+        // to update the accessibility tree correctly in this function.
+        if (m_cacheUpdateDeferredCount >= 3 && !Accessibility::inRenderTreeOrStyleUpdate(document())) {
+            // Layout is being thrashed before we get a chance to update, so stop waiting and just force it.
+            m_cacheUpdateDeferredCount = 0;
+            document().updateLayoutIgnorePendingStylesheets();

AG: we are doing this when there is no view, unlike before this change. Does that matter? or we should return if there is now view.

AG: why not to do document().updateLayoutIgnorePendingStylesheets the first time? I.e., if it is good after three attempts, why not do it in the first try.

+        } else {
+            // Wait for layout to trigger another async cache update.
+            ++m_cacheUpdateDeferredCount;
+            return;
+        }
+    }
+
     bool markedRelationsDirty = false;
     auto markRelationsDirty = [&] () {
         if (!markedRelationsDirty) {
diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index 3e806cb1a840..c819f3814d29 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -587,7 +587,7 @@ private:

     void focusCurrentModal();

-    void performCacheUpdateTimerFired();
+    void performCacheUpdateTimerFired() { performDeferredCacheUpdate(); }

     void postTextStateChangeNotification(AccessibilityObject*, const AXTextStateChangeIntent&, const VisibleSelection&);

@@ -734,6 +734,8 @@ private:
     bool m_performingDeferredCacheUpdate { false };
     double m_loadingProgress { 0 };

+    unsigned m_cacheUpdateDeferredCount { 0 };
+
     // Relationships between objects.
     HashMap<AXID, AXRelations> m_relations;
     bool m_relationsNeedUpdate { true };

[Andres Gonzalez]

diff --git a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
index 0c272a4954c7..de7cc173d7af 100644
--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
@@ -1112,8 +1112,11 @@ bool AccessibilityRenderObject::computeAccessibilityIsIgnored() const

     if (WeakPtr renderText = dynamicDowncast<RenderText>(m_renderer.get())) {
         // Text elements with no rendered text, or only whitespace should not be part of the AX tree.
-        if (!renderText->hasRenderedText())
+        if (!renderText->hasRenderedText()) {
+            // Layout must be clean to make the right decision here (because hasRenderedText() can return false solely because layout is dirty).
+            ASSERT(!renderText->needsLayout() || !renderText->text().length());

AG: This is the cause of the bug, right? Not clear to me how we are preventing this from happening with the change in the performDeferredCacheUpdate.

             return true;
+        }

         if (renderText->text().containsOnly<isASCIIWhitespace>()) {
 #if ENABLE(AX_THREAD_TEXT_APIS)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 8f8605257851..47d153f944ed 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -144,7 +144,7 @@ Ref<AXIsolatedTree> AXIsolatedTree::create(AXObjectCache& axObjectCache)
     }

     auto& document = axObjectCache.document();
-    if (!document.view()->layoutContext().isInRenderTreeLayout() && !document.inRenderTreeUpdate() && !document.inStyleRecalc())
+    if (!Accessibility::inRenderTreeOrStyleUpdate(document))
         document.updateLayoutIgnorePendingStylesheets();

     tree->m_maxTreeDepth = document.settings().maximumHTMLParserDOMTreeDepth();

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Tyler Wilcock from comment #6)
> +    // It's unexpected for this function to run in the middle of a render
> tree or style update.
> +    ASSERT(!Accessibility::inRenderTreeOrStyleUpdate(document()));
> 
> AG: is Accessibility::inRenderTreeOrStyleUpdate equivalent to layout dirty?
> If so, we know we'll hit this assert in some cases, because that's the bug
> we are dealing with.
TW: They're not equivalent. inRenderTreeOrStyleUpdate is true when we're in the
middle of a render tree update (i.e. actively tearing down or creating new renderers) or style recalculation. Dirty layout is a flag only indicating layout needs to be recomputed, not that anything is actively in progress, so they're different states. Layout must not happen when either of these two processes are in progress, and it's also unexpected that we ever call performDeferredCacheUpdate while either are in progress.

> +    if (!document().view() || document().view()->needsLayout()) {
> +        // Layout became dirty while waiting to performDeferredCacheUpdate,
> and we require clean layout
> +        // to update the accessibility tree correctly in this function.
> +        if (m_cacheUpdateDeferredCount >= 3 &&
> !Accessibility::inRenderTreeOrStyleUpdate(document())) {
> +            // Layout is being thrashed before we get a chance to update,
> so stop waiting and just force it.
> +            m_cacheUpdateDeferredCount = 0;
> +            document().updateLayoutIgnorePendingStylesheets();
> 
> AG: we are doing this when there is no view, unlike before this change. Does
> that matter? or we should return if there is now view.
TW: Good point! I think we should just return if there is no view.

> AG: why not to do document().updateLayoutIgnorePendingStylesheets the first
> time? I.e., if it is good after three attempts, why not do it in the first
> try.
TW: Layout is expensive, so the idea with this was to "latch on" to a layout that already happened if it's possible to do so rather than triggering one ourselves. This also increases de-duplication opportunities. But if it gets continuously thrashed between clean and dirty and we can't ride on the back of a clean layout, then we force one because we have to actually update eventually.

But in retrospect, maybe this behavior is confusing...the function name is "performDeferredCacheUpdate", so it would be confusing for callers to use it expecting the AX tree state to be up-to-date afterwards, but it actually got deferred due to dirty layout. What do you think is the best route forward?

>      if (WeakPtr renderText = dynamicDowncast<RenderText>(m_renderer.get())) {
>          // Text elements with no rendered text, or only whitespace should not be part of the AX tree.
> -        if (!renderText->hasRenderedText())
> +        if (!renderText->hasRenderedText()) {
> +            // Layout must be clean to make the right decision here (because hasRenderedText() can return false solely because layout is dirty).
> +            ASSERT(!renderText->needsLayout() || !renderText->text().length());
> 
> AG: This is the cause of the bug, right? Not clear to me how we are preventing this from happening with the change in the performDeferredCacheUpdate.
TW: Before, we used to guarantee that layout was clean before handling deferred children change updates (critical in updating the isolated tree correctly), https://bugs.webkit.org/show_bug.cgi?id=256403 broke that guarantee. This patch fixes it.

It is, however, true that `computeAccessibilityIsIgnored` can be called at any time, i.e. outside of performDeferredCacheUpdate where we again guarantee layout is clean. But this problem also existed before https://bugs.webkit.org/show_bug.cgi?id=256403, so this patch at least moves us back in the right direction. Guaranteeing layout is clean from all the places computeAccessibilityIsIgnored can be called is something we should tackle in the future (is it possible?)

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> (In reply to Andres Gonzalez from comment #7)
> > (In reply to Tyler Wilcock from comment #6)
> > +    // It's unexpected for this function to run in the middle of a render
> > tree or style update.
> > +    ASSERT(!Accessibility::inRenderTreeOrStyleUpdate(document()));
> > 
> > AG: is Accessibility::inRenderTreeOrStyleUpdate equivalent to layout dirty?
> > If so, we know we'll hit this assert in some cases, because that's the bug
> > we are dealing with.
> TW: They're not equivalent. inRenderTreeOrStyleUpdate is true when we're in
> the
> middle of a render tree update (i.e. actively tearing down or creating new
> renderers) or style recalculation. Dirty layout is a flag only indicating
> layout needs to be recomputed, not that anything is actively in progress, so
> they're different states. Layout must not happen when either of these two
> processes are in progress, and it's also unexpected that we ever call
> performDeferredCacheUpdate while either are in progress.

AG: thanks for the clarification.

> 
> > +    if (!document().view() || document().view()->needsLayout()) {
> > +        // Layout became dirty while waiting to performDeferredCacheUpdate,
> > and we require clean layout
> > +        // to update the accessibility tree correctly in this function.
> > +        if (m_cacheUpdateDeferredCount >= 3 &&
> > !Accessibility::inRenderTreeOrStyleUpdate(document())) {
> > +            // Layout is being thrashed before we get a chance to update,
> > so stop waiting and just force it.
> > +            m_cacheUpdateDeferredCount = 0;
> > +            document().updateLayoutIgnorePendingStylesheets();
> > 
> > AG: we are doing this when there is no view, unlike before this change. Does
> > that matter? or we should return if there is now view.
> TW: Good point! I think we should just return if there is no view.
> 
> > AG: why not to do document().updateLayoutIgnorePendingStylesheets the first
> > time? I.e., if it is good after three attempts, why not do it in the first
> > try.
> TW: Layout is expensive, so the idea with this was to "latch on" to a layout
> that already happened if it's possible to do so rather than triggering one
> ourselves. This also increases de-duplication opportunities. But if it gets
> continuously thrashed between clean and dirty and we can't ride on the back
> of a clean layout, then we force one because we have to actually update
> eventually.
> 
> But in retrospect, maybe this behavior is confusing...the function name is
> "performDeferredCacheUpdate", so it would be confusing for callers to use it
> expecting the AX tree state to be up-to-date afterwards, but it actually got
> deferred due to dirty layout. What do you think is the best route forward?

AG: Could you please add a comment explaining this rationale in this patch? I think there are some open questions in this issue that we need to think about. The reason this call was moved from the post layout task to an event queue is because some of our processing in this method can trigger JS execution, and that in turn can trigger a layout. So, what would it happen if that is still happening in the middle of the execution of this method? Wouldn't be better to avoid triggering any JS, and thus be able to run as a post layout task?

> 
> >      if (WeakPtr renderText = dynamicDowncast<RenderText>(m_renderer.get())) {
> >          // Text elements with no rendered text, or only whitespace should not be part of the AX tree.
> > -        if (!renderText->hasRenderedText())
> > +        if (!renderText->hasRenderedText()) {
> > +            // Layout must be clean to make the right decision here (because hasRenderedText() can return false solely because layout is dirty).
> > +            ASSERT(!renderText->needsLayout() || !renderText->text().length());
> > 
> > AG: This is the cause of the bug, right? Not clear to me how we are preventing this from happening with the change in the performDeferredCacheUpdate.
> TW: Before, we used to guarantee that layout was clean before handling
> deferred children change updates (critical in updating the isolated tree
> correctly), https://bugs.webkit.org/show_bug.cgi?id=256403 broke that
> guarantee. This patch fixes it.
> 
> It is, however, true that `computeAccessibilityIsIgnored` can be called at
> any time, i.e. outside of performDeferredCacheUpdate where we again
> guarantee layout is clean. But this problem also existed before
> https://bugs.webkit.org/show_bug.cgi?id=256403, so this patch at least moves
> us back in the right direction. Guaranteeing layout is clean from all the
> places computeAccessibilityIsIgnored can be called is something we should
> tackle in the future (is it possible?)

AG: the fact that we can trigger a layout just by checking whether an object should be ignored or not, it is troublesome. So I think we are going to address that root problem.

[Tyler Wilcock]

Created attachment 469622 [details]
Patch

[Tyler Wilcock]

> > (In reply to Andres Gonzalez from comment #7)
> > > +    if (!document().view() || document().view()->needsLayout()) {
> > > +        // Layout became dirty while waiting to performDeferredCacheUpdate,
> > > and we require clean layout
> > > +        // to update the accessibility tree correctly in this function.
> > > +        if (m_cacheUpdateDeferredCount >= 3 &&
> > > !Accessibility::inRenderTreeOrStyleUpdate(document())) {
> > > +            // Layout is being thrashed before we get a chance to update,
> > > so stop waiting and just force it.
> > > +            m_cacheUpdateDeferredCount = 0;
> > > +            document().updateLayoutIgnorePendingStylesheets();
> > > 
> > > AG: why not to do document().updateLayoutIgnorePendingStylesheets the first
> > > time? I.e., if it is good after three attempts, why not do it in the first
> > > try.
> > TW: Layout is expensive, so the idea with this was to "latch on" to a layout
> > that already happened if it's possible to do so rather than triggering one
> > ourselves. This also increases de-duplication opportunities. But if it gets
> > continuously thrashed between clean and dirty and we can't ride on the back
> > of a clean layout, then we force one because we have to actually update
> > eventually.
> > 
> > But in retrospect, maybe this behavior is confusing...the function name is
> > "performDeferredCacheUpdate", so it would be confusing for callers to use it
> > expecting the AX tree state to be up-to-date afterwards, but it actually got
> > deferred due to dirty layout. What do you think is the best route forward?
> 
> AG: Could you please add a comment explaining this rationale in this patch?
> I think there are some open questions in this issue that we need to think
> about. The reason this call was moved from the post layout task to an event
> queue is because some of our processing in this method can trigger JS
> execution, and that in turn can trigger a layout. So, what would it happen
> if that is still happening in the middle of the execution of this method?
> Wouldn't be better to avoid triggering any JS, and thus be able to run as a
> post layout task?
TW: Added this rationale to the commit message! I also added a ForceLayout flag since the last patch, allowing callers to explicitly opt in to or out of this deferral behavior, addressing the problem I mentioned above (it would otherwise be confusing for some callers for the AX tree not to be updated after performDeferredCacheUpdate). Let me know if you don't agree with this or have some other idea here.

You said "Wouldn't it be better to avoid triggering any JS, and thus be able to run as a post layout task?"

Possibly, but that could bring other problems. One way we can trigger JS through the cache update AXObjectCache::focusCurrentModal() by calling AccessibilityNodeObject::setFocused(). We would have to make just this part async, which might come with unexpected focusing behavior for users, e.g. trampling author focus. There  may be other ways we can trigger JS that we'd have to account for too, not sure.

[EWS]

Committed 273844@main (0025697e6026): <https://commits.webkit.org/273844@main>

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