Bug 914891 - Bail gracefully on sticky positioning with no scroll container. r=dholbert
authorCorey Ford <cford@mozilla.com>
Wed, 11 Sep 2013 16:30:56 -0700
changeset 146638 82d94735ba7f2d4f03fce39a03b453dd5c1f5ce3
parent 146637 5ec98e8b1be9f619dca62e255bf22e46a96fb1e4
child 146639 feea0997db3669060dc3991bae0a69a29dd85bea
push id33665
push userdholbert@mozilla.com
push dateThu, 12 Sep 2013 02:08:04 +0000
treeherdermozilla-inbound@82d94735ba7f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs914891
milestone26.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 914891 - Bail gracefully on sticky positioning with no scroll container. r=dholbert
layout/generic/StickyScrollContainer.cpp
layout/generic/StickyScrollContainer.h
layout/generic/crashtests/914891.html
layout/generic/crashtests/crashtests.list
layout/generic/nsFrame.cpp
layout/generic/nsHTMLReflowState.cpp
--- a/layout/generic/StickyScrollContainer.cpp
+++ b/layout/generic/StickyScrollContainer.cpp
@@ -36,23 +36,27 @@ StickyScrollContainer::StickyScrollConta
 
 StickyScrollContainer::~StickyScrollContainer()
 {
   mScrollFrame->RemoveScrollPositionListener(this);
 }
 
 // static
 StickyScrollContainer*
-StickyScrollContainer::StickyScrollContainerForFrame(nsIFrame* aFrame)
+StickyScrollContainer::GetStickyScrollContainerForFrame(nsIFrame* aFrame)
 {
   nsIScrollableFrame* scrollFrame =
     nsLayoutUtils::GetNearestScrollableFrame(aFrame->GetParent(),
       nsLayoutUtils::SCROLLABLE_SAME_DOC |
       nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN);
-  NS_ASSERTION(scrollFrame, "Need a scrolling container");
+  if (!scrollFrame) {
+    // We might not find any, for instance in the case of
+    // <html style="position: fixed">
+    return nullptr;
+  }
   FrameProperties props = static_cast<nsIFrame*>(do_QueryFrame(scrollFrame))->
     Properties();
   StickyScrollContainer* s = static_cast<StickyScrollContainer*>
     (props.Get(StickyScrollContainerProperty()));
   if (!s) {
     s = new StickyScrollContainer(scrollFrame);
     props.Set(StickyScrollContainerProperty(), s);
   }
@@ -85,18 +89,17 @@ void
 StickyScrollContainer::ComputeStickyOffsets(nsIFrame* aFrame)
 {
   nsIScrollableFrame* scrollableFrame =
     nsLayoutUtils::GetNearestScrollableFrame(aFrame->GetParent(),
       nsLayoutUtils::SCROLLABLE_SAME_DOC |
       nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN);
 
   if (!scrollableFrame) {
-    // Not sure how this would happen, but bail if it does.
-    NS_ERROR("Couldn't find a scrollable frame");
+    // Bail.
     return;
   }
 
   nsSize scrollContainerSize = scrollableFrame->GetScrolledFrame()->
     GetContentRectRelativeToSelf().Size();
 
   nsMargin computedOffsets;
   const nsStylePosition* position = aFrame->StylePosition();
--- a/layout/generic/StickyScrollContainer.h
+++ b/layout/generic/StickyScrollContainer.h
@@ -21,20 +21,20 @@ class nsIFrame;
 class nsIScrollableFrame;
 
 namespace mozilla {
 
 class StickyScrollContainer MOZ_FINAL : public nsIScrollPositionListener
 {
 public:
   /**
-   * Find the StickyScrollContainer associated with the scroll container of
-   * the given frame, creating it if necessary.
+   * Find (and create if necessary) the StickyScrollContainer associated with
+   * the scroll container of the given frame, if a scroll container exists.
    */
-  static StickyScrollContainer* StickyScrollContainerForFrame(nsIFrame* aFrame);
+  static StickyScrollContainer* GetStickyScrollContainerForFrame(nsIFrame* aFrame);
 
   /**
    * Find the StickyScrollContainer associated with the given scroll frame,
    * if it exists.
    */
   static StickyScrollContainer* GetStickyScrollContainerForScrollFrame(nsIFrame* aScrollFrame);
 
   void AddFrame(nsIFrame* aFrame) {
new file mode 100644
--- /dev/null
+++ b/layout/generic/crashtests/914891.html
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html style="position: fixed;">
+<head>
+<meta charset="UTF-8">
+</head>
+<body>
+<div style="position: sticky;"></div>
+</body>
+</html>
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@@ -495,8 +495,9 @@ load 849603.html
 test-pref(layout.css.flexbox.enabled,true) load 851396-1.html
 test-pref(layout.css.flexbox.enabled,true) load 854263-1.html
 test-pref(layout.css.flexbox.enabled,true) load 862947-1.html
 needs-focus pref(accessibility.browsewithcaret,true) load 868906.html
 test-pref(layout.css.flexbox.enabled,true) load 866547-1.html
 asserts(1-4) test-pref(layout.css.flexbox.enabled,true) load 876074-1.html # bug 876749
 load 885009-1.html
 load 893523.html
+test-pref(layout.css.sticky.enabled,true) load 914891.html
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -507,17 +507,21 @@ nsFrame::Init(nsIContent*      aContent,
   }
   const nsStyleDisplay *disp = StyleDisplay();
   if (disp->HasTransform(this)) {
     // The frame gets reconstructed if we toggle the -moz-transform
     // property, so we can set this bit here and then ignore it.
     mState |= NS_FRAME_MAY_BE_TRANSFORMED;
   }
   if (disp->mPosition == NS_STYLE_POSITION_STICKY) {
-    StickyScrollContainer::StickyScrollContainerForFrame(this)->AddFrame(this);
+    StickyScrollContainer* ssc =
+      StickyScrollContainer::GetStickyScrollContainerForFrame(this);
+    if (ssc) {
+      ssc->AddFrame(this);
+    }
   }
 
   if (nsLayoutUtils::FontSizeInflationEnabled(PresContext()) || !GetParent()
 #ifdef DEBUG
       // We have assertions that check inflation invariants even when
       // font size inflation is not enabled.
       || true
 #endif
@@ -588,18 +592,21 @@ nsFrame::DestroyFrom(nsIFrame* aDestruct
   NS_ASSERTION(!GetNextSibling() && !GetPrevSibling(),
                "Frames should be removed before destruction.");
   NS_ASSERTION(aDestructRoot, "Must specify destruct root");
   MOZ_ASSERT(!HasAbsolutelyPositionedChildren());
 
   nsSVGEffects::InvalidateDirectRenderingObservers(this);
 
   if (StyleDisplay()->mPosition == NS_STYLE_POSITION_STICKY) {
-    StickyScrollContainer::StickyScrollContainerForFrame(this)->
-      RemoveFrame(this);
+    StickyScrollContainer* ssc =
+      StickyScrollContainer::GetStickyScrollContainerForFrame(this);
+    if (ssc) {
+      ssc->RemoveFrame(this);
+    }
   }
 
   // Get the view pointer now before the frame properties disappear
   // when we call NotifyDestroyingFrame()
   nsView* view = GetView();
   nsPresContext* presContext = PresContext();
 
   nsIPresShell *shell = presContext->GetPresShell();
--- a/layout/generic/nsHTMLReflowState.cpp
+++ b/layout/generic/nsHTMLReflowState.cpp
@@ -847,18 +847,21 @@ nsHTMLReflowState::ApplyRelativePosition
   } else {
     props.Set(nsIFrame::NormalPositionProperty(), new nsPoint(*aPosition));
   }
 
   const nsStyleDisplay* display = aFrame->StyleDisplay();
   if (NS_STYLE_POSITION_RELATIVE == display->mPosition) {
     *aPosition += nsPoint(aComputedOffsets.left, aComputedOffsets.top);
   } else if (NS_STYLE_POSITION_STICKY == display->mPosition) {
-    *aPosition = StickyScrollContainer::StickyScrollContainerForFrame(aFrame)->
-      ComputePosition(aFrame);
+    StickyScrollContainer* ssc =
+      StickyScrollContainer::GetStickyScrollContainerForFrame(aFrame);
+    if (ssc) {
+      *aPosition = ssc->ComputePosition(aFrame);
+    }
   }
 }
 
 nsIFrame*
 nsHTMLReflowState::GetHypotheticalBoxContainer(nsIFrame* aFrame,
                                                nscoord& aCBLeftEdge,
                                                nscoord& aCBWidth)
 {