Bug 235683 - Make modifications to the isolated Tree atomic.
Summary: Make modifications to the isolated Tree atomic.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-26 19:14 PST by Andres Gonzalez
Modified: 2022-02-02 10:43 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.31 KB, patch)
2022-01-26 19:34 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (19.61 KB, patch)
2022-02-01 14:40 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (19.65 KB, patch)
2022-02-01 17:55 PST, 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 2022-01-26 19:14:39 PST
Make modifications to the isolated Tree atomic.
Comment 1 Radar WebKit Bug Importer 2022-01-26 19:14:54 PST
<rdar://problem/88108412>
Comment 2 Andres Gonzalez 2022-01-26 19:34:22 PST
Created attachment 450094 [details]
Patch
Comment 3 chris fleizach 2022-01-26 23:20:21 PST
Comment on attachment 450094 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        or AX secondary thread, is blocked. To acomplish this and still be

accomplish

> Source/WebCore/ChangeLog:18
> +        happens on the main thread without blocking the AX thread, and thn do

thn > then

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:232
> +    if (!parentID.isValid()) {

do we need to handle this parentID.isValid() case separately? it seems like we could check if the !parentID we don't process the siblings

then we won't have an early return in this function

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:373
> +    changes.reverse();

can you add a comment why we want to reverse this vector
Comment 4 Andres Gonzalez 2022-02-01 14:40:10 PST
Created attachment 450569 [details]
Patch
Comment 5 Andres Gonzalez 2022-02-01 14:43:46 PST
(In reply to chris fleizach from comment #3)
> Comment on attachment 450094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450094&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        or AX secondary thread, is blocked. To acomplish this and still be
> 
> accomplish
> 
> > Source/WebCore/ChangeLog:18
> > +        happens on the main thread without blocking the AX thread, and thn do
> 
> thn > then
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:232
> > +    if (!parentID.isValid()) {
> 
> do we need to handle this parentID.isValid() case separately? it seems like
> we could check if the !parentID we don't process the siblings
> 
> then we won't have an early return in this function
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:373
> > +    changes.reverse();
> 
> can you add a comment why we want to reverse this vector

All comments are addressed in latest revision. Thanks.
Comment 6 chris fleizach 2022-02-01 14:47:47 PST
Comment on attachment 450569 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:227
>      ASSERT(isMainThread());

can we assert that the lock is held in here

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:245
> +void AXIsolatedTree::queueChanges(const Vector<NodeChange>& changes, const Vector<AXID>& subtreeRemovals)

should this be named queueChangesAndRemovals?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:392
> +    Vector<NodeChange> addNodeAncestry(AXCoreObject&);

this method might be better names as

nodeAncestryChanges
Comment 7 Tyler Wilcock 2022-02-01 16:37:54 PST
Comment on attachment 450569 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:404
> +        AXLOG(makeString("removing subtree for object", axID.loggingString()));

This log is not necessary. removeSubtreeFromNodeMap already logs this:

 AXLOG(makeString("Removing subtree for axID ", axID.loggingString()));
Comment 8 Tyler Wilcock 2022-02-01 16:41:55 PST
Comment on attachment 450569 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:353
> +    // Since the NodeChanges are added to the changes vector in a child -> parent traversal instead of the usual parent -> child traversal, we may need to reverse changes.

"We may need to reverse changes"

IMO, this language (and the underlying behavior) should be definite -- either we do need to do the reverse, or we don't. Based on the beginning of the comment, seems like it necessary?
Comment 9 Tyler Wilcock 2022-02-01 17:05:56 PST
When testing this patch in debug on YouTube, I hit this ASSERT:

ASSERTION FAILED: m_nodeMap.contains(objectID)
./accessibility/isolatedtree/AXIsolatedTree.cpp(240) : void WebCore::AXIsolatedTree::queueChange(const WebCore::AXIsolatedTree::NodeChange &)
1   0x1330bd334 WTFCrash
2   0x147089344 WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1493b88ac WebCore::AXIsolatedTree::queueChange(WebCore::AXIsolatedTree::NodeChange const&)
4   0x1493b7e44 WebCore::AXIsolatedTree::queueChanges(WTF::Vector<WebCore::AXIsolatedTree::NodeChange, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WTF::Vector<WTF::ObjectIdentifier<WebCore::AXIDType>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)

This is because this sequence of events is possible:

1. AXObjectCache::remove(AXID)
2. AXIsolatedTree::removeNode(AXID), which performs m_nodeMap.remove(axID) without also removing this AXIDs subtree from the node map
3. AXIsolatedTree::updateChildren(AXCoreObject& axObject) is called with the parent of AXID
4. auto oldChildrenIDs = m_nodeMap.get(axObject.objectID()); still contains this removed AXID
5. Because AXID is rightly considered removed (not part of newChildrenIDs), we call removeSubtreeFromNodeMap(AXID) and then hit this assert because it was already removed from the map in step 2

Maybe AXIsolatedTree::removeNode(AXID) needs to removeSubtreeFromNodeMap(AXID) and m_pendingSubtreeRemovals.append(axID)?
Comment 10 Tyler Wilcock 2022-02-01 17:27:02 PST
Nevermind, ignore that sequence after step 3 and my suggestion, they're wrong. The stacktrace shows the order of execution. I'll investigate some more.
Comment 11 Andres Gonzalez 2022-02-01 17:55:27 PST
Created attachment 450599 [details]
Patch
Comment 12 Andres Gonzalez 2022-02-01 18:02:46 PST
(In reply to chris fleizach from comment #6)
> Comment on attachment 450569 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450569&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:227
> >      ASSERT(isMainThread());
> 
> can we assert that the lock is held in here

Done.

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:245
> > +void AXIsolatedTree::queueChanges(const Vector<NodeChange>& changes, const Vector<AXID>& subtreeRemovals)
> 
> should this be named queueChangesAndRemovals?

Renamed it. Was considering to rename it queueAdditionsAndRemovals, but then we have NodeChange which is an addition but we already named Change. So I opted for now for your suggestion.

> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:392
> > +    Vector<NodeChange> addNodeAncestry(AXCoreObject&);
> 
> this method might be better names as
> 
> nodeAncestryChanges

Yes, done. Thanks.
Comment 13 Andres Gonzalez 2022-02-01 18:04:27 PST
(In reply to Tyler Wilcock from comment #7)
> Comment on attachment 450569 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450569&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:404
> > +        AXLOG(makeString("removing subtree for object", axID.loggingString()));
> 
> This log is not necessary. removeSubtreeFromNodeMap already logs this:
> 
>  AXLOG(makeString("Removing subtree for axID ", axID.loggingString()));

Yes, removed it. Thanks.
Comment 14 Andres Gonzalez 2022-02-01 18:06:27 PST
(In reply to Tyler Wilcock from comment #8)
> Comment on attachment 450569 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450569&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:353
> > +    // Since the NodeChanges are added to the changes vector in a child -> parent traversal instead of the usual parent -> child traversal, we may need to reverse changes.
> 
> "We may need to reverse changes"
> 
> IMO, this language (and the underlying behavior) should be definite --
> either we do need to do the reverse, or we don't. Based on the beginning of
> the comment, seems like it necessary?

Made it assertive.
Comment 15 EWS 2022-02-02 10:43:45 PST
Committed r288963 (246692@main): <https://commits.webkit.org/246692@main>

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