Fix for InspectorOverlay violating document lifecycle expectations

Fix for InspectorOverlay violating document lifecycle expectations

Under some circumstances it's possible for InspectorOverlay::update to
trigger InspectorOverlay::invalidate which can lead to a style recalc
within a layout which violates document lifecycle expectations.

For details of how the above could happen, see:
https://code.google.com/p/chromium/issues/detail?id=432129#c77

The fix is to make InspectorOverlay::invalidate do nothing
during an InspectorOverlay::update.

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

[alex clarke (OOO till 29th), 2014-12-04 14:46:18]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[caseq, 2014-12-04 14:47:50]

caseq@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[alex clarke (OOO till 29th), 2014-12-05 11:21:01]

Please take a look.  I'm not that familiar with this part of the code base, but
this patch seems to fix the problems.

If you want to try and reproduce the issue yourself, apply these patches: 
https://codereview.chromium.org/747993004/ and
https://codereview.chromium.org/768173003/ and run the
inspector/elements/styles/inject-stylesheet.html layout test.

[dgozman, 2014-12-06 12:34:18]

I will try this out on Monday to understand what happens.

https://codereview.chromium.org/777213002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorOverlay.cpp (right):

https://codereview.chromium.org/777213002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorOverlay.cpp:374: m_client->highlight();
This call usually just adds/removes a GraphicsLayer, which should not cause any
layouts or style invalidations.

Perhaps, the problem is in layout call here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

This only happens during page load when inspector overlay is triggered before
layout (e.g. breakpoint at the start). Should we just check that webview does
not already doing layout?

[alex clarke (OOO till 29th), 2014-12-08 15:44:40]

On 2014/12/06 12:34:18, dgozman wrote:
> I will try this out on Monday to understand what happens.
> 
>
https://codereview.chromium.org/777213002/diff/20001/Source/core/inspector/In...
> File Source/core/inspector/InspectorOverlay.cpp (right):
> 
>
https://codereview.chromium.org/777213002/diff/20001/Source/core/inspector/In...
> Source/core/inspector/InspectorOverlay.cpp:374: m_client->highlight();
> This call usually just adds/removes a GraphicsLayer, which should not cause
any
> layouts or style invalidations.
> 
> Perhaps, the problem is in layout call here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> This only happens during page load when inspector overlay is triggered before
> layout (e.g. breakpoint at the start). Should we just check that webview does
> not already doing layout?

That sounds like it should work, but how to detect if it's doing a layout?  I
tried inspecting
m_webViewImpl->mainFrameImpl()->frame()->document()->lifecycle().state() but its
always  DocumentLifecycle::StyleClean rather than one of the layout states.

Perhaps I should add a variable to WebDevToolsAgentImpl to track when it's in a
layout?

[Sami, 2014-12-08 16:07:11]

On 2014/12/08 15:44:40, alexclarke1 wrote:
> On 2014/12/06 12:34:18, dgozman wrote:
> > I will try this out on Monday to understand what happens.
> > 
> >
>
https://codereview.chromium.org/777213002/diff/20001/Source/core/inspector/In...
> > File Source/core/inspector/InspectorOverlay.cpp (right):
> > 
> >
>
https://codereview.chromium.org/777213002/diff/20001/Source/core/inspector/In...
> > Source/core/inspector/InspectorOverlay.cpp:374: m_client->highlight();
> > This call usually just adds/removes a GraphicsLayer, which should not cause
> any
> > layouts or style invalidations.
> > 
> > Perhaps, the problem is in layout call here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > This only happens during page load when inspector overlay is triggered
before
> > layout (e.g. breakpoint at the start). Should we just check that webview
does
> > not already doing layout?
> 
> That sounds like it should work, but how to detect if it's doing a layout?  I
> tried inspecting
> m_webViewImpl->mainFrameImpl()->frame()->document()->lifecycle().state() but
its
> always  DocumentLifecycle::StyleClean rather than one of the layout states.

Is this perhaps looking at a different document? Someone mentioned devtools
having multiple documents. The assert that was failing had to do with the
lifecycle being VisualUpdatePending, so clearly some document is in that state
:)

> Perhaps I should add a variable to WebDevToolsAgentImpl to track when it's in
a
> layout?

Not a bad idea but I'll let folks who know that code better comment.

[dgozman, 2014-12-08 17:11:18]

I was wrong with my assumptions. The problem occurs with an overlay document:
|m_client->highlight()| call causes a paint while update is in progress and many
styles are invalidated.

This fix is exactly what we need. lgtm

[alex clarke (OOO till 29th), 2014-12-08 17:14:02]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2014-12-08 17:14:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777213002/60001

[commit-bot: I haz the power, 2014-12-08 18:39:26]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186718