Make sure Document::updateLayoutTree*() is called before Element::isFocusable().

Make sure Document::updateLayoutTree*() is called before Element::isFocusable().

BUG=251163
Committed: https://crrev.com/401d7aa6f8dcd690badf4bf1145f17fd19cd599f
Cr-Commit-Position: refs/heads/master@{#373765}

[tkent, 2016-02-04 00:33:57]

Patchset #1 (id:1) has been deleted

[tkent, 2016-02-04 03:10:06]

tkent@chromium.org changed reviewers:
+ falken@chromium.org

[tkent, 2016-02-04 03:10:06]

falken@, would you review this please?

[dmazzoni, 2016-02-04 04:10:56]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2016-02-04 04:10:56]

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:522:
document()->updateLayoutTreeIgnorePendingStylesheets();
I'm worried that this could trigger bugs - this could get called from inside the
call stack of an AXLayoutObject and a call to updateLayoutTree...() could end up
deleting the AXLayoutObject's layoutObject out from under it, potentially
leading to a crash or UAF.

For the purposes of accessibility, we can safely ignore
layoutObjectIsFocusable() - we just need to know whether the DOM supports focus
and is not inert. (We already have layout object visibility checks that happen
later, when we know layout is clean.)

What would you think about adding something like
isFocusableIgnoringLayoutTree()?

[tkent, 2016-02-04 05:47:32]

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:522:
document()->updateLayoutTreeIgnorePendingStylesheets();
On 2016/02/04 at 04:10:56, dmazzoni wrote:
> I'm worried that this could trigger bugs - this could get called from inside
the call stack of an AXLayoutObject and a call to updateLayoutTree...() could
end up deleting the AXLayoutObject's layoutObject out from under it, potentially
leading to a crash or UAF.
> 
> For the purposes of accessibility, we can safely ignore
layoutObjectIsFocusable() - we just need to know whether the DOM supports focus
and is not inert. (We already have layout object visibility checks that happen
later, when we know layout is clean.)
> 
> What would you think about adding something like
isFocusableIgnoringLayoutTree()?

I think this is safe.
 - It seems AXLayoutObject doesn't use
AXNodeObject::determineAccessibilityRole().
 - Even if AXLayoutObject does, layout tree must be up to date when
determineAccessibilityRole() is called and
updateLayoutTreeIgnorePendingStyleSheets() does nothing.

I think adding isFocusableIgnoringLayoutTree() is hard because
Node::hasEditableStyle() depends on layout tree, and probably ax code wants to
know editability.

[dmazzoni, 2016-02-04 06:39:42]

On 2016/02/04 05:47:32, tkent wrote:
> I think this is safe.
>  - It seems AXLayoutObject doesn't use
> AXNodeObject::determineAccessibilityRole().

Ah, you're right. It seems less likely this would be an issue.

[falken, 2016-02-05 04:42:40]

lgtm. Thanks for fixing this very old bug.

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:2434:
ASSERT(!document().isActive() || !document().childNeedsStyleRecalc());
Was the comment about using childNeedsStyleRecalc() rather than just
needsStyleRecalc() useful?

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:415: // e.g. Calling
Document::updateLayoutTreeIgnorePendingStylesheets().
comment nits: one space after period is consistent with rest of comment, and
"Calling" -> "by calling" seems better.

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:592: // However, we must not
retrieve layout information like position and size.
"we" -> "it" is more consistent with the previous sentence

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:595: // TODO(tkent): Rename this to
isFocusableStyle.
good idea!

[tkent, 2016-02-05 04:57:45]

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.cpp:2434:
ASSERT(!document().isActive() || !document().childNeedsStyleRecalc());
On 2016/02/05 at 04:42:40, falken wrote:
> Was the comment about using childNeedsStyleRecalc() rather than just
needsStyleRecalc() useful?

Ah, the comment is still valid.  I add it again.

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:415: // e.g. Calling
Document::updateLayoutTreeIgnorePendingStylesheets().
On 2016/02/05 at 04:42:40, falken wrote:
> comment nits: one space after period is consistent with rest of comment, and
"Calling" -> "by calling" seems better.

Done.

https://codereview.chromium.org/1667623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Element.h:592: // However, we must not
retrieve layout information like position and size.
On 2016/02/05 at 04:42:40, falken wrote:
> "we" -> "it" is more consistent with the previous sentence

Done.

[tkent, 2016-02-05 04:58:08]

The CQ bit was checked by tkent@chromium.org

[tkent, 2016-02-05 04:58:08]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1667623002/#ps40001
(title: "update comments")

[commit-bot: I haz the power, 2016-02-05 04:58:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667623002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667623002/40001

[commit-bot: I haz the power, 2016-02-05 08:06:58]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-02-05 08:07:43]

Description was changed from

==========
Make sure Document::updateLayoutTree*() is called before Element::isFocusable().

BUG=251163
==========

to

==========
Make sure Document::updateLayoutTree*() is called before Element::isFocusable().

BUG=251163

Committed: https://crrev.com/401d7aa6f8dcd690badf4bf1145f17fd19cd599f
Cr-Commit-Position: refs/heads/master@{#373765}
==========

[commit-bot: I haz the power, 2016-02-05 08:07:44]

Patchset 2 (id:??) landed as
https://crrev.com/401d7aa6f8dcd690badf4bf1145f17fd19cd599f
Cr-Commit-Position: refs/heads/master@{#373765}