- Revision
- 120862
- Author
- [email protected]
- Date
- 2012-06-20 14:20:29 -0700 (Wed, 20 Jun 2012)
Log Message
Crash on accessing a removed renderer from percent height descendant map.
https://bugs.webkit.org/show_bug.cgi?id=88017
Reviewed by Eric Seidel.
Source/WebCore:
Test: fast/block/percent-height-descendant-not-removed-crash2.html
* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::hasPercentHeightContainerMap): helper to tell
if we have a height container map.
(WebCore):
(WebCore::RenderBlock::hasPercentHeightDescendant): change from a debug
only function to a regular function for use. no need to null check
for a percent height container map in this function.
(WebCore::RenderBlock::clearPercentHeightDescendantsFrom): helper to
clear all percent height descendants under us.
(WebCore::RenderBlock::removePercentHeightDescendantIfNeeded): helper to
clear the box if it exists in the percent height descendant map.
* rendering/RenderBlock.h:
(RenderBlock):
* rendering/RenderBox.cpp:
(WebCore::RenderBox::willBeDestroyed): remove the assert and change the
percent height detection check to use removePercentHeightDescendantIfNeeded.
We shouldn't rely on logicalHeight().isPercent() as it can change when our
writing mode changes. Instead, just query the map directly to see if we exist.
(WebCore::RenderBox::styleDidChange): when our writing mode changes from
horizontal to vertical or vice versa, we clear all our descendants from
the percent height descendant map. Cache the value of isHorizontalWritingMode()
before it changes in styleDidChange and compare it with the new value
(can't use oldStyle->isHorizontalWritingMode() since it can be inherited
and already updated).
LayoutTests:
* fast/block/percent-height-descendant-not-removed-crash2-expected.txt: Added.
* fast/block/percent-height-descendant-not-removed-crash2.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (120861 => 120862)
--- trunk/LayoutTests/ChangeLog 2012-06-20 21:15:18 UTC (rev 120861)
+++ trunk/LayoutTests/ChangeLog 2012-06-20 21:20:29 UTC (rev 120862)
@@ -1,3 +1,13 @@
+2012-06-20 Abhishek Arya <[email protected]>
+
+ Crash on accessing a removed renderer from percent height descendant map.
+ https://bugs.webkit.org/show_bug.cgi?id=88017
+
+ Reviewed by Eric Seidel.
+
+ * fast/block/percent-height-descendant-not-removed-crash2-expected.txt: Added.
+ * fast/block/percent-height-descendant-not-removed-crash2.html: Added.
+
2012-06-20 Julien Chaffraix <[email protected]>
REGRESSION(r113885): Margin not properly applied to elements with align=center
Added: trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2-expected.txt (0 => 120862)
--- trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2-expected.txt 2012-06-20 21:20:29 UTC (rev 120862)
@@ -0,0 +1,2 @@
+WebKit bug 88017 - Crash due to renderer not removed from height descendant map.
+PASS. WebKit didn't crash.
Added: trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2.html (0 => 120862)
--- trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2.html (rev 0)
+++ trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2.html 2012-06-20 21:20:29 UTC (rev 120862)
@@ -0,0 +1,32 @@
+<html>
+<head>
+<style>
+#test1 {
+ height: 1px;
+ -webkit-writing-mode: vertical-rl;
+}
+#test1:nth-child(3) {
+ height: auto;
+}
+</style>
+<script>
+if (window.layoutTestController)
+ layoutTestController.dumpAsText();
+
+_onload_ = function() {
+ document.body.appendChild(document.createElement('form'));
+ test1 = document.createElement('input');
+ test1.setAttribute('id', 'test1');
+ test1.setAttribute('type', 'range');
+ document.body.appendChild(test1);
+ document.designMode = 'on';
+ document.execCommand('selectall');
+ document.execCommand('FormatBlock', false, '<'+'pre>');
+ document.body.offsetTop;
+ document.body.innerHTML = "WebKit bug 88017 - Crash due to renderer not removed from height descendant map.<br />PASS. WebKit didn't crash.";
+}
+</script>
+</head>
+<body>
+</body>
+</html>
\ No newline at end of file
Property changes on: trunk/LayoutTests/fast/block/percent-height-descendant-not-removed-crash2.html
___________________________________________________________________
Added: svn:executable
Modified: trunk/Source/WebCore/ChangeLog (120861 => 120862)
--- trunk/Source/WebCore/ChangeLog 2012-06-20 21:15:18 UTC (rev 120861)
+++ trunk/Source/WebCore/ChangeLog 2012-06-20 21:20:29 UTC (rev 120862)
@@ -1,3 +1,37 @@
+2012-06-20 Abhishek Arya <[email protected]>
+
+ Crash on accessing a removed renderer from percent height descendant map.
+ https://bugs.webkit.org/show_bug.cgi?id=88017
+
+ Reviewed by Eric Seidel.
+
+ Test: fast/block/percent-height-descendant-not-removed-crash2.html
+
+ * rendering/RenderBlock.cpp:
+ (WebCore::RenderBlock::hasPercentHeightContainerMap): helper to tell
+ if we have a height container map.
+ (WebCore):
+ (WebCore::RenderBlock::hasPercentHeightDescendant): change from a debug
+ only function to a regular function for use. no need to null check
+ for a percent height container map in this function.
+ (WebCore::RenderBlock::clearPercentHeightDescendantsFrom): helper to
+ clear all percent height descendants under us.
+ (WebCore::RenderBlock::removePercentHeightDescendantIfNeeded): helper to
+ clear the box if it exists in the percent height descendant map.
+ * rendering/RenderBlock.h:
+ (RenderBlock):
+ * rendering/RenderBox.cpp:
+ (WebCore::RenderBox::willBeDestroyed): remove the assert and change the
+ percent height detection check to use removePercentHeightDescendantIfNeeded.
+ We shouldn't rely on logicalHeight().isPercent() as it can change when our
+ writing mode changes. Instead, just query the map directly to see if we exist.
+ (WebCore::RenderBox::styleDidChange): when our writing mode changes from
+ horizontal to vertical or vice versa, we clear all our descendants from
+ the percent height descendant map. Cache the value of isHorizontalWritingMode()
+ before it changes in styleDidChange and compare it with the new value
+ (can't use oldStyle->isHorizontalWritingMode() since it can be inherited
+ and already updated).
+
2012-06-20 Julien Chaffraix <[email protected]>
REGRESSION(r113885): Margin not properly applied to elements with align=center
Modified: trunk/Source/WebCore/rendering/RenderBlock.cpp (120861 => 120862)
--- trunk/Source/WebCore/rendering/RenderBlock.cpp 2012-06-20 21:15:18 UTC (rev 120861)
+++ trunk/Source/WebCore/rendering/RenderBlock.cpp 2012-06-20 21:20:29 UTC (rev 120862)
@@ -3932,17 +3932,49 @@
return gPercentHeightDescendantsMap ? gPercentHeightDescendantsMap->get(this) : 0;
}
-#if !ASSERT_DISABLED
+bool RenderBlock::hasPercentHeightContainerMap()
+{
+ return gPercentHeightContainerMap;
+}
+
bool RenderBlock::hasPercentHeightDescendant(RenderBox* descendant)
{
- ASSERT(descendant);
- if (!gPercentHeightContainerMap)
- return false;
- HashSet<RenderBlock*>* containerSet = gPercentHeightContainerMap->take(descendant);
- return containerSet && containerSet->size();
+ // We don't null check gPercentHeightContainerMap since the caller
+ // already ensures this and we need to call this function on every
+ // descendant in clearPercentHeightDescendantsFrom().
+ ASSERT(gPercentHeightContainerMap);
+ return gPercentHeightContainerMap->contains(descendant);
}
-#endif
+void RenderBlock::removePercentHeightDescendantIfNeeded(RenderBox* descendant)
+{
+ // We query the map directly, rather than looking at style's
+ // logicalHeight()/logicalMinHeight()/logicalMaxHeight() since those
+ // can change with writing mode/directional changes.
+ if (!hasPercentHeightContainerMap())
+ return;
+
+ if (!hasPercentHeightDescendant(descendant))
+ return;
+
+ removePercentHeightDescendant(descendant);
+}
+
+void RenderBlock::clearPercentHeightDescendantsFrom(RenderBox* parent)
+{
+ ASSERT(gPercentHeightContainerMap);
+ for (RenderObject* curr = parent->firstChild(); curr; curr = curr->nextInPreOrder(parent)) {
+ if (!curr->isBox())
+ continue;
+
+ RenderBox* box = toRenderBox(curr);
+ if (!hasPercentHeightDescendant(box))
+ continue;
+
+ removePercentHeightDescendant(box);
+ }
+}
+
template <RenderBlock::FloatingObject::Type FloatTypeValue>
inline void RenderBlock::FloatIntervalSearchAdapter<FloatTypeValue>::collectIfNeeded(const IntervalType& interval) const
{
Modified: trunk/Source/WebCore/rendering/RenderBlock.h (120861 => 120862)
--- trunk/Source/WebCore/rendering/RenderBlock.h 2012-06-20 21:15:18 UTC (rev 120861)
+++ trunk/Source/WebCore/rendering/RenderBlock.h 2012-06-20 21:20:29 UTC (rev 120862)
@@ -108,9 +108,10 @@
void addPercentHeightDescendant(RenderBox*);
static void removePercentHeightDescendant(RenderBox*);
HashSet<RenderBox*>* percentHeightDescendants() const;
-#if !ASSERT_DISABLED
+ static bool hasPercentHeightContainerMap();
static bool hasPercentHeightDescendant(RenderBox*);
-#endif
+ static void clearPercentHeightDescendantsFrom(RenderBox*);
+ static void removePercentHeightDescendantIfNeeded(RenderBox*);
void setHasMarkupTruncation(bool b) { m_hasMarkupTruncation = b; }
bool hasMarkupTruncation() const { return m_hasMarkupTruncation; }
Modified: trunk/Source/WebCore/rendering/RenderBox.cpp (120861 => 120862)
--- trunk/Source/WebCore/rendering/RenderBox.cpp 2012-06-20 21:15:18 UTC (rev 120861)
+++ trunk/Source/WebCore/rendering/RenderBox.cpp 2012-06-20 21:20:29 UTC (rev 120862)
@@ -134,24 +134,17 @@
{
clearOverrideSize();
- RenderStyle* styleToUse = style();
- if (styleToUse && (styleToUse->logicalHeight().isPercent() || styleToUse->logicalMinHeight().isPercent() || styleToUse->logicalMaxHeight().isPercent()))
- RenderBlock::removePercentHeightDescendant(this);
+ if (style()) {
+ RenderBlock::removePercentHeightDescendantIfNeeded(this);
- if (styleToUse) {
if (RenderView* view = this->view()) {
if (FrameView* frameView = view->frameView()) {
- if (styleToUse->position() == FixedPosition)
+ if (style()->position() == FixedPosition)
frameView->removeFixedObject(this);
}
}
}
- // If the following assertion fails, logicalHeight()/logicalMinHeight()/
- // logicalMaxHeight() values are changed from a percent value to a non-percent
- // value during laying out. It causes a use-after-free bug.
- ASSERT(!RenderBlock::hasPercentHeightDescendant(this));
-
RenderBoxModelObject::willBeDestroyed();
}
@@ -232,12 +225,16 @@
void RenderBox::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
{
+ // Horizontal writing mode definition is updated in RenderBoxModelObject::updateBoxModelInfoFromStyle,
+ // (as part of the RenderBoxModelObject::styleDidChange call below). So, we can safely cache the horizontal
+ // writing mode value before style change here.
+ bool oldHorizontalWritingMode = isHorizontalWritingMode();
+
RenderBoxModelObject::styleDidChange(diff, oldStyle);
RenderStyle* newStyle = style();
if (needsLayout() && oldStyle) {
- if (oldStyle && (oldStyle->logicalHeight().isPercent() || oldStyle->logicalMinHeight().isPercent() || oldStyle->logicalMaxHeight().isPercent()))
- RenderBlock::removePercentHeightDescendant(this);
+ RenderBlock::removePercentHeightDescendantIfNeeded(this);
// Normally we can do optimized positioning layout for absolute/fixed positioned objects. There is one special case, however, which is
// when the positioned object's margin-before is changed. In this case the parent has to get a layout in order to run margin collapsing
@@ -247,6 +244,10 @@
parent()->setChildNeedsLayout(true);
}
+ if (RenderBlock::hasPercentHeightContainerMap() && firstChild()
+ && oldHorizontalWritingMode != isHorizontalWritingMode())
+ RenderBlock::clearPercentHeightDescendantsFrom(this);
+
// If our zoom factor changes and we have a defined scrollLeft/Top, we need to adjust that value into the
// new zoomed coordinate space.
if (hasOverflowClip() && oldStyle && newStyle && oldStyle->effectiveZoom() != newStyle->effectiveZoom()) {
