Last Comment Bug 348887 - Crash [@ nsFrameList::DestroyFrames] on reload with -moz-column-count, -moz-inline-block and blockquote::first-letter
: Crash [@ nsFrameList::DestroyFrames] on reload with -moz-column-count, -moz-i...
Status: VERIFIED FIXED
[sg:critical?]
: crash, regression, testcase, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 350267
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-16 12:00 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
9 users (show)
dveditz: blocking1.7.14-
dveditz: blocking‑aviary1.0.9-
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes on reload) (463 bytes, text/html)
2006-08-16 12:01 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
inbetween testcase that freezes and eats memory (2.02 KB, text/html)
2006-08-16 12:02 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
original testcase (21.34 KB, text/html)
2006-08-16 12:02 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch A (1.86 KB, patch)
2006-08-16 17:56 PDT, Mats Palmgren (:mats)
roc: review+
roc: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
Patch B (1.68 KB, patch)
2006-08-21 20:51 PDT, Mats Palmgren (:mats)
roc: review+
roc: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-16 12:00:20 PDT
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 [:mwargers] (not working for Mozilla) 2006-08-16 12:01:31 PDT
Created attachment 234041 [details]
testcase (crashes on reload)
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-16 12:02:15 PDT
Created attachment 234042 [details]
inbetween testcase that freezes and eats memory
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-16 12:02:50 PDT
Created attachment 234043 [details]
original testcase
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-16 12:18:28 PDT
Doesn't crash with a 2004-05-26 build, crashes directly with a 2004-10-10 build.
Comment 5 Mats Palmgren (:mats) 2006-08-16 17:54:25 PDT
###!!! 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 (:mats) 2006-08-16 17:56:03 PDT
Created attachment 234124 [details] [diff] [review]
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 [:bz] 2006-08-16 22:08:56 PDT
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 2006-08-17 12:12:55 PDT
Putting on the 1.8.1/FF2 radar...
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-08-17 15:23:01 PDT
Comment on attachment 234124 [details] [diff] [review]
Patch A

I like
Comment 10 Daniel Veditz [:dveditz] 2006-08-18 11:13:09 PDT
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 (:mats) 2006-08-21 20:42:57 PDT
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 (:mats) 2006-08-21 20:51:08 PDT
Created attachment 234906 [details] [diff] [review]
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) 2006-08-23 02:36:05 PDT
Comment on attachment 234906 [details] [diff] [review]
Patch B

Agree
Comment 14 Mats Palmgren (:mats) 2006-08-24 21:56:53 PDT
"Patch B" checked in to trunk at 2006-08-24 21:17 PDT.
Comment 15 Mike Schroepfer 2006-08-25 10:50:48 PDT
Comment on attachment 234124 [details] [diff] [review]
Patch A

a=schrep for drivers
Comment 16 Daniel Veditz [:dveditz] 2006-08-25 11:25:39 PDT
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 (:mats) 2006-08-25 17:21:43 PDT
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 (:mats) 2006-08-25 17:22:49 PDT
(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) 2006-08-25 21:40:56 PDT
I think yes.
Comment 20 Mats Palmgren (:mats) 2006-08-25 22:16:35 PDT
(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] 2006-08-27 22:59:00 PDT
Comment on attachment 234906 [details] [diff] [review]
Patch B

a=dveditz for 1.8.0 branch.
Comment 22 Mats Palmgren (:mats) 2006-08-28 02:38:29 PDT
Patch B landed on MOZILLA_1_8_0_BRANCH at 2006-08-28 00:49 PDT.
Comment 23 Tracy Walker [:tracy] 2006-08-29 11:31:29 PDT
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 (:mats) 2006-09-09 12:40:41 PDT
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 2006-09-11 17:07:11 PDT
Comment on attachment 234906 [details] [diff] [review]
Patch B

a=schrep for drivers.
Comment 26 Mats Palmgren (:mats) 2006-09-11 19:00:31 PDT
Patch B checked in to MOZILLA_1_8_BRANCH at 2006-09-11 17:46 PDT.
Comment 27 Bob Clary [:bc:] 2009-04-24 11:00:27 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/fd6a6e73be23

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