Fix renderer assert after autofill

Fix renderer assert after autofill

PasswordAutofillAgent may modify value of newFocusedNode in BlurEvent
dispatched above, while newFocusedNode->isFocusable() expects that
newFocusedNode->needsLayout() not to be true, updateLayout() here in
case that happens.

BUG=251163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158348

[Jiang Jiang, 2013-07-31 10:15:02]

Hi, could you please take a look at this CL and see it's the right approach?

[falken, 2013-07-31 18:27:01]

Elliott would be a good reviewer for this. I'm not sure it's OK to force a
layout here.

https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3178: // Autofill client may have modified the
value of newFocusNode, thus require
nit: newFocusNode -> newFocusedNode

[tkent, 2013-07-31 21:52:05]

I think we can reproduce the assertion failure without the autofill feature. A
blur event handler for the old focused element updates the style of the new
focused element, etc.  So, we can write a test.

https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3181: RenderObject* renderer = newFocusedNode ?
newFocusedNode->renderer() : 0;
It looks this code is not compilable.  There is no newFocusedNode variable in
this function.

[esprehn, 2013-07-31 22:20:25]

I'm not super comfortable adding a synchronous recalcStyle/ layout down there.
Are you sure this is safe with all the callers?

If you do add it you should do updateLayoutIgnorePendingStyleSheet() and not go
poking at the layout bits on the render tree.

https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:3183: updateLayout();
You shouldn't reach into the render tree and inspect layout state like this, and
manually calling updateLayout is definitely bad.

Perhaps you want to call updateLayoutIgnorePendingStylesheets() ? I see some
callers of setFocusedElement already do that.

Even then this looks scary, now every time we change the focused node you
trigger a style recalc and layout which is bad.

[Jiang Jiang, 2013-08-02 08:54:36]

On 2013/07/31 22:20:25, esprehn wrote:
> I'm not super comfortable adding a synchronous recalcStyle/ layout down there.
> Are you sure this is safe with all the callers?
> 
> If you do add it you should do updateLayoutIgnorePendingStyleSheet() and not
go
> poking at the layout bits on the render tree.
> 
> https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp#...
> Source/core/dom/Document.cpp:3183: updateLayout();
> You shouldn't reach into the render tree and inspect layout state like this,
and
> manually calling updateLayout is definitely bad.
> 
> Perhaps you want to call updateLayoutIgnorePendingStylesheets() ? I see some
> callers of setFocusedElement already do that.
> 
> Even then this looks scary, now every time we change the focused node you
> trigger a style recalc and layout which is bad.

Do you mean that I shouldn't check for renderer->needsLayout(), just call
updateLayoutIgnorePendingStylesheets() here? But I only want to trigger layout
when it actually need to.

Do you have an alternative solution? and how is calling renderer->needsLayout()
here different from calling it in newFocusedElement->isFocusable() just a few
lines below?

[esprehn, 2013-08-03 18:07:33]

On 2013/08/02 08:54:36, Jiang wrote:
> On 2013/07/31 22:20:25, esprehn wrote:
> > I'm not super comfortable adding a synchronous recalcStyle/ layout down
there.
> > Are you sure this is safe with all the callers?
> > 
> > If you do add it you should do updateLayoutIgnorePendingStyleSheet() and not
> go
> > poking at the layout bits on the render tree.
> > 
> > https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp
> > File Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/21346002/diff/1/Source/core/dom/Document.cpp#...
> > Source/core/dom/Document.cpp:3183: updateLayout();
> > You shouldn't reach into the render tree and inspect layout state like this,
> and
> > manually calling updateLayout is definitely bad.
> > 
> > Perhaps you want to call updateLayoutIgnorePendingStylesheets() ? I see some
> > callers of setFocusedElement already do that.
> > 
> > Even then this looks scary, now every time we change the focused node you
> > trigger a style recalc and layout which is bad.
> 
> Do you mean that I shouldn't check for renderer->needsLayout(), just call
> updateLayoutIgnorePendingStylesheets() here? But I only want to trigger layout
> when it actually need to.
> 
> Do you have an alternative solution? and how is calling
renderer->needsLayout()
> here different from calling it in newFocusedElement->isFocusable() just a few
> lines below?

updateLayout*() doesn't do anything if there's things that don't need layout.
Checking only that one renderer is not enough though, what if that renderer's
ancestors needed layout? That's equivalent but you'd miss that case.

[esprehn, 2013-08-04 01:12:41]

Sorry that was phrased super confusingly. Calling updateLayout if nothing needs
layout is a noop so it's fine to just call it.

[esprehn, 2013-09-25 20:03:04]

Lgtm, would be better with a test but this is okay. You should talk to isherman
about testing auto fill I think.

[Jiang Jiang, 2013-09-25 20:24:27]

Rebase

[commit-bot: I haz the power, 2013-09-25 20:24:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jiangj@opera.com/21346002/16001

[commit-bot: I haz the power, 2013-09-26 01:54:21]

Change committed as 158348

[esprehn, 2013-12-12 01:28:57]

On 2013/09/26 01:54:21, I haz the power (commit-bot) wrote:
> Change committed as 158348

Reverted in https://codereview.chromium.org/113093005/