Bug 254798

Summary: AXObjectCache::characterOffsetFromVisiblePosition can deref a nullptr when underlying renderer is destroyed
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Tyler Wilcock 2023-03-30 23:41:56 PDT 
AXObjectCache::characterOffsetFromVisiblePosition creates an AX object from the node backing a VisiblePosition at the beginning of the method. Then, it does non-trivial work that could cause the renderer backing the AX object to be destroyed, and afterwards unconditionally deferences that AX object's node() at the end of the method. This can cause a null pointer dereference crash (because AccessibilityRenderObject::node() depends on a non-null renderer), and is generally poor pointer hygiene.
Comment 1 Radar WebKit Bug Importer 2023-03-30 23:42:08 PDT 
<rdar://problem/107459184>
Comment 2 Tyler Wilcock 2023-03-30 23:42:54 PDT 
rdar://103456792
Comment 3 Tyler Wilcock 2023-03-30 23:47:09 PDT 
Created attachment 465696 [details]
Patch
Comment 4 chris fleizach 2023-03-31 10:46:43 PDT 
Do we still want to check that this is NOT null first?

 deepPos.deprecatedNode();
Comment 5 Tyler Wilcock 2023-03-31 10:56:30 PDT 
(In reply to chris fleizach from comment #4)
> Do we still want to check that this is NOT null first?
> 
>  deepPos.deprecatedNode();
We should be safe because if `deepPos.deprecatedNode()` were null, this check just above dereferencing it would return:

if (visiblePos.isNull())
   return CharacterOffset();
Comment 6 EWS 2023-03-31 11:58:15 PDT 
Committed 262432@main (7d93b07962d5): <https://commits.webkit.org/262432@main>

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