Bug 260389 - AX: Improve smart pointer usage in AXObjectCache
Summary: AX: Improve smart pointer usage in AXObjectCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-08-18 09:14 PDT by Joshua Hoffman
Modified: 2023-08-21 07:20 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.68 KB, patch)
2023-08-18 10:55 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (4.17 KB, patch)
2023-08-18 11:19 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Hoffman 2023-08-18 09:14:13 PDT
Improve the usage of smart pointers in AXObjectCache
Comment 1 Radar WebKit Bug Importer 2023-08-18 09:14:25 PDT
<rdar://problem/114090467>
Comment 2 Joshua Hoffman 2023-08-18 09:14:57 PDT
rdar://113803539
Comment 3 Joshua Hoffman 2023-08-18 10:55:26 PDT
Created attachment 467328 [details]
Patch
Comment 4 Tyler Wilcock 2023-08-18 11:09:14 PDT
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.
Comment 5 Joshua Hoffman 2023-08-18 11:14:35 PDT
(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.
Comment 6 Joshua Hoffman 2023-08-18 11:19:09 PDT
Created attachment 467330 [details]
Patch
Comment 7 EWS 2023-08-18 19:33:40 PDT
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].
Comment 8 Andres Gonzalez 2023-08-21 07:20:34 PDT
(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.