427017 - Crash [@ nsIFrame::GetPositionIgnoringScrolling] on print preview

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes with current testcase on print preview.

This regressed between 2008-01-23 and 2008-01-24:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-01-23+04&maxdate=2008-01-24+09&cvsroot=%2Fcvsroot
So I think a regression from bug 368079, somehow.

http://crash-stats.mozilla.com/report/index/cd2a0f8c-01f8-11dd-ab4b-001a4bd43ef6
0  	nsIFrame::GetPositionIgnoringScrolling()  	 mozilla/layout/generic/nsIFrame.h:712
1 	nsHTMLReflowState::CalculateHypotheticalBox(nsPresContext*, nsIFrame*, nsIFrame*, int, int, nsHTMLReflowState const*, nsHypotheticalBox&) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1050
2 	nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsHTMLReflowState const*, int, int) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1100
3 	xul.dll@0x2ae998 	
4 	nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) 	mozilla/layout/generic/nsHTMLReflowState.cpp:307
5 	nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsHTMLReflowState const&, nsIFrame*, nsSize const&, int, int, int) 	mozilla/layout/generic/nsHTMLReflowState.cpp:176
6 	nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, nsHTMLReflowState const&, int, int, int, nsIFrame*, unsigned int&, nsRect*) 	mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:414
7 	@0x3fffffff

Comment 1

[Daniel Holbert [:dholbert]]

Crashes in Linux as well.  OS/Platform --> All.

Comment 2

[Damon Sicore (:damons)]

Per roc, this should definitely block.

Comment 3

[Daniel Holbert [:dholbert]]

I get these assertions & warnings leading up to the crash:

###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131
###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131
###!!! ASSERTION: cannot call GetUsedPadding on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file mozilla/layout/generic/nsFrame.cpp, line 589
###!!! ASSERTION: cannot call GetUsedBorder on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file mozilla/layout/generic/nsFrame.cpp, line 560
###!!! ASSERTION: comparing iterators over different lists: 'mListLink == aOther.mListLink', file mozilla/layout/generic/nsLineBox.h, line 690
###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131
###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131
WARNING: Frames were in different child lists???: file mozilla/layout/base/nsLayoutUtils.cpp, line 478
###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file mozilla/layout/base/nsPresShell.cpp, line 3225
WARNING: Frames were in different child lists???: file mozilla/layout/base/nsLayoutUtils.cpp, line 478
WARNING: Frames were in different child lists???: file mozilla/layout/base/nsLayoutUtils.cpp, line 478
###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file mozilla/layout/base/nsPresShell.cpp, line 3225
###!!! ASSERTION: cannot call GetUsedPadding on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file mozilla/layout/generic/nsFrame.cpp, line 589
###!!! ASSERTION: cannot call GetUsedBorder on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file mozilla/layout/generic/nsFrame.cpp, line 560
###!!! ASSERTION: Should hit cbrs->frame before we run off the frame tree!: 'aContainingBlock', file mozilla/layout/generic/nsHTMLReflowState.cpp, line 1049

Comment 4

[Daniel Holbert [:dholbert]]

For ease-of-reading, here's that same list of assertions & warnings, sorted and with duplicates removed and with linebreaks in between.

###!!! ASSERTION: cannot call GetUsedBorder on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file mozilla/layout/generic/nsFrame.cpp, line 560

###!!! ASSERTION: cannot call GetUsedPadding on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file mozilla/layout/generic/nsFrame.cpp, line 589

###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file mozilla/layout/base/nsPresShell.cpp, line 3225

###!!! ASSERTION: comparing iterators over different lists: 'mListLink == aOther.mListLink', file mozilla/layout/generic/nsLineBox.h, line 690

###!!! ASSERTION: Should hit cbrs->frame before we run off the frame tree!: 'aContainingBlock', file mozilla/layout/generic/nsHTMLReflowState.cpp, line 1049

###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131

WARNING: Frames were in different child lists???: file mozilla/layout/base/nsLayoutUtils.cpp, line 478

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (asserts)

This reduced testcase doesn't crash, but it triggers these messages on entering print-preview:

###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131

###!!! ASSERTION: comparing iterators over different lists: 'mListLink == aOther.mListLink', file mozilla/layout/generic/nsLineBox.h, line 690

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> ###!!! ASSERTION: Someone didn't override Reflow or ComputeAutoSize: 'Not
> Reached', file mozilla/layout/generic/nsLeafFrame.cpp, line 131

We're hitting this assertion in the do-nothing function 'nsLeafFrame::GetIntrinsicHeight()', with the 'this' pointer referring to a nsPageBreakFrame.

In other words, nsPageBreakFrame doesn't implement GetIntrinsicHeight, but it needs to, in order to avoid hitting this NS_NOTREACHED.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (asserts)

On Print-Preview, this slightly-more-minimized testcase triggers the same assertions that were mentioned in Comment 5.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix for 'comparing iterators over different lists' assertion

Not sure if it's correct, but this is a simple fix for the "comparing iterators over different lists" assertion that we hit on all of this bug's testcases.

(It still doesn't fix the crash on the first testcase, however.)

The patch is hopefully pretty self-explanatory.  Basically, we were failing that assertion during the "lineBox != blockFrame->end_lines()" comparison, because we were comparing a line from the body's *overflow* list against the end-marker for the body's *main* list.  And those are two separate lists, so that's an invalid comparison.

AFAIK, it's completely reasonable for lineBox to be in the overflow list at that point  -- when we initialize "iter", we basically look for the line that has  aPlaceholderFrame, first searching the block's normal lines, and then searching the overflow lines.  In this particular case, aPlaceholderFrame is in the overflow lines, so iter (and lineBox) end up pointing there.

Comment 9

[fantasai]

I think nsLeafFrame's implementation of returning 0 for GetIntrinsicHeight should be fine for nsPageBreakFrame. nsPageBreakFrame is not really a good way of implementing page breaking, but we can't fix that in this cycle. (Rewriting page breaking is likely to be my coding project for the next cycle.)

I'm wondering if absolute or fixed positioned elements should ignore any page-breaking properties set on that element. We might want a patch to nsCSSFrameConstructor to not generate them for these types of frames in any case. I don't think our positioning code is expecting positioned frames to have nsPageBreakFrame siblings.

Comment 10

[Daniel Holbert [:dholbert]]

By the way:  the fix I just posted (attachment 315018 [details] [diff] [review]) doesn't fix any of the other assertions mentioned in comment 3  and comment 4 -- it just fixes the "comparing iterators" asserts.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (adds fantasai's suggestions from comment 9)

(In reply to comment #9)
> I think nsLeafFrame's implementation of returning 0 for GetIntrinsicHeight
> should be fine for nsPageBreakFrame.

Cool, that's what I thought.  This patch gives nsPageBreakFrame its very own implementation, so that it doesn't trigger the NS_NOTREACHED in nsLeafFrame. (Alternately, we could remove the notreached, but I'm assuming it's there for a reason.)

> I'm wondering if absolute or fixed positioned elements should ignore any
> page-breaking properties set on that element. We might want a patch to
> nsCSSFrameConstructor to not generate them for these types of frames in any
> case.

This patch adds that as well.

Results:
--------
This patch fixes the crash -- yay!  It leaves us with these assertions/warnings on the original testcase, when starting print-preview:

WARNING: Frames were in different child lists???: file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/base/nsLayoutUtils.cpp, line 478
###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/base/nsPresShell.cpp, line 3225
WARNING: Frames were in different child lists???: file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/base/nsLayoutUtils.cpp, line 478
WARNING: Frames were in different child lists???: file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/base/nsLayoutUtils.cpp, line 478
###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/base/nsPresShell.cpp, line 3225
###!!! ASSERTION: cannot call GetUsedPadding on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/generic/nsFrame.cpp, line 589
###!!! ASSERTION: cannot call GetUsedBorder on a dirty frame not currently being reflowed: 'nsLayoutUtils::sDisableGetUsedXAssertions || !NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /scratch/work/builds/trunk.08-04-10.10-14/mozilla/layout/generic/nsFrame.cpp, line 560

Comment 12

[Daniel Holbert [:dholbert]]

fix v2 also passes reftests.

Comment 13

[fantasai]

Comment on attachment 315026 [details] [diff] [review]
fix v2 (adds fantasai's suggestions from comment 9)

Very nice. Looks good to me. :)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: reftest patch (based on 'testcase')

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 315026 [details] [diff] [review]
fix v2 (adds fantasai's suggestions from comment 9)

switching r? from roc to dbaron.

(I just remembered that it's currently Saturday morning in New Zealand, and I'd like to get this in sooner rather than later if possible)

Comment 16

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

   // See if page-break-before is set for all elements except row groups, rows, cells 
   // (these are handled internally by tables) and construct a page break frame if so.

Do you need to change this comment?

+      if (iter.GetInOverflow() ? 
+          lineBox != blockFrame->GetOverflowLines()->end() :
+          lineBox != blockFrame->end_lines()) {

Probably should add a method to nsBlockInFlowLineIterator to get the end-of-line-list iterator.

sr+ with those fixed.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 315026 [details] [diff] [review]
fix v2 (adds fantasai's suggestions from comment 9)

Cool, thanks for those suggestions -- posting updated patch shortly.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (adds reftest & roc's suggestions in comment 16)

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: interdiff between fixes v2 and v3

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 315199 [details] [diff] [review]
fix v3 (adds reftest & roc's suggestions in comment 16)

(In reply to comment #16)
> sr+ with those fixed.

Marking fixed patch r+sr, per comment 16.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: fix v3a ( s/line_iterator/nsBlockFrame::line_iterator)

Oops, posted wrong version of patch before -- the only change in this new version is that I replaced "line_iterator" with "nsBlockFrame::line_iterator" in the function header here:

  +nsBlockFrame::line_iterator
  +nsBlockInFlowLineIterator::End()

to fix a compile issue.

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: interdiff between fixes v2 and v3a

Here's a new interdiff, just for thoroughness' sake.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 315205 [details] [diff] [review]
fix v3a ( s/line_iterator/nsBlockFrame::line_iterator)

Requesting approval for fix v3a.  Justification:
 - Fixes multiple assertions & a scary-looking crash (see comment 2)
 - Patch is pretty small & low-risk
 - Includes a regression reftest :)

(Note that I set r+sr myself on roc's behalf, per comments 16 and 20.)

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315205 [details] [diff] [review]
fix v3a ( s/line_iterator/nsBlockFrame::line_iterator)

a1.9=beltzner

Comment 25

[Daniel Holbert [:dholbert]]

fix v3a checked in.

Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1473; previous revision: 1.1472
done
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.947; previous revision: 3.946
done
Checking in layout/generic/nsBlockFrame.h;
/cvsroot/mozilla/layout/generic/nsBlockFrame.h,v  <--  nsBlockFrame.h
new revision: 3.270; previous revision: 3.269
done
Checking in layout/generic/nsHTMLReflowState.cpp;
/cvsroot/mozilla/layout/generic/nsHTMLReflowState.cpp,v  <--  nsHTMLReflowState.cpp
new revision: 1.299; previous revision: 1.298
done
Checking in layout/generic/nsPageFrame.cpp;
/cvsroot/mozilla/layout/generic/nsPageFrame.cpp,v  <--  nsPageFrame.cpp
new revision: 3.180; previous revision: 3.179
done
Checking in layout/generic/nsPageFrame.h;
/cvsroot/mozilla/layout/generic/nsPageFrame.h,v  <--  nsPageFrame.h
new revision: 3.62; previous revision: 3.61
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/427017-1.xhtml,v
done
Checking in layout/reftests/bugs/427017-1.xhtml;
/cvsroot/mozilla/layout/reftests/bugs/427017-1.xhtml,v  <--  427017-1.xhtml
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.439; previous revision: 1.438
done

Comment 26

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041206 Minefield/3.0pre

No crash anymore on print preview.