Closed Bug 427017 Opened 14 years ago Closed 14 years ago

Crash [@ nsIFrame::GetPositionIgnoringScrolling] on print preview

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: dholbert)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(5 files, 5 obsolete files)

Attached file 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
Flags: blocking1.9?
Crashes in Linux as well.  OS/Platform --> All.
OS: Windows XP → All
Hardware: PC → All
Per roc, this should definitely block.
Flags: blocking1.9? → blocking1.9+
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
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
Attached file 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
(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.
Attached file testcase 3 (asserts)
On Print-Preview, this slightly-more-minimized testcase triggers the same assertions that were mentioned in Comment 5.
Attachment #315000 - Attachment description: testcase 2 (doesn't crash) → testcase 2 (asserts)
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.
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.
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.
(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
Attachment #315018 - Attachment is obsolete: true
fix v2 also passes reftests.
Attachment #315026 - Flags: superreview?(roc)
Attachment #315026 - Flags: review?(fantasai.bugs)
Comment on attachment 315026 [details] [diff] [review]
fix v2 (adds fantasai's suggestions from comment 9)

Very nice. Looks good to me. :)
Attachment #315026 - Flags: review?(fantasai.bugs) → review+
Assignee: nobody → 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)
Attachment #315026 - Flags: superreview?(roc) → superreview?(dbaron)
   // 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 on attachment 315026 [details] [diff] [review]
fix v2 (adds fantasai's suggestions from comment 9)

Cool, thanks for those suggestions -- posting updated patch shortly.
Attachment #315026 - Flags: superreview?(dbaron)
Attachment #315026 - Attachment is obsolete: true
Attachment #315160 - Attachment is obsolete: true
Attachment #315199 - Attachment description: patch v3 (adds reftest & roc's suggestions in comment 16) → fix v3 (adds reftest & roc's suggestions in comment 16)
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.
Attachment #315199 - Flags: superreview+
Attachment #315199 - Flags: review+
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.
Attachment #315199 - Attachment is obsolete: true
Attachment #315205 - Flags: superreview+
Attachment #315205 - Flags: review+
Here's a new interdiff, just for thoroughness' sake.
Attachment #315201 - Attachment is obsolete: true
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.)
Attachment #315205 - Flags: approval1.9?
Comment on attachment 315205 [details] [diff] [review]
fix v3a ( s/line_iterator/nsBlockFrame::line_iterator)

a1.9=beltzner
Attachment #315205 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Priority: -- → P2
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 365359
Crash Signature: [@ nsIFrame::GetPositionIgnoringScrolling]
You need to log in before you can comment on or make changes to this bug.