Created attachment 271309 [details]
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
This can lead to:
###!!! ASSERTION: used padding property missing (out of memory?): 'p', file mozilla/layout/generic/nsFrame.cpp, line 763
I think there's an sg:critical crash [@ nsBlockFrame::Destroy] lurking around here somewhere.
This leads to lots of other crashes and assertions, so it makes it hard to look for other bugs.
Created attachment 273481 [details] [diff] [review]
This patch is enough to fix the assertion in this testcase... does it fix the problem in general, Jesse?
Created attachment 273676 [details] [diff] [review]
Fixed version of patch
Fix the corresponding check in region destruction.
Created attachment 273677 [details]
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]
Created attachment 273693 [details]
testcase that still asserts with the patch in comment 5
I have no clue what to do about that testcase; it doesn't assert for me.
Created attachment 273697 [details]
testcase without -moz-appearance and list elements
(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.)
Created attachment 273713 [details] [diff] [review]
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.
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.)
Checked in. (Jesse, would you mind turning some of your asserting/crashing cases into reftests?)
Created attachment 273883 [details]
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.
Created attachment 273884 [details]
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).
Reftests checked in.
Comment on attachment 273713 [details] [diff] [review]
approved for 18.104.22.168, a=dveditz for release-drivers
Fix checked into the 1.8 branch