Fix dcheck failure in Document.cpp and don't fire a11y events for children changed on leaf or leaf descendant [chromium/src : master]
Aaron Leventhal (Gerrit)
This change is ready for review.
2 comments:
File third_party/WebKit/Source/core/dom/Document.cpp:
Patch Set #2, Line 6239:
return toHTMLDialogElement(top_layer_elements_.back().Get());Dcheck was failing when the top_layer_elements_.back() element is a pseudo backdrop element, which can occur when added here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Element.cpp?type=cs&sq=package:chromium&l=3491Note that I did see some stack traces for this line as well.
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:
Patch Set #3, Line 2593:
cached_is_descendant_of_leaf_node_) {In reality using this cached value is enough to avoid the dcheck failure but the Document.cpp fixes the root cause (or is at least closer to the root cause).
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Aaron Leventhal would like Hayato Ito to review this change.
Fix dcheck failure in Document.cpp and don't fire a11y events for children changed on leaf or leaf descendant
CL #1 intentionally doesn't pass the test. I noticed that we trigger a dcheck failure and wanted to suggest a fix.
CL #2 passes by ensuring that the pseudo backdrop is never converted to a dialog element in Document.cpp
CL #3 is more efficient and avoids the extra work of recomputing tons of a11y attributes for each ChildrenChanged() call -- after all we just want to know if the ancestors were leaf nodes the last time this object could have descended from them.
Bug: None
Change-Id: I3275e6cb0dff8f2a308521d97cbd99d2efb8a217
---
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index 526e910..f09367e 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -6233,7 +6233,8 @@
}
HTMLDialogElement* Document::ActiveModalDialog() const {
- if (top_layer_elements_.IsEmpty())
+ if (top_layer_elements_.IsEmpty() ||
+ !isHTMLDialogElement(top_layer_elements_.back()))
return 0;
return toHTMLDialogElement(top_layer_elements_.back().Get());
}
diff --git a/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp b/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
index b5fc4e6..bce32fa 100644
--- a/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
@@ -2582,10 +2582,15 @@
if (!GetNode() && !GetLayoutObject())
return;
- // If this is not part of the accessibility tree because an ancestor
- // has only presentational children, invalidate this object's children but
- // skip sending a notification and skip walking up the ancestors.
- if (AncestorForWhichThisIsAPresentationalChild()) {
+ // If this is not part of the accessibility then invalidate this object's
+ // children but skip sending a notification and skip walking up the ancestors.
+ // Cases where this happens:
+ // - an ancestor has only presentational children, or
+ // - this or an ancestor is a leaf node
+ // Uses |cached_is_descendant_of_leaf_node_| to avoid updating cached
+ // attributes for eachc change via | UpdateCachedAttributeValuesIfNeeded()|.
+ if (AncestorForWhichThisIsAPresentationalChild() || !CanHaveChildren() ||
+ cached_is_descendant_of_leaf_node_) {
SetNeedsToUpdateChildren();
return;
}
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
+foolip, I see you had some code that would have fixed this in the following CL:
https://codereview.chromium.org/2107233002/
Aaron Leventhal (Gerrit)
+foolip can you look at the Document.cpp changes? It's basically what you had in the reverted CL I mentioned.
Aaron Leventhal (Gerrit)
hayato, foolip - I believe this fixes some real crashes.
Aaron Leventhal (Gerrit)
Aaron Leventhal uploaded patch set #7 to this change.
Fix dcheck failure in Document.cpp and don't fire a11y events for children changed on leaf or leaf descendant
Bug: None
Change-Id: I3275e6cb0dff8f2a308521d97cbd99d2efb8a217
---
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
2 files changed, 16 insertions(+), 7 deletions(-)
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
CL #1 intentionally doesn't pass the test. I noticed that we trigger a dcheck failure and wanted to suggest a fix.
CL #2 passes by ensuring that the pseudo backdrop is never converted to a dialog element in Document.cpp
CL #3 is more efficient and avoids the extra work of recomputing tons of a11y attributes for each ChildrenChanged() call -- after all we just want to know if the ancestors were leaf nodes the last time this object could have descended from them.
Dominic Mazzoni (Gerrit)
Patch set 7:Code-Review +1
1 comment:
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:
Patch Set #6, Line 2585:
// If this is not part of the accessibility then invalidate this object'snit: dropped the word "tree" in "accessibility tree"
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Philip Jägenstedt (Gerrit)
File third_party/WebKit/Source/core/dom/Document.cpp:
As it happens, this is exactly the change will be needed when also adding fullscreen elements to top layer.
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Is there any bug id or a test? It might be better to have it to understand the context.
Aaron Leventhal (Gerrit)
Patch Set 7:
Is there any bug id or a test? It might be better to have it to understand the context.
@hayato, here is some context:
- The first patch set in this CL causes the crash consistently, as can be seen in the test failures
- I have filed https://bugs.chromium.org/p/chromium/issues/detail?id=757451 -- in it you can see which line in Element.cpp adds a different type of element (a pseudo backdrop element) to top_layer_elements_ . However, Document::ActiveModalDialog() assumes that all the elements in that list are HTML dialog elements.
Aaron Leventhal (Gerrit)
Aaron Leventhal uploaded patch set #8 to this change.
Fix dcheck failure in Document.cpp and don't fire a11y events for children changed on leaf or leaf descendant
Bug: 757451
Change-Id: I3275e6cb0dff8f2a308521d97cbd99d2efb8a217
---
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
2 files changed, 16 insertions(+), 7 deletions(-)
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Alice Boxhall (Gerrit)
1 comment:
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:
Patch Set #2, Line 2585:
this is not part of the accessibilitySlightly misleading: it's more like "if this node's children are not a part of the accessibility tree", right?
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Thanks. I still might miss something, but is there any relationship between two changes, Document.cpp and AxNodeObject.cpp? I don't see any relationship between these from the description. It might be better to make it clear.
Aaron Leventhal (Gerrit)
1 comment:
Patch Set #2, Line 2585:
this is not part of the accessibility
Slightly misleading: it's more like "if this node's children are not a part of the accessibility tre […]
Right. Will fix.
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
LGTM
It would be still difficult to see why the change of AXNodeObject::ChildrenChanged() would trigger DCHECK in Document.cpp.
How the change of AXNodeObject::ChildrenChanged() and Document's _top_layer_elements are related?
It would be better to record how the change of AXNodeObject::ChildrenChanged()
would be a trigger to insert an unexpected thing in _top_layer_elements, AXNodeObject::ChildrenChanged(), if you know more context.
If these two things are not directly related, it would be better to make it clear in the CL's description.
Patch set 10:Code-Review +1
Aaron Leventhal (Gerrit)
Patch Set 10: Code-Review+1
LGTM
It would be still difficult to see why the change of AXNodeObject::ChildrenChanged() would trigger DCHECK in Document.cpp.
How the change of AXNodeObject::ChildrenChanged() and Document's _top_layer_elements are related?
It would be better to record how the change of AXNodeObject::ChildrenChanged()
would be a trigger to insert an unexpected thing in _top_layer_elements, AXNodeObject::ChildrenChanged(), if you know more context.If these two things are not directly related, it would be better to make it clear in the CL's description.
They are related in that it changes the timing of when Document::ActiveModalDialog() is called. I didn't look into it more deeply because it's clear programming error. The list of elements is not guaranteed to be dialog elements, and the code in ActiveModalDialog() assumes it is. Yet we clearly see the pseudo backdrop elements elsewhere in code.
So although I don't understand 100% why it worked before, it's clear we were getting lucky. Although really, we weren't getting lucky, because go/crash shows that we do crash there.
To me this patch is as simple as a logical implementation of type safety -- not using a variable in two different ways.
Aaron Leventhal (Gerrit)
Patch set 10:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Fix dcheck failure in Document.cpp and don't fire a11y events for children changed on leaf or leaf descendant
Bug: 757451
Change-Id: I3275e6cb0dff8f2a308521d97cbd99d2efb8a217
Reviewed-on: https://chromium-review.googlesource.com/619367
Reviewed-by: Hayato Ito <hay...@chromium.org>
Reviewed-by: Dominic Mazzoni <dmaz...@chromium.org>
Reviewed-by: Philip Jägenstedt <foo...@chromium.org>
Commit-Queue: Aaron Leventhal <aleve...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496685}
---
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index fa0bb41..aa54c64 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -6251,9 +6251,13 @@
}
HTMLDialogElement* Document::ActiveModalDialog() const {
- if (top_layer_elements_.IsEmpty())
- return 0;
- return toHTMLDialogElement(top_layer_elements_.back().Get());
+ for (auto it = top_layer_elements_.rbegin(); it != top_layer_elements_.rend();
+ ++it) {
+ if (isHTMLDialogElement(*it))
+ return toHTMLDialogElement((*it).Get());
+ }
+
+ return nullptr;
}
void Document::exitPointerLock() {
diff --git a/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp b/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
index 59b493f..af5e1c4 100644
--- a/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
@@ -2624,11 +2624,13 @@
if (!GetNode() && !GetLayoutObject())
return;
- // If this is not part of the accessibility tree then invalidate this object's
- // children but skip sending a notification and skip walking up the ancestors.
- // This occurs when the ancestor is a leaf node.
- // Uses |cached_is_descendant_of_leaf_node_| to avoid expense of updating
- // cached attribs for each change via |UpdateCachedAttributeValuesIfNeeded()|.
+ // If this node's children are not part of the accessibility tree then
+ // invalidate the children but skip notification and walking up the ancestors.
+ // Cases where this happens:
+ // - an ancestor has only presentational children, or
+ // - this or an ancestor is a leaf node
+ // Uses |cached_is_descendant_of_leaf_node_| to avoid updating cached
+ // attributes for eachc change via | UpdateCachedAttributeValuesIfNeeded()|.
if (!CanHaveChildren() || cached_is_descendant_of_leaf_node_) {
SetNeedsToUpdateChildren();
return;
To view, visit change 619367. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Patch Set 10:
Patch Set 10: Code-Review+1
LGTM
It would be still difficult to see why the change of AXNodeObject::ChildrenChanged() would trigger DCHECK in Document.cpp.
How the change of AXNodeObject::ChildrenChanged() and Document's _top_layer_elements are related?
It would be better to record how the change of AXNodeObject::ChildrenChanged()
would be a trigger to insert an unexpected thing in _top_layer_elements, AXNodeObject::ChildrenChanged(), if you know more context.If these two things are not directly related, it would be better to make it clear in the CL's description.
They are related in that it changes the timing of when Document::ActiveModalDialog() is called. I didn't look into it more deeply because it's clear programming error. The list of elements is not guaranteed to be dialog elements, and the code in ActiveModalDialog() assumes it is. Yet we clearly see the pseudo backdrop elements elsewhere in code.
So although I don't understand 100% why it worked before, it's clear we were getting lucky. Although really, we weren't getting lucky, because go/crash shows that we do crash there.
To me this patch is as simple as a logical implementation of type safety -- not using a variable in two different ways.
Thank you. Yeah, I agree you didn't have to look deeper. If I were you, I didn't look deeper. :)
It's sometimes helpful to record that two things are not directly related in the description or in the code reviews when we do git-blame around these change in the future. That will prevent us from having a wrong assumption.
Aaron Leventhal (Gerrit)
Patch Set 11:
Patch Set 10:
Patch Set 10: Code-Review+1
LGTM
It would be still difficult to see why the change of AXNodeObject::ChildrenChanged() would trigger DCHECK in Document.cpp.
How the change of AXNodeObject::ChildrenChanged() and Document's _top_layer_elements are related?
It would be better to record how the change of AXNodeObject::ChildrenChanged()
would be a trigger to insert an unexpected thing in _top_layer_elements, AXNodeObject::ChildrenChanged(), if you know more context.If these two things are not directly related, it would be better to make it clear in the CL's description.
They are related in that it changes the timing of when Document::ActiveModalDialog() is called. I didn't look into it more deeply because it's clear programming error. The list of elements is not guaranteed to be dialog elements, and the code in ActiveModalDialog() assumes it is. Yet we clearly see the pseudo backdrop elements elsewhere in code.
So although I don't understand 100% why it worked before, it's clear we were getting lucky. Although really, we weren't getting lucky, because go/crash shows that we do crash there.
To me this patch is as simple as a logical implementation of type safety -- not using a variable in two different ways.
Thank you. Yeah, I agree you didn't have to look deeper. If I were you, I didn't look deeper. :)
It's sometimes helpful to record that two things are not directly related in the description or in the code reviews when we do git-blame around these change in the future. That will prevent us from having a wrong assumption.
I see, makes sense!