Improve "bad parent" and "bad child list" assertions in line boxes
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 1 Mar 2014 23:22:56 +0000 (23:22 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 1 Mar 2014 23:22:56 +0000 (23:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=125656

Reviewed by Sam Weinig.

My previous fix for this problem was incomplete. This continuation of that fix addresses
the flaw in the original and adds additional lifetime checking so problems can be seen in
debug builds without a memory debugger.

* rendering/InlineBox.cpp:
(WebCore::InlineBox::assertNotDeleted): Added. Poor man's memory debugging helper.
(WebCore::InlineBox::~InlineBox): Refactored body into a new function named
invalidateParentChildList. Added code to update the deletion sentinel to record
that this object is deleted.
(WebCore::InlineBox::setHasBadParent): Moved here from header since this debug-only
feature does not need to be inlined. Added a call to assertNotDeleted.
(WebCore::InlineBox::invalidateParentChildList): Added. Refactored from the destructor,
this is used by RenderTextLineBoxes.

* rendering/InlineBox.h: Added the deletion sentinel, and called it in the parent
function. Also changed the expansion/setExpansion functions to use the type name "int",
since we don't use the type name "signed" in the WebKit coding style.

* rendering/InlineFlowBox.cpp:
(WebCore::InlineFlowBox::~InlineFlowBox): Call setHasBadChildList rather than doing the
setHasBadParent work on children directly, to avoid code duplication.
(WebCore::InlineFlowBox::setHasBadChildList): Moved here from header. Added code to set
"has bad parent" on all children, something we previously did only on destruction. Also
added assertNotDeleted.
(WebCore::InlineFlowBox::checkConsistency): Added call to assertNotDeleted. Also tweaked
code style and variable names a little bit.

* rendering/InlineFlowBox.h: Moved setHasBadChildList out of the header when it's on.
The empty version for ASSERT_WITH_SECURITY_IMPLICATION_DISABLED is still in the header.

* rendering/RenderTextLineBoxes.cpp:
(WebCore::RenderTextLineBoxes::invalidateParentChildLists): Call the new
InlineBox::invalidateParentChildList function instead of calling setHasBadChildList directly.
The new function checks m_hasBadParent, something we couldn't do here.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@164928 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/InlineBox.cpp
Source/WebCore/rendering/InlineBox.h
Source/WebCore/rendering/InlineFlowBox.cpp
Source/WebCore/rendering/InlineFlowBox.h
Source/WebCore/rendering/RenderTextLineBoxes.cpp

index 117e5a2..8910a79 100644 (file)
@@ -1,3 +1,45 @@
+2014-03-01  Darin Adler  <darin@apple.com>
+
+        Improve "bad parent" and "bad child list" assertions in line boxes
+        https://bugs.webkit.org/show_bug.cgi?id=125656
+
+        Reviewed by Sam Weinig.
+
+        My previous fix for this problem was incomplete. This continuation of that fix addresses
+        the flaw in the original and adds additional lifetime checking so problems can be seen in
+        debug builds without a memory debugger.
+
+        * rendering/InlineBox.cpp:
+        (WebCore::InlineBox::assertNotDeleted): Added. Poor man's memory debugging helper.
+        (WebCore::InlineBox::~InlineBox): Refactored body into a new function named
+        invalidateParentChildList. Added code to update the deletion sentinel to record
+        that this object is deleted.
+        (WebCore::InlineBox::setHasBadParent): Moved here from header since this debug-only
+        feature does not need to be inlined. Added a call to assertNotDeleted.
+        (WebCore::InlineBox::invalidateParentChildList): Added. Refactored from the destructor,
+        this is used by RenderTextLineBoxes.
+
+        * rendering/InlineBox.h: Added the deletion sentinel, and called it in the parent
+        function. Also changed the expansion/setExpansion functions to use the type name "int",
+        since we don't use the type name "signed" in the WebKit coding style.
+
+        * rendering/InlineFlowBox.cpp:
+        (WebCore::InlineFlowBox::~InlineFlowBox): Call setHasBadChildList rather than doing the
+        setHasBadParent work on children directly, to avoid code duplication.
+        (WebCore::InlineFlowBox::setHasBadChildList): Moved here from header. Added code to set
+        "has bad parent" on all children, something we previously did only on destruction. Also
+        added assertNotDeleted.
+        (WebCore::InlineFlowBox::checkConsistency): Added call to assertNotDeleted. Also tweaked
+        code style and variable names a little bit.
+
+        * rendering/InlineFlowBox.h: Moved setHasBadChildList out of the header when it's on.
+        The empty version for ASSERT_WITH_SECURITY_IMPLICATION_DISABLED is still in the header.
+
+        * rendering/RenderTextLineBoxes.cpp:
+        (WebCore::RenderTextLineBoxes::invalidateParentChildLists): Call the new
+        InlineBox::invalidateParentChildList function instead of calling setHasBadChildList directly.
+        The new function checks m_hasBadParent, something we couldn't do here.
+
 2014-03-01  Benjamin Poulain  <benjamin@webkit.org>
 
         Optimized querySelector(All) when selector contains #id
index eb2b90e..2f3cec4 100644 (file)
@@ -43,6 +43,7 @@ struct SameSizeAsInlineBox {
     float c;
     uint32_t d : 32;
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+    unsigned s;
     bool f;
 #endif
 };
@@ -50,11 +51,31 @@ struct SameSizeAsInlineBox {
 COMPILE_ASSERT(sizeof(InlineBox) == sizeof(SameSizeAsInlineBox), InlineBox_size_guard);
 
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+
+void InlineBox::assertNotDeleted() const
+{
+    ASSERT(m_deletionSentinel == deletionSentinelNotDeletedValue);
+}
+
 InlineBox::~InlineBox()
 {
+    invalidateParentChildList();
+    m_deletionSentinel = deletionSentinelDeletedValue;
+}
+
+void InlineBox::setHasBadParent()
+{
+    assertNotDeleted();
+    m_hasBadParent = true;
+}
+
+void InlineBox::invalidateParentChildList()
+{
+    assertNotDeleted();
     if (!m_hasBadParent && m_parent)
         m_parent->setHasBadChildList();
 }
+
 #endif
 
 void InlineBox::removeFromParent()
@@ -64,6 +85,7 @@ void InlineBox::removeFromParent()
 }
 
 #ifndef NDEBUG
+
 const char* InlineBox::boxName() const
 {
     return "InlineBox";
@@ -101,6 +123,7 @@ void InlineBox::showBox(int printedCharacters) const
         fputc(' ', stderr);
     fprintf(stderr, "\t%s %p\n", renderer().renderName(), &renderer());
 }
+
 #endif
 
 float InlineBox::logicalHeight() const
index f296067..e76056f 100644 (file)
@@ -37,6 +37,8 @@ class InlineBox {
 public:
     virtual ~InlineBox();
 
+    void assertNotDeleted() const;
+
     virtual void deleteLine() = 0;
     virtual void extractLine() = 0;
     virtual void attachLine() = 0;
@@ -69,7 +71,6 @@ public:
     virtual void paint(PaintInfo&, const LayoutPoint&, LayoutUnit lineTop, LayoutUnit lineBottom) = 0;
     virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom) = 0;
 
-public:
 #ifndef NDEBUG
     void showTreeForThis() const;
     void showLineTreeForThis() const;
@@ -148,6 +149,7 @@ public:
 
     InlineFlowBox* parent() const
     {
+        assertNotDeleted();
         ASSERT_WITH_SECURITY_IMPLICATION(!m_hasBadParent);
         return m_parent;
     }
@@ -237,6 +239,7 @@ public:
 
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
     void setHasBadParent();
+    void invalidateParentChildList();
 #endif
 
     int expansion() const { return m_bitfields.expansion(); }
@@ -368,6 +371,7 @@ protected:
         , m_renderer(renderer)
         , m_logicalWidth(0)
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+        , m_deletionSentinel(deletionSentinelNotDeletedValue)
         , m_hasBadParent(false)
 #endif
     {
@@ -383,6 +387,7 @@ protected:
         , m_logicalWidth(logicalWidth)
         , m_bitfields(firstLine, constructed, dirty, extracted, isHorizontal)
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+        , m_deletionSentinel(deletionSentinelNotDeletedValue)
         , m_hasBadParent(false)
 #endif
     {
@@ -401,14 +406,17 @@ protected:
     void setHasHyphen(bool hasHyphen) { m_bitfields.setHasEllipsisBoxOrHyphen(hasHyphen); }    
     bool canHaveLeadingExpansion() const { return m_bitfields.hasSelectedChildrenOrCanHaveLeadingExpansion(); }
     void setCanHaveLeadingExpansion(bool canHaveLeadingExpansion) { m_bitfields.setHasSelectedChildrenOrCanHaveLeadingExpansion(canHaveLeadingExpansion); }
-    signed expansion() { return m_bitfields.expansion(); }
-    void setExpansion(signed expansion) { m_bitfields.setExpansion(expansion); }
+    int expansion() { return m_bitfields.expansion(); }
+    void setExpansion(int expansion) { m_bitfields.setExpansion(expansion); }
     
     // For InlineFlowBox and InlineTextBox
     bool extracted() const { return m_bitfields.extracted(); }
 
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
 private:
+    static const unsigned deletionSentinelNotDeletedValue = 0xF0F0F0F0U;
+    static const unsigned deletionSentinelDeletedValue = 0xF0DEADF0U;
+    unsigned m_deletionSentinel;
     bool m_hasBadParent;
 #endif
 };
@@ -417,16 +425,15 @@ private:
     TYPE_CASTS_BASE(ToValueTypeName, InlineBox, object, object->predicate, object.predicate)
 
 #if ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+
 inline InlineBox::~InlineBox()
 {
 }
-#endif
 
-#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
-inline void InlineBox::setHasBadParent()
+inline void InlineBox::assertNotDeleted() const
 {
-    m_hasBadParent = true;
 }
+
 #endif
 
 } // namespace WebCore
index 12fb613..60f2aa8 100644 (file)
@@ -50,13 +50,22 @@ struct SameSizeAsInlineFlowBox : public InlineBox {
 COMPILE_ASSERT(sizeof(InlineFlowBox) == sizeof(SameSizeAsInlineFlowBox), InlineFlowBox_should_stay_small);
 
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+
 InlineFlowBox::~InlineFlowBox()
 {
-    if (!m_hasBadChildList) {
-        for (InlineBox* child = firstChild(); child; child = child->nextOnLine())
-            child->setHasBadParent();
-    }
+    setHasBadChildList();
+}
+
+void InlineFlowBox::setHasBadChildList()
+{
+    assertNotDeleted();
+    if (m_hasBadChildList)
+        return;
+    for (InlineBox* child = firstChild(); child; child = child->nextOnLine())
+        child->setHasBadParent();
+    m_hasBadChildList = true;
 }
+
 #endif
 
 LayoutUnit InlineFlowBox::getFlowSpacingLogicalWidth()
@@ -1664,15 +1673,16 @@ void InlineFlowBox::showLineTreeAndMark(const InlineBox* markedBox1, const char*
 
 void InlineFlowBox::checkConsistency() const
 {
+    assertNotDeleted();
     ASSERT_WITH_SECURITY_IMPLICATION(!m_hasBadChildList);
 #ifdef CHECK_CONSISTENCY
-    const InlineBox* prev = 0;
-    for (const InlineBox* child = m_firstChild; child; child = child->nextOnLine()) {
+    const InlineBox* previousChild = nullptr;
+    for (const InlineBox* child = firstChild(); child; child = child->nextOnLine()) {
         ASSERT(child->parent() == this);
-        ASSERT(child->prevOnLine() == prev);
-        prev = child;
+        ASSERT(child->prevOnLine() == previousChild);
+        previousChild = child;
     }
-    ASSERT(prev == m_lastChild);
+    ASSERT(previousChild == m_lastChild);
 #endif
 }
 
index f0eabcb..ebe0aa7 100644 (file)
@@ -352,18 +352,21 @@ private:
 INLINE_BOX_OBJECT_TYPE_CASTS(InlineFlowBox, isInlineFlowBox())
 
 #ifdef NDEBUG
+
 inline void InlineFlowBox::checkConsistency() const
 {
 }
+
 #endif
 
+#if ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+
 inline void InlineFlowBox::setHasBadChildList()
 {
-#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
-    m_hasBadChildList = true;
-#endif
 }
 
+#endif
+
 } // namespace WebCore
 
 #ifndef NDEBUG
index 4ad4c15..45d5183 100644 (file)
@@ -697,10 +697,8 @@ RenderTextLineBoxes::~RenderTextLineBoxes()
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
 void RenderTextLineBoxes::invalidateParentChildLists()
 {
-    for (auto box = m_first; box; box = box->nextTextBox()) {
-        if (auto parent = box->parent())
-            parent->setHasBadChildList();
-    }
+    for (auto box = m_first; box; box = box->nextTextBox())
+        box->invalidateParentChildList();
 }
 #endif