Avoid updateStyleAndLayoutTree in determineAccessibilityRole

Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch replaces it to supportsFocus(), since the main purpose is
to give elements with tabIndex explicitly set get some role.

This is a speculative fix.

BUG=590369, 647602, 665168
Committed: https://crrev.com/338b38e06760302a8010bfea008865f55db4db0c
Cr-Commit-Position: refs/heads/master@{#435009}

[kojii, 2016-11-28 08:03:34]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 08:03:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-11-28 08:04:46]

Description was changed from

==========
WIP: ax-update

BUG=
==========

to

==========
WIP: ax-update

BUG=590369, 647602, 665168
==========

[commit-bot: I haz the power, 2016-11-28 09:20:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 09:20:27]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-11-28 09:28:58]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 09:29:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-11-28 09:41:16]

Description was changed from

==========
WIP: ax-update

BUG=590369, 647602, 665168
==========

to

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue should then be updated when the next layout
occurs.

BUG=590369, 647602, 665168
==========

[kojii, 2016-11-28 09:42:18]

Description was changed from

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue should then be updated when the next layout
occurs.

BUG=590369, 647602, 665168
==========

to

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue() should then be updated when the next layout
occurs.

BUG=590369, 647602, 665168
==========

[commit-bot: I haz the power, 2016-11-28 11:18:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 11:18:42]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-11-28 12:57:10]

kojii@chromium.org changed reviewers:
+ dmazzoni@chromium.org, eae@chromium.org, tkent@chromium.org

[kojii, 2016-11-28 12:57:11]

tkent@, dmazzoni@, PTAL. cc/eae@ if any opinions, approach 4 (discarded) is
layout.

I investigated 4 possible approaches and currently I'm currently in favor of the
approach 2.

Approach 1 (PS1): Prevent updating layout only when AXObjectCache::textChanged
is called for a LayoutObject. Since the LayoutObject needs updates, assumes
determineAccessibleRole will be called again and will be correct eventually, but
if the assumption doesn't stand, we'll see incorrect role value when a canvas
fallback content with ::first-letter is dynamically updated.

Approach 2 (PS2): Simplified approach 1. If the assumption in PS1 stands, we
don't have to limit it only for AXObjectCache::textChanged(LayoutObject*). This
approach always disables updating layout. Safer and simpler. More breakage is
possible though if the assumption doesn't stand, but it's still limited to
canvas fallback content with ::first-letter is dynamically updated, so not a
common case.

Approach 3 (discarded, crrev.com/2530343002): Tried to delay
determineAccessibleRole() by moving from AXNodeObject::init() to roleValue().
This approach didn't work well because roleValue() is so often called, mostly
deep in the callstacks. Hard to avoid calling it during textChanged(), and easy
to break in future.

Approach 4 (discarded): Define LayoutText::setText() can destroy itself, and fix
its callers. There are many callers of setText(), from attach, detach, layout,
or timer. To fix them all isn't realistic, hard to test, and easy to break in
future.

I'm trying to create a stable repro, but I'm not succeeding yet.

[kojii, 2016-11-28 12:57:32]

Description was changed from

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue() should then be updated when the next layout
occurs.

BUG=590369, 647602, 665168
==========

to

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue() should then be updated when the next layout
occurs.

This fix is speculative.

BUG=590369, 647602, 665168
==========

[kojii, 2016-11-28 12:57:52]

Description was changed from

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue() should then be updated when the next layout
occurs.

This fix is speculative.

BUG=590369, 647602, 665168
==========

to

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue() should then be updated when the next layout
occurs.

This is a speculative fix.

BUG=590369, 647602, 665168
==========

[tkent, 2016-11-28 23:16:05]

On 2016/11/28 at 12:57:11, kojii wrote:
> tkent@, dmazzoni@, PTAL. cc/eae@ if any opinions, approach 4 (discarded) is
layout.
> 
> I investigated 4 possible approaches and currently I'm currently in favor of
the approach 2.
> 
> Approach 1 (PS1): Prevent updating layout only when AXObjectCache::textChanged
is called for a LayoutObject. Since the LayoutObject needs updates, assumes
determineAccessibleRole will be called again and will be correct eventually, but
if the assumption doesn't stand, we'll see incorrect role value when a canvas
fallback content with ::first-letter is dynamically updated.
> 
> Approach 2 (PS2): Simplified approach 1. If the assumption in PS1 stands, we
don't have to limit it only for AXObjectCache::textChanged(LayoutObject*). This
approach always disables updating layout. Safer and simpler. More breakage is
possible though if the assumption doesn't stand, but it's still limited to
canvas fallback content with ::first-letter is dynamically updated, so not a
common case.

If we're sure that determineAccessibleRole will be called again, PS2 is
acceptable for a short-term security fix, I think.

However, if so, can we skip the first call of determineAccesibleRole? Can we
postpone AX tree update until layout clean state?

[dmazzoni, 2016-11-28 23:25:29]

In this particular case, let's call
Element::supportsFocus() instead. The main
purpose of that line is to make sure that a
generic element with tabIndex explicitly set
gets some role. The layout checks for
focusability aren't critical here; a false
positive would be harmless.

If there are other places in the accessibility
code where you see the same issue, we can
consider them separately. There may be cases
where we want to do something different.

[kojii, 2016-11-29 01:27:39]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 01:28:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-11-29 01:29:47]

Thank you Dominic and tkent for the prompt reviews.

On 2016/11/28 at 23:25:29, dmazzoni wrote:
> In this particular case, let's call
> Element::supportsFocus() instead. The main
> purpose of that line is to make sure that a
> generic element with tabIndex explicitly set
> gets some role. The layout checks for
> focusability aren't critical here; a false
> positive would be harmless.

I see, thank you for the great advice! Fixed accordingly in PS3.

> If there are other places in the accessibility
> code where you see the same issue, we can
> consider them separately. There may be cases
> where we want to do something different.

I'll make sure to consult you, thank you!

[kojii, 2016-11-29 01:46:06]

Description was changed from

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch gives up computing isFocusable() if layout tree needs
updates. The roleValue() should then be updated when the next layout
occurs.

This is a speculative fix.

BUG=590369, 647602, 665168
==========

to

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch replaces it to supportsFocus(), since the main purpose is
to give elements with tabIndex explicitly set get some role.

This is a speculative fix.

BUG=590369, 647602, 665168
==========

[commit-bot: I haz the power, 2016-11-29 03:29:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 03:29:46]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-11-29 16:16:13]

lgtm

[dmazzoni, 2016-11-29 16:16:20]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-11-29 16:16:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 16:20:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480436180934630,
"parent_rev": "19c4bcb5329e0de604d4dcc2496043ad0aa01d82", "commit_rev":
"84e2fa6a0258d85d64bce3d9a78522cc3c4ec252"}

[commit-bot: I haz the power, 2016-11-29 16:20:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-29 16:23:28]

Description was changed from

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch replaces it to supportsFocus(), since the main purpose is
to give elements with tabIndex explicitly set get some role.

This is a speculative fix.

BUG=590369, 647602, 665168
==========

to

==========
Avoid updateStyleAndLayoutTree in determineAccessibilityRole

This patch avoids updating layout tree in
AXNodeObject::determineAccessibilityRole().

Element::isFocusable() requires styles to be updated. However, when
layout code calls determineAccessibilityRole(), updating layout tree
should be avoided since it may destroy the calling object.

This patch replaces it to supportsFocus(), since the main purpose is
to give elements with tabIndex explicitly set get some role.

This is a speculative fix.

BUG=590369, 647602, 665168

Committed: https://crrev.com/338b38e06760302a8010bfea008865f55db4db0c
Cr-Commit-Position: refs/heads/master@{#435009}
==========

[commit-bot: I haz the power, 2016-11-29 16:23:29]

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