570160 - [regression] crash when printing or in print preview and A4 as page format [@ nsIFrame::SetNextSibling]

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Ilja Sekler]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100604 Firefox/3.7a5pre
Build Identifier: 

Firefox crashes when printing or in print preview for the $URL if page format is not Letter (tested with A4).

Good: 2009-10-02, SourceStamp=a80414164888

Bad: 2009-10-03, SourceStamp=0a7dd88dbe67

Regression window: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-10-02+04%3A26%3A27&enddate=2009-10-03+02%3A36%3A30



Reproducible: Always

Steps to Reproduce:
1. Set page format to A4
2. Load <http://www.musicalcriticism.com/concerts/usherhall-rsno-clein-0510.shtml>
3. Print or print preview the page.
Actual Results:  
Crash.



The regression window is identical with <https://bugzilla.mozilla.org/show_bug.cgi?id=553972#c15>, so this bug might be a dupe of Bug 553972, which should be reopened in this case.

The crash report whith an old official nightly while searching for the first bad build: <http://crash-stats.mozilla.com/report/index/bp-66b61fb8-7e07-4fb5-9ac9-c973a2100604>

Wasn't able to make a minimal testcase yet.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

crashes on windows xp too with 1.9.3 20100604040314

bp-9d5d052b-8df2-4dd9-9344-c75882100604
Signature	nsIFrame::SetNextSibling(nsIFrame*)
UUID	9d5d052b-8df2-4dd9-9344-c75882100604
Time 	2010-06-04 10:54:10.198115
Uptime	39
Last Crash	47257 seconds before submission
Product	Firefox
Version	3.7a5pre
Build ID	20100604040314
Branch	1.9.3
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x20
User Comments	Bug 570160

Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsIFrame::SetNextSibling(nsIFrame*) 	layout/generic/nsIFrame.h:952
1 	xul.dll 	nsFrameList::RemoveFrame(nsIFrame*) 	layout/generic/nsFrameList.cpp:133
2 	xul.dll 	nsBlockFrame::CollectFloats(nsIFrame*,nsFrameList&,int,int) 	layout/generic/nsBlockFrame.cpp:6645
3 	xul.dll 	nsBlockFrame::CollectFloats(nsIFrame*,nsFrameList&,int,int) 	layout/generic/nsBlockFrame.cpp:6651
4 	xul.dll 	nsBlockFrame::PushLines(nsBlockReflowState&,nsLineList_iterator) 	layout/generic/nsBlockFrame.cpp:4273
5 	xul.dll 	nsBlockFrame::PlaceLine(nsBlockReflowState&,nsLineLayout&,nsLineList_iterator,nsFloatManager::SavedState*,nsRect&,int&,int*) 	
6 	xul.dll 	nsAStreamCopier::Process() 	xpcom/io/nsStreamUtils.cpp:380
7 	nspr4.dll 	_PR_MD_UNLOCK 	nsprpub/pr/src/md/windows/w95cv.c:344
8 	nspr4.dll 	PR_Unlock 	nsprpub/pr/src/threads/combined/prulock.c:347
9 	xul.dll 	nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&,nsLineLayout&,nsLineList_iterator,nsIFrame*,LineReflowStatus*) 	layout/generic/nsBlockFrame.cpp:3722
10 	xul.dll 	nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&,nsLineLayout&,nsLineList_iterator,nsFlowAreaRect&,int&,nsFloatManager::SavedState*,int*,LineReflowStatus*,int) 	layout/generic/nsBlockFrame.cpp:3653
11 	nspr4.dll 	PR_Unlock 	nsprpub/pr/src/threads/combined/prulock.c:347
12 		@0x12b967

Comment 2

[Mats Palmgren (inactive)]

Attachment: framedump

###!!! ASSERTION: why CreateContinuingFrame for a non-splittable frame?: 'aFrame->GetSplittableType() != NS_FRAME_NOT_SPLITTABLE', file c:/moz/hg2/layout/base/nsCSSFrameConstructor.cpp, line 8512
###!!! ASSERTION: OOF must be first continuation: '!aFrame || !aFrame->GetPrevContinuation()', file c:\moz\hg2\layout\generic\nsPlaceholderFrame.h, line 115
###!!! ASSERTION: float manager state should match saved state: 'aState->mX == mX && aState->mY == mY && aState->mFloatInfoCount == mFloats.Length()', file c:\moz\hg2\layout\generic\nsFloatManager.h, line 263
###!!! ASSERTION: available space should never grow: 'aOldAvailableSpace.x <= aNewAvailableSpace.x && aOldAvailableSpace.XMost() >= aNewAvailableSpace.XMost()', file c:/moz/hg2/layout/generic/nsBlockFrame.cpp, line 828
###!!! ASSERTION: Broken frame linkage: 'prevSibling && prevSibling->GetNextSibling() == aFrame', file c:/moz/hg2/layout/generic/nsFrameList.cpp, line 132
###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', file c:\moz\hg2\layout\generic\nsIFrame.h, line 951


The first assertion is caused by trying to create a continuation for
a placeholder frame...

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 4

[Mats Palmgren (inactive)]

Patch coming up...

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Call SplitFloat() for incomplete placeholders also in the break-after case.

It would be fairly easy to re-arrange the code to share the 
SplitFloat/CreateNextInFlow bits for the break-after and
!NS_FRAME_IS_FULLY_COMPLETE blocks.  Let me know if you'd prefer that
and I'll make a new patch.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

Comment 7

[fantasai]

Yes, I think that would be better.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Rearranging the code, should be functionally the same as the last patch.

Comment 9

[fantasai]

Comment on attachment 454447 [details] [diff] [review]
Patch rev. 2

Overall, looks good. I think, however, that this bit

+  PRBool isBreakAfter = NS_INLINE_IS_BREAK_AFTER(aStatus);
+  if (isBreakAfter ? NS_FRAME_IS_NOT_COMPLETE(aStatus)
+                   : !NS_FRAME_IS_FULLY_COMPLETE(aStatus)) {
+  [...]
+  if (isBreakAfter) {

should be
+  if (!NS_FRAME_IS_FULLY_COMPLETE(aStatus)) {
+  [...]
+  if (NS_INLINE_IS_BREAK_AFTER(aStatus)) {

r+ with that change, unless you disagree

Comment 10

[Mats Palmgren (inactive)]

Ok, I thought about that too, but I kept the old logic to minimize risk.
(the old logic doesn't make a continuation for
  "isBreakAfter && NS_FRAME_OVERFLOW_INCOMPLETE")
Are you saying that combination cannot occur or is the old logic
simply broken and it should have made a continuation in that case too?
(I'll add an assertion if it shouldn't occur)

Comment 11

[fantasai]

I believe it should have made a continuation in that case, too. I suspect I didn't update it to handle overflow containers in that instance because there was no code to handle placeholders, so it seemed there were nothing to handle.

Comment 12

[Mats Palmgren (inactive)]

Ok, thanks for clarifying.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Updated per review comments.  I'll land this when the tree reopens...

Comment 14

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/c0536c4f3a90