ASSERT on renderer lifecycle

Source/core/rendering/RenderObject.cpp

Index: Source/core/rendering/RenderObject.cpp
diff --git a/Source/core/rendering/RenderObject.cpp b/Source/core/rendering/RenderObject.cpp
index cfc8dc672b84f99ed6b950e27c8a718ec2fb1343..90a9baac5d4d2a947b765bfc1c34f186251361c9 100644
--- a/Source/core/rendering/RenderObject.cpp
+++ b/Source/core/rendering/RenderObject.cpp
@@ -96,6 +96,12 @@ using namespace std;
 
 namespace WebCore {
 
+namespace {
+
+static RenderObjectLifecycleMode gRenderObjectLifecycleMode = RenderObjectLifecycleUpdatesOnlyInRecalcStyle;
+
+} // namespace
+
 using namespace HTMLNames;
 
 #ifndef NDEBUG
@@ -143,6 +149,8 @@ void RenderObject::operator delete(void* ptr)
 
 RenderObject* RenderObject::createObject(Element* element, RenderStyle* style)
 {
+    ASSERT(isAllowedToAccessLifecycle(element->document()));
+
     // Minimal support for content properties replacing an entire element.
     // Works only if we have exactly one piece of content and it's a URL.
     // Otherwise acts as if we didn't support this feature.
@@ -296,6 +304,8 @@ bool RenderObject::requiresAnonymousTableWrappers(const RenderObject* newChild)
 
 void RenderObject::addChild(RenderObject* newChild, RenderObject* beforeChild)
 {
+    ASSERT(isAllowedToAccessLifecycle(document()));
+
     RenderObjectChildList* children = virtualChildren();
     ASSERT(children);
     if (!children)
@@ -334,6 +344,8 @@ void RenderObject::addChild(RenderObject* newChild, RenderObject* beforeChild)
 
 void RenderObject::removeChild(RenderObject* oldChild)
 {
+    ASSERT(isAllowedToAccessLifecycle(document()) || document().lifecycle().state() >= DocumentLifecycle::Stopping);

[ojan, 2014/05/06 05:51:10]

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:49]

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.

+
     RenderObjectChildList* children = virtualChildren();
     ASSERT(children);
     if (!children)
@@ -2763,6 +2775,8 @@ void RenderObject::removeFromRenderFlowThreadRecursive(RenderFlowThread* renderF
 
 void RenderObject::destroyAndCleanupAnonymousWrappers()
 {
+    DisableRenderObjectLifecycleAsserts disabler;
+
     // If the tree is destroyed, there is no need for a clean-up phase.
     if (documentBeingDestroyed()) {
         destroy();
@@ -3410,6 +3424,16 @@ void RenderObject::clearRepaintState()
     setMayNeedInvalidation(false);
 }
 
+bool RenderObject::isAllowedToAccessLifecycle(Document& document)

[ojan, 2014/05/06 05:48:17]

This name is a bit funny. I might call it something
like...isAllowedToModifyRenderTreeStructure or
renderTreeStructureModificationsAllowed. Those are kinda wordy. :(

[dsinclair, 2014/05/06 15:55:49]

Done.

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

+{
+    if (gRenderObjectLifecycleMode == RenderObjectLifecycleUpdatesAnyState)
+        return true;
+    return document.lifecycle().state() == DocumentLifecycle::InStyleRecalc;
+}
+
+DisableRenderObjectLifecycleAsserts::DisableRenderObjectLifecycleAsserts()
+    : m_disabler(gRenderObjectLifecycleMode, RenderObjectLifecycleUpdatesAnyState) { }
+
 } // namespace WebCore
 
 #ifndef NDEBUG