ASSERT on renderer lifecycle

ASSERT on renderer lifecycle

This CL adds document lifecycle assertions in for RenderObject::createObject,
RnederObject::addChild and RenderObject::removeChild.

In all cases we assert that we are currently in the InRecalcStyle state.

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

[ojan, 2014-05-06 05:48:16]

This looks like what I had in mind. Just a couple naming nits.

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3427: bool
RenderObject::isAllowedToAccessLifecycle(Document& document)
This name is a bit funny. I might call it something
like...isAllowedToModifyRenderTreeStructure or
renderTreeStructureModificationsAllowed. Those are kinda wordy. :(

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1286: class
DisableRenderObjectLifecycleAsserts {
I'd put Deprecated in the name so that it's clear in the calling locations that
this is a bad thing. Otherwise, people will just copy-paste them without reading
this header file's FIXME.

[ojan, 2014-05-06 05:51:09]

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:347:
ASSERT(isAllowedToAccessLifecycle(document()) || document().lifecycle().state()
>= DocumentLifecycle::Stopping);
I think in the end we want to remove the >= Stopping clause. We really shouldn't
need to do all the teardown that we do during stopping. It's silly that we go
and clear all the pointers and keep the tree in a consistent state when we're
just going to throw it away. But...for now, I'd be happy with a FIXME. The other
violations of this lifecycle assert are much more important than the performance
optimization of not doing so much teardown during Stopping.

[dsinclair, 2014-05-06 15:55:48]

I also filed bugs for each of the groups of disablings. There is a meta tracking
bug at crbug.com/370457.

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:347:
ASSERT(isAllowedToAccessLifecycle(document()) || document().lifecycle().state()
>= DocumentLifecycle::Stopping);
On 2014/05/06 05:51:10, ojan wrote:
> I think in the end we want to remove the >= Stopping clause. We really
shouldn't
> need to do all the teardown that we do during stopping. It's silly that we go
> and clear all the pointers and keep the tree in a consistent state when we're
> just going to throw it away. But...for now, I'd be happy with a FIXME. The
other
> violations of this lifecycle assert are much more important than the
performance
> optimization of not doing so much teardown during Stopping.

Done.

This ended up being covered by the disabler in
RenderObject::destroyAndCleanupAnonymousWrappers(). When we fix crbug.com/370463
we may want to put this back in, or fix the teardown code at the same time.

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3427: bool
RenderObject::isAllowedToAccessLifecycle(Document& document)
On 2014/05/06 05:48:17, ojan wrote:
> This name is a bit funny. I might call it something
> like...isAllowedToModifyRenderTreeStructure or
> renderTreeStructureModificationsAllowed. Those are kinda wordy. :(

Done.

Thanks, I couldn't think of anything good. I don't mind wordy.

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/267053002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1286: class
DisableRenderObjectLifecycleAsserts {
On 2014/05/06 05:48:17, ojan wrote:
> I'd put Deprecated in the name so that it's clear in the calling locations
that
> this is a bad thing. Otherwise, people will just copy-paste them without
reading
> this header file's FIXME.

Done.

[dsinclair, 2014-05-06 18:01:44]

PTAL.

[esprehn, 2014-05-06 18:24:28]

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3432: if (gModifyRenderTreeStructureMode
== ModifyRenderTreeStructureAnyState)
Put this bool in the disabler itself, then ask the disabler. Remove the enum.

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3434: return document.lifecycle().state()
== DocumentLifecycle::InStyleRecalc;
This should be a method on DocumentLifecycle, there's already one about DOM
mutations there. Just add another for this.

[leviw_travelin_and_unemployed, 2014-05-06 18:41:29]

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:4041: // style. crbug.com/370458
I'd vote for a slightly broader comment to the effect that we shouldn't modify
the render tree during layout, not that we should only modify it in recalc
style.

[dsinclair, 2014-05-07 16:24:39]

PTAL.

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:4041: // style. crbug.com/370458
On 2014/05/06 18:41:29, leviw wrote:
> I'd vote for a slightly broader comment to the effect that we shouldn't modify
> the render tree during layout, not that we should only modify it in recalc
> style.

Done.

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3432: if (gModifyRenderTreeStructureMode
== ModifyRenderTreeStructureAnyState)
On 2014/05/06 18:24:29, esprehn wrote:
> Put this bool in the disabler itself, then ask the disabler. Remove the enum.

Done.

https://codereview.chromium.org/267053002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3434: return document.lifecycle().state()
== DocumentLifecycle::InStyleRecalc;
On 2014/05/06 18:24:29, esprehn wrote:
> This should be a method on DocumentLifecycle, there's already one about DOM
> mutations there. Just add another for this.

Done.

[eseidel, 2014-05-07 16:48:21]

https://codereview.chromium.org/267053002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2780:
DeprecatedDisableModifyRenderTreeStructureAsserts disabler;
It seems like the bug talked about moving this up into Node::detach()?

[dsinclair, 2014-05-07 20:32:33]

https://codereview.chromium.org/267053002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2780:
DeprecatedDisableModifyRenderTreeStructureAsserts disabler;
On 2014/05/07 16:48:22, eseidel wrote:
> It seems like the bug talked about moving this up into Node::detach()?


Done.

I kept the fixme over the move as it sounds like there is still some question on
the correct behaviour.

[esprehn, 2014-05-08 05:41:25]

It's correct to modify the render tree inside ::detach outside recalcStyle, that
happens inside removeChild. Otherwise we'd allow a detached Element to still
have a renderer which is bad.

We can land this and iterate, but I'd suggest a different approach where we add
a bit to DocumentLifecycle that's "static int s_detachCount" and
increment/decrement that when detaching to avoid the asserts in
RenderObjectChildList.

void Node::detach()
{
   ASSERT(document().lifecycle().stateAllowsDetach());
   DocumentLifecycle::DetachScope willDetach(document().lifecycle());

   ...
}

DocumentLifecycle::DetachScope's constructor increments the counter,
it's destructor decrements it.

stateAllowsDetach() returns m_state != InPerformLayout && m_state !=
InCompositingUpdate which brings some sanity there.

DocumentLifecycle::stateAllowsRenderTreeMutations() returns
  m_detachCount || m_state == InStyleRecalc.

but lgtm, we can make these changes next.

https://codereview.chromium.org/267053002/diff/140001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/267053002/diff/140001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:999: // layout. crbug.com/370463
This doesn't need a fix me, it's correct to modify the render tree inside
::detach.

https://codereview.chromium.org/267053002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:3433: :
m_disabler(gModifyRenderTreeStructureAnyState, true) { }
These braces are placed wrong.

[dsinclair, 2014-05-08 19:32:10]

https://codereview.chromium.org/267053002/diff/140001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/267053002/diff/140001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:999: // layout. crbug.com/370463
On 2014/05/08 05:41:25, esprehn wrote:
> This doesn't need a fix me, it's correct to modify the render tree inside
> ::detach.

Done.

https://codereview.chromium.org/267053002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/267053002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:3433: :
m_disabler(gModifyRenderTreeStructureAnyState, true) { }
On 2014/05/08 05:41:25, esprehn wrote:
> These braces are placed wrong.

Done.

[dsinclair, 2014-05-08 19:32:44]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-05-08 19:34:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dsinclair@chromium.org/267053002/160001

[commit-bot: I haz the power, 2014-05-08 20:52:29]

Change committed as 173677