Enforce clearing renderers' paint invalidation state

Enforce clearing renderers' paint invalidation state

The ASSERT checks that no flags cleared by clearPaintInvalidationState()
is still set up after calling repaint-after-compositing-update.

The ASSERT is skipped for SVG subtree as we have an optimization that
skips them. We could reproduce the exact condition but it seems not
very useful to do so.

The change caught several bugs (yay!):
- one in shouldCheckForPaintInvalidationAfterLayout()
that made the ASSERT trip on fast/repaint/table-row.html.
- one where setShouldDoFullPaintInvalidationAfterLayout didn't
mark the containing block chain for invalidation.

BUG=385169
R=dsinclair@chromium.org, leviw@chromium.org

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

[dsinclair, 2014-07-11 20:48:55]

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3400:
ASSERT(paintInvalidationStateIsDirty());
Is this needed? We're basically checking that we never clear the state of a
renderer that wasn't dirty?

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:251: if (paintInvalidationStateIsDirty()) {
if (!paintInvalidationStateIsDirt())
  return;

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:252: showRenderTreeForThis();
Is this included on purpose?

[Julien - ping for review, 2014-07-11 20:52:46]

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:3400:
ASSERT(paintInvalidationStateIsDirty());
On 2014/07/11 20:48:55, dsinclair wrote:
> Is this needed? We're basically checking that we never clear the state of a
> renderer that wasn't dirty?

It's here for 2 reasons:
- Ensure that clearPaintInvalidationState and paintInvalidationStateIsDirty stay
in sync.
- Make sure we don't call this function on renderers that don't need it (and we
don't :)). We have seen with SVG that tree walks can be expensive so I think
it's a good ASSERT to have.

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:251: if (paintInvalidationStateIsDirty()) {
On 2014/07/11 20:48:55, dsinclair wrote:
> if (!paintInvalidationStateIsDirt())
>   return;

A'right.

https://codereview.chromium.org/389573008/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:252: showRenderTreeForThis();
On 2014/07/11 20:48:55, dsinclair wrote:
> Is this included on purpose?

It is, see assertRendererLaidOut() above. The reason is that it makes debugging
way easier.

[dsinclair, 2014-07-11 20:55:00]

lgtm

[leviw_travelin_and_unemployed, 2014-07-14 17:32:54]

lgtm2!

[Julien - ping for review, 2014-07-14 17:57:27]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-14 17:58:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/389573008/20001

[commit-bot: I haz the power, 2014-07-14 18:39:46]

Change committed as 178089

[falken, 2014-07-15 01:38:12]

On 2014/07/14 18:39:46, I haz the power (commit-bot) wrote:
> Change committed as 178089

Some ChromeOS browser tests are hitting this assertion:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...

I think I have to roll it out to unblock the Blink roll.

[falken, 2014-07-15 02:10:41]

On 2014/07/15 01:38:12, falken wrote:
> On 2014/07/14 18:39:46, I haz the power (commit-bot) wrote:
> > Change committed as 178089
> 
> Some ChromeOS browser tests are hitting this assertion:
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
> 
> I think I have to roll it out to unblock the Blink roll.

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

[Julien - ping for review, 2014-07-18 21:53:38]

On 2014/07/15 at 02:10:41, falken wrote:
> On 2014/07/15 01:38:12, falken wrote:
> > On 2014/07/14 18:39:46, I haz the power (commit-bot) wrote:
> > > Change committed as 178089
> > 
> > Some ChromeOS browser tests are hitting this assertion:
> >
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Tes...
> > 
> > I think I have to roll it out to unblock the Blink roll.
> 
> Reverted in https://codereview.chromium.org/393673003/

I have fixed the CrOS issues, it was related to
shouldDoFullPaintInvalidationIfSelfPaintingLayer and is another instance of
forgetting to account for / mark the container block chain in this case.

Unfortunately I couldn't get a reduction nor do I fully understand how it is
possible to get in this situation.

[Julien - ping for review, 2014-07-18 23:47:23]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:47:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/389573008/60001

[commit-bot: I haz the power, 2014-07-19 04:21:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16233)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17849)

[commit-bot: I haz the power, 2014-07-19 05:00:28]

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

[commit-bot: I haz the power, 2014-07-19 05:00:29]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16242)

[Julien - ping for review, 2014-07-21 15:22:19]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-21 15:23:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/389573008/70001

[commit-bot: I haz the power, 2014-07-21 16:27:10]

Change committed as 178569

[Peter Kasting, 2014-07-21 23:37:25]

A revert of this CL has been created in
https://codereview.chromium.org/408083002/ by pkasting@chromium.org.

The reason for reverting is: Causing consistent assertion failures on Mac.  See
e.g.
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28db...
.  A sample stack:

STDERR: SHOULD NEVER BE REACHED
STDERR: ../../third_party/WebKit/Source/core/rendering/RenderObject.h(258) :
void blink::RenderObject::assertRendererClearedPaintInvalidationState() const
STDERR: 1   0x99d08a7
blink::RenderObject::assertRendererClearedPaintInvalidationState() const
STDERR: 2   0x99bc4ee
blink::RenderObject::assertSubtreeClearedPaintInvalidationState() const
STDERR: 3   0x99af007 blink::FrameView::invalidateTreeIfNeeded()
STDERR: 4   0x99b82ee blink::FrameView::invalidateTreeIfNeededRecursive()
STDERR: 5   0x99b710d blink::FrameView::updateLayoutAndStyleForPainting()
STDERR: 6   0x9d169f2
blink::PageAnimator::updateLayoutAndStyleForPainting(blink::LocalFrame*)
STDERR: 7   0x709b4fb blink::PageWidgetDelegate::layout(blink::Page*,
blink::LocalFrame*)
STDERR: 8   0x720c8a4 blink::WebViewImpl::layout()
STDERR: 9   0xfc6ee content::EventSender::MouseMoveTo(gin::Arguments*)
STDERR: 10  0xf9be7 content::EventSenderBindings::MouseMoveTo(gin::Arguments*)
STDERR: 11  0x113572 base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(gin::Arguments*)>::Run(content::EventSenderBindings*,
gin::Arguments* const&)
STDERR: 12  0x1134af base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(gin::Arguments*)>, void
()(content::EventSenderBindings* const&, gin::Arguments*
const&)>::MakeItSo(base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(gin::Arguments*)>,
content::EventSenderBindings* const&, gin::Arguments* const&)
STDERR: 13  0x11342b base::internal::Invoker<0,
base::internal::BindState<base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(gin::Arguments*)>, void
()(content::EventSenderBindings*, gin::Arguments*), void ()()>, void
()(content::EventSenderBindings*,
gin::Arguments*)>::Run(base::internal::BindStateBase*,
content::EventSenderBindings* const&, gin::Arguments* const&)
STDERR: 14  0x113c03 base::Callback<void ()(content::EventSenderBindings*,
gin::Arguments*)>::Run(content::EventSenderBindings* const&, gin::Arguments*
const&) const
STDERR: 15  0x113b5e gin::internal::Invoker<void, content::EventSenderBindings*,
gin::Arguments*, void, void, void, void>::Go(gin::Arguments*,
base::Callback<void ()(content::EventSenderBindings*, gin::Arguments*)> const&,
content::EventSenderBindings* const&, gin::Arguments* const&)
STDERR: 16  0x113ab5 gin::internal::Dispatcher<void
()(content::EventSenderBindings*,
gin::Arguments*)>::DispatchToCallback(v8::FunctionCallbackInfo<v8::Value>
const&)
STDERR: 17  0x4f44b1e v8::internal::FunctionCallbackArguments::Call(void
(*)(v8::FunctionCallbackInfo<v8::Value> const&))
STDERR: 18  0x4f9c17e
_ZN2v88internalL21Builtin_HandleApiCallEiPPNS0_6ObjectEPNS0_7IsolateE
STDERR: 19  0x4150a4b6.

[Xianzhu, 2014-07-22 16:13:56]

jchaffraix@, before resolving the assertion failure, could you create a change
to add markContainingBlockChainForPaintInvalidation() first? My changes depend
on it.

[Julien - ping for review, 2014-07-22 19:39:52]

On 2014/07/22 at 16:13:56, wangxianzhu wrote:
> jchaffraix@, before resolving the assertion failure, could you create a change
to add markContainingBlockChainForPaintInvalidation() first? My changes depend
on it.

I don't really think we should land anything that makes the code more complex
until we have this ASSERT in place. That's because we have already uncovered
tons of bugs by having it and I wish we had had it earlier.

Also the gardener unfortunately didn't give us the courtesy of notifying ahead
of the revert and now we are stuck for a single test on 2 platforms for which we
have no try job coverage. This change is super important and blocking other
people so I will be disabling this test and reland the change while I
investigate the issue.

[Julien - ping for review, 2014-07-22 20:19:09]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-07-22 20:19:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jchaffraix@chromium.org/389573008/90001

[commit-bot: I haz the power, 2014-07-22 23:34:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-23 01:28:47]

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

[commit-bot: I haz the power, 2014-07-23 01:28:48]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Julien - ping for review, 2014-07-23 01:44:06]

Committed patchset #7 manually as r178719 (presubmit successful).