790260 - Print preview on a certain page crashes Firefox

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Pedro de Oliveira]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.89 Safari/537.1

Steps to reproduce:

Open the following URL: http://www.festadoavante.pcp.pt/2012/como-chegar

Then go to menu "File", and click on "Print Preview".


Actual results:

Firefox gets freezed, the only solution is to kill it.


Expected results:

Get a print preview of the page.

Comment 1

[Pedro de Oliveira]

Its not really a CRASH, it just hangs until its killed.

Comment 2

[[:Aleksej]]

Hangs on Linux x86_64 with Fx 15.0 and Nightly 2012-09-11. Maximum CPU usage, slow memory growth.

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

Same 'hang' on latest m-c Win32 hourly build on win7 x64
went 'not responding' - had to kill process.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0

Comment 4

[[:Aleksej]]

kill -ILL on a process:
bp-f4effc66-2499-4db6-9d1b-56a952120911
bp-b07c1a05-a389-483c-ae69-a2b3d2120911

Comment 5

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/373d1b393f28
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100805 Minefield/4.0b4pre ID:20100805200154
Hang:
http://hg.mozilla.org/mozilla-central/rev/8ab7ef79b673
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100805 Minefield/4.0b4pre ID:20100805224228
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=373d1b393f28&tochange=8ab7ef79b673

Suspected: Bug 563584

Comment 6

[(mostly gone) XtC4UaLL [:xtc4uall]]

Maybe Dupe of/related to Bug 647792?

Comment 7

[Mats Palmgren (inactive)]

This is still reproducible in Nightly.  It looks like we're pushing a float
that doesn't fit to the next page without making progress on the layout
so we push it forward ad infinitum. 

A minimized testcase would help here.

Comment 8

[Alice0775 White]

Attachment: reduced html

Browser may stop responding when print preview landscape.


Steps to reproduce:
1. Open attached reduced html
2. Alt > "File" > Page Setup > Landscape
3. Print Preview

Actual Results:
Browser hangs

Comment 9

[Lee Salzman [:lsalzman]]

It looks like, running Alice's reduced test case, it gets stuck in nsSimplePageSequenceFrame::Reflow creating new overflow frames even as it is iterating over them:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSimplePageSequenceFrame.cpp?from=nsSimplePageSequenceFrame.cpp#301

Comment 10

[Markus Stange [:mstange]]

This bug has a testcase now. Mats, do you want to take another look?

Comment 11

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

I'll look into this.

Comment 12

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

There's a float "esquerda" containing a very tall image followed by another float "carro". Somehow we get into a situation where we start a new page, we reflow "esquerda" at the top of the page, and "carro" is a float child of "esquerda" whose placeholder is after the image but which has NS_FRAME_IS_PUSHED_FLOAT set on it. So we reflow "carro" first via ReflowPushedFloats, then reflow the image. The image doesn't fit, and although it's adjacent to the top of the page, because it's impacted by the "carro" float, we break before the image, so reflow of "esquerda" completes without us having placed anything, so "esquerda" gets pushed to the next page and everything repeats.

I think the problem is that NS_FRAME_IS_PUSHED_FLOAT is set incorrectly on "carro". It's incorrect because in this case the float's parent does contain the placeholder.

Comment 13

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

Attachment: MozReview Request: Bug 790260. Frames that we pull into our normal float list from our overflow-float-list should not be treated as pushed floats. r=mats

Bug 790260. Frames that we pull into our normal float list from our overflow-float-list should not be treated as pushed floats. r=mats

Comment 14

[Mats Palmgren (inactive)]

I have stepped this through in a debugger and I agree that the float that
DrainSelfOverflowList() pulls in should not have NS_FRAME_IS_PUSHED_FLOAT
set in this case.  It seems to me this is an illegal state though and
that the error occurred earlier.

We set the PUSHED_FLOAT bit here:
#0  nsBlockReflowState::PushFloatPastBreak (..., aFloat=0x7fffc2f9f420)
#1  nsBlockReflowState::FlowAndPlaceFloat
#2  nsBlockReflowState::AddFloat
#3  nsLineLayout::AddFloat
#4  nsLineLayout::ReflowFrame (..., aFrame=0x7fffc2f9f668 ...
#5  nsBlockFrame::ReflowInlineFrame
#6  nsBlockFrame::DoReflowInlineFrames (this=0x7fffc21ec668 ...
#7  nsBlockFrame::ReflowInlineFrames
...
which results in this (pruned) frame tree:

Block(div)(1)@7fffc21ec668
  line 7fffc2f9fd18:
    Block(div)(1)
      line 7fffc2f9f3d0:
        ImageFrame(img)
      >
    >
  >
  line 7fffc2f9fd68:
    Placeholder(div)@7fffc2f9f668 ... outOfFlowFrame=Block(div)@7fffc2f9f420
  >
  PushedFloatsList 0x7fffc5ef11e0 <
    Block(div)(3)@7fffc2f9f420 [state=000b160100d00702] <
      line 7fffc2f9fcc8:
        Block(h2)
          line 7fffc2f9fc78:
            Text(0)"Se Vier de Autom\u00f3vel"
...>


This is a valid tree since the float is on a PushedFloatsList
list and has PUSHED_FLOAT set.

After completing the stack frames above, the parent eventually
decides to push the line 7fffc2f9fd68:

#0  nsBlockFrame::PushLines (this=0x7fffc21ec668, ...
#1  nsBlockFrame::PushTruncatedLine
#2  nsBlockFrame::PlaceLine
#3  nsBlockFrame::DoReflowInlineFrames
#4  nsBlockFrame::ReflowInlineFrames
...

The frame tree is the same as above at entry to PushLines.
After completing PushLines we have this tree:

Block(div)(1)@7fffc21ec668 
  line 7fffc2f9fd18:
    Block(div)(1)
      line 7fffc2f9f3d0:
        ImageFrame(img)
      >
    >
  >
  Overflow-lines 0x7fffc1d8c2c0/0x7fffc1d8c2d0 <
    line 7fffc2f9fd68:
      Placeholder(div)@7fffc2f9f668 ... outOfFlowFrame=Block(div)@7fffc2f9f420
    >
  >
  OverflowOutOfFlowList 0x7fffc5ef11f0 <
    Block(div)(3)@7fffc2f9f420 [state=000b160100d00702]
      line 7fffc2f9fcc8:
        Block(h2)
          line 7fffc2f9fc78:
            Text(0)"Se Vier de Autom\u00f3vel"
          >
...>

This seems invalid to me.  Since we're pushing the line with placeholder,
the float will certainly NOT be a pushed float when it's picked up by
the next-in-flow (or as in this case, pulled back in by the original
block).

Comment 15

[Mats Palmgren (inactive)]

Attachment: wip

PushLines calls CollectFloats to collect all floats associated
with the lines it's going to push here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=b002f72aa051#4576
If I remove the bit here instead then that also fixes the crash.

This seems like a better fix to me.  It's completely untested
so far though, except for the crash testcase here...

What do you think?

Comment 16

[Mats Palmgren (inactive)]

Let's see what Try says:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0455d4776174

Comment 17

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

Comment on attachment 8668723 [details] [diff] [review]
wip

Review of attachment 8668723 [details] [diff] [review]:
-----------------------------------------------------------------

I agree!!!

Comment 18

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

I agree!!!

Comment 19

[Mats Palmgren (inactive)]

OK. I'm stealing this to investigate whether we should actually do this in
CollectFloats itself instead.  The only other caller of CollectFloats is
nsBlockFrame::ReparentFloats, which is called from a few places but only
after we pull in a line from a next-in-flow AFAICT.  I suspect that floats
that we collect for placeholders on those lines should probably have their
pushed-float bit removed too.

Comment 20

[Mats Palmgren (inactive)]

Attachment: framedump 1

Here's one such case, for layout/generic/crashtests/436194-1.html.

We're about to reflow the first column, which will pull in the
first line from the next column, which has a placeholder (blue)
with a float that has a next-in-flow (red) on the PushedFloats list.
CollectFloats will collect both of these and ReparentFloats will
append them to mFloats.  Keeping the IS_PUSHED_FLOAT bit in this
case doesn't seem right.

Comment 21

[Mats Palmgren (inactive)]

FTR, the above also happens in:
layout/base/crashtests/589787.html
layout/generic/crashtests/810726.html

Comment 22

[Mats Palmgren (inactive)]

Attachment: fix

I think we should do this in CollectFloats instead, since all the consumers
needs to remove this bit.  In nsBlockFrame we're pushing/pulling a line
and collect floats for any placeholders on that line.  In nsInlineFrame
we're pushing/pulling an nsInlineFrame continuation and collect floats
for placeholder descendants of that continuation (through the ReparentFloats()
call in nsInlineFrame::ReparentFloatsForInlineChild to be precise).
The placeholder and collected floats will always end up in the same block,
and the floats always goes to the mFloats or OverflowOutOfFlows lists
which shouldn't have this bit when the placeholder is in the same block.

I sprinkled a few assertions about this bit that I think should hold.
We should probably add some sanity checking for floats on different
lists in nsBlockFrame::VerifyOverflowSituation too.  I'll file a follow-up
bug on that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40db0de39ed

(I'll land your crashtest as well.)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc298a55b2f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab65cc134189

Comment 24

[Mats Palmgren (inactive)]

(Filed follow-up bug 1212143 on adding sanity checks in VerifyOverflowSituation.)

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc298a55b2f7
https://hg.mozilla.org/mozilla-central/rev/ab65cc134189