Avoid calling isFocusable() during updateLayout.

Avoid calling isFocusable() during updateLayout.

isFocusable() expects the layout is up-to-date, but in the updateLayout code path
one callsite has existed.  The call could be delayed after the update layout
is complete, and the delayed callback already does the check again.
So it is safe to remove the callsite without breaking the whole logic.

This removes pre-condition to make a callback request, so there could be
extra callbacks could be created which might impact performance. Considering
the assertion was added 5+months ago and this is the first report,
hitting the code path should be very infrequent and performance impact
should be negligible.

BUG=627074
TEST=fast/dom/focus-and-update-layout-crash.html

[kochi, 2016-07-20 09:39:36]

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

[kochi, 2016-07-20 09:39:37]

tkent-san,
could you review this?

[kochi, 2016-07-20 09:39:42]

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

[commit-bot: I haz the power, 2016-07-20 09:40:01]

Dry run: 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-07-20 12:07:53]

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

[commit-bot: I haz the power, 2016-07-20 12:07:54]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-07-21 00:38:50]

https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1734: if (m_focusedElement &&
!m_focusedElement->isFocusable())
At this line, we already updated style and layout tree, didn't we?
Why we hit the DCHECK failure in isFocusable()?

[kochi, 2016-07-21 01:32:49]

https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1734: if (m_focusedElement &&
!m_focusedElement->isFocusable())
On 2016/07/21 00:38:50, tkent wrote:
> At this line, we already updated style and layout tree, didn't we?
> Why we hit the DCHECK failure in isFocusable()?

Hmm, yes.
Yesterday I saw another case that needsUpdateLayout flag is not cleared
after layout : https://bugs.chromium.org/p/chromium/issues/detail?id=605746
Maybe some other layout parts introduced a bug recently.

[esprehn, 2016-07-23 08:37:24]

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

[esprehn, 2016-07-23 08:37:25]

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

https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:1734: if (m_focusedElement)
Note that we can't land this change anyway, since clearFocusedElementSoon()
posts a task. This change would have made all calls to
updateStyleAndLayoutTree() post an unconditional additional task.

not lgtm

[esprehn, 2016-07-23 08:37:51]

On 2016/07/23 at 08:37:25, esprehn wrote:
>
https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:1734: if (m_focusedElement)
> Note that we can't land this change anyway, since clearFocusedElementSoon()
posts a task. This change would have made all calls to
updateStyleAndLayoutTree() post an unconditional additional task.
> 
> not lgtm

Sorry that's all calls when some element is focused. :)

[kochi, 2016-07-25 08:36:30]

On 2016/07/23 08:37:25, esprehn wrote:
>
https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2163893002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:1734: if (m_focusedElement)
> Note that we can't land this change anyway, since clearFocusedElementSoon()
> posts a task. This change would have made all calls to
> updateStyleAndLayoutTree() post an unconditional additional task.
> 
> not lgtm

Yeah, indeed.
I'm reinvestigating the issue and will have a new fix.