From 69849dd1cc2eec7b1f8ad27527c4ce690e921983 Mon Sep 17 00:00:00 2001 From: Mats Palmgren Date: Mon, 14 Dec 2020 15:13:35 +0000 Subject: [PATCH] Bug 1663222 - Move any overflow-container continuations on the OverflowContainers list for the children we pull from our prev-in-flow to the ExcessOverflowContainers to keep frames ordered. r=TYLin CLOSED TREE Making them non-overflow containers is valid but then we can't just append them to mFrames since that'll put them on the last line which may result in some lines in-between the pulled child and its continuation. That violates the invariant that in-flow continuations in the same parent must be on consecutive lines (unless they are overflow containers in which case they need to be on the [Excess]OverflowContainers lists). (The crash happens because if we push the lines in-between the two continuations to the OverflowLines, then we won't find the 2nd continuation on the first line of OverflowLines.) (Note also that this change triggers the assertion in DrainExcessOverflowContainersList, and depends on bug 1680406 to fix that and merge the lists.) Differential Revision: https://phabricator.services.mozilla.com/D99560 --- layout/generic/crashtests/1663222.html | 19 +++++++++++++ layout/generic/crashtests/1670336.html | 33 +++++++++++++++++++++++ layout/generic/crashtests/crashtests.list | 2 ++ layout/generic/nsBlockFrame.cpp | 31 +++++++++++---------- layout/generic/nsContainerFrame.cpp | 16 +++++------ layout/generic/nsContainerFrame.h | 13 +++++++++ 6 files changed, 91 insertions(+), 23 deletions(-) create mode 100644 layout/generic/crashtests/1663222.html create mode 100644 layout/generic/crashtests/1670336.html diff --git a/layout/generic/crashtests/1663222.html b/layout/generic/crashtests/1663222.html new file mode 100644 index 000000000000..132436c3fc9a --- /dev/null +++ b/layout/generic/crashtests/1663222.html @@ -0,0 +1,19 @@ + + + + +s.ykED*-U6p]68"FX7wpo&mK`
cx.6W.2;3\52A diff --git a/layout/generic/crashtests/1670336.html b/layout/generic/crashtests/1670336.html new file mode 100644 index 000000000000..34e5fa2d76c1 --- /dev/null +++ b/layout/generic/crashtests/1670336.html @@ -0,0 +1,33 @@ + + + + + +
diff --git a/layout/generic/crashtests/crashtests.list b/layout/generic/crashtests/crashtests.list index 835c12d00f70..a18a380d6336 100644 --- a/layout/generic/crashtests/crashtests.list +++ b/layout/generic/crashtests/crashtests.list @@ -785,6 +785,8 @@ load 1648577.html load 1652618.html 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 diff --git a/layout/generic/nsBlockFrame.cpp b/layout/generic/nsBlockFrame.cpp index e5f63248b52a..36ca75327343 100644 --- a/layout/generic/nsBlockFrame.cpp +++ b/layout/generic/nsBlockFrame.cpp @@ -5167,25 +5167,22 @@ bool nsBlockFrame::DrainOverflowLines() { // Make all the frames on the overflow line list mine. ReparentFrames(overflowLines->mFrames, prevBlock, this); - // Collect overflow containers from our [Excess]OverflowContainers lists - // that are continuations from the frames we picked up from our - // prev-in-flow. We'll append these to mFrames to ensure the continuations + // Collect overflow containers from our OverflowContainers list that are + // continuations from the frames we picked up from our prev-in-flow, then + // prepend those to ExcessOverflowContainers to ensure the continuations // are ordered. - auto HasOverflowContainers = [this]() -> bool { - return GetOverflowContainers() || GetExcessOverflowContainers(); - }; - nsFrameList ocContinuations; - if (HasOverflowContainers()) { + if (GetOverflowContainers()) { + nsFrameList ocContinuations; for (auto* f : overflowLines->mFrames) { auto* cont = f; bool done = false; while (!done && (cont = cont->GetNextContinuation()) && cont->GetParent() == this) { bool onlyChild = !cont->GetPrevSibling() && !cont->GetNextSibling(); - if (MaybeStealOverflowContainerFrame(cont)) { - cont->RemoveStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER); + if (cont->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER) && + TryRemoveFrame(OverflowContainersProperty(), cont)) { ocContinuations.AppendFrame(nullptr, cont); - done = onlyChild && !HasOverflowContainers(); + done = onlyChild; continue; } break; @@ -5194,6 +5191,13 @@ bool nsBlockFrame::DrainOverflowLines() { break; } } + if (!ocContinuations.IsEmpty()) { + if (nsFrameList* eoc = GetExcessOverflowContainers()) { + eoc->InsertFrames(nullptr, nullptr, ocContinuations); + } else { + SetExcessOverflowContainers(std::move(ocContinuations)); + } + } } // Make the overflow out-of-flow frames mine too. @@ -5231,7 +5235,6 @@ bool nsBlockFrame::DrainOverflowLines() { mLines.splice(mLines.begin(), overflowLines->mLines); NS_ASSERTION(overflowLines->mLines.empty(), "splice should empty list"); delete overflowLines; - AddFrames(ocContinuations, mFrames.LastChild(), nullptr); didFindOverflow = true; } } @@ -6221,8 +6224,8 @@ void nsBlockFrame::DoRemoveFrameInternal(nsIFrame* aDeletedFrame, } while (line != line_end && aDeletedFrame) { - NS_ASSERTION(this == aDeletedFrame->GetParent(), "messed up delete code"); - NS_ASSERTION(line->Contains(aDeletedFrame), "frame not in line"); + MOZ_ASSERT(this == aDeletedFrame->GetParent(), "messed up delete code"); + MOZ_ASSERT(line->Contains(aDeletedFrame), "frame not in line"); if (!(aFlags & FRAMES_ARE_EMPTY)) { line->MarkDirty(); diff --git a/layout/generic/nsContainerFrame.cpp b/layout/generic/nsContainerFrame.cpp index 33624286119c..b0990d7ecaa7 100644 --- a/layout/generic/nsContainerFrame.cpp +++ b/layout/generic/nsContainerFrame.cpp @@ -1406,15 +1406,14 @@ void nsContainerFrame::DisplayOverflowContainers( } } -static bool TryRemoveFrame(nsIFrame* aFrame, - nsContainerFrame::FrameListPropertyDescriptor aProp, - nsIFrame* aChildToRemove) { - nsFrameList* list = aFrame->GetProperty(aProp); +bool nsContainerFrame::TryRemoveFrame(FrameListPropertyDescriptor aProp, + nsIFrame* aChildToRemove) { + nsFrameList* list = GetProperty(aProp); if (list && list->StartRemoveFrame(aChildToRemove)) { // aChildToRemove *may* have been removed from this list. if (list->IsEmpty()) { - Unused << aFrame->TakeProperty(aProp); - list->Delete(aFrame->PresShell()); + Unused << TakeProperty(aProp); + list->Delete(PresShell()); } return true; } @@ -1425,11 +1424,10 @@ bool nsContainerFrame::MaybeStealOverflowContainerFrame(nsIFrame* aChild) { bool removed = false; if (MOZ_UNLIKELY(aChild->HasAnyStateBits(NS_FRAME_IS_OVERFLOW_CONTAINER))) { // Try removing from the overflow container list. - removed = ::TryRemoveFrame(this, OverflowContainersProperty(), aChild); + removed = TryRemoveFrame(OverflowContainersProperty(), aChild); if (!removed) { // It might be in the excess overflow container list. - removed = - ::TryRemoveFrame(this, ExcessOverflowContainersProperty(), aChild); + removed = TryRemoveFrame(ExcessOverflowContainersProperty(), aChild); } } return removed; diff --git a/layout/generic/nsContainerFrame.h b/layout/generic/nsContainerFrame.h index a15c37526063..2aba7cad6cbe 100644 --- a/layout/generic/nsContainerFrame.h +++ b/layout/generic/nsContainerFrame.h @@ -806,6 +806,19 @@ class nsContainerFrame : public nsSplittableFrame { nsIFrame* aFrame, bool aReparentSiblings); + /** + * Try to remove aChildToRemove from the frame list stored in aProp. + * If aChildToRemove was removed from the aProp list and that list became + * empty, then aProp is removed from this frame and deleted. + * @note if aChildToRemove isn't on the aProp frame list, it might still be + * removed from whatever list it happens to be on, so use this method + * carefully. This method is primarily meant for removing frames from the + * [Excess]OverflowContainers lists. + * @return true if aChildToRemove was removed from some list + */ + bool TryRemoveFrame(FrameListPropertyDescriptor aProp, + nsIFrame* aChildToRemove); + // ========================================================================== /* * Convenience methods for traversing continuations