- Revision
- 164928
- Author
- [email protected]
- Date
- 2014-03-01 15:22:56 -0800 (Sat, 01 Mar 2014)
Log Message
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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (164927 => 164928)
--- trunk/Source/WebCore/ChangeLog 2014-03-01 23:05:02 UTC (rev 164927)
+++ trunk/Source/WebCore/ChangeLog 2014-03-01 23:22:56 UTC (rev 164928)
@@ -1,3 +1,45 @@
+2014-03-01 Darin Adler <[email protected]>
+
+ 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 <[email protected]>
Optimized querySelector(All) when selector contains #id
Modified: trunk/Source/WebCore/rendering/InlineBox.cpp (164927 => 164928)
--- trunk/Source/WebCore/rendering/InlineBox.cpp 2014-03-01 23:05:02 UTC (rev 164927)
+++ trunk/Source/WebCore/rendering/InlineBox.cpp 2014-03-01 23:22:56 UTC (rev 164928)
@@ -43,6 +43,7 @@
float c;
uint32_t d : 32;
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+ unsigned s;
bool f;
#endif
};
@@ -50,11 +51,31 @@
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 @@
}
#ifndef NDEBUG
+
const char* InlineBox::boxName() const
{
return "InlineBox";
@@ -101,6 +123,7 @@
fputc(' ', stderr);
fprintf(stderr, "\t%s %p\n", renderer().renderName(), &renderer());
}
+
#endif
float InlineBox::logicalHeight() const
Modified: trunk/Source/WebCore/rendering/InlineBox.h (164927 => 164928)
--- trunk/Source/WebCore/rendering/InlineBox.h 2014-03-01 23:05:02 UTC (rev 164927)
+++ trunk/Source/WebCore/rendering/InlineBox.h 2014-03-01 23:22:56 UTC (rev 164928)
@@ -37,6 +37,8 @@
public:
virtual ~InlineBox();
+ void assertNotDeleted() const;
+
virtual void deleteLine() = 0;
virtual void extractLine() = 0;
virtual void attachLine() = 0;
@@ -69,7 +71,6 @@
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 @@
InlineFlowBox* parent() const
{
+ assertNotDeleted();
ASSERT_WITH_SECURITY_IMPLICATION(!m_hasBadParent);
return m_parent;
}
@@ -237,6 +239,7 @@
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
void setHasBadParent();
+ void invalidateParentChildList();
#endif
int expansion() const { return m_bitfields.expansion(); }
@@ -368,6 +371,7 @@
, m_renderer(renderer)
, m_logicalWidth(0)
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
+ , m_deletionSentinel(deletionSentinelNotDeletedValue)
, m_hasBadParent(false)
#endif
{
@@ -383,6 +387,7 @@
, 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 @@
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 @@
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
Modified: trunk/Source/WebCore/rendering/InlineFlowBox.cpp (164927 => 164928)
--- trunk/Source/WebCore/rendering/InlineFlowBox.cpp 2014-03-01 23:05:02 UTC (rev 164927)
+++ trunk/Source/WebCore/rendering/InlineFlowBox.cpp 2014-03-01 23:22:56 UTC (rev 164928)
@@ -50,13 +50,22 @@
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::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
}
Modified: trunk/Source/WebCore/rendering/InlineFlowBox.h (164927 => 164928)
--- trunk/Source/WebCore/rendering/InlineFlowBox.h 2014-03-01 23:05:02 UTC (rev 164927)
+++ trunk/Source/WebCore/rendering/InlineFlowBox.h 2014-03-01 23:22:56 UTC (rev 164928)
@@ -352,18 +352,21 @@
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
Modified: trunk/Source/WebCore/rendering/RenderTextLineBoxes.cpp (164927 => 164928)
--- trunk/Source/WebCore/rendering/RenderTextLineBoxes.cpp 2014-03-01 23:05:02 UTC (rev 164927)
+++ trunk/Source/WebCore/rendering/RenderTextLineBoxes.cpp 2014-03-01 23:22:56 UTC (rev 164928)
@@ -697,10 +697,8 @@
#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
