363729 - Crash [@ nsIFrame::GetPositionIgnoringScrolling] on print preview that uses position: fixed

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

See upcoming testcase, that crashes Mozilla on print preview.
It doesn't crash with a 2006-12-07 build, but it does crash with a 2006-12-08 build, so I think this is a regression from bug 362708.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Talkback ID: TB27277037G
nsIFrame::GetPositionIgnoringScrolling  [mozilla\layout\generic\nsiframe.h, line 662]
nsHTMLReflowState::CalculateHypotheticalBox  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 900]
nsHTMLReflowState::InitAbsoluteConstraints  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 955]
nsHTMLReflowState::InitConstraints  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 1626]
nsHTMLReflowState::Init  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 233]
nsHTMLReflowState::nsHTMLReflowState  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 181]
nsAbsoluteContainingBlock::ReflowAbsoluteFrame  [mozilla\layout\generic\nsabsolutecontainingblock.cpp, line 363]
0x02b1b77c
0x01909f00

Comment 2

[Bernd]

"so I think this is a regression from bug 362708."

Thats questionable the patch in bug touched only splitting of table rowgroups, the testcase does not mention tables so its much more probable that the reflow branch landing is to blame.

Comment 3

[Martijn Wargers (dead)]

Sorry, you're correct, it is much more likely this is a regression from the reflow branch landing.

Comment 4

[Martijn Wargers (dead)]

This has become wfm somehow between 2007-02-13 and 2007-02-14:
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=2007-02-13+04&maxdate=2007-02-14+09&cvsroot=%2Fcvsroot
Not really sure what fixed it.

Comment 5

[Carsten Book [:Tomcat]]

Attachment: minimised testcase

Reopen- Still crash:

STR: 
1.) Open Attachment
2.) Print Preview
3.) Crash

TB TB30223504Z
Stack Signature	 nsIFrame::GetPositionIgnoringScrolling 2acb6cd9
Product ID	FirefoxTrunk
Build ID	2007031304
Trigger Time	2007-03-14 03:08:49.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	firefox.exe + (001778fd)
URL visited	testcase 363729
User Comments	
Since Last Crash	191 sec
Total Uptime	1152 sec
Trigger Reason	Access violation
Source File, Line No.	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsiframe.h, line 662
Stack Trace 	
nsIFrame::GetPositionIgnoringScrolling  [mozilla/layout/generic/nsiframe.h, line 662]
nsHTMLReflowState::CalculateHypotheticalBox  [mozilla/layout/generic/nshtmlreflowstate.cpp, line 903]
nsHTMLReflowState::InitAbsoluteConstraints  [mozilla/layout/generic/nshtmlreflowstate.cpp, line 958]
nsHTMLReflowState::InitConstraints  [mozilla/layout/generic/nshtmlreflowstate.cpp, line 1642]
nsHTMLReflowState::Init  [mozilla/layout/generic/nshtmlreflowstate.cpp, line 251]
nsHTMLReflowState::nsHTMLReflowState  [mozilla/layout/generic/nshtmlreflowstate.cpp, line 179]
nsAbsoluteContainingBlock::ReflowAbsoluteFrame  [mozilla/layout/generic/nsabsolutecontainingblock.cpp, line 367]

Comment 6

[Carsten Book [:Tomcat]]

reopen see comment #5

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase, even more minimized

Thanks for that testcase, Carsten. However it's possible to minimize it even a bit further.

Comment 8

[Martijn Wargers (dead)]

Still crashes, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070520 Minefield/3.0a5pre

Comment 9

[Leon Sha]

http://lxr.mozilla.org/seamonkey/source/layout/generic/nsHTMLReflowState.cpp#1013
1013       NS_ASSERTION(aContainingBlock,
1014                    "Should hit cbrs->frame before we run off the frame tree!");
1015       cbOffset += aContainingBlock->GetPositionIgnoringScrolling();

aContainingBlock is null here.

Comment 10

[Daniel Holbert [:dholbert]]

I get crash using:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072413 Minefield/3.0a7pre

Marking OS: All

Comment 11

[Daniel Holbert [:dholbert]]

From stepping through nsHTMLReflowState::InitAbsoluteConstraints and nsHTMLReflowState::CalculateHypotheticalBox, it seems that the problem is this:

We make this assumption:
  this->mCBReflowState->frame 
    is equal to (or possibly contains)
  this->frame's placeholder's nearest Containing Block

(this assumption is the basis for the loop at
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsHTMLReflowState.cpp#1031 )
(note that in the code, "cbrs" == this->mCBReflowState)

In this specific case, that assumption doesn't hold.  We run into a situation where 
this->frame and this->mCBReflowState->frame are in the second page, but this->frame's placeholder and that placeholder's nearest Containing Block are in the first page.  So, when we're tracing the parents of this->frame's placeholder, we never hit mCBReflowState->frame (a.k.a. cbrs->frame), and we run off the frame tree.

So, it seems that either:
 (A) this situation should never happen -- i.e. placeholder should always be on same page as the thing it's placeholder for
 -- OR --
 (B) this situation is valid, and we're just not handling it.

I'm not yet sure which of these is the case.

Comment 12

[Daniel Holbert [:dholbert]]

See also bug 389496.

Comment 13

[Eli Friedman]

I don't believe it is valid for a frame to have its placeholder on a different page.

Comment 14

[Daniel Holbert [:dholbert]]

See also bug 389619.

Comment 15

[Daniel Holbert [:dholbert]]

My initial patch for bug 389619 seems to fix the crash.  

After applying that patch, we end up with the second page having just 1 placeholder, which points to a TableOuter in the first page's Fixed-list.  There's now no Fixed-list on the second page at all.

This still has the curious property of having a Fixed-list frame and its placeholder being on different pages, which is what originally caused the crash (although the roles of the pages were reversed before -- i.e. we got a crash with a fixed frame on the *second* page whose placeholder was in the *first* page).

But even though this somewhat strange situation still exists, the code path that got caught on it and caused the crash isn't getting triggered after applying the patch for bug 389619.  I'm going to investigate why.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> But even though this somewhat strange situation still exists, the code path
> that got caught on it and caused the crash isn't getting triggered after
> applying the patch for bug 389619.  I'm going to investigate why.

Ah, here's the reason why.

It looks like we don't crash after my patch because the critical bit of code occurs during the reflow of a nsPageFrame, and it only checks the fixed-elements **in that nsPageFrame**.  And, importantly, each nsPageFrame is completely built and reflowed before we think about making the next one.

In more detail -- after the first PageContentFrame is built, we have something like this:
 Page #1
   PageContent
     Block(Body)
       [content]
       Overflow
         Placeholder for A
   Fixed-List
      Fixed Frame A
 
 ( No page 2 at all yet )

Towards the end of reflowing that first page, we hit the critical code path, which assumes that this page's fixed-list elements have their placeholders all on the same page.  This assumption holds (right now *everything's* on the same page), so we don't crash.

Then we build the second page, and the frame tree looks like this:

 Page #1
   PageContent
     Block(Body)
       [content]
   Fixed-List
      Fixed Frame A

 Page #2
   PageContent
     Block(Body)
       Placeholder for A

Now, we hit the critical code path for the second page.  The second page doesn't have any fixed-list elements, so the assumption trivially holds, and we don't crash.

Bottom line is that, from the standpoint of this bug's original crash: 
 - It's ok to have a Placeholder in a later page that points to fixed content in an earlier page
 - It's *not* ok to have a Placeholder in an earlier page that points to fixed content in a later page.

Comment 17

[Daniel Holbert [:dholbert]]

Oops, turns out I was confused -- my patch for bug 389619 doesn't fix the crash.  It was actually the existence of (new) Bug 389767 masking the crash.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: testcase using explicit table (crashes trunk *and* branch)

Branch wasn't susceptible to the previous testcase because it doesn't support the "display: inline-table" attribute.

Here's a version of Martijn's testcase that has the table explicitly specified.  This crashes both branch and trunk on print-preview.

Comment 19

[Daniel Holbert [:dholbert]]

Talked to dbaron about this a bit -- the root cause of this bug (in both branch and trunk) is that we don't allow tables to be containers for absolute content, when we really should.  (I think Bug 5016, Bug 10209, Bug 217002, and Bug 320865 all relate to the same issue.)
 
This means that, in the testcase, the frame for the "position: absolute" span ends up being a child of the **body** frame, instead of where it should be, a child of the fixed-position table.

So when we're building the second page and we copy the fixed content from the first page to the second page, we end up adding a new copy of the "position: absolute" span on the **first page's body** instead of putting it on the second page. 

Moreover, this new copy of the absolute span contains a placeholder pointing to the new copy of the innermost fixed-position span, which is (correctly) placed on the **second page**.  Hence, we hit the case where we've got fixed content on the second page whose placeholder is on the first page, and that causes the crash. (as described in Comment 11)

Comment 20

[Daniel Holbert [:dholbert]]

Unassigning myself from this bug for now, in the hopes that someone else with more experience in the workings of absolute content containers will be able to implement the fix I described in comment 19.

Comment 21

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

Making tables support abs-pos children seems like too much work for 1.9.

Comment 22

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

Attachment: fix

This patch works around the problem in ReplicateFixedFrames by pushing a null absolute containing block before we construct the replica frames. This prevents any of those constructed frames from being made abs-pos children of anything outside the replica frame tree. They just don't get absolutely positioned. Too bad.

It also includes another fix for an assertion triggered by this testcase about the height of an nsPageBreakFrame being negative. The root problem is that nsBlockReflowState::ComputeBlockAvailSpace can compute a negative available height when we reflow a block whose top margin is taller than the available height.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 286941 [details] [diff] [review]
fix

I like it.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 286941 [details] [diff] [review]
fix

Seems like you shuold just pass null for the absolute containing block two lines above rather than pushing it.  With that, or with a good reason not to, sr=dbaron.

We should probably make making tables (etc.) be absolute containing blocks a pretty high priority next release, though...

Comment 25

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

You're right, I'll just pass null there.

Comment 26

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

checked in with that

Comment 27

[Mats Palmgren (inactive)]

Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7384af4aeaa3

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7384af4aeaa3