DevTools Accessibility pane: fix crash when inspecting shadow roots (issue 2524993003 by aboxhall@chromium.org)

abox...@chromium.org

unread,

Nov 28, 2016, 12:31:57 PM11/28/16

to dgo...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

Reviewers: dgozman
CL: https://codereview.chromium.org/2524993003/

Message:
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.

Description:
DevTools Accessibility pane: fix crash when inspecting shadow roots

BUG=

Affected files (+20, -3 lines):
M third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp

Index: third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
diff --git a/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp b/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
index ace24ad3ead43568754da86e591101a32ea3f2c0..bdb5be0394ba7d061b3d4adc0dfd6d39c9ac95ec 100644
--- a/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
@@ -10,6 +10,7 @@
#include "core/dom/Element.h"
#include "core/dom/Node.h"
#include "core/dom/NodeList.h"
+#include "core/dom/shadow/ElementShadow.h"
#include "core/inspector/IdentifiersFactory.h"
#include "core/inspector/InspectorDOMAgent.h"
#include "core/inspector/InspectorStyleSheet.h"
@@ -510,11 +511,14 @@ void InspectorAccessibilityAgent::populateDOMNodeRelatives(
nodeObject.setChildIds(std::move(childIds));

// Walk up parents until an AXObject can be found.
- Node* parentNode = FlatTreeTraversal::parent(inspectedDOMNode);
+ Node* parentNode = inspectedDOMNode.isShadowRoot()
+ ? &toShadowRoot(inspectedDOMNode).host()
+ : FlatTreeTraversal::parent(inspectedDOMNode);
AXObject* parentAXObject = cache.getOrCreate(parentNode);
while (parentNode && !parentAXObject) {
- parentNode = FlatTreeTraversal::parent(inspectedDOMNode);
- parentAXObject = cache.getOrCreate(parentNode);
+ parentNode = parentNode->isShadowRoot()
+ ? &toShadowRoot(parentNode)->host()
+ : FlatTreeTraversal::parent(*parentNode);
};

if (!parentAXObject)
@@ -545,7 +549,20 @@ void InspectorAccessibilityAgent::findDOMNodeChildren(
Node& inspectedDOMNode,
std::unique_ptr<protocol::Array<AXNode>>& nodes,
AXObjectCacheImpl& cache) const {
+ if (inspectedDOMNode.isShadowRoot() &&
+ &parentNode == toShadowRoot(inspectedDOMNode).host()) {
+ childIds->addItem(String::number(kIDForInspectedNodeWithNoAXNode));
+ return;
+ }
NodeList* childNodes = parentNode.childNodes();
+ if (!childNodes->length() && parentNode.isElementNode()) {
+ Element& parentElement = toElement(parentNode);
+ ElementShadow* elementShadow = parentElement.shadow();
+ if (elementShadow) {
+ ShadowRoot& shadowRoot = elementShadow->youngestShadowRoot();
+ childNodes = shadowRoot.childNodes();
+ }
+ }
for (size_t i = 0; i < childNodes->length(); ++i) {
Node* childNode = childNodes->item(i);
if (childNode == &inspectedDOMNode) {

dgo...@chromium.org

unread,

Nov 28, 2016, 1:10:00 PM11/28/16

to abox...@chromium.org, chromium...@chromium.org, abox...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

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?

https://codereview.chromium.org/2524993003/

abox...@chromium.org

unread,

Dec 1, 2016, 7:30:06 PM12/1/16

to dgo...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

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/

abox...@chromium.org

unread,

Dec 1, 2016, 7:30:14 PM12/1/16

to dgo...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(left):

https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp#oldcode513
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/shadow/FlatTreeTraversal.h?sq=package:chromium&rcl=1480624189&l=44

https://codereview.chromium.org/2524993003/

abox...@chromium.org

unread,

Dec 2, 2016, 12:56:18 PM12/2/16

to dgo...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

bump :)

https://codereview.chromium.org/2524993003/

dgo...@chromium.org

unread,

Dec 2, 2016, 1:04:16 PM12/2/16

to abox...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

lgtm modulo unnecessary line removal

https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(left):

https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp#oldcode517
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:517:
parentAXObject = cache.getOrCreate(parentNode);
Where did this line go?

https://codereview.chromium.org/2524993003/

abox...@chromium.org

unread,

Dec 2, 2016, 5:25:11 PM12/2/16

to dgo...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(left):

https://codereview.chromium.org/2524993003/diff/20001/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp#oldcode517
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.

https://codereview.chromium.org/2524993003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 5:26:06 PM12/2/16

to abox...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2524993003/40001

https://codereview.chromium.org/2524993003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 9:22:30 PM12/2/16

to abox...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

Committed patchset #3 (id:40001)

https://codereview.chromium.org/2524993003/

commit-bot@chromium.org via codereview.chromium.org

unread,

Dec 2, 2016, 9:27:16 PM12/2/16

to abox...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, aboxhal...@chromium.org, nektar...@chromium.org, yuzo+...@chromium.org, har...@chromium.org, nek...@chromium.org, je_jul...@chromium.org, dmazzon...@chromium.org, dtseng...@chromium.org, blink-...@chromium.org, dmaz...@chromium.org

Patchset 3 (id:??) landed as
https://crrev.com/411fec439a951cce5e95aa26bd2fbc303820dc4e
Cr-Commit-Position: refs/heads/master@{#436140}

https://codereview.chromium.org/2524993003/

Reply all

Reply to author

Forward