Bug 1683126 - Make nsCanvasFrame::Reflow more robust. r=TYLin
authorMats Palmgren <mats@mozilla.com>
Mon, 01 Feb 2021 23:46:17 +0000
changeset 565527 35053583b9a751daed53f428f12219425a84004c
parent 565526 ee7b88395348b5850a5340610cd525201b5fcadb
child 565528 8522d4a89bbd9e8bab2374efa5ceeff4a7900c0c
push id38161
push userabutkovits@mozilla.com
push dateTue, 02 Feb 2021 03:35:00 +0000
treeherdermozilla-central@babdc3b3a300 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersTYLin
bugs1683126, 1655630, 1392106
milestone87.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 1683126 - Make nsCanvasFrame::Reflow more robust. r=TYLin This patch improves nsCanvasFrame::Reflow in a few ways. First, we now iterate over mFrames and reflow every child. This makes it more robust vis-à-vis the order of any placeholders and the root frame, and also resilient against a missing root frame (this fixes the fatal assertion in this bug). We now also actually reflow all placeholders which wasn't the case before. It seems like a prudent thing to do. I also added a separate nsReflowStatus for each child. We now also call SetOverflowAreasToDesiredBounds() in all cases. We failed to do that in the 'mFrames.IsEmpty()' case before, which triggered the assertions in bug 1655630 and bug 1392106. Differential Revision: https://phabricator.services.mozilla.com/D103592
layout/generic/crashtests/1683126.html
layout/generic/crashtests/crashtests.list
layout/generic/nsCanvasFrame.cpp
layout/reftests/bugs/reftest.list
layout/reftests/pagination/reftest.list
new file mode 100644
--- /dev/null
+++ b/layout/generic/crashtests/1683126.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html class="reftest-paged">
+<head>
+    <style>
+        * {
+            border-block-width: 1151557106.9598303in ! important;
+            border-block-end-style: double;
+        }
+
+        * * {
+            position: fixed;
+        }
+    </style>
+</head>
+</html>
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@@ -337,17 +337,17 @@ load 473894-1.html
 load 476241-1.html
 load 477731-1.html
 load 477928.html
 load 478131-1.html
 load 478170-1.html
 load 478185-1.html
 load 478504.html
 asserts-if(!Android,0-1) load 479938-1.html # Bug 575011
-asserts(20) load 480345-1.html # bug 1655630
+load 480345-1.html
 load 481921.html
 load 489462-1.html
 load 489477.html
 load 489480-1.xhtml
 load 489647-1.html
 load 493111-1.html
 load 493118-1.html
 load 493649.html
@@ -402,17 +402,17 @@ load 553504-1.xhtml
 load 564368-1.xhtml
 load 564968.xhtml
 load 569193-1.html
 load 570160.html
 load 570289-1.html
 load 571618-1.svg
 asserts(0-1) load 571975-1.html # bug 574889
 load 571995.xhtml
-asserts(1) load 574958.xhtml # bug 1655630
+load 574958.xhtml
 asserts(0-6) load 578977.html # bug 757305
 load 580504-1.xhtml
 load 585598-1.xhtml
 load 586806-1.html
 load 586806-2.html
 load 586806-3.html
 load 586973-1.html
 load 589002-1.html
@@ -786,8 +786,9 @@ load 1652897.html
 asserts(0-7) load 1654925.html
 load 1663222.html
 load 1666592.html
 load 1670336.html
 HTTP load 1677518-1.html
 load 1680406.html
 load 1681788.html
 HTTP load 1682882.html
+load 1683126.html
--- a/layout/generic/nsCanvasFrame.cpp
+++ b/layout/generic/nsCanvasFrame.cpp
@@ -716,37 +716,44 @@ void nsCanvasFrame::Reflow(nsPresContext
     }
   }
 
   // Set our size up front, since some parts of reflow depend on it
   // being already set.  Note that the computed height may be
   // unconstrained; that's ok.  Consumers should watch out for that.
   SetSize(nsSize(aReflowInput.ComputedWidth(), aReflowInput.ComputedHeight()));
 
-  // Reflow our one and only normal child frame. It's either the root
+  // Reflow our children.  Typically, we only have one child - the root
   // element's frame or a placeholder for that frame, if the root element
-  // is abs-pos or fixed-pos. We may have additional children which
-  // are placeholders for continuations of fixed-pos content, but those
-  // don't need to be reflowed. The normal child is always comes before
-  // the fixed-pos placeholders, because we insert it at the start
-  // of the child list, above.
-  ReflowOutput kidDesiredSize(aReflowInput);
-  if (mFrames.IsEmpty()) {
-    // We have no child frame, so return an empty size
-    aDesiredSize.Width() = aDesiredSize.Height() = 0;
-  } else if (mFrames.FirstChild() != mPopupSetFrame) {
-    nsIFrame* kidFrame = mFrames.FirstChild();
+  // is abs-pos or fixed-pos.  Note that this child might be missing though
+  // if that frame was Complete in one of our earlier continuations.  This
+  // happens when we create additional pages purely to make room for painting
+  // overflow (painted by BuildPreviousPageOverflow in nsPageFrame.cpp).
+  // We may have additional children which are placeholders for continuations
+  // of fixed-pos content, see nsCSSFrameConstructor::ReplicateFixedFrames.
+  // We may also have a nsPopupSetFrame child (mPopupSetFrame).
+  const WritingMode wm = aReflowInput.GetWritingMode();
+  aDesiredSize.SetSize(wm, aReflowInput.ComputedSize());
+  aDesiredSize.SetOverflowAreasToDesiredBounds();
+  nsIFrame* nextKid = nullptr;
+  for (auto* kidFrame = mFrames.FirstChild(); kidFrame; kidFrame = nextKid) {
+    nextKid = kidFrame->GetNextSibling();
+    if (kidFrame == mPopupSetFrame) {
+      // This child is handled separately after this loop.
+      continue;
+    }
+
+    ReflowOutput kidDesiredSize(aReflowInput);
     bool kidDirty = kidFrame->HasAnyStateBits(NS_FRAME_IS_DIRTY);
-
     WritingMode kidWM = kidFrame->GetWritingMode();
     auto availableSize = aReflowInput.AvailableSize(kidWM);
     nscoord bOffset = 0;
     nscoord canvasBSizeSum = 0;
-    WritingMode wm = aReflowInput.GetWritingMode();
     if (prevCanvasFrame && availableSize.BSize(kidWM) != NS_UNCONSTRAINEDSIZE &&
+        !kidFrame->IsPlaceholderFrame() &&
         StaticPrefs::layout_display_list_improve_fragmentation()) {
       for (auto* pif = prevCanvasFrame; pif;
            pif = static_cast<nsCanvasFrame*>(pif->GetPrevInFlow())) {
         canvasBSizeSum += pif->BSize(kidWM);
         auto* pifChild = pif->PrincipalChildList().FirstChild();
         if (pifChild) {
           nscoord layoutOverflow = pifChild->BSize(kidWM) - canvasBSizeSum;
           // A negative value means that the :root frame does not fill
@@ -763,61 +770,60 @@ void nsCanvasFrame::Reflow(nsPresContext
             layoutOverflow = so.BEnd(kidWM) - canvasBSizeSum;
           }
           bOffset = std::max(bOffset, layoutOverflow);
         }
       }
       availableSize.BSize(kidWM) -= bOffset;
     }
 
-    LogicalSize finalSize = aReflowInput.ComputedSize();
     if (MOZ_LIKELY(availableSize.BSize(kidWM) > 0)) {
       ReflowInput kidReflowInput(aPresContext, aReflowInput, kidFrame,
                                  availableSize);
 
       if (aReflowInput.IsBResizeForWM(kidReflowInput.GetWritingMode()) &&
           kidFrame->HasAnyStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE)) {
         // Tell our kid it's being block-dir resized too.  Bit of a
         // hack for framesets.
         kidReflowInput.SetBResize(true);
       }
 
       nsSize containerSize = aReflowInput.ComputedPhysicalSize();
       LogicalMargin margin = kidReflowInput.ComputedLogicalMargin(kidWM);
       LogicalPoint kidPt(kidWM, margin.IStart(kidWM), margin.BStart(kidWM));
       (kidWM.IsOrthogonalTo(wm) ? kidPt.I(kidWM) : kidPt.B(kidWM)) += bOffset;
 
-      // Reflow the frame
+      nsReflowStatus kidStatus;
       ReflowChild(kidFrame, aPresContext, kidDesiredSize, kidReflowInput, kidWM,
-                  kidPt, containerSize, ReflowChildFlags::Default, aStatus);
+                  kidPt, containerSize, ReflowChildFlags::Default, kidStatus);
 
-      // Complete the reflow and position and size the child frame
       FinishReflowChild(kidFrame, aPresContext, kidDesiredSize, &kidReflowInput,
                         kidWM, kidPt, containerSize,
                         ReflowChildFlags::ApplyRelativePositioning);
 
-      if (!aStatus.IsFullyComplete()) {
+      if (!kidStatus.IsFullyComplete()) {
         nsIFrame* nextFrame = kidFrame->GetNextInFlow();
-        NS_ASSERTION(nextFrame || aStatus.NextInFlowNeedsReflow(),
+        NS_ASSERTION(nextFrame || kidStatus.NextInFlowNeedsReflow(),
                      "If it's incomplete and has no nif yet, it must flag a "
                      "nif reflow.");
         if (!nextFrame) {
           nextFrame = aPresContext->PresShell()
                           ->FrameConstructor()
                           ->CreateContinuingFrame(kidFrame, this);
           SetOverflowFrames(nsFrameList(nextFrame, nextFrame));
           // Root overflow containers will be normal children of
           // the canvas frame, but that's ok because there
           // aren't any other frames we need to isolate them from
           // during reflow.
         }
-        if (aStatus.IsOverflowIncomplete()) {
+        if (kidStatus.IsOverflowIncomplete()) {
           nextFrame->AddStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER);
         }
       }
+      aStatus.MergeCompletionStatusFrom(kidStatus);
 
       // If the child frame was just inserted, then we're responsible for making
       // sure it repaints
       if (kidDirty) {
         // But we have a new child, which will affect our background, so
         // invalidate our whole rect.
         // Note: Even though we request to be sized to our child's size, our
         // scroll frame ensures that we are always the size of the viewport.
@@ -825,40 +831,41 @@ void nsCanvasFrame::Reflow(nsPresContext
         // (0, 0). We only want to invalidate GetRect() since Get*OverflowRect()
         // could also include overflow to our top and left (out of the viewport)
         // which doesn't need to be painted.
         nsIFrame* viewport = PresContext()->GetPresShell()->GetRootFrame();
         viewport->InvalidateFrame();
       }
 
       // Return our desired size. Normally it's what we're told, but
-      // sometimes we can be given an unconstrained height (when a window
-      // is sizing-to-content), and we should compute our desired height.
-      if (aReflowInput.ComputedBSize() == NS_UNCONSTRAINEDSIZE) {
+      // sometimes we can be given an unconstrained block-size (when a window
+      // is sizing-to-content), and we should compute our desired block-size.
+      if (aReflowInput.ComputedBSize() == NS_UNCONSTRAINEDSIZE &&
+          !kidFrame->IsPlaceholderFrame()) {
+        LogicalSize finalSize = aReflowInput.ComputedSize();
         finalSize.BSize(wm) =
             kidFrame->GetLogicalSize(wm).BSize(wm) +
             kidReflowInput.ComputedLogicalMargin(wm).BStartEnd(wm);
+        aDesiredSize.SetSize(wm, finalSize);
+        aDesiredSize.SetOverflowAreasToDesiredBounds();
       }
+      aDesiredSize.mOverflowAreas.UnionWith(kidDesiredSize.mOverflowAreas +
+                                            kidFrame->GetPosition());
+    } else if (kidFrame->IsPlaceholderFrame()) {
+      // Placeholders always fit even if there's no available block-size left.
     } else {
       // This only occurs in paginated mode.  There is no available space on
       // this page due to reserving space for overflow from a previous page,
       // so we push our child to the next page.  Note that we can have some
       // placeholders for fixed pos. frames in mFrames too, so we need to be
       // careful to only push `kidFrame`.
-      MOZ_ASSERT(!kidFrame->IsPlaceholderFrame(),
-                 "we should never push fixed pos placeholders");
       mFrames.RemoveFrame(kidFrame);
       SetOverflowFrames(nsFrameList(kidFrame, kidFrame));
       aStatus.SetIncomplete();
     }
-
-    aDesiredSize.SetSize(wm, finalSize);
-    aDesiredSize.SetOverflowAreasToDesiredBounds();
-    aDesiredSize.mOverflowAreas.UnionWith(kidDesiredSize.mOverflowAreas +
-                                          kidFrame->GetPosition());
   }
 
   if (prevCanvasFrame) {
     ReflowOverflowContainerChildren(aPresContext, aReflowInput,
                                     aDesiredSize.mOverflowAreas,
                                     ReflowChildFlags::Default, aStatus);
   }
 
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -267,17 +267,17 @@ skip-if(Android&&asyncPan) == 243519-1.h
 == 243519-4c.html 243519-4-ref.html
 == 243519-4d.html 243519-4-ref.html
 == 243519-4e.html 243519-4-ref.html
 == 243519-4f.html 243519-4-ref.html
 == 243519-5a.html 243519-5-ref.html
 == 243519-5b.html 243519-5-ref.html
 == 243519-5c.html 243519-5-ref.html
 == 243519-5d.html 243519-5-ref.html
-asserts(4) == 243519-6.html 243519-6-ref.html # bug 1655630
+== 243519-6.html 243519-6-ref.html
 == 243519-7.html 243519-7-ref.html
 == 243519-8.svg 243519-8-ref.svg
 == 243519-9a.html 243519-9-ref.html
 == 243519-9b.html 243519-9-ref.html
 == 243519-9c.html 243519-9-ref.html
 == 243519-9d.html 243519-9-ref.html
 == 243519-9e.html 243519-9-ref.html
 == 243519-9f.html 243519-9-ref.html
--- a/layout/reftests/pagination/reftest.list
+++ b/layout/reftests/pagination/reftest.list
@@ -1,22 +1,22 @@
 # For more pagination tests, see layout/reftests/w3c-css/submitted/css21/pagination/
 #   and layout/reftests/w3c-css/submitted/multicol3/
 # Pagination tests
 # asserts(3) == abspos-breaking-000.xhtml abspos-breaking-000.ref.xhtml # bug 1067755, 1135556
-asserts(3-7) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-001.xhtml abspos-breaking-000.ref.xhtml # Bug 1655630, Bug 1392106
-asserts(3-6) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-002.xhtml abspos-breaking-000.ref.xhtml # Bug 1655630, Bug 1392106
-asserts(2-3) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-003.html abspos-breaking-003-ref.html # Bug 1655630, Bug 1392106
-asserts(4-6) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-004.html abspos-breaking-004-ref.html # Bug 1655630, Bug 1392106
-asserts(4-6) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-005.html abspos-breaking-005-ref.html # Bug 1655630, Bug 1392106
-asserts(4) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-006.html abspos-breaking-006-ref.html # Bug 1655630, Bug 1392106
-asserts(2) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-007.html abspos-breaking-007-ref.html # Bug 1655630, Bug 1392106
-asserts(2) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-008.html abspos-breaking-008-ref.html # Bug 1655630, Bug 1392106
-asserts(2) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-009.html abspos-breaking-009-ref.html # Bug 1655630, Bug 1392106
-asserts(2) random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) == abspos-breaking-010.html abspos-breaking-010-ref.html # Bug 1655630, Bug 1392106
+== abspos-breaking-001.xhtml abspos-breaking-000.ref.xhtml
+== abspos-breaking-002.xhtml abspos-breaking-000.ref.xhtml
+== abspos-breaking-003.html abspos-breaking-003-ref.html
+== abspos-breaking-004.html abspos-breaking-004-ref.html
+== abspos-breaking-005.html abspos-breaking-005-ref.html
+== abspos-breaking-006.html abspos-breaking-006-ref.html
+== abspos-breaking-007.html abspos-breaking-007-ref.html
+== abspos-breaking-008.html abspos-breaking-008-ref.html
+== abspos-breaking-009.html abspos-breaking-009-ref.html
+== abspos-breaking-010.html abspos-breaking-010-ref.html
 == abspos-breaking-011.html abspos-breaking-011-ref.html # Bug 1392106
 == abspos-breaking-dynamic-001.html abspos-breaking-dynamic-001-ref.html
 == abspos-breaking-dynamic-002.html abspos-breaking-dynamic-002-ref.html
 == abspos-breaking-dynamic-003.html abspos-breaking-dynamic-003-ref.html
 == abspos-breaking-dynamic-004.html abspos-breaking-dynamic-001-ref.html
 == abspos-breaking-dynamic-005.html abspos-breaking-dynamic-005-ref.html
 == abspos-overflow-01.xhtml abspos-overflow-01.ref.xhtml
 == abspos-overflow-01-cols.xhtml abspos-overflow-01-cols.ref.xhtml