Added: trunk/LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash-expected.txt (0 => 187502)
--- trunk/LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash-expected.txt 2015-07-28 19:42:03 UTC (rev 187502)
@@ -0,0 +1,2 @@
+This tests that changing the container's position from fixed to absolute is safe, when child container with fixed position is present.
+PASS, if no crash or assert in debug.
Added: trunk/LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html (0 => 187502)
--- trunk/LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html (rev 0)
+++ trunk/LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash.html 2015-07-28 19:42:03 UTC (rev 187502)
@@ -0,0 +1,23 @@
+<html>
+<body>
+ <div id="toabsolute" style="position: fixed;">
+ <div style="position: fixed;"></div>
+ </div>
+ <p id="foo">
+ This tests that changing the container's position from fixed to absolute is safe, when
+ child container with fixed position is present.<br>
+ PASS, if no crash or assert in debug.
+ </p>
+ <script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+ var m = document.getElementById("foo");
+ var c = document.getElementById("toabsolute");
+
+ c.offsetHeight;
+ c.style.position = "absolute";
+ c.style.width = m.offsetWidth + "px";
+ c.offsetHeight;
+ </script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (187501 => 187502)
--- trunk/Source/WebCore/ChangeLog 2015-07-28 19:38:13 UTC (rev 187501)
+++ trunk/Source/WebCore/ChangeLog 2015-07-28 19:42:03 UTC (rev 187502)
@@ -1,3 +1,28 @@
+2015-07-27 David Hyatt <[email protected]>
+
+ ASSERTION FAILED: !currBox->needsLayout() loading bing maps (and apple.com/music and nytimes)
+ https://bugs.webkit.org/show_bug.cgi?id=93891
+
+ Reviewed by Simon Fraser.
+
+ Added new tests in fast/dynamic.
+
+ Change tracking of positioned objects to always insert objects that need a layout in the
+ end of the ListHashMap for RenderViews. This ensures that dependencies between nested
+ positioned objects that both need a layout by the RenderView are resolved in the correct order.
+
+ Don't cache the end object when walking the ListHashMap to do layouts of positioned objects,
+ since that list is getting updated dynamically as earlier objects can mark and insert new
+ objects into the list during their layouts.
+
+ * rendering/RenderBlock.cpp:
+ (WebCore::RenderBlock::layoutPositionedObject):
+ (WebCore::RenderBlock::layoutPositionedObjects):
+ (WebCore::RenderBlock::insertIntoTrackedRendererMaps):
+ (WebCore::RenderBlock::insertPositionedObject):
+ (WebCore::RenderBlock::removePositionedObject):
+ * rendering/RenderBlock.h:
+
2015-07-28 Simon Fraser <[email protected]>
Fix builds using PathCairo.
Modified: trunk/Source/WebCore/rendering/RenderBlock.cpp (187501 => 187502)
--- trunk/Source/WebCore/rendering/RenderBlock.cpp 2015-07-28 19:38:13 UTC (rev 187501)
+++ trunk/Source/WebCore/rendering/RenderBlock.cpp 2015-07-28 19:42:03 UTC (rev 187502)
@@ -1293,71 +1293,75 @@
return margin;
}
-void RenderBlock::layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly)
+void RenderBlock::layoutPositionedObject(RenderBox& r, bool relayoutChildren, bool fixedPositionObjectsOnly)
{
- TrackedRendererListHashSet* positionedDescendants = positionedObjects();
- if (!positionedDescendants)
+ estimateRegionRangeForBoxChild(r);
+
+ // A fixed position element with an absolute positioned ancestor has no way of knowing if the latter has changed position. So
+ // if this is a fixed position element, mark it for layout if it has an abspos ancestor and needs to move with that ancestor, i.e.
+ // it has static position.
+ markFixedPositionObjectForLayoutIfNeeded(r);
+ if (fixedPositionObjectsOnly) {
+ r.layoutIfNeeded();
return;
+ }
- for (auto it = positionedDescendants->begin(), end = positionedDescendants->end(); it != end; ++it) {
- RenderBox& r = **it;
+ // When a non-positioned block element moves, it may have positioned children that are implicitly positioned relative to the
+ // non-positioned block. Rather than trying to detect all of these movement cases, we just always lay out positioned
+ // objects that are positioned implicitly like this. Such objects are rare, and so in typical DHTML menu usage (where everything is
+ // positioned explicitly) this should not incur a performance penalty.
+ if (relayoutChildren || (r.style().hasStaticBlockPosition(isHorizontalWritingMode()) && r.parent() != this))
+ r.setChildNeedsLayout(MarkOnlyThis);
- estimateRegionRangeForBoxChild(r);
+ // If relayoutChildren is set and the child has percentage padding or an embedded content box, we also need to invalidate the childs pref widths.
+ if (relayoutChildren && r.needsPreferredWidthsRecalculation())
+ r.setPreferredLogicalWidthsDirty(true, MarkOnlyThis);
+
+ r.markForPaginationRelayoutIfNeeded();
+
+ // We don't have to do a full layout. We just have to update our position. Try that first. If we have shrink-to-fit width
+ // and we hit the available width constraint, the layoutIfNeeded() will catch it and do a full layout.
+ if (r.needsPositionedMovementLayoutOnly() && r.tryLayoutDoingPositionedMovementOnly())
+ r.clearNeedsLayout();
+
+ // If we are paginated or in a line grid, compute a vertical position for our object now.
+ // If it's wrong we'll lay out again.
+ LayoutUnit oldLogicalTop = 0;
+ bool needsBlockDirectionLocationSetBeforeLayout = r.needsLayout() && view().layoutState()->needsBlockDirectionLocationSetBeforeLayout();
+ if (needsBlockDirectionLocationSetBeforeLayout) {
+ if (isHorizontalWritingMode() == r.isHorizontalWritingMode())
+ r.updateLogicalHeight();
+ else
+ r.updateLogicalWidth();
+ oldLogicalTop = logicalTopForChild(r);
+ }
- // A fixed position element with an absolute positioned ancestor has no way of knowing if the latter has changed position. So
- // if this is a fixed position element, mark it for layout if it has an abspos ancestor and needs to move with that ancestor, i.e.
- // it has static position.
- markFixedPositionObjectForLayoutIfNeeded(r);
- if (fixedPositionObjectsOnly) {
- r.layoutIfNeeded();
- continue;
- }
+ r.layoutIfNeeded();
- // When a non-positioned block element moves, it may have positioned children that are implicitly positioned relative to the
- // non-positioned block. Rather than trying to detect all of these movement cases, we just always lay out positioned
- // objects that are positioned implicitly like this. Such objects are rare, and so in typical DHTML menu usage (where everything is
- // positioned explicitly) this should not incur a performance penalty.
- if (relayoutChildren || (r.style().hasStaticBlockPosition(isHorizontalWritingMode()) && r.parent() != this))
- r.setChildNeedsLayout(MarkOnlyThis);
-
- // If relayoutChildren is set and the child has percentage padding or an embedded content box, we also need to invalidate the childs pref widths.
- if (relayoutChildren && r.needsPreferredWidthsRecalculation())
- r.setPreferredLogicalWidthsDirty(true, MarkOnlyThis);
-
- r.markForPaginationRelayoutIfNeeded();
-
- // We don't have to do a full layout. We just have to update our position. Try that first. If we have shrink-to-fit width
- // and we hit the available width constraint, the layoutIfNeeded() will catch it and do a full layout.
- if (r.needsPositionedMovementLayoutOnly() && r.tryLayoutDoingPositionedMovementOnly())
- r.clearNeedsLayout();
-
- // If we are paginated or in a line grid, compute a vertical position for our object now.
- // If it's wrong we'll lay out again.
- LayoutUnit oldLogicalTop = 0;
- bool needsBlockDirectionLocationSetBeforeLayout = r.needsLayout() && view().layoutState()->needsBlockDirectionLocationSetBeforeLayout();
- if (needsBlockDirectionLocationSetBeforeLayout) {
- if (isHorizontalWritingMode() == r.isHorizontalWritingMode())
- r.updateLogicalHeight();
- else
- r.updateLogicalWidth();
- oldLogicalTop = logicalTopForChild(r);
- }
+ // Lay out again if our estimate was wrong.
+ if (needsBlockDirectionLocationSetBeforeLayout && logicalTopForChild(r) != oldLogicalTop) {
+ r.setChildNeedsLayout(MarkOnlyThis);
+ r.layoutIfNeeded();
+ }
+ if (updateRegionRangeForBoxChild(r)) {
+ r.setNeedsLayout(MarkOnlyThis);
r.layoutIfNeeded();
-
- // Lay out again if our estimate was wrong.
- if (needsBlockDirectionLocationSetBeforeLayout && logicalTopForChild(r) != oldLogicalTop) {
- r.setChildNeedsLayout(MarkOnlyThis);
- r.layoutIfNeeded();
- }
-
- if (updateRegionRangeForBoxChild(r)) {
- r.setNeedsLayout(MarkOnlyThis);
- r.layoutIfNeeded();
- }
}
}
+void RenderBlock::layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly)
+{
+ TrackedRendererListHashSet* positionedDescendants = positionedObjects();
+ if (!positionedDescendants)
+ return;
+
+ // Do not cache positionedDescendants->end() in a local variable, since |positionedDescendants| can be mutated
+ // as it is walked. We always need to fetch the new end() value dynamically.
+ for (auto it = positionedDescendants->begin(); it != positionedDescendants->end(); ++it)
+ layoutPositionedObject(**it, relayoutChildren, fixedPositionObjectsOnly);
+}
+
void RenderBlock::markPositionedObjectsForLayout()
{
TrackedRendererListHashSet* positionedDescendants = positionedObjects();
@@ -2086,7 +2090,7 @@
return beforeBlock;
}
-void RenderBlock::insertIntoTrackedRendererMaps(RenderBox& descendant, TrackedDescendantsMap*& descendantsMap, TrackedContainerMap*& containerMap)
+void RenderBlock::insertIntoTrackedRendererMaps(RenderBox& descendant, TrackedDescendantsMap*& descendantsMap, TrackedContainerMap*& containerMap, bool forceNewEntry)
{
if (!descendantsMap) {
descendantsMap = new TrackedDescendantsMap;
@@ -2098,6 +2102,12 @@
descendantSet = new TrackedRendererListHashSet;
descendantsMap->set(this, std::unique_ptr<TrackedRendererListHashSet>(descendantSet));
}
+
+ if (forceNewEntry) {
+ descendantSet->remove(&descendant);
+ containerMap->remove(&descendant);
+ }
+
bool added = descendantSet->add(&descendant).isNewEntry;
if (!added) {
ASSERT(containerMap->get(&descendant));
@@ -2109,7 +2119,7 @@
if (!containerSet) {
containerSet = new HashSet<RenderBlock*>;
containerMap->set(&descendant, std::unique_ptr<HashSet<RenderBlock*>>(containerSet));
- }
+ }
ASSERT(!containerSet->contains(this));
containerSet->add(this);
}
@@ -2157,7 +2167,7 @@
if (o.isRenderFlowThread())
return;
- insertIntoTrackedRendererMaps(o, gPositionedDescendantsMap, gPositionedContainerMap);
+ insertIntoTrackedRendererMaps(o, gPositionedDescendantsMap, gPositionedContainerMap, isRenderView());
}
void RenderBlock::removePositionedObject(RenderBox& o)
Modified: trunk/Source/WebCore/rendering/RenderBlock.h (187501 => 187502)
--- trunk/Source/WebCore/rendering/RenderBlock.h 2015-07-28 19:38:13 UTC (rev 187501)
+++ trunk/Source/WebCore/rendering/RenderBlock.h 2015-07-28 19:42:03 UTC (rev 187502)
@@ -313,6 +313,8 @@
virtual void layout() override;
void layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly = false);
+ void layoutPositionedObject(RenderBox&, bool relayoutChildren, bool fixedPositionObjectsOnly);
+
void markFixedPositionObjectForLayoutIfNeeded(RenderObject& child);
LayoutUnit marginIntrinsicLogicalWidthForChild(RenderBox&) const;
@@ -415,7 +417,7 @@
// FIXME-BLOCKFLOW: Remove virtualizaion when all callers have moved to RenderBlockFlow
virtual bool hasLines() const { return false; }
- void insertIntoTrackedRendererMaps(RenderBox& descendant, TrackedDescendantsMap*&, TrackedContainerMap*&);
+ void insertIntoTrackedRendererMaps(RenderBox& descendant, TrackedDescendantsMap*&, TrackedContainerMap*&, bool forceNewEntry = false);
static void removeFromTrackedRendererMaps(RenderBox& descendant, TrackedDescendantsMap*&, TrackedContainerMap*&);
void createFirstLetterRenderer(RenderElement* firstLetterBlock, RenderText* currentTextChild);
