Bug 244941 – AX: AXIsolatedTree and some objects within are not cleaned up upon page reload or navigation

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

AXIsolatedTree and some objects within are not cleaned up upon page reload or navigation

[Radar WebKit Bug Importer]

<rdar://problem/99708493>

[Tyler Wilcock]

rdar://99574478

[Tyler Wilcock]

Created attachment 462207 [details]
Patch

[chris fleizach]

Comment on attachment 462207 [details]
Patch

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

> COMMIT_MESSAGE:7
> +Prior to this patch, we never seem to prperly clean up any `AXIsolatedTree`s

properly

> COMMIT_MESSAGE:18
> +the trees representation in treeIDCache() and treePageCache().

tree's

> Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityController.idl:33
> +    AccessibilityUIElement getRetainedElement();

getRetainedElement() -> retainedElement() ?

[Tyler Wilcock]

Created attachment 462208 [details]
Patch

[Tyler Wilcock]

Created attachment 462209 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #4)
> Comment on attachment 462207 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=462207&action=review
> 
> > COMMIT_MESSAGE:7
> > +Prior to this patch, we never seem to prperly clean up any `AXIsolatedTree`s
> 
> properly
> 
> > COMMIT_MESSAGE:18
> > +the trees representation in treeIDCache() and treePageCache().
> 
> tree's
> 
> > Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityController.idl:33
> > +    AccessibilityUIElement getRetainedElement();
> 
> getRetainedElement() -> retainedElement() ?

Fixed all of these.

[Tyler Wilcock]

Created attachment 462216 [details]
Patch

[Andres Gonzalez]

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

--- a/LayoutTests/accessibility/wrapper-destroyed-on-reload.html
+++ a/LayoutTests/accessibility/wrapper-destroyed-on-reload.html

I would call this test wrapper-retained-through-reload.html which is what the test really does.

+    var testOutput = "This test ensures that when a page is reloaded, we properly clean up the wrappers and AX objects from the previous iteration of the page.\n\n";
+    // This is expected behavior because the wrappers and AX objects from the previous iteration of the page (pre-reload) refer
+    // to DOM, render objects, etc. that no longer exist post-reload.

I don't think this test ensures we clean up the wrappers. It ensures that if the wrapper is retained by a client, the AccessibilityController in the TestRunner, after the reload the wrapper is invalid, meaning it wraps no backing object.

The AccessibilityController is holding to an AccessibilityUIElement that in turn is retaining the wrapper m_element. So the test should be testing that m_element is invalid/defunct, meaning that it doesn't have a backing object.

+            // Note that this actually reloads the test, too, which means the testOutput above will be written once,
+            // then thrown away with the reload, then written again, upon which we should enter the `else` below.
+            internals.forceReload(true);

Doesn't the axButton variable get re-initialized too? Maybe not because it is const?

+            // The wrapper and AX object that back this AccessibilityUIElement should've been destroyed by the reload,
+            // so no role should be returned here.

I don't think the wrapper (m_element) could have been destroyed, it shouldn't have been destroyed. The AX backing object should. Maybe we need to add a method to the wrapper to determine whether it has a backing object or not. So not sure if by checking the role we can tell for sure whether the wrapper is detached or null.

+            testOutput += `Role of #button element retained before forced reload: ${accessibilityController.retainedElement().role}\n`;

Can we clarify this message to indicate that is after the forced reload? A bit confusing to me whether the "before" refers to the retained or to the reload.

[Tyler Wilcock]

Created attachment 462239 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #8)
> > Created attachment 462216 [details]
> > Patch
> Doesn't the axButton variable get re-initialized too? Maybe not because it
> is const?
It is re-initialized, but the AccessibilityController holds a reference to the original AccessibilityUIElement (which is what axButton is). Then when the page reloads, `const axButton = ...` is run again and a new and different AccessibilityUIElement is created.

[Tyler Wilcock]

Created attachment 462244 [details]
Patch

[Tyler Wilcock]

Created attachment 462245 [details]
Patch

[Tyler Wilcock]

Created attachment 462248 [details]
Patch

[Tyler Wilcock]

Created attachment 462292 [details]
Patch

[EWS]

Committed 254419@main (c808394fb05e): <https://commits.webkit.org/254419@main>

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