Bug 236250

Summary: Prevent removal of isolated objects from a different parent than the current parent.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: AccessibilityAssignee: 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 Flags
Patch
none
Patch none

Andres Gonzalez
Reported 2022-02-07 11:44:18 PST
Prevent removal of isolated objects from a different parent than the current parent.
Attachments
Patch (11.00 KB, patch)
2022-02-07 11:57 PST, Andres Gonzalez
no flags
Patch (11.32 KB, patch)
2022-02-07 13:19 PST, Andres Gonzalez
no flags
Radar WebKit Bug Importer
Comment 1 2022-02-07 11:45:18 PST
Andres Gonzalez
Comment 2 2022-02-07 11:57:52 PST
chris fleizach
Comment 3 2022-02-07 12:21:00 PST
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?
Tyler Wilcock
Comment 4 2022-02-07 12:21:42 PST
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."
chris fleizach
Comment 5 2022-02-07 12:22:04 PST
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?
Andres Gonzalez
Comment 6 2022-02-07 13:19:43 PST
Andres Gonzalez
Comment 7 2022-02-07 13:27:22 PST
(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.
Andres Gonzalez
Comment 8 2022-02-07 13:28:49 PST
(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..."
Andres Gonzalez
Comment 9 2022-02-07 13:30:31 PST
(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.
EWS
Comment 10 2022-02-07 19:33:03 PST
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].
Note You need to log in before you can comment on or make changes to this bug.