Bug 213435 – AXIsolatedTree::generateSubtree should properly assign the generated subtree to its parent node.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andres Gonzalez]

AXIsolatedTree::generateSubtree should properly assign the generated subtree to its parent node.

[Andres Gonzalez]

Created attachment 402400 [details]
Patch

[Andres Gonzalez]

<rdar://problem/64420184>

[Darin Adler]

Comment on attachment 402400 [details]
Patch

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

review+ but I don’t understand the correctness of the isLocked/LockHolder use.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:164
> +void AXIsolatedTree::updateChildrenIDs(AXID axID, Vector<AXID>&& childrenIDs)

Seems like we missed an opportunity to move the vector in one of the places below that do it. The make_pair could use WTFMove, I guess. Without that, the function just copies the vector twice.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:174
> +    if (m_changeLogLock.isLocked())
> +        m_pendingChildrenUpdates.append(std::make_pair(axID, childrenIDs));
> +    else {
> +        LockHolder locker { m_changeLogLock };
> +        m_pendingChildrenUpdates.append(std::make_pair(axID, childrenIDs));
> +    }

Is this really a safe way to use a lock? I’m surprised it’s OK and not racy to check isLocked like this.

Seems an inelegant idiom; looks like an attempt to simulate recursive lock semantics.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:379
> -    LockHolder locker { m_changeLogLock };
> -    m_pendingSubtreeRemovals.append(axID);
> +    {
> +        LockHolder locker { m_changeLogLock };
> +        m_pendingSubtreeRemovals.append(axID);
> +    }

This change is entirely stylistic, I guess? Doesn’t make any semantic change.

[Andres Gonzalez]

Created attachment 402494 [details]
Patch

[Andres Gonzalez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 402400 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402400&action=review
> 
> review+ but I don’t understand the correctness of the isLocked/LockHolder
> use.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:164
> > +void AXIsolatedTree::updateChildrenIDs(AXID axID, Vector<AXID>&& childrenIDs)
> 
> Seems like we missed an opportunity to move the vector in one of the places
> below that do it. The make_pair could use WTFMove, I guess. Without that,
> the function just copies the vector twice.

Fixed.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:174
> > +    if (m_changeLogLock.isLocked())
> > +        m_pendingChildrenUpdates.append(std::make_pair(axID, childrenIDs));
> > +    else {
> > +        LockHolder locker { m_changeLogLock };
> > +        m_pendingChildrenUpdates.append(std::make_pair(axID, childrenIDs));
> > +    }
> 
> Is this really a safe way to use a lock? I’m surprised it’s OK and not racy
> to check isLocked like this.
> 
> Seems an inelegant idiom; looks like an attempt to simulate recursive lock
> semantics.

Fixed. No more check for isLocked().
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:379
> > -    LockHolder locker { m_changeLogLock };
> > -    m_pendingSubtreeRemovals.append(axID);
> > +    {
> > +        LockHolder locker { m_changeLogLock };
> > +        m_pendingSubtreeRemovals.append(axID);
> > +    }
> 
> This change is entirely stylistic, I guess? Doesn’t make any semantic change.

Reverted this that was a leftover of an intermediate change.

[Darin Adler]

Comment on attachment 402494 [details]
Patch

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

r=me assuming all the tests pass

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:101
> +    AXIsolatedTreeID m_treeID;

Typically we put all the function members before any of the data members. This could go back where it was before, or if the order of data members matters, at least down after the functions.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:128
> +    // Called on main thread to updates both m_nodeMap and m_pendingChildrenUpdates.
> +    void updateChildrenIDs(AXID axID, Vector<AXID>&& childrenIDs);

"updates" -> "update"

Typically we put all the function members before any of the data members, so this could go above m_axObjectCache.

[EWS]

Committed r263378: <https://trac.webkit.org/changeset/263378>

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

[Radar WebKit Bug Importer]

<rdar://problem/64614814>