Bug 213435 - AXIsolatedTree::generateSubtree should properly assign the generated subtree to its parent node.
Summary: AXIsolatedTree::generateSubtree should properly assign the generated subtree ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-20 13:22 PDT by Andres Gonzalez
Modified: 2020-06-22 16:56 PDT (History)
10 users (show)

See Also:


Attachments
Patch (8.17 KB, patch)
2020-06-20 13:29 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2020-06-22 12:39 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2020-06-20 13:22:51 PDT
AXIsolatedTree::generateSubtree should properly assign the generated subtree to its parent node.
Comment 1 Andres Gonzalez 2020-06-20 13:29:42 PDT
Created attachment 402400 [details]
Patch
Comment 2 Andres Gonzalez 2020-06-20 14:07:50 PDT
<rdar://problem/64420184>
Comment 3 Darin Adler 2020-06-21 13:39:38 PDT
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.
Comment 4 Andres Gonzalez 2020-06-22 12:39:15 PDT
Created attachment 402494 [details]
Patch
Comment 5 Andres Gonzalez 2020-06-22 12:45:03 PDT
(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.
Comment 6 Darin Adler 2020-06-22 12:50:04 PDT
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.
Comment 7 EWS 2020-06-22 16:55:02 PDT
Committed r263378: <https://trac.webkit.org/changeset/263378>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402494 [details].
Comment 8 Radar WebKit Bug Importer 2020-06-22 16:56:17 PDT
<rdar://problem/64614814>