Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1144363004:
When serializing accessibility tree, skip invalid children. (Closed)
|
Created:
5 years, 7 months ago by dmazzoni Modified:
5 years, 6 months ago Reviewers:
aboxhall CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen serializing accessibility tree, skip invalid children.
See bug for specific repro in the wild, but essentially we need to check
if the child is valid just before serializing, and not trust the list of
children of a node.
BUG=479743
Committed: https://crrev.com/8dc07f045c6287da3404fa8e0fd8b1b3960afbfa
Cr-Commit-Position: refs/heads/master@{#332748}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix Windows link #
Messages
Total messages: 22 (9 generated)
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org
https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... File ui/accessibility/ax_tree_serializer.h (right): https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... ui/accessibility/ax_tree_serializer.h:450: // Skip if the same child is included more than once. Note: the code below this comment was correct before, the but the comment above it ("No need to do anything more with children that aren't new") was wrong - new_child_ids isn't the set of child ids that are newly added, it's just the new set of child ids after this update. The purpose of this line wasn't to avoid serializing nodes that haven't changed, it was to avoid serializing the same child twice if a parent lists the same child twice in its child list.
https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... File ui/accessibility/ax_tree_serializer.h (right): https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... ui/accessibility/ax_tree_serializer.h:420: size_t serialized_node_index = out_update->nodes.size(); Why do this _and_ put serialized_node inside a scoped block? https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... ui/accessibility/ax_tree_serializer.h:425: // vector is resized. How can the vector get resized? Could we come up with a test for that situation?
https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... File ui/accessibility/ax_tree_serializer.h (right): https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... ui/accessibility/ax_tree_serializer.h:420: size_t serialized_node_index = out_update->nodes.size(); On 2015/06/01 16:46:46, aboxhall wrote: > Why do this _and_ put serialized_node inside a scoped block? So we can reference out_update->nodes[serialized_node_index] below https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... ui/accessibility/ax_tree_serializer.h:425: // vector is resized. On 2015/06/01 16:46:46, aboxhall wrote: > How can the vector get resized? Could we come up with a test for that situation? Because we call this function recursively on each child, and then finish writing |this| last. Imagine you have this tree: A B C D This function serializes A, so out_update->nodes is: [A] Recursively serialize B, so out_update->nodes is: [A B] Recursively serialize C, so out_update->nodes is: [A B C] Recursively serialize D, so out_update->nodes is: [A B C D] After doing the children, we update the first element's list of child_ids - but a pointer to out_update->nodes[0] could be invalid because we resized out_update->nodes. ASAN already caught this error.
lgtm https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... File ui/accessibility/ax_tree_serializer.h (right): https://codereview.chromium.org/1144363004/diff/1/ui/accessibility/ax_tree_se... ui/accessibility/ax_tree_serializer.h:425: // vector is resized. On 2015/06/01 16:53:18, dmazzoni wrote: > On 2015/06/01 16:46:46, aboxhall wrote: > > How can the vector get resized? Could we come up with a test for that > situation? > > Because we call this function recursively on each child, and then finish writing > |this| last. > > Imagine you have this tree: > > A > B C D > > This function serializes A, so out_update->nodes is: [A] > Recursively serialize B, so out_update->nodes is: [A B] > Recursively serialize C, so out_update->nodes is: [A B C] > Recursively serialize D, so out_update->nodes is: [A B C D] > After doing the children, we update the first element's list of child_ids - but > a pointer to out_update->nodes[0] could be invalid because we resized > out_update->nodes. > > ASAN already caught this error. Got it, thanks.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1144363004/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1144363004/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1144363004/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/1144363004/#ps20001 (title: "Fix Windows link")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1144363004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8dc07f045c6287da3404fa8e0fd8b1b3960afbfa Cr-Commit-Position: refs/heads/master@{#332748} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
