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

RESOLVED FIXED

Status

()

Core
Layout: Floats
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: Eli Friedman)

Tracking

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

Trunk
x86
Mac OS X
assertion, fixed1.8.1.8, testcase
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

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

Comment 1

10 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

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

Comment 3

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

Updated

10 years ago
Assignee: nobody → sharparrow1
(Assignee)

Comment 4

10 years ago
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?
(Assignee)

Comment 5

10 years ago
Created attachment 273676 [details] [diff] [review]
Fixed version of patch

Fix the corresponding check in region destruction.
Attachment #273481 - Attachment is obsolete: true
(Reporter)

Comment 6

10 years ago
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]
(Reporter)

Comment 7

10 years ago
Created attachment 273693 [details]
testcase that still asserts with the patch in comment 5
(Assignee)

Comment 8

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

Comment 9

10 years ago
Created attachment 273697 [details]
testcase without -moz-appearance and list elements
Attachment #273693 - Attachment is obsolete: true
(Reporter)

Comment 10

10 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

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

Comment 12

10 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.)
Attachment #273713 - Flags: superreview+
Attachment #273713 - Flags: review?(roc)
Attachment #273713 - Flags: review+
(Assignee)

Comment 13

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

Comment 14

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

Comment 15

10 years ago
Created attachment 273884 [details]
387201-1-ref.html
(Reporter)

Comment 16

10 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?

Updated

10 years ago
Blocks: 389378
(Reporter)

Updated

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

Comment 17

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

Updated

10 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

Updated

10 years ago
Depends on: 400406
You need to log in before you can comment on or make changes to this bug.