Found this bug while modifying testcases for bug 363729. To reproduce, do a print-preview of the testcase. The second page should have only one row of "___" and only one row of "+++" below that. But instead, we get an extra copy of the "+++", superimposed on top of the "___". This bug is also present in Firefox 2.0 and Firefox 1.5. Haven't tested Firefox 1.0. IE7 displays this correctly. (with just one copy of each row)

Here's a similar testcase with a few more levels of nesting, to show an exaggerated version of the bug. On this one, we get several rows of superimposed _, +, /, and ! characters.

Figured out the problem. When we first construct the PrintPreview frametree, we do it all on one page. This page's framedump looks something like this: http://pastebin.mozilla.org/177644 Notice that the actual page content only has **ONE** placeholder, even though there are two elements in the Fixed-list. This is because the placeholder for the second fixed element is **in** the first fixed element. Now, to create the second page, there's a point where we call CreateContinuingFrame on the first page's Area frame. Towards the end of that CreateContinuingFrame call (at nsCSSFrameConstructor.cpp:10678), we iterate across and copy **every element in the first page's fixed-list*** and the resulting list of copies becomes the new PageContent's frame list. This is bad -- we shouldn't copy everything in the first page's fixed-list. Only the first placeholder belongs in the second page's PageContent (the way it is on the first page), so we should only copy the first element of fixed-content. In fact, just copying that first fixed element will automagically copy the second element, because the second element is a child of the first in the content tree. When we *explicitly* copy the second fixed element at the end of CreateContinuingFrame, we create a duplicate **erroneous** copy, in addition to the automagical copy. This is why we get the two lines on the second page. We're seeing both the automagical copy and the erroneous copy. SO: I'd propose that at the end of CreateContinuingFrame, we only copy those fixed-elements whose placeholders are descendants of the frame that we're copying. Will post a patch to that effect soon. (if it works)

This patch implements the fix described at the end of comment 2. (at the end of CreateContinuingFrame, only copy those fixed-elements whose placeholders are descendants of the frame that we're copying.)

here's the no-whitespace-changes version of the previously posted patch

Comment on attachment 273917 [details] [diff] [review] initial patch Note that this patch fixes Bug 363729 (blocking1.9) as well. If this patch is the right thing to do, I'll mark the bugs as duplicates.

Use nsLayoutUtils::IsProperAncestorFrame. And in your comment mention that aFrame is the page content frame's sole normal child (and maybe assert that).

Weird... In my fresh trunk checkout from 11:53am today, I'm can't get *any* fixed text to appear on the second page. I think that's the reason my patch seemed to fix bug 363729 -- when I apply the patch to an earlier build, I still get bug 363729 showing itself. And when I try to reproduce bug 363729 using this morning's fresh build, I see no crash. I'm filing this new issue as a separate bug, which will need to be resolved before either this bug or bug 363729 can be closed, because neither bug can be reproduced until the new issue is addressed.

(In reply to comment #7) > I'm filing this new issue as a separate bug bug 389767, to be exact.

Incorporating roc's comments.

cvs diff -w version of patch v2

Note: Currently, as described in bug 389767, one needs to back out the final patch for bug 379349 in order to get any fixed-position content on the second page. Without backing that patch out, this bug's patches won't do anything, and the testcases will have no fixed content on their second page.

+ NS_ASSERTION(prevPageContentFrame->GetFirstChild(nsGkAtoms::normal) == aFrame + && aFrame->GetNextSibling() == nsnull, + "aFrame should be the prevPageContentFrame's sole normal child"); You pass nsnull to GetFirstChild to get the normal children. Doesn't this assert always fire? && aFrame->GetNextSibling() == nsnull, You could write !aFrame->GetNextSibling(). + if (origPlaceholder) { + if (nsLayoutUtils::IsProperAncestorFrame(aFrame, origPlaceholder)) { Use && so you only need one if.

Thanks roc for the comments. You're right about the assertion -- it triggered all the time because I was using nsGkAtoms::normal instead of nsnull. My bad -- I should've caught that. Here's a new version, incorporating roc's suggestions.

roc, thanks for the review. Adding checkin-needed to keywords.

I'll check it in in a couple hours, when I'll be able to watch the tree cycle. :)

Rather, I'll check it in when the tree reopens.

Comment on attachment 274520 [details] [diff] [review] patch v3 (no whitespace changes) a1.9=dbaron

fantasai's amended patch looks good to me. I tested it, too, and it fixes the issue with no problems.

Fix checked in. Thanks dholbert!