1108104 - position:fixed element is missing on the first page in Print Preview when styled with downloadable font

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Follow-up from bug 423192 comment 10.

(I'm filing this in Layout rather than Print Preview b/c I see that it's
the frame tree that is wrong.)

STR
1. save the attached testcase to a local file
2. load the file
3. Print Preview

ACTUAL RESULT
"This is a header!" is missing on the first page (it's present on subsequent pages).

EXPECTED RESULT
"This is a header!" on all pages.

NOTE
The bug only occurs when the preference gfx.downloadable_fonts.enabled
is true (default).

Comment 1

[Mats Palmgren (inactive)]

Attachment: frame tree

The out-of-flow frame (red) that should've been on the first page has been
placed on the FixedList of the second page for some reason.

Comment 2

[Mats Palmgren (inactive)]

FTR, there are also some assertions - when entering Print Preview:

###!!! ASSERTION: style context has old rule node: 'mRoots[i]->RuleNode()->RuleTree() == mRuleTree', layout/style/nsStyleSet.cpp, line 254
###!!! ASSERTION: old rule tree still referenced: 'Not Reached', layout/style/nsStyleSet.cpp, line 2081

and when closing Print Preview:
###!!! ASSERTION: destroying style context from old rule tree too late: 'styleSet->GetRuleTree() == mRuleNode->RuleTree() || styleSet->IsInRuleTreeReconstruct()', layout/style/nsStyleContext.cpp, line 92
###!!! ASSERTION: destroying style context from old rule tree too late: 'styleSet->GetRuleTree() == mRuleNode->RuleTree() || styleSet->IsInRuleTreeReconstruct()', layout/style/nsStyleContext.cpp, line 92
###!!! ASSERTION: destroying style context from old rule tree too late: 'styleSet->GetRuleTree() == mRuleNode->RuleTree() || styleSet->IsInRuleTreeReconstruct()', layout/style/nsStyleContext.cpp, line 92

The assertions may be a separate issue though.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1 - Remove mFixedContainingBlock. Make GetAbsoluteContainingBlock() check for an ancestor with the right frame type instead.

There are multiple underlying bugs here, I'll describe them one by one.

What happens in Print Preview on the testcase is that the (fixed pos)
element (see the frame dump above) using the remote font gets
a restyle request, which leads to a RecreateFramesForContent call.

When inserting the new frame, we create a nsFrameConstructorState
and in particular call GetAbsoluteContainingBlock which in this case
returns mFixedContainingBlock as the fixed pos. container:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1ff07cb9adaf#5996
mFixedContainingBlock is assigned when creating a ViewportFrame
or PageContentFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1ff07cb9adaf#2676
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1ff07cb9adaf#2953
Thus, when printing, it's the PageContentFrame in the last printed page.
So that's why RecreateFramesForContent inserts the new fixed pos
frame into the last page.

The proposed solution is to remove mFixedContainingBlock as
a state and instead make GetAbsoluteContainingBlock return the
appropriate ancestor frame.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - Move incremental reflow hack from nsSimplePageSequenceFrame::Reflow to nsTableOuterFrame. This is to avoid breaking pages that don't even use tables.

So, with the above fix the frame tree is now correct but it
still doesn't render the fixed pos content on the first page.
That's because we have intentionally disabled incremental reflows
in Print Mode ("until we've taught tables how to do it right in
paginated mode"):
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?rev=11fa2110679d#172

I suggest that we move that hack to nsTableOuterFrame::Reflow
instead so that pages that doesn't use tables isn't crippled by it.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3 - Deal with the placeholder being on a different page than the out-of-flow frame.

So, with those two fixes we now get a crash in
layout/base/crashtests/675246-1.xhtml
This testcase creates a frame tree where the placeholder and its
out-of-flow are on different pages (even before the earlier patches),
but we have never reflowed such a frame tree though (because of the
wallpaper removed in part 1).  It's a null-pointer crash in
CalculateHypotheticalBox because we never find the 'cbrs->frame' in
the ancestors on page 2, b/c it's the PageContentFrame on page 1.
I think we can just subtract the offset from the root to 'cbrs->frame'
at this point to get the correct offset.

Comment 6

[Mats Palmgren (inactive)]

Attachment: float-clear-000-print.html frame dumps

And there's another crash in ReflowPushedFloats on
layout/reftests/pagination/float-clear-000-print.html
(Again, a frame tree we haven't reflowed due to the wallpaper removed in
part 1).  Here we have two consecutive (sibling) pushed floats in the
frame list we're iterating, we save the current frame's next-sibling
so we can continue iterating from there, but in this case the
next-sibling is also the next-in-flow and it gets pushed.
Here are some frame dumps to illustrate what happens.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 4 - Don't use the current next-sibling as a reference point where to continue the loop, since that frame may be pushed if it's also the next-in-flow. Instead, use the frame from the last iterati

The fix for that is to keep track of the last iterated frame instead
and use that as the reference point where to continue the iteration.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 5 - Don't report a reflow as NS_FRAME_NOT_COMPLETE when it's actually complete. Because it will give the frame the wrong BSize.

And lastly, there is now a layout error for
layout/reftests/pagination/float-clear-000-print.html
https://tbpl.mozilla.org/?tree=Try&rev=7ac702469208
This is because our reflow of pushed floats isn't stable (i.e. reflowing
it twice yeilds different results).  The problem here is that we change
the reflow status from COMPLETE to NOT_COMPLETE when our next-in-flow
has pushed floats.  This is bad since it affects the height of the
current frame (we end up in the last 'else' block in ComputeFinalSize).
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=1289066eccbe#1548

As the note says, this code is a wallpaper for a columns crash:
http://hg.mozilla.org/mozilla-central/rev/2ed966e4fa58
I'm not sure if it's still needed, but relaxing the condition somewhat
(making the reflow status OVERFLOW_INCOMPLETE when our next-in-flow
is an overflow container) seems safe for now and it fixes the layout
bug.

https://tbpl.mozilla.org/?tree=Try&rev=0a09d78c8196

Comment 9

[Mats Palmgren (inactive)]

Attachment: reftest

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8533768 [details] [diff] [review]
part 2 - Move incremental reflow hack from nsSimplePageSequenceFrame::Reflow to nsTableOuterFrame. This is to avoid breaking pages that don't even use tables.

Review of attachment 8533768 [details] [diff] [review]:
-----------------------------------------------------------------

This is rather scary since it enables incremental reflows in paginated documents for most content other than tables. A lot of those code paths will have been tested by multicol, but some won't, or not well. This is still the right thing to do but we probably should alert our fuzzers.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> This is rather scary since it enables incremental reflows in paginated
> documents for most content other than tables. A lot of those code paths will
> have been tested by multicol, but some won't, or not well. This is still the
> right thing to do but we probably should alert our fuzzers.

For a while we disabled breaking floats in multicol containers because of incremental reflow bugs. I don't remember if we ever re-enabled that. If we did not, then this change exposes us to those bugs again. Please check.

Comment 12

[Mats Palmgren (inactive)]

It's still disabled (bug 447660) - I responded there.  I think the risk is lower
for printing since incremental reflows are rare in this context.

I can add back the removed chunk in nsSimplePageSequenceFrame behind a pref
if you want -- then we have the option to disable it with a hotfix.  If that
option isn't needed then we can land it as is I think -- it's trivial to add
that chunk back during Beta if we find bugs.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK but please stay alert :-)

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d2fa3b4cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/940e9ff05cfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a373a63d7de
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe60976ce023
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9c81ceec62
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2184fba8cc1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c85d2fa3b4cb
https://hg.mozilla.org/mozilla-central/rev/940e9ff05cfe
https://hg.mozilla.org/mozilla-central/rev/9a373a63d7de
https://hg.mozilla.org/mozilla-central/rev/fe60976ce023
https://hg.mozilla.org/mozilla-central/rev/8c9c81ceec62
https://hg.mozilla.org/mozilla-central/rev/c2184fba8cc1
https://hg.mozilla.org/mozilla-central/rev/85d72b79dd43

Comment 16

[Mats Palmgren (inactive)]

Bug 1152354 added back the incremental reflow hack in nsSimplePageSequenceFrame
so I suspect this bug still occurs.  I filed bug 1157012.

Comment 17

[josephus.ni]

(In reply to Mats Palmgren (:mats) from comment #16)
> Bug 1152354 added back the incremental reflow hack in
> nsSimplePageSequenceFrame
> so I suspect this bug still occurs.  I filed bug 1157012.


Yes, this bug still occurs:  zencellose.zenglobalsolutions.com/datasheets/test3.html you can try to print and see that first page is missing.  Sometimes in preview it is there, sometimes not, but header and footer never makes it to paper on first page.