Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2524993003:
DevTools Accessibility pane: fix crash when inspecting shadow roots (Closed)
|
Created:
4 years ago by aboxhall Modified:
4 years ago Reviewers:
dgozman CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, haraken, nektarios, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, blink-reviews, dmazzoni Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools Accessibility pane: fix crash when inspecting shadow roots
BUG=
Committed: https://crrev.com/411fec439a951cce5e95aa26bd2fbc303820dc4e
Cr-Commit-Position: refs/heads/master@{#436140}
Patch Set 1 #Patch Set 2 : Rationalise getting shadowRoot #
Total comments: 3
Patch Set 3 : Replace missing line #Messages
Total messages: 23 (13 generated)
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
aboxhall@chromium.org changed reviewers: + dgozman@chromium.org
I spent a couple of hours trying to write a test for this before giving up - I couldn't figure out how to get the frontend node ID for the shadow root in any straightforward way, and it quickly devolved into previously unheard of depths of promise hell.
Where exactly in the code did the crash happen? It's not obvious to me. Re: test, is it possible to grab the shadow host and ask for children accessibility instead?
On 2016/11/28 18:10:00, dgozman wrote: > Where exactly in the code did the crash happen? It's not obvious to me. > > Re: test, is it possible to grab the shadow host and ask for children > accessibility instead? No; the shadowRoot will only be traversed when it is explicitly inspected, as otherwise it is an "ignored" node and not included in the tree.
https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp (left): https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:513: Node* parentNode = FlatTreeTraversal::parent(inspectedDOMNode); The crash was here: FlatTreeTraversal explicitly does not deal with ShadowRoots. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/shado...
bump :)
lgtm modulo unnecessary line removal https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp (left): https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:517: parentAXObject = cache.getOrCreate(parentNode); Where did this line go?
https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp (left): https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:517: parentAXObject = cache.getOrCreate(parentNode); On 2016/12/02 18:04:16, dgozman wrote: > Where did this line go? Yikes. Replaced.
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2524993003/#ps40001 (title: "Replace missing line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480717515190210,
"parent_rev": "548e3aa876e6a39ca2489dc3425de5d4a6f347d9", "commit_rev":
"a9401f39241f813c14f25db7200c22c378668b47"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== DevTools Accessibility pane: fix crash when inspecting shadow roots BUG= ========== to ========== DevTools Accessibility pane: fix crash when inspecting shadow roots BUG= Committed: https://crrev.com/411fec439a951cce5e95aa26bd2fbc303820dc4e Cr-Commit-Position: refs/heads/master@{#436140} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/411fec439a951cce5e95aa26bd2fbc303820dc4e Cr-Commit-Position: refs/heads/master@{#436140} |
