Hang with float and large padding and margin

VERIFIED FIXED

Status

()

Core
Layout: Floats
--
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: dholbert)

Tracking

({hang, regression, testcase})

Trunk
x86
All
hang, regression, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCo])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 265245 [details]
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.
(Reporter)

Comment 2

11 years ago
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.
(Reporter)

Comment 5

11 years ago
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.
(Reporter)

Comment 7

11 years ago
Vlad, you have an idea what's going on here?

Comment 8

11 years ago
Dupe of bug 381074?

Comment 9

11 years ago
(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!
Created attachment 287571 [details] [diff] [review]
hack-ish patch (shows the problem)

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.
Created attachment 287576 [details] [diff] [review]
patch v1
Attachment #287571 - Attachment is obsolete: true
Created attachment 287580 [details] [diff] [review]
patch v1a

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+
Created attachment 287610 [details] [diff] [review]
reftests (as patch)
Created attachment 287613 [details] [diff] [review]
reftests (as patch) v2

(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
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RwCo][needs landing] → [dbaron-1.9:RwCo]
(Reporter)

Comment 20

10 years ago
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.