Bug 260389 – AX: Improve smart pointer usage in AXObjectCache

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

Improve the usage of smart pointers in AXObjectCache

[Radar WebKit Bug Importer]

<rdar://problem/114090467>

[Joshua Hoffman]

rdar://113803539

[Joshua Hoffman]

Created attachment 467328 [details]
Patch

[Tyler Wilcock]

Comment on attachment 467328 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:2291
> +    RefPtr protectedElement { element };
> +
>      if (nodeAndRendererAreValid(element) && rendererNeedsDeferredUpdate(*element->renderer())) {
> -        m_deferredAttributeChange.append({ element, attrName, oldValue, newValue });
> +        m_deferredAttributeChange.append({ protectedElement, attrName, oldValue, newValue });
>          if (!m_performCacheUpdateTimer.isActive())
>              m_performCacheUpdateTimer.startOneShot(0_s);
>          AXLOG(makeString("Deferring handling of attribute ", attrName.localName().string(), " for element ", element->debugDescription()));
>          return;
>      }
> -    handleAttributeChange(element, attrName, oldValue, newValue);
> +    handleAttributeChange(protectedElement.get(), attrName, oldValue, newValue);

I don't think we need to protect this element with a RefPtr if !rendererNeedsDeferredUpdate(*element->renderer()) (because otherwise we append it to m_deferredAttributeChange which stores it as a WeakPtr). I think we only need to protect it if we're going to directly call handleAttributeChange with it.

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 467328 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467328&action=review
> 
> > -    handleAttributeChange(element, attrName, oldValue, newValue);
> > +    handleAttributeChange(protectedElement.get(), attrName, oldValue, newValue);
> 
> I don't think we need to protect this element with a RefPtr if
> !rendererNeedsDeferredUpdate(*element->renderer()) (because otherwise we
> append it to m_deferredAttributeChange which stores it as a WeakPtr). I
> think we only need to protect it if we're going to directly call
> handleAttributeChange with it.

Gotcha—yeah that makes sense. I can move the refptr protection to before the handleAttributeChange call.

[Joshua Hoffman]

Created attachment 467330 [details]
Patch

[EWS]

Committed 267064@main (4e6fd1fb5461): <https://commits.webkit.org/267064@main>

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

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #6)
> Created attachment 467330 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp

@@ -2178,10 +2178,10 @@ void AXObjectCache::handleActiveDescendantChange(Element& element, const AtomStr
     }

     // Handle active-descendant changes when the target allows for it, or the controlled object allows for it.
-    AccessibilityObject* target = nullptr;
+    RefPtr<AccessibilityObject> target { nullptr };

The RefPtr default constructor initializes the pointer to nullptr so the initializer list is unnecessary here.