Crash [@ nsIFrame::GetPositionIgnoringScrolling] on print preview

VERIFIED FIXED

Status

()

defect
P2
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: dholbert)

Tracking

({crash, regression, testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 5 obsolete attachments)

Reporter

Description

11 years ago
Posted 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
Reporter

Updated

11 years ago
Flags: blocking1.9?
Assignee

Comment 1

11 years ago
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+
Assignee

Comment 3

11 years ago
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
Assignee

Comment 4

11 years ago
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
Assignee

Comment 5

11 years ago
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
Assignee

Comment 6

11 years ago
(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.
Assignee

Comment 7

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

Updated

11 years ago
Attachment #315000 - Attachment description: testcase 2 (doesn't crash) → testcase 2 (asserts)
Assignee

Comment 8

11 years ago
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

11 years ago
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.
Assignee

Comment 10

11 years ago
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.
Assignee

Comment 11

11 years ago
(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
Assignee

Comment 12

11 years ago
fix v2 also passes reftests.
Assignee

Updated

11 years ago
Attachment #315026 - Flags: superreview?(roc)
Attachment #315026 - Flags: review?(fantasai.bugs)

Comment 13

11 years ago
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

Updated

11 years ago
Assignee: nobody → dholbert
Assignee

Comment 15

11 years ago
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.
Assignee

Comment 17

11 years ago
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)
Assignee

Comment 18

11 years ago
Attachment #315026 - Attachment is obsolete: true
Attachment #315160 - Attachment is obsolete: true
Assignee

Comment 19

11 years ago
Assignee

Updated

11 years ago
Attachment #315199 - Attachment description: patch v3 (adds reftest & roc's suggestions in comment 16) → fix v3 (adds reftest & roc's suggestions in comment 16)
Assignee

Comment 20

11 years ago
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+
Assignee

Comment 21

11 years ago
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+
Assignee

Comment 22

11 years ago
Here's a new interdiff, just for thoroughness' sake.
Attachment #315201 - Attachment is obsolete: true
Assignee

Comment 23

11 years ago
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+
Assignee

Comment 25

11 years ago
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: 11 years ago
Flags: in-testsuite+
Priority: -- → P2
Resolution: --- → FIXED
Reporter

Comment 26

11 years ago
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

Updated

10 years ago
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.