46043 - [FLOAT] (Adds on?) site crashes

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Devoti Paolo]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; m17) Gecko/20000720
BuildID:    2000072008

open the url the browser hangs forever, no page refresh at all but is not crashing.

Reproducible: Always
Steps to Reproduce:
1.open the url http://www.radio-stations.net/language/esperanto.htm
2.Wait to have the page loaded
3.

Actual Results:  The browser is dead

Expected Results:  page loaded

Comment 1

[Peter 'Rufus' Nelson]

Accepting, adding crash keyword (because it crashed on me) and making test

Comment 2

[Peter 'Rufus' Nelson]

I can no longer reproduce the crash on thing page after the first crash.  I am
guessing that the crash (which I confirmed) and the reported crash have to do
with the add-banner, and hense is practicly impossible to testcase.

Comment 3

[Devoti Paolo]

Attachment: page with removed the banners that kills the browser(win98 2000072008)

Comment 4

[Devoti Paolo]

After removing the banners the browser is still crashing

Comment 5

[Mats Palmgren (inactive)]

I dont get any crashes, but I do see the deadlock before the page is loaded.
I have a small testcase for that (maybe it will crash for others.)
I am using build 2000-07-20-08 on Windows 98 SE.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase causing deadlock

Comment 7

[Peter 'Rufus' Nelson]

I still do not see any crash or deadlock on the original page, or the first test
case.  However I do see a deadlock on the second test case, but only at 800x600
resolution, not at a higher resolution.  It seems like to me that the 3
<IFRAME>'s get layed out like this:

<IFRAME><IFRAME>
                <IFRAME>

and that for some reason the <ol> gets layed out to the LEFT of this, which is
off the page and crashes.  This would explain why at higher resolutions it does
not crash (there's enough space to put the <ol> to the left)

Comment 8

[Doron Rosenberg (IBM)]

so, this is a iframe issue?

Comment 9

[Asa Dotzler [:asa]]

over to HTML Element

Comment 10

[Devoti Paolo]

win98 build 2000072108
I'm getting the effect I described with attachment I provided at 1280x1024 16 
bit color resolution. Actually the browser hangs, it's not a crash now.

Comment 11

[harishd]

Triaging clayton's list:
------------------------

I do see a crash ( loading 2nd attachment )!! Here is the trace.

nsDebug::Abort(const char * 0x020b2320, int 4137) line 315 + 3 bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineBox * 
0x02df4950, int * 0x0012c3b8, int 0, int 0) line 4137 + 16 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x02df4950, int 
* 0x0012c3b8, int 0) line 3262 + 29 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2951 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x02df48c8, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1756 + 15 bytes
nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Initial, nsIFrame * 0x02df48c8, const nsRect & {...}, int 0, int 
0, int 1, nsMargin & {...}, unsigned int & 0) line 519 + 36 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x02df48c8, const nsRect & {...}, 
int 0, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 344 + 50 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x02df4a74, int * 0x0012cf10) line 3880 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x02df4a74, int 
* 0x0012cf10, int 0) line 3144 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2951 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x02df4840, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1756 + 15 bytes
nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Initial, nsIFrame * 0x02df4840, const nsRect & {...}, int 1, int 
0, int 1, nsMargin & {...}, unsigned int & 0) line 519 + 36 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x02df4840, const nsRect & {...}, 
int 1, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 344 + 50 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x02df4b28, int * 0x0012da68) line 3880 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x02df4b28, int 
* 0x0012da68, int 1) line 3144 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2951 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x02e7cb2c, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1756 + 15 bytes
nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Incremental, nsIFrame * 0x02e7cb2c, const nsRect & {...}, int 1, 
int 0, int 1, nsMargin & {...}, unsigned int & 0) line 519 + 36 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x02e7cb2c, const nsRect & {...}, 
int 1, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 344 + 50 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x02e7cb78, int * 0x0012e5c0) line 3880 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x02e7cb78, int 
* 0x0012e5c0, int 1) line 3144 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2951 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x02e7cae0, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 1756 + 15 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x02e7cae0, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 693 + 31 bytes
CanvasFrame::Reflow(CanvasFrame * const 0x02e7bc6c, nsIPresContext * 0x03b63f00, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 306
nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0, int 0, int 0, int 10800, int 10215, int 1) line 813
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x02e7ca78, 
nsBoxLayoutState & {...}) line 484 + 52 bytes
nsBox::Layout(nsBox * const 0x02e7ca78, nsBoxLayoutState & {...}) line 1002
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x02e7bd84, nsBoxLayoutState 
& {...}) line 377
nsBox::Layout(nsBox * const 0x02e7bd84, nsBoxLayoutState & {...}) line 1002
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x02e7bd84, 
const nsRect & {...}) line 593 + 16 bytes
nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x02e7bd84, 
const nsRect & {...}) line 1003 + 17 bytes
nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1086
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x02e7bce0, nsBoxLayoutState 
& {...}) line 1011 + 15 bytes
nsBox::Layout(nsBox * const 0x02e7bce0, nsBoxLayoutState & {...}) line 1002
nsBoxFrame::Reflow(nsBoxFrame * const 0x02e7bca8, nsIPresContext * 0x03b63f00, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 728
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x02e7bca8, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 719 + 25 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x02e7bca8, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, 
int 0, unsigned int 0, unsigned int & 0) line 693 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x02e7bc30, nsIPresContext * 
0x03b63f00, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 546
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x03c17330, 
nsIPresContext * 0x03b63f00, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 145
PresShell::ProcessReflowCommands(int 0) line 4209
PresShell::FlushPendingNotifications(PresShell * const 0x03ba61b0) line 3308
PresShell::DidCauseReflow() line 4159
PresShell::ContentAppended(PresShell * const 0x03ba61b8, nsIDocument * 
0x0445b8f0, nsIContent * 0x03ba2888, int 0) line 3411
nsDocument::ContentAppended(nsDocument * const 0x0445b8f0, nsIContent * 
0x03ba2888, int 0) line 1805


Giving bug to waterson and ccing buster and nisheeth.

Comment 12

[Andreas Franke (gone)]

Setting OS to All. I can reproduce this crash (abort) with a PC/Linux debug
build from 7/21:

Steps to reproduce:
1.) Load the original URL or the first attachment
2 [details] [diff] [review].) Resize the browser window to make it larger

Alternative steps to reproduce:
1.) Load the second attachment
2 [details] [diff] [review].) Resize the browser window to make it very small
3.) Resize the browser window to make it very large

The last shell output before the "###!!! Abort:..." is:
> Block(li)(1)@0x4250acf8: yikes! spinning on a line over 1000 times!

Comment 13

[Chris Waterson]

crasher

Comment 14

[Chris Waterson]

Attachment: even simpler test case

Comment 15

[Chris Waterson]

The simplified test case illustrates that this bug is a problem with floaters; 
adding grafitti to indicate this. (To repro with that test case, use 
viewer, or shrink your browser window down to l.t. 620px width.)

N.B. that if we reverse the order of the <div>'s (so that the 300px div comes 
first), we'll just flat out ignore the 600px float with respect to available 
space computation.

Also, dbaron notes my inline CSS is invalid. Need to add 'px' if its to be 
accepted in other than quirks mode.

Comment 16

[Chris Waterson]

Attachment: fix

Comment 17

[Chris Waterson]

I am too stupid at this hour to understand what troy was thinking when he put
the "aBand->mLeft < rightEdge" test in here. I can't see how this would've ever
worked, so I took it out, and it seems to fix the bug.

Comment 18

[buster]

The code in question is:
  while ((aBand->mTop == topOfBand) && (aBand->mLeft < rightEdge)) {
It looks to me that this is saying, "do the loop for all rects whose top edge is 
the top of the original rect (topOfBand) AND whose left edge is inside the 
requested rect", or graphically:
           .
+---+-+----.-+-+-------+   <-- topOfBand
| 1 | |    . | |       |
+---| | 2  . | |   3   |
    | |    . | +-------+
    + +----.-+
           .
           .  <-- rightEdge

In this example, rects 1 and 2 would be considered, because they share the 
topEdge and their left boundary is to the left of rightEdge.  Rect3 is not 
considered because it's beyond the bounds of the requested area.  The dead 
spaces between the numbered rects are meant to represent available space.

This seems reasonable.  Looking at the test case and the proposed patch, I can't 
figure out why your change makes it work.  I think this needs some more careful 
analysis.  It's a pretty important function, and I can't convince myself the fix 
won't introduce regressions in pages with interesting floaters.

Chris, let me know if you want me to work with you on this, or if you're just 
going to keep going on it.

Comment 19

[Chris Waterson]

Thanks for the explanation. I'll look at the code some more, maybe we can talk
later today.

Comment 20

[Chris Waterson]

Attachment: better fix

Comment 21

[Chris Waterson]

I think this is the right fix. The problem was that the block band data be
advanced to a new line that had no available space, according to the space
manager, but would fail to clear the mLeftFloaters and mRightFloaters variables.
This would cause the frame think that it was "impacted by floaters", so we'd
miss the special case in CanPlaceFrame() when it's "not safe to break". This
would cause us to continually try to re-break the frame over and over again.

buster: whatdya think?

Comment 22

[buster]

too sleepy to grok this tonight.  I'll look into it tomorrow am.

Comment 23

[buster]

r=buster
I haven't actually tested it, but reading the code it looks like the right thing 
to do.  Good work.

Comment 24

[Chris Waterson]

checked in.

Comment 25

[Chris Petersen]

Marking verified in the the Aug 30th build.

Comment 26

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Comment 27

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/5a6def05ccbc