Bug 241650 – AX: AccessibilityObject::insertChild does not check the validity of the insertionIndex while processing grandchildren

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

This can cause crashes.

[Radar WebKit Bug Importer]

<rdar://problem/95240529>

[Tyler Wilcock]

Created attachment 460262 [details]
Patch

[Tyler Wilcock]

rdar://94895437

[chris fleizach]

Comment on attachment 460262 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:636
> +                    if (insertionIndex > m_children.size())

do we want to insert this at position 0 in this case? or discard completely?

[Tyler Wilcock]

(In reply to chris fleizach from comment #4)
> Comment on attachment 460262 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460262&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:636
> > +                    if (insertionIndex > m_children.size())
> 
> do we want to insert this at position 0 in this case? or discard completely?
I think we want to discard this grandchild (and any following) entirely, since they may no longer be the right children after the layout that caused m_children to be cleared.

Also, when this happens, m_childrenInitialized should always become false (it does in the crash I was chasing down), meaning we will add the actually-correct children in the next call to children(true).

[Andres Gonzalez]

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

It seems that what we are doing in this whole block is to insert the grandchildren if the newChild is ignored. Would it make it clearer if add a method called insertchildren(const Vector&, size_t index), and then we could write in the body of this block: 
    if (descendIfIgnored == DescendIfIgnored::Yes
        && child->accessibilityIsIgnored()) 
        insertChildren(child->children(), index);

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 460262 [details]
> > Patch
> 
> It seems that what we are doing in this whole block is to insert the
> grandchildren if the newChild is ignored. Would it make it clearer if add a
> method called insertchildren(const Vector&, size_t index), and then we could
> write in the body of this block: 
>     if (descendIfIgnored == DescendIfIgnored::Yes
>         && child->accessibilityIsIgnored()) 
>         insertChildren(child->children(), index);
I tried this out and unfortunately we'd also need to pass a third AXAncestorFlags parameter (to capture the flags of the ignored object), which IMO makes this refactor not worth it.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #7)
> (In reply to Andres Gonzalez from comment #6)
> > (In reply to Tyler Wilcock from comment #2)
> > > Created attachment 460262 [details]
> > > Patch
> > 
> > It seems that what we are doing in this whole block is to insert the
> > grandchildren if the newChild is ignored. Would it make it clearer if add a
> > method called insertchildren(const Vector&, size_t index), and then we could
> > write in the body of this block: 
> >     if (descendIfIgnored == DescendIfIgnored::Yes
> >         && child->accessibilityIsIgnored()) 
> >         insertChildren(child->children(), index);
> I tried this out and unfortunately we'd also need to pass a third
> AXAncestorFlags parameter (to capture the flags of the ignored object),
> which IMO makes this refactor not worth it.

No, you can do

    auto ancestorFlags = computeAncestorFlags();

inside insertObjects(...), no need to pass it as a param. Changed the name from insertchildren to insertObjects because in this case you would be inserting the grandChildren. I believe it would make the code maintainable.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #8)
> (In reply to Tyler Wilcock from comment #7)
> > (In reply to Andres Gonzalez from comment #6)
> > > (In reply to Tyler Wilcock from comment #2)
> > > > Created attachment 460262 [details]
> > > > Patch
> > > 
> > > It seems that what we are doing in this whole block is to insert the
> > > grandchildren if the newChild is ignored. Would it make it clearer if add a
> > > method called insertchildren(const Vector&, size_t index), and then we could
> > > write in the body of this block: 
> > >     if (descendIfIgnored == DescendIfIgnored::Yes
> > >         && child->accessibilityIsIgnored()) 
> > >         insertChildren(child->children(), index);
> > I tried this out and unfortunately we'd also need to pass a third
> > AXAncestorFlags parameter (to capture the flags of the ignored object),
> > which IMO makes this refactor not worth it.
> 
> No, you can do
> 
>     auto ancestorFlags = computeAncestorFlags();
> 
> inside insertObjects(...), no need to pass it as a param. Changed the name
> from insertchildren to insertObjects because in this case you would be
> inserting the grandChildren. I believe it would make the code maintainable.

The name insertChildren is fine, thought for a moment that insertObjects was better but it is not, since you are inserting children that in this case happen to be grandchildren.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #8)
> (In reply to Tyler Wilcock from comment #7)
> > (In reply to Andres Gonzalez from comment #6)
> > > (In reply to Tyler Wilcock from comment #2)
> > > > Created attachment 460262 [details]
> > > > Patch
> > > 
> > > It seems that what we are doing in this whole block is to insert the
> > > grandchildren if the newChild is ignored. Would it make it clearer if add a
> > > method called insertchildren(const Vector&, size_t index), and then we could
> > > write in the body of this block: 
> > >     if (descendIfIgnored == DescendIfIgnored::Yes
> > >         && child->accessibilityIsIgnored()) 
> > >         insertChildren(child->children(), index);
> > I tried this out and unfortunately we'd also need to pass a third
> > AXAncestorFlags parameter (to capture the flags of the ignored object),
> > which IMO makes this refactor not worth it.
> 
> No, you can do
> 
>     auto ancestorFlags = computeAncestorFlags();
> 
> inside insertObjects(...), no need to pass it as a param. Changed the name
> from insertchildren to insertObjects because in this case you would be
> inserting the grandChildren. I believe it would make the code maintainable.
The current behavior is to use the ancestor flags of the ignored child rather than `this` as you suggest. Using `this` ancestor flags might be OK...but I don't want to make that behavior change in this patch. Let's address this refactor in a separate patch.

[EWS]

Committed r295618 (251623@main): <https://commits.webkit.org/251623@main>

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