Bug 257071 – AX: WebAccessibilityObjectWrapperBase and WKAccessibilityWebPageObjectBase should hold WeakPtrs to their associated backing objects

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Continue the transition to ubiquitous smart pointer usage per https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines.

[Radar WebKit Bug Importer]

<rdar://problem/109591802>

[Tyler Wilcock]

rdar://109586801

[Tyler Wilcock]

Created attachment 466429 [details]
Patch

[Tyler Wilcock]

Created attachment 466430 [details]
Patch

[Tyler Wilcock]

Created attachment 466434 [details]
Patch

[Andres Gonzalez]

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

--- a/Source/WTF/wtf/ThreadSafeWeakPtr.h
+++ b/Source/WTF/wtf/ThreadSafeWeakPtr.h

+    size_t strongRefCount() const
+    {
+        Locker locker { m_lock };
+        return m_strongReferenceCount;
+    }
+
We shouldn't do this because immediately after releasing the lock, this value can change, so the returned value is meaningless as pointed out by @Alex Christensen in a previous review.

[Tyler Wilcock]

Created attachment 466449 [details]
Patch

[Tyler Wilcock]

Created attachment 466455 [details]
Patch

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm

@@ -377,11 +377,11 @@ - (id)attachmentView
 - (WebCore::AXCoreObject*)axBackingObject
 {
     if (isMainThread())
-        return m_axObject;
+        return m_axObject.get();

 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
     ASSERT(AXObjectCache::isIsolatedTreeEnabled());
-    return m_isolatedObject;
+    return m_isolatedObject.get().get();

I think we need to change this method to return a RefPtr instead of a raw pointer. Between the

+    return m_isolatedObject.get().get();

and the initialization of the RefPtr in the caller (e.g., accessibilityAttributeValue), this guy can go null.
The same applies to updateObjectBackingStore.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #8)
> > Created attachment 466455 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
> +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
> 
> @@ -377,11 +377,11 @@ - (id)attachmentView
>  - (WebCore::AXCoreObject*)axBackingObject
>  {
>      if (isMainThread())
> -        return m_axObject;
> +        return m_axObject.get();
> 
>  #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
>      ASSERT(AXObjectCache::isIsolatedTreeEnabled());
> -    return m_isolatedObject;
> +    return m_isolatedObject.get().get();
> 
> I think we need to change this method to return a RefPtr instead of a raw
> pointer. Between the
> 
> +    return m_isolatedObject.get().get();
> 
> and the initialization of the RefPtr in the caller (e.g.,
> accessibilityAttributeValue), this guy can go null.
> The same applies to updateObjectBackingStore.

We'll do this in a separate patch to minimize risk.

[EWS]

Committed 264429@main (9c6ff227993d): <https://commits.webkit.org/264429@main>

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