Bug 243920

Summary:

AX: Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface

Product:

WebKit

Reporter:

Tyler Wilcock <tyler_w>

Component:

Accessibility

Assignee:

Tyler Wilcock <tyler_w>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, hi, jcraig, jdiggs, joepeck, pangle, samuel_white, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Description Tyler Wilcock 2022-08-13 14:08:33 PDT

Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface

Comment 1 Radar WebKit Bug Importer 2022-08-13 14:08:43 PDT

<rdar://problem/98622522>

Comment 2 Tyler Wilcock 2022-08-13 14:14:44 PDT

Created attachment 461599 [details]
Patch

Comment 3 Tyler Wilcock 2022-08-14 19:01:14 PDT

Created attachment 461628 [details]
Patch

Comment 4 Tyler Wilcock 2022-08-14 19:13:08 PDT

Created attachment 461629 [details]
Patch

Comment 5 chris fleizach 2022-08-14 22:57:37 PDT

Comment on attachment 461629 [details]
Patch

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

> Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp:885
>  
> +    auto* liveObject = dynamicDowncast<AccessibilityObject>(m_coreObject);

do we need to check that this is not nil in all these places below? we're already checking !m_coreObject above. can we combine those checks so we don' have to modify more code below

> Source/WebCore/inspector/InspectorAuditAccessibilityObject.cpp:-178
> -        computedProperties.hidden = axObject->isHidden();

should the Audit class only be dealing in AccessibilityObjects instead of AXCoreObject?

Comment 6 Tyler Wilcock 2022-08-15 20:08:06 PDT

Created attachment 461660 [details]
Patch

Comment 7 Tyler Wilcock 2022-08-15 20:13:59 PDT

(In reply to chris fleizach from comment #5)
> Comment on attachment 461629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=461629&action=review
> 
> > Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp:885
> >  
> > +    auto* liveObject = dynamicDowncast<AccessibilityObject>(m_coreObject);
> 
> do we need to check that this is not nil in all these places below? we're
> already checking !m_coreObject above. can we combine those checks so we don'
> have to modify more code below
Taking your suggestion would involve changing every use of m_coreObject to liveObject in all of these functions, which feels like a big change (it wouldn't make sense to early-null-check-and-return liveObject and then use m_coreObject throughout the rest of the function).

What's in the patch now is the most minimally invasive way of changing the wrapper to handle this change, I think. In an ideal world, having this wrapper hold either AccessibilityObject* or AXIsolatedObject* (rather than AXCoreObject*) would prevent this problem.

> > Source/WebCore/inspector/InspectorAuditAccessibilityObject.cpp:-178
> > -        computedProperties.hidden = axObject->isHidden();
> 
> should the Audit class only be dealing in AccessibilityObjects instead of
> AXCoreObject?
Yes, good call.

Comment 8 EWS 2022-08-16 22:36:28 PDT

Committed 253506@main (864652524aa0): <https://commits.webkit.org/253506@main>

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