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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: jruderman, Assigned: sharparrow1)

Tracking

(Blocks 1 bug, {assertion, fixed1.8.1.8, testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
Posted file 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
(Reporter)

Comment 1

12 years ago
This can lead to:

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

Comment 2

12 years ago
I think there's an sg:critical crash [@ nsBlockFrame::Destroy] lurking around here somewhere.
Flags: blocking1.9?
(Reporter)

Comment 3

12 years ago
This leads to lots of other crashes and assertions, so it makes it hard to look for other bugs.
(Assignee)

Updated

12 years ago
Assignee: nobody → sharparrow1
(Assignee)

Comment 4

12 years ago
Posted patch Possible patch (obsolete) — Splinter Review
This patch is enough to fix the assertion in this testcase... does it fix the problem in general, Jesse?
(Assignee)

Comment 5

12 years ago
Posted patch Fixed version of patch (obsolete) — Splinter Review
Fix the corresponding check in region destruction.
Attachment #273481 - Attachment is obsolete: true
(Reporter)

Comment 6

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

+ crash [@ nsSpaceManager::BandRect::IsOccupiedBy]
(Reporter)

Comment 7

12 years ago
(Assignee)

Comment 8

12 years ago
I have no clue what to do about that testcase; it doesn't assert for me.
(Reporter)

Comment 9

12 years ago
Attachment #273693 - Attachment is obsolete: true
(Reporter)

Comment 10

12 years ago
(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.)
(Assignee)

Comment 11

12 years ago
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.
Attachment #273676 - Attachment is obsolete: true
Attachment #273713 - Flags: review?(roc)
(Reporter)

Comment 12

12 years ago
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.)
(Assignee)

Comment 13

12 years ago
Checked in.  (Jesse, would you mind turning some of your asserting/crashing cases into reftests?)
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Reporter)

Comment 14

12 years ago
Posted file 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.
(Reporter)

Comment 15

12 years ago
Posted file 387201-1-ref.html
(Reporter)

Comment 16

12 years ago
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).
Flags: blocking1.9?
Blocks: 389378
(Reporter)

Updated

12 years ago
Flags: blocking1.8.1.7?
(Reporter)

Comment 17

12 years ago
Reftests checked in.
Flags: in-testsuite? → in-testsuite+
(Assignee)

Updated

12 years ago
Depends on: 391412
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Comment on attachment 273713 [details] [diff] [review]
Complete patch

approved for 1.8.1.8, a=dveditz for release-drivers
Attachment #273713 - Flags: approval1.8.1.8+
Fix checked into the 1.8 branch
Keywords: fixed1.8.1.8
Depends on: 400406
You need to log in before you can comment on or make changes to this bug.