AX: AccessibilityScrollView should be resilient to destruction of underlying scroll view

[Tyler Wilcock]

Sometimes, we can get into a state where we have a valid AccessibilityScrollView, backed by an iframe, with a destroyed WeakPtr<ScrollView> m_scrollView. When this happens, all of the content inside the iframe becomes inaccessible. We need to handle the case where an iframe changes the frame it holds (e.g. respect the current frame in HTMLFrameOwner::contentFrame, which can change).

[Radar WebKit Bug Importer]

<rdar://problem/89066221>

[Tyler Wilcock]

Created attachment 452311 [details]
Patch

[chris fleizach]

patch looks good. is there any way to test this?

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AccessibilityScrollView.cpp
+++ a/Source/WebCore/accessibility/AccessibilityScrollView.cpp

+AXObjectCache* AccessibilityScrollView::currentAxObjectCache() const
+{
+    if (auto* cache = axObjectCache())
+        return cache;
+
+    // It's possible that the document associated with `this` was destroyed (and thus axObjectCache() will return null),
+    // but also that `this` still represents a valid frame owner (e.g. iframe). Return the cache of the iframe in this case.
+    return m_frameOwnerElement ? m_frameOwnerElement->document().axObjectCache() : nullptr;
+}

Can we just override AXCoreObject::axObjectCache instead of adding a new method? 

The comment here only makes sense for the reader who knows the implementation of AccessibilityObject::axObjectCache. this could be written more succinctly and clearer like:

    auto* document = this->document();
    return document ? document->axObjectCache() :
        m_frameOwnerElement ? m_frameOwnerElement->document().axObjectCache() : nullptr;

Or perhaps better yet to override document(), and then you wouldn't need to override axObjectCache().

+Document* AccessibilityScrollView::contentDocument() const
+{
+    if (m_frameOwnerElement)
+        return m_frameOwnerElement->contentDocument();
+    return nullptr;
+}

do we need this method separate from document()? Or we can just override document()?

If we need this, it could be written in two lines with the ? :, which I think it is better.

+FrameView* AccessibilityScrollView::contentDocumentFrameView() const
+{
+    if (auto* document = contentDocument())
+        return document->view();
+    return nullptr;
+}

Can we override documentFrameView() instead?

@@ -157,7 +194,7 @@ AccessibilityScrollbar* AccessibilityScrollView::addChildScrollbar(Scrollbar* sc
     if (!scrollbar)
         return nullptr;

-    AXObjectCache* cache = axObjectCache();
+    AXObjectCache* cache = currentAxObjectCache();

No need for this if you override axObjectCache().


 AccessibilityObject* AccessibilityScrollView::webAreaObject() const
 {
-    if (!is<FrameView>(m_scrollView))
-        return nullptr;
+    auto* document = contentDocument();
+    if (is<FrameView>(m_scrollView))
+        document = downcast<FrameView>(*m_scrollView).frame().document();

-    Document* document = downcast<FrameView>(*m_scrollView).frame().document();
     if (!document || !document->hasLivingRenderTree())
         return nullptr;

-    if (AXObjectCache* cache = axObjectCache())
+    if (auto* cache = currentAxObjectCache())
         return cache->getOrCreate(document);
I think this can be simplified with the right overrides.

 AccessibilityObject* AccessibilityScrollView::parentObject() const
 {
-    if (!is<FrameView>(m_scrollView))
+    if (!is<FrameView>(m_scrollView)) {
+        if (m_frameOwnerElement) {
+            if (auto* cache = m_frameOwnerElement->document().axObjectCache())
+                return cache->getOrCreate(m_frameOwnerElement.get());
+        }
         return nullptr;
+    }

     AXObjectCache* cache = axObjectCache();
     if (!cache)

Can this be simplified with the right overrides? i.e., no need to get the cache in two different places.

 ScrollableArea* AccessibilityScrollView::getScrollableAreaIfScrollable() const
 {
-    return m_scrollView.get();
+    return currentScrollView();
 }

Can we inline this in the header like scrollView() for consistency?

--- a/Source/WebCore/accessibility/AccessibilityScrollView.h
+++ a/Source/WebCore/accessibility/AccessibilityScrollView.h

+    // Returns the document for m_frameOwnerElement's current `contentFrame` (if present).
+    Document* contentDocument() const;
+    FrameView* contentDocumentFrameView() const;
...
+    AXObjectCache* currentAxObjectCache() const;

Do we need these, or we can just override document() and documentFrameView()?

[Tyler Wilcock]

Created attachment 452400 [details]
Patch

[Tyler Wilcock]

Fixed all of your comments. Still trying to get a minimal test case reproducing the issue.

[Andres Gonzalez]

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

 bool AccessibilityScrollView::isAttachment() const
 {
-    return m_scrollView && m_scrollView->platformWidget();
+    if (auto* scrollView = currentScrollView())
+        return scrollView->platformWidget();
+    return false;

I personally prefer the two line version:

auto* scrollView = currentScrollView();
return scrollView ? scrollView->platformWidget() : false;

But not big deal.

 PlatformWidget AccessibilityScrollView::platformWidget() const
 {
-    return m_scrollView ? m_scrollView->platformWidget() : nullptr;
+    if (auto* scrollView = currentScrollView())
+        return scrollView->platformWidget();
+    return nullptr;
 }

Same here:

auto* scrollView = currentScrollView();
return scrollView ? scrollView->platformWidget() : nullptr;

[Tyler Wilcock]

Created attachment 452465 [details]
Minimal reproduction of issue

[EWS]

Committed r290130 (247474@main): <https://commits.webkit.org/247474@main>

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