Bug 257071

Summary: AX: WebAccessibilityObjectWrapperBase and WKAccessibilityWebPageObjectBase should hold WeakPtrs to their associated backing objects
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, benjamin, cdumez, cfleizach, cmarcelo, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Tyler Wilcock
Reported 2023-05-19 18:17:47 PDT
Continue the transition to ubiquitous smart pointer usage per https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines.
Attachments
Patch (3.67 KB, patch)
2023-05-19 18:20 PDT, Tyler Wilcock 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (4.56 KB, patch)
2023-05-19 18:36 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (6.97 KB, patch)
2023-05-20 00:41 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (5.66 KB, patch)
2023-05-22 10:53 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (7.56 KB, patch)
2023-05-22 17:49 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2023-05-19 18:17:59 PDT
<rdar://problem/109591802>
Tyler Wilcock
Comment 2 2023-05-19 18:19:16 PDT
rdar://109586801
Tyler Wilcock
Comment 3 2023-05-19 18:20:18 PDT
Created attachment 466429 [details] Patch
Tyler Wilcock
Comment 4 2023-05-19 18:36:47 PDT
Created attachment 466430 [details] Patch
Tyler Wilcock
Comment 5 2023-05-20 00:41:59 PDT
Created attachment 466434 [details] Patch
Andres Gonzalez
Comment 6 2023-05-22 08:08:13 PDT
(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
Comment 7 2023-05-22 10:53:05 PDT
Created attachment 466449 [details] Patch
Tyler Wilcock
Comment 8 2023-05-22 17:49:17 PDT
Created attachment 466455 [details] Patch
Andres Gonzalez
Comment 9 2023-05-22 19:52:16 PDT
(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
Comment 10 2023-05-23 04:28:22 PDT
(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
Comment 11 2023-05-23 11:11:29 PDT
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].
Note You need to log in before you can comment on or make changes to this bug.