387201 - "ASSERTION: If we asked for force-fit, it should have been placed" with float, padding

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers:

###!!! ASSERTION: If we asked for force-fit, it should have been placed: 'placed || !forceFit', file mozilla/layout/generic/nsBlockReflowState.cpp, line 548

Comment 1

[Jesse Ruderman]

This can lead to:

###!!! ASSERTION: used padding property missing (out of memory?): 'p', file mozilla/layout/generic/nsFrame.cpp, line 763

Comment 2

[Jesse Ruderman]

I think there's an sg:critical crash [@ nsBlockFrame::Destroy] lurking around here somewhere.

Comment 3

[Jesse Ruderman]

This leads to lots of other crashes and assertions, so it makes it hard to look for other bugs.

Comment 4

[Eli Friedman]

Attachment: Possible patch

This patch is enough to fix the assertion in this testcase... does it fix the problem in general, Jesse?

Comment 5

[Eli Friedman]

Attachment: Fixed version of patch

Fix the corresponding check in region destruction.

Comment 6

[Jesse Ruderman]

Attachment: testcase that crashes with the patch in comment 4

###!!! ASSERTION: no bands: '!mBandList.IsEmpty()', file /Users/jruderman/trunk/mozilla/layout/generic/nsSpaceManager.cpp, line 934

+ crash [@ nsSpaceManager::BandRect::IsOccupiedBy]

Comment 7

[Jesse Ruderman]

Attachment: testcase that still asserts with the patch in comment 5

Comment 8

[Eli Friedman]

I have no clue what to do about that testcase; it doesn't assert for me.

Comment 9

[Jesse Ruderman]

Attachment: testcase without -moz-appearance and list elements

Comment 10

[Jesse Ruderman]

(I used DOM Inspector's "CSS Style Rules" mode to figure out what html.css rules were applying to OL and LI elements, allowing me to turn those into divs, and I used its "Computed Styles" mode to figure out what -moz-appearance: checkbox was doing to the element's height and width.)

Comment 11

[Eli Friedman]

Attachment: Complete patch

I think this version covers everything... the IsEmpty changes aren't really necessary, but they seem correct anyway.

The change that fixes this basically forces the method to return some data, even for a zero-width space.

Comment 12

[Jesse Ruderman]

Sweet!  This patch fixes all 5 places where I was seeing the assertion, even though they each triggered different assertions or crashes after this one.  (The other assertions / crashes are gone now, too.)

Comment 13

[Eli Friedman]

Checked in.  (Jesse, would you mind turning some of your asserting/crashing cases into reftests?)

Comment 14

[Jesse Ruderman]

Attachment: 387201-1.html

Testcase (for reftest) based on the one in comment 0.  In an unpatched build, it triggers the assertion and incorrectly appears blank.

The corresponding reference (which I'll attach next) looks the same in unpatched and patched builds, and is not blank.

The difference between the testcase and reference is the presence of a zero-width, floating div.  Since there are no elements with "clear" nearby, Eli says it's expected that the presence of a zero-width, floating div won't affect layout.

Comment 15

[Jesse Ruderman]

Attachment: 387201-1-ref.html

Comment 16

[Jesse Ruderman]

In addition to that pair, I'll check in two tests that showed bugs in the partial patches on this bug.  Those will be "equals blank" reftests, really testing for assertions/crashes rather than testing the rendering.

* "Testcase that crashes with the patch in comment 4" (comment 6).

* "Testcase without -moz-appearance and list elements" (comment 9).

Comment 17

[Jesse Ruderman]

Reftests checked in.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 273713 [details] [diff] [review]
Complete patch

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 19

[Daniel Veditz [:dveditz]]

Fix checked into the 1.8 branch