348887 - Crash [@ nsFrameList::DestroyFrames] on reload with -moz-column-count, -moz-inline-block and blockquote::first-letter

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes Mozilla on reload.
It crashes with current trunk builds, with Firefox1.5.0.6, but not with Mozilla1.7.13.


Talkback ID: TB22162189Z
nsFrameList::DestroyFrames   nsLineBox::DeleteLineList   nsFrameList::DestroyFrames   nsLineBox::DeleteLineList   nsLineBox::DeleteLineList   nsFrameList::DestroyFrames   CanvasFrame::Destroy   nsFrameList::DestroyFrames   nsFrameList::DestroyFrames   nsFrameManager::Destroy   DocumentViewerImpl::Destroy   DocumentViewerImpl::Show   nsPresContext::EnsureVisible   PresShell::UnsuppressAndInvalidate   PresShell::UnsuppressPainting

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase (crashes on reload)

Comment 2

[Martijn Wargers (dead)]

Attachment: inbetween testcase that freezes and eats memory

Comment 3

[Martijn Wargers (dead)]

Attachment: original testcase

Comment 4

[Martijn Wargers (dead)]

Doesn't crash with a 2004-05-26 build, crashes directly with a 2004-10-10 build.

Comment 5

[Mats Palmgren (inactive)]

###!!! ASSERTION: Out of flow frame doesn't have the expected parent: 'outOfFlowFrame->GetParent() == this', file nsBlockFrame.cpp, line 7159
###!!! ASSERTION: Parent not consistent with exepectations: 'aOldParent == aFrame->GetParent()', file nsBlockFrame.cpp, line 577
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file nsBlockReflowState.cpp, line 840
WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache: file nsBlockFrame.cpp, line 7207
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file nsBlockReflowState.cpp, line 840
WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache: file nsBlockFrame.cpp, line 7207
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file nsBlockReflowState.cpp, line 840
WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache: file nsBlockFrame.cpp, line 7207
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file nsBlockReflowState.cpp, line 840
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file nsBlockReflowState.cpp, line 840
WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache: file nsBlockFrame.cpp, line 7207
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file nsBlockReflowState.cpp, line 840
WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache: file nsBlockFrame.cpp, line 7207

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch A

<blockquote style="display: -moz-inline-block;">

GetStyleDisplay()->IsBlockLevel() is false.
IsFloatContainingBlock() is true (it's a nsBlockFrame).

This fixes it for me, although I'm a bit unsure what I have in my
tree at the moment ;-)  I'll do some more analysis tomorrow...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fwiw, I think this looks like the right change...  IsBlockLevel() is more about display-role at the moment (in CSS3 Box speak), whereas we care about display-model.

Comment 8

[Mike Schroepfer]

Putting on the 1.8.1/FF2 radar...

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234124 [details] [diff] [review]
Patch A

I like

Comment 10

[Daniel Veditz [:dveditz]]

If 1.8.1 gets this we want it in 1.8.0.7 also (unless it misses, in which case 1.8.0.8)

Comment 11

[Mats Palmgren (inactive)]

Patch A: checked in to trunk at 2006-08-21 20:04 PDT.
(will ask for branch approvals in a couple of days)

-> FIXED

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch B

I scanned layout/generic for similar code and I think I've found one more.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.788&root=/cvsroot&mark=4877#4854
What do you think?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234906 [details] [diff] [review]
Patch B

Agree

Comment 14

[Mats Palmgren (inactive)]

"Patch B" checked in to trunk at 2006-08-24 21:17 PDT.

Comment 15

[Mike Schroepfer]

Comment on attachment 234124 [details] [diff] [review]
Patch A

a=schrep for drivers

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 234124 [details] [diff] [review]
Patch A

approved for 1.8.0 branch, a=dveditz for drivers

What about Patch B for the 1.8/1.8.0 branch?

Comment 17

[Mats Palmgren (inactive)]

Patch A: checked in to MOZILLA_1_8_BRANCH 2006-08-25 14:47 PDT.
Patch A: checked in to MOZILLA_1_8_0_BRANCH at 2006-08-25 14:56 PDT.

On 1.8/1.8.0 branches I get (SeaMonkey debug Linux/gtk2):
 testcase:           no assertions, no crash
 inbetween testcase: several nasty assertions, but no crash
 original testcase:  several nasty assertions and crash (unrelated to this
                     code, I got the same crash before the change)

on trunk I get:
 testcase:           no assertions, no crash
 inbetween testcase: 1 assertion and 1 warning, but no crash
                     ASSERTION: bad float placement: 'NS_SUCCEEDED(rv)',
                      file nsBlockReflowState.cpp, line 1019
                     WARNING: aFrame is already associated with a region:
                      file nsSpaceManager.cpp, line 809
 original testcase:  no assertions, no crash

I tried "Patch B" on branches as well, with the same test results.

Comment 18

[Mats Palmgren (inactive)]

(In reply to comment #16)
> What about Patch B for the 1.8/1.8.0 branch?

I haven't seen any cases where this problem is triggered; I'm guessing the risk
is about the same as Patch A; I'm guessing it could prevent crashes in fuzz
tests similar to Patch A... so, I'm leaning towards yes...

roc, what do you think?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think yes.

Comment 20

[Mats Palmgren (inactive)]

(I filed bug 350267 to followup the branch crash in comment 17, the trunk
assertion+warning is covered by bug 321896 I think)

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 234906 [details] [diff] [review]
Patch B

a=dveditz for 1.8.0 branch.

Comment 22

[Mats Palmgren (inactive)]

Patch B landed on MOZILLA_1_8_0_BRANCH at 2006-08-28 00:49 PDT.

Comment 23

[Tracy Walker [:tracy]]

verified on bon echo build from 20060829 on Windows and Mac 1.5.0.7 build from 20060829

verified trunk per comment #17

note still seeing crash for bug 350267 with the original testcase

Comment 24

[Mats Palmgren (inactive)]

Although this bug is marked "verified1.8.1" there is still a second patch
that requests "approval1.8.1". It has already landed on trunk and 1.8.0.7.
Patch B is not needed to fix the crash in this bug, it's just to cover a
potential similar crash at a different place (see comment 18/19).
There are no known regressions, bug 350267 is just a followup on an unrelated
crash.

Comment 25

[Mike Schroepfer]

Comment on attachment 234906 [details] [diff] [review]
Patch B

a=schrep for drivers.

Comment 26

[Mats Palmgren (inactive)]

Patch B checked in to MOZILLA_1_8_BRANCH at 2006-09-11 17:46 PDT.

Comment 27

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/fd6a6e73be23