| Summary: | Prevent removal of isolated objects from a different parent than the current parent. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andres Gonzalez <andresg_22> | ||||||
| Component: | Accessibility | Assignee: | Andres Gonzalez <andresg_22> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Andres Gonzalez
2022-02-07 11:44:18 PST
Created attachment 451133 [details]
Patch
Comment on attachment 451133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451133&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:499 > + if (axParentID != m_nodeMap.get(axID).parentID) { will this cause a problem from the root node that has no parent? Comment on attachment 451133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451133&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:500 > + AXLOG(makeString("Removing object from a different parent ", axParentID.loggingString(), ", actual parent ", m_nodeMap.get(axID).parentID.loggingString(), ", bailing out.")); Starting the log message with "Removing object" implies that we are actually going through with this operation. Maybe instead it could read something like: "Attempted to remove object from a different parent (ID 123) than its node map parent (ID 321). Bailing out." m_nodeMap now references an ID and a struct with the parentID and childIds Why do we need the struct in m_nodePa if the key is already the parentID? Created attachment 451142 [details]
Patch
(In reply to chris fleizach from comment #3) > Comment on attachment 451133 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451133&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:499 > > + if (axParentID != m_nodeMap.get(axID).parentID) { > > will this cause a problem from the root node that has no parent? No, because in that case both axParentID and m_nodeMap.get(objectID).parentID should be 0. (In reply to Tyler Wilcock from comment #4) > Comment on attachment 451133 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451133&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:500 > > + AXLOG(makeString("Removing object from a different parent ", axParentID.loggingString(), ", actual parent ", m_nodeMap.get(axID).parentID.loggingString(), ", bailing out.")); > > Starting the log message with "Removing object" implies that we are actually > going through with this operation. Maybe instead it could read something > like: > > "Attempted to remove object from a different parent (ID 123) than its node > map parent (ID 321). Bailing out." Done: "Tried to remove..." (In reply to chris fleizach from comment #5) > m_nodeMap now references an ID and a struct with the parentID and childIds > > Why do we need the struct in m_nodePa if the key is already the parentID? The key is the object ID, the struct stores its parent ID and children IDs. Fixed up the comments a bit and the variable naming to make it more evident. Committed r289355 (246943@main): <https://commits.webkit.org/246943@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451142 [details]. |