Closed Bug 381152 Opened 17 years ago Closed 17 years ago

Hang with float and large padding and margin

Categories

(Core :: Layout: Floats, defect)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: dholbert)

References

Details

(Keywords: hang, regression, testcase, Whiteboard: [dbaron-1.9:RwCo])

Attachments

(2 files, 4 obsolete files)

Attached file testcase
See testcase, which hangs current trunk builds when loading.
It doesn't hang on branch builds.
This regressed between 2006-02-22 and 2006-02-25, this is the period where cairo was turned on on windows, so I guess that might somehow be the cause.
There doesn't seem to be any layout specific check-ins that could have caused this.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070517 Minefield/3.0a5pre
It doesn't hang here.
Strange... doesn't it hang even a little bit for you?
No, it just loads a blank page. ADM Athlon XP 2000+ Processor.
On another computer it hangs indeed. Not responsive in any case while processor usage is 50-60%. I see no blank page. Processor is Intel Duo Core.
Maybe it depends on the video card? I'm using a NVidia Geforce Go7300 (with a Pentium Centrino 1.80GHz).
Yeah, maybe. The computer where Firefox gets unresponsive has also that NVidea Geforce Go 7300. The laptop without problems has an ATI Radeon.
Vlad, you have an idea what's going on here?
Dupe of bug 381074?
(In reply to comment #8)
> Dupe of bug 381074?
> 

I think so... it's hanging in the same place.
Depends on: 381074
Not a dupe of bug 381074.  That bug's now patched, and the patch doesn't fix this one.
Occurs on Linux, too.
OS-->All.
OS: Windows XP → All
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RwCo]
I get these warnings just before the hang:

WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/xpcom/nsUnitConversion.h, line 100
WARNING: Overflowed nscoord_MAX in conversion to nscoord: file ../../dist/include/xpcom/nsUnitConversion.h, line 100
Area(div)(1)@0x819a2b8: UpdateBand: bad caller: height WAS 473741888(0x1c3cba40)
nsBlockReflowContext: Area(div)(1)@0x819a2b8 metrics=0,1547483712!
This hack-ish patch shows (part of) the problem here.

The problem starts with the "floatMargin.TopBottom()" call here in nsBlockReflowState.cpp:

 762   nsSize floatSize = floatFrame->GetSize() +
 763                      nsSize(floatMargin.LeftRight(), floatMargin.TopBottom());

Here, floatMargin is a nsMargin with 
  {top = 1073741824, right = 0, bottom = -599999936, left = 0}

and the nsMargin::TopBottom call just returns top + bottom.

So we end up doing arithmetic near nscoord_MAX, which is dangerous.  This gets us into more trouble when we add the outer float's nscoord_MAX padding later on.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #287571 - Attachment is obsolete: true
Attached patch patch v1aSplinter Review
The idea of this patch is to just fix the "gets us into trouble" part of comment #13:

> So we end up doing arithmetic near nscoord_MAX, which is dangerous.  This gets
> us into more trouble when we add the outer float's nscoord_MAX padding later
> on.

This patch restricts nsSpaceManager to make its bands have, at most, nscoord_MAX height. This fixes the cases (as in this bug's testcase) where
  rect.y ~= nscoord_MAX
  rect.height ~= nscoord_MAX
 which means
  rect.YMost() = 2 * nscoord_MAX

Once we start working with signed-integer values close to 2 * nscoord_MAX, we run the risk of overflowing into negative numbers, which is probably why we end up hanging.

BTW, these nscoord_MAX values come from the enormous padding and margins  -- they're not "unconstrained_height" sentinel values.

Note 1: This patch is based on the last section of the draft patch for Bug 393656.

Note 2: I removed the first part of "patch v1" to create "patch v1a" because that first part wasn't necessary to fix this bug's testcases. We may still want to include that bit as well, though.
Assignee: nobody → dholbert
Attachment #287576 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #287580 - Flags: review?(roc)
Comment on attachment 287580 [details] [diff] [review]
patch v1a

This looks fine. I'm a bit worried that trying to fix all possible integer overflows this way is going to add tons of unreadable code and take forever, though.
Attachment #287580 - Flags: superreview+
Attachment #287580 - Flags: review?(roc)
Attachment #287580 - Flags: review+
Attached patch reftests (as patch) (obsolete) — Splinter Review
Attached patch reftests (as patch) v2 (obsolete) — Splinter Review
(added line for reftest.list)
Whiteboard: [dbaron-1.9:RwCo] → [dbaron-1.9:RwCo][needs landing]
Attachment #287610 - Attachment is obsolete: true
patch v1a landed

/cvsroot/mozilla/layout/generic/nsSpaceManager.cpp,v  <--  nsSpaceManager.cpp
new revision: 3.88; previous revision: 3.87
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RwCo][needs landing] → [dbaron-1.9:RwCo]
Again thanks for fixing this bug!

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110705 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
I checked in this bug's original testcase as a crashtest:
http://hg.mozilla.org/mozilla-central/rev/e21014e86bad
Flags: in-testsuite? → in-testsuite+
Attachment #287613 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: