Bug 237104 – Fix for AXIsolatedTree::updateChildren in the case when is called for a non-existing isolated object.

• Component: Accessibility
• Status: NEW

[Andres Gonzalez]

AXIsolatedTree::updateChildren should update the entire subtree if there are no differences in the children of the existing isolated object.

[Radar WebKit Bug Importer]

<rdar://problem/89372850>

[Andres Gonzalez]

Created attachment 453015 [details]
Patch

[Andres Gonzalez]

Created attachment 453303 [details]
Patch

[Darin Adler]

Comment on attachment 453303 [details]
Patch

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

review- because of the move semantic mistake

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:293
> +    axChildrenIDs.reserveInitialCapacity(axChildrenCopy.size());

If it’s going to be common to return a vector much smaller than this, we may want to use shrinkToFit. On the other hand, if it’s a temporary that’s immediately used and deallocated then not so important.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:295
> +        collectNodeChangesForSubtree(*axChild, axObject.objectID(), attachWrapper, changes, idsBeingChanged, WTFMove(filter));

This isn’t safe. We can’t use the function after moving it. I suggest passing the Function as const&, not &&.

In some parts of our code, we figure out ways do to this kind of thing without recursion.

[Darin Adler]

Comment on attachment 453303 [details]
Patch

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

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:295
>> +        collectNodeChangesForSubtree(*axChild, axObject.objectID(), attachWrapper, changes, idsBeingChanged, WTFMove(filter));
> 
> This isn’t safe. We can’t use the function after moving it. I suggest passing the Function as const&, not &&.
> 
> In some parts of our code, we figure out ways do to this kind of thing without recursion.

My language here was imprecise:

We can’t use the function again after moving *from* it, so the next line of code where we call filter is a programming mistake. Best fix is probably to change this to take const Function<bool(const AXCoreObject)>& instead, and removing the WTFMove.

[Andres Gonzalez]

Created attachment 453340 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 453303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453303&action=review
> 
> >> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:295
> >> +        collectNodeChangesForSubtree(*axChild, axObject.objectID(), attachWrapper, changes, idsBeingChanged, WTFMove(filter));
> > 
> > This isn’t safe. We can’t use the function after moving it. I suggest passing the Function as const&, not &&.
> > 
> > In some parts of our code, we figure out ways do to this kind of thing without recursion.
> 
> My language here was imprecise:
> 
> We can’t use the function again after moving *from* it, so the next line of
> code where we call filter is a programming mistake. Best fix is probably to
> change this to take const Function<bool(const AXCoreObject)>& instead, and
> removing the WTFMove.

Fixed both issues that you pointed out. Concerning the move of a Function, thought that use after move would be ok in this case because the only state that Function is probably keeping is the address of the lambda, and that should be valid after the move, assuming that the move is not zeroing out that address. I.e., for Function, move and copy are the same. But I may be missing something about the implementation of Function. Also agree that for consistency with the move semantics, shouldn't be used after move. Thanks!

[Darin Adler]

(In reply to Andres Gonzalez from comment #7)
> Fixed both issues that you pointed out. Concerning the move of a Function,
> thought that use after move would be ok in this case because the only state
> that Function is probably keeping is the address of the lambda, and that
> should be valid after the move, assuming that the move is not zeroing out
> that address. I.e., for Function, move and copy are the same. But I may be
> missing something about the implementation of Function. Also agree that for
> consistency with the move semantics, shouldn't be used after move. Thanks!

Yes, I can’t emphasize this enough:

it would be *very* dangerous to use after move because we designed a particular class to be usable after moving; makes little sense but is of course possible to make a class work this way. We should absolutely *not* rely on WTF::Function being designed for use after moving from, even if it happens to be true due to implementation details at time of this writing.

In fact, we should probably go the other direction and create debug assertions for classes we design to catch use-after-move mistakes.

[Andres Gonzalez]

Created attachment 453424 [details]
Patch

[Darin Adler]

Comment on attachment 453424 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:437
> +    auto isAncestor = Function<FilterResponse(const AXCoreObject&)>([&axDescendant] (const AXCoreObject& axAncestor) {

I’d think we’d want this local to just be the lambda and construct the function as a side effect of passing it to collectNodeChangesForSubtree. Did you try that?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:398
> +    enum class FilterResponse : uint8_t {
> +        Include,
> +        Skip,
> +        Stop,
> +    };

I think these read better as one-liners.

[Andres Gonzalez]

Created attachment 453439 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #10)
> Comment on attachment 453424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453424&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:437
> > +    auto isAncestor = Function<FilterResponse(const AXCoreObject&)>([&axDescendant] (const AXCoreObject& axAncestor) {
> 
> I’d think we’d want this local to just be the lambda and construct the
> function as a side effect of passing it to collectNodeChangesForSubtree. Did
> you try that?

Tried several variants with no luck, except this one. Either get

error: no viable conversion from '(lambda at ./accessibility/isolatedtree/AXIsolatedTree.cpp:437:23)' to 'const Function<WebCore::AXIsolatedTree::FilterResponse (const WebCore::AXCoreObject &)>'

or

error: no matching constructor for initialization of 'const Function<WebCore::AXIsolatedTree::FilterResponse (const WebCore::AXCoreObject &)> &'

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:398
> > +    enum class FilterResponse : uint8_t {
> > +        Include,
> > +        Skip,
> > +        Stop,
> > +    };
> 
> I think these read better as one-liners.

Yes, done. Thanks.