Last Comment Bug 387201 - "ASSERTION: If we asked for force-fit, it should have been placed" with float, padding
: "ASSERTION: If we asked for force-fit, it should have been placed" with float...
Status: RESOLVED FIXED
: assertion, fixed1.8.1.8, testcase
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Eli Friedman
:
Mentors:
Depends on: 391412 400406
Blocks: randomclasses 389378
  Show dependency treegraph
 
Reported: 2007-07-06 18:24 PDT by Jesse Ruderman
Modified: 2007-10-19 10:42 PDT (History)
5 users (show)
dveditz: blocking1.8.1.8+
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (263 bytes, text/html)
2007-07-06 18:24 PDT, Jesse Ruderman
no flags Details
Possible patch (957 bytes, patch)
2007-07-23 15:48 PDT, Eli Friedman
no flags Details | Diff | Splinter Review
Fixed version of patch (1.68 KB, patch)
2007-07-24 15:51 PDT, Eli Friedman
no flags Details | Diff | Splinter Review
testcase that crashes with the patch in comment 4 (192 bytes, text/html)
2007-07-24 15:51 PDT, Jesse Ruderman
no flags Details
testcase that still asserts with the patch in comment 5 (337 bytes, text/html)
2007-07-24 17:23 PDT, Jesse Ruderman
no flags Details
testcase without -moz-appearance and list elements (314 bytes, text/html)
2007-07-24 18:04 PDT, Jesse Ruderman
no flags Details
Complete patch (2.50 KB, patch)
2007-07-24 20:11 PDT, Eli Friedman
roc: review+
roc: superreview+
dveditz: approval1.8.1.8+
Details | Diff | Splinter Review
387201-1.html (296 bytes, text/html)
2007-07-25 17:59 PDT, Jesse Ruderman
no flags Details
387201-1-ref.html (271 bytes, text/html)
2007-07-25 17:59 PDT, Jesse Ruderman
no flags Details

Description Jesse Ruderman 2007-07-06 18:24:14 PDT
Created attachment 271309 [details]
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 2007-07-06 18:41:43 PDT
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 2007-07-16 15:16:33 PDT
I think there's an sg:critical crash [@ nsBlockFrame::Destroy] lurking around here somewhere.
Comment 3 Jesse Ruderman 2007-07-22 14:09:47 PDT
This leads to lots of other crashes and assertions, so it makes it hard to look for other bugs.
Comment 4 Eli Friedman 2007-07-23 15:48:41 PDT
Created attachment 273481 [details] [diff] [review]
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 2007-07-24 15:51:08 PDT
Created attachment 273676 [details] [diff] [review]
Fixed version of patch

Fix the corresponding check in region destruction.
Comment 6 Jesse Ruderman 2007-07-24 15:51:12 PDT
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]
Comment 7 Jesse Ruderman 2007-07-24 17:23:32 PDT
Created attachment 273693 [details]
testcase that still asserts with the patch in comment 5
Comment 8 Eli Friedman 2007-07-24 17:28:56 PDT
I have no clue what to do about that testcase; it doesn't assert for me.
Comment 9 Jesse Ruderman 2007-07-24 18:04:19 PDT
Created attachment 273697 [details]
testcase without -moz-appearance and list elements
Comment 10 Jesse Ruderman 2007-07-24 18:08:33 PDT
(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 2007-07-24 20:11:28 PDT
Created attachment 273713 [details] [diff] [review]
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 2007-07-24 22:21:42 PDT
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 2007-07-25 14:53:17 PDT
Checked in.  (Jesse, would you mind turning some of your asserting/crashing cases into reftests?)
Comment 14 Jesse Ruderman 2007-07-25 17:59:02 PDT
Created attachment 273883 [details]
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 2007-07-25 17:59:41 PDT
Created attachment 273884 [details]
387201-1-ref.html
Comment 16 Jesse Ruderman 2007-07-25 18:12:01 PDT
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 2007-08-02 21:23:27 PDT
Reftests checked in.
Comment 18 Daniel Veditz [:dveditz] 2007-10-03 10:53:14 PDT
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] 2007-10-03 12:49:38 PDT
Fix checked into the 1.8 branch

Note You need to log in before you can comment on or make changes to this bug.