Spun off from bug 1402766. What follows mostly duplicates bug 1402766 comment 5 and bug 1402766 comment 8. Testcase is attached. It shows missing content. In particular, the "ello" does not get shown. In a debug build, it triggers multiple assertions like so: ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /home/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsLayoutUtils.cpp, line 7674 which all come down to the same thing: we did some layout, then deleted a next-in-flow (nsBlockReflowContext::ReflowBlock calling nsBlockFrame::DeleteNextInFlowChild) that still had kids. What happens here is that the <div id="float"> ends up with a first continuation containing the first inline of the ib split and an empty block from the ib split, and a second continuation that contains the continuation of the ib split block (which contains that innermost div's block) and the trailing ib split inline. Alright, so now we go to reflow that float's first continuation a second time (presumably from column-balancing; I didn't really check and it probably doesn't matter). We land in ReflowDirtyLines, reflow the inline line and it's complete. Then we look at the block line, call nsBlockFrame::ReflowBlockFrame, and this condition tests true: if ((!aState.mReflowInput.mFlags.mIsTopOfPage || clearedFloats) && availSpace.BSize(wm) < 0) { because our availSpace.BSize(wm) == -1 and we're not top-of-page (we had the inline line before us). Then ReflowBlockFrame does: *aKeepReflowGoing = false; if (ShouldAvoidBreakInside(aState.mReflowInput)) { aState.mReflowStatus.SetInlineLineBreakBeforeAndReset(); } else { PushLines(aState, aLine.prev()); aState.mReflowStatus.SetIncomplete(); } In our case we have "page-break-inside: avoid", so we do the SetInlineLineBreakBeforeAndReset() bit but do NOT change aState.mReflowStatus to incomplete. The docs for SetInlineLineBreakBeforeAndReset() say "Note that other frame completion status isn't expected to matter after calling this method"... but it turns out to matter. The Reset() bit inside SetInlineLineBreakBeforeAndReset() set mCompletion to FullyComplete, which it was anyway. Alright, now we return from ReflowBlockFrame to ReflowLine, then to ReflowDirtyLines. We've set keepGoing to false and aState.mReflowInput.WillReflowAgainForClearance() is false. Our line has things on it, so we leave the line box in place and break from the loop. We don't pull anything from our next-in-flow (which is fine in this case) and return back to nsBlockFrame::Reflow. This merges overflow container kid status and pushed float status into aState.mReflowStatus, but it's still FullyComplete. We do some final bookkeeping, then hand out aState.mReflowStatus, returning to nsBlockReflowContext::ReflowBlock. Now in ReflowBlock we have this condition: if (!aFrameReflowStatus.IsInlineBreakBefore() || (mFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) { In our case we _do_ have IsInlineBreakBefore() true (because we called SetInlineLineBreakBeforeAndReset()), but we're the float, so NS_FRAME_OUT_OF_FLOW is set. So we enter this code, and then it does: // If frame is complete and has a next-in-flow, we need to delete // them now. Do not do this when a break-before is signaled because // the frame is going to get reflowed again (and may end up wanting // a next-in-flow where it ends up), unless it is an out of flow frame. if (aFrameReflowStatus.IsFullyComplete()) { Our status is complete, so we enter _that_ code and delete our next in flow, together with its descendants. All the rest of the problems follow. The code in ReflowBlockFrame that returned early while claiming we were complete dates back to the initial landing of bug 685012 (which implemented "page-break-inside: avoid") back in 2012. The code in nsBlockReflowContext::ReflowBlock that ignores the break-before status for out-of-flows is ... old. That "unless it is an out of flow frame" bit in the comment dates back to the fix for bug 145305 in 2002 (patch by karnaze, r=alexsavulov... I had thought we no longer had code like that in the tree). The rest of that comment and most of the surrounding code is the initial commit of nsBlockReflowContext by kipp back in 1998. Alright, so for _printing_ maybe this was all OK somehow, because printing doesn't do incremental reflow (at least not back in 2002). So at the time when karnaze added that code the problem of having break-before on an out-of-flow that had a trailing block with a continuation in the next block couldn't really arise, maybe: that requires doing another reflow after the continuations were created. Even then, I'd think tables could trigger it... But in any case, the fix for bug 685012 ended up putting us into this state when "avoid" is used and we have a non-fitting block. For this specific testcase, we could check, at the point when we make the SetInlineLineBreakBeforeAndReset() call, whether the block has a next-in-flow, and if so mark ourselves incomplete. But what if this isn't even the first line, and the block involved _is_ complete, but we have a later line that isn't complete? I really don't understand how avoiding pushing lines in the ShouldAvoidBreakInside() case can be sound in general. There is a good chance that this bug is causing bug 743364 and bug 766868 and bug 1144641. Mats said (bug 1402766 comment 10) that he'll look into this.

At first glance, the "|| (mFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)" part just seems wrong to me. If we remove it we should get the right behavior - we'll push the float here: http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/layout/generic/BlockReflowInput.cpp#931-932 IIRC, the css-break properties don't apply to other out-of-flows since they don't create "class A" break opportunities: https://www.w3.org/TR/css-break-3/#btw-blocks (I need to check that though.)

> If we remove it we should get the right behavior Note that that check was initially added when we implemented pagination of floats. So it's been in the tree for a _long_ time.... Maybe it's not needed anymore, of course.

As far as I can tell from code archeology, testing completion status when there is a break-before status has always been bogus, so this bug has existed since the NS_FRAME_OUT_OF_FLOW exception was added in bug 145305. The original patch and bug comments don't give any clues as to why it was added.

(The assertions in bug 1402766 comment 5 also occurs with Stylo disabled, fwiw.)

> (The assertions in bug 1402766 comment 5 also occurs with Stylo disabled, fwiw.) Right, they're do to this bug, which is not stylo-specific.

Comment on attachment 8915183 [details] [diff] [review] fix+tests This looks plausible, but I don't remember enough about block reflow to review this. Daniel, do you think you can review it?

Sure, I'll take a look. FWIW, if I view the testcase in current Nightly and choose "inspect", I get a content-process crash (SIGSEGV on address 0x58), in mozilla::FrameProperties::GetInternal, called from nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit. e.g. bp-cf34abb6-b7fd-4fe6-92e2-4fab40171004

Comment on attachment 8915183 [details] [diff] [review] fix+tests Review of attachment 8915183 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/crashtests/1402766.html @@ +1,3 @@ > +<style> > +#htmlvar00009 { page-break-inside: avoid; } > +* { padding-left: 1vw; border-right: solid green 3em; } Looks like this crashtest already landed as part of bug 1402766 (which was probably only able to add it because of the nsInlineFrame.cpp early-return added in that same patch, I'm guessing): https://hg.mozilla.org/mozilla-central/rev/71d02a129beb (Though -- that commit named it 1405443.html, not 1402766.html. But it's this same file, AFAICT. I guess that means we'll have a reftest and a crashtest with the same name, since you're adding a reftest called 1405443.html here. This seems fine to me. *shrug*) Anyway, looks like you don't need to bother adding it, and you probably want to rebase this patch on top of that commit (and remove the chunk of nsInlineFrame.cpp that was added there: https://hg.mozilla.org/releases/mozilla-beta/rev/a894d887c0ba#l3.13

> called from nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit. Right, that was bug 1402766, as you discovered. ;)

Comment on attachment 8915381 [details] [diff] [review] fix+tests Review of attachment 8915381 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable -- r=me. (I wish I had a better understanding of why the OOF special-case [the thing being removed here] was there in the first place, but per mats' archeology in comment 3, it sounds like we don't have that. In any case, it's believably bogus at this point.)

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd25b77ff072 Don't delete next-in-flows when the state IsInlineBreakBefore. r=dholbert