533379 - "ASSERTION: overflow list w/o frames" or "ABORT: running past end" with -moz-column, list-style-position

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

First testcase:

###!!! ASSERTION: overflow list w/o frames: 'mFrames.NotEmpty()', file /Users/jruderman/mozilla-central/layout/generic/nsInlineFrame.cpp, line 375

Second testcase:

###!!! ABORT: running past end: 'mCurrent != mListLink', file /Users/jruderman/mozilla-central/layout/generic/nsLineBox.h, line 637

Comment 1

[Jesse Ruderman]

Attachment: testcase 1

Comment 2

[Jesse Ruderman]

Attachment: testcase 2 (aborts debug builds when loaded)

Comment 3

[Mats Palmgren (inactive)]

I have a patch for this one...

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Looks like a regression from bug 512471.  I suspect that the if-condition
really meant to test 'aFromLine == ...' rather than testing nsLineBox
pointers?  It looks simpler to just test the parameter that says if it's
an overflow line or not.  Changing the if-condition fixes the ABORT_IF_FALSE
crash in debug builds, then testcase 2 just asserts as testcase 1.

The assert is caused by nsInlineFrame::PullOneFrame removing the last
frame on the principal child list, while still having an overflow frame.
Not sure if we really should have overflow at that point, but moving the
overflow frames to mFrames when it becomes empty fixes the assert and pass
TryServer unit tests.

(It appears there's still a rendering error for testcase 2 - the 'A' is black
when it should be red according to the first-letter style.  I think it's
an unrelated bug.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 416806 [details] [diff] [review]
Patch rev. 1

> I suspect that the if-condition really meant to test 'aFromLine == ...' rather
> than testing nsLineBox pointers? 

aFromLine == fromLine, no?

Point taken about not doing begin() on an empty line list, though.  ;)

Comment 6

[Mats Palmgren (inactive)]

> aFromLine == fromLine, no?

No, that would always be true since fromLine is initialized from aFromLine
(implicitly using "operator pointer()").

> Point taken about not doing begin() on an empty line list, though.  ;)

Creating the iterator isn't the problem, but there's an implicit conversion
to nsLineBox* through "operator pointer()" to be able to compare it with
fromLine and inside that operator there's a NS_ABORT_IF_FALSE which
aborts DEBUG builds if the list is empty.  In an opt build we compare
fromLine to a bogus pointer which actually does the right thing -
always executing the 'else' block. So this bug isn't a security problem.

Comment 7

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ac78d85dd932
http://hg.mozilla.org/mozilla-central/rev/ec1f0c9136d0
http://hg.mozilla.org/mozilla-central/rev/bf4186834437

Comment 8

[Boris Zbarsky [:bzbarsky]]

> No, that would always be true

Right, so |fromLine == aFromContainer->mLines.begin()| and the |aFromLine == aFromContainer->mLines.begin()| you were suggesting are completely equivalent.  Or did I misunderstand what you were suggesting in comment 4?

Comment 9

[Mats Palmgren (inactive)]

They invoke different operator methods... but yeah, I see now that
nsLineList_iterator::operator==" has its own a NS_ABORT_IF_FALSE(...
"comparing iterators over different lists") which would trigger in this case.

Comment 10

[Carsten Book [:Tomcat]]

the assertion ###!!! ASSERTION: overflow list w/o frames: 'mFrames.NotEmpty()', file /work/mozilla/builds/1.9.2/mozilla/layout/generic/nsInlineFrame.cpp, line 374
is also triggered by the testcase from bug 542136 , so shouldn't this patch also not land on 1.9.2 ?

Comment 11

[Timothy Nikkel (:tnikkel)]

If we take this we would also probably want bug 537645.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Bug 537645 is pretty huge in terms of patch size. Do we think that this is something that we need to take on 1.9.2 for security reasons (there's no sg: rating), or just assertion cleanup.

Comment 13

[Timothy Nikkel (:tnikkel)]

Are you looking at the right bug? Bug 537645 has a 1.42kb patch and an sg: rating.

Comment 14

[Daniel Veditz [:dveditz]]

Does this affect 1.9.1, too?

Comment 15

[Mats Palmgren (inactive)]

> Does this affect 1.9.1, too?

No.  The crash in this bug is a regression from bug 512471 and does not
affect 1.9.2 or 1.9.1.  The nsBlockFrame change fixed this part.

The nsInlineFrame change fixed "ASSERTION: overflow list w/o frames"
which do occur on 1.9.2 and 1.9.1, but the code deals with it:
http://mxr.mozilla.org/mozilla1.9.2/source/layout/generic/nsInlineFrame.cpp#371
http://mxr.mozilla.org/mozilla1.9.1/source/layout/generic/nsInlineFrame.cpp#354
so it's harmless and doesn't seem worth fixing on branches.