[columns] Crash [@ nsLineBox::DeleteLineList] with moz-column-count and generated content

RESOLVED FIXED

Status

()

Core
Layout
--
critical
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: mats)

Tracking

(5 keywords)

Trunk
x86
All
crash, regression, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] uses freed memory, crash signature)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
See upcoming testcase, which crashes current trunk build and Firefox1.5.05.
Talkback ID: TB20910283Z
0x0000012c
nsLineBox::DeleteLineList   nsBlockFrame::DoRemoveFrame   nsBlockFrame::RemoveFrame   nsFrameManager::RemoveFrame   nsCachedStyleData::Destroy  

I'm afraid it window size dependant, so the testcase contains a window.resizeTo(1440, 600); function, your browser should allow the resize.
(Reporter)

Comment 1

11 years ago
Created attachment 229114 [details]
testcase
(Reporter)

Comment 2

11 years ago
Created attachment 229116 [details]
Original testcase
(Assignee)

Comment 3

11 years ago
Reproducable on Linux, trunk and 1.8 branch debug builds.
Looks like a bad overflow line list in the crashes I have.

(gdb) bt
#0  0xdddddddd in ?? ()
#1  0x410b2708 in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&) (aPresContext=0x8925070, aLines=@0x8783680) at nsLineBox.cpp:369
#2  0x41060265 in nsBlockFrame::Destroy() (this=0x89dfec0) at nsBlockFrame.cpp:305
#3  0x4106b97a in nsBlockFrame::DoRemoveFrame(nsIFrame*, int, int) (this=0x8809ec8, aDeletedFrame=0x89dfec0, aDestroyFrames=1,
    aRemoveOnlyFluidContinuations=0) at nsBlockFrame.cpp:5889
#4  0x4106b1ef in nsBlockFrame::RemoveFrame(nsIAtom*, nsIFrame*) (this=0x8809ec8, aListName=0x0, aOldFrame=0x89dfec0) at nsBlockFrame.cpp:5671
#5  0x41030e33 in nsFrameManager::RemoveFrame(nsIFrame*, nsIAtom*, nsIFrame*) (this=0x88dfecc, aParentFrame=0x89a17f8, aListName=0x0, aOldFrame=0x89dfec0)
    at nsFrameManager.cpp:720
#6  0x40ff66a3 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, int, int) (this=0x88e0118, aContainer=0x88ed920, aChild=0x8937008,
    aIndexInContainer=142646984, aInReinsertContent=1) at nsCSSFrameConstructor.cpp:10081
#7  0x40ff5c6b in nsCSSFrameConstructor::ReinsertContent(nsIContent*, nsIContent*) (this=0x88e0118, aContainer=0x88ed920, aChild=0x8937008)
    at nsCSSFrameConstructor.cpp:9694
#8  0x40ffb620 in nsCSSFrameConstructor::WipeContainingBlock(nsFrameConstructorState&, nsIFrame*, nsIFrame*, nsIFrame*) (this=0x88e0118, aState=@0xbfffe2a0,

(gdb) fr 2
#2  0x41451932 in nsBlockFrame::Destroy(nsPresContext*) (this=0x89b2298, aPresContext=0x8760770) at nsBlockFrame.cpp:308
308         nsLineBox::DeleteLineList(aPresContext, *overflowLines);
(gdb) p overflowLines
$1 = (nsLineList *) 0x89b300c
(gdb) p *overflowLines
$2 = {mLink = {_mNext = 0x89ab47c, _mPrev = 0xdddddddd}}
(gdb)
OS: Windows XP → All
(Assignee)

Comment 4

11 years ago
I have a fix, I think...
Assignee: nobody → mats.palmgren
(Assignee)

Comment 5

11 years ago
We crash with the same stack also on 1.8.0 branch.
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
(Assignee)

Comment 6

11 years ago
Created attachment 229427 [details]
Frame dump and stack

We have a frame in |mLines| with a sibling in the overflow lines
and when we try to destroy the overflow lines we will access the
already destroyed sibling frame:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.783&root=/cvsroot&mark=301,306#286

which was destroyed when traversing the sibling chain here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineBox.cpp&rev=1.109&root=/cvsroot&mark=367-371#360
(Assignee)

Comment 7

11 years ago
Created attachment 229428 [details] [diff] [review]
Patch rev. 1
Attachment #229428 - Flags: superreview?(roc)
Attachment #229428 - Flags: review?(roc)
Flags: blocking1.8.0.5? → blocking1.8.0.6?
(In reply to comment #6)
> which was destroyed when traversing the sibling chain here:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineBox.cpp&rev=1.109&root=/cvsroot&mark=367-371#360

...which is the wrong way to delete the frames in a line; you need to check the child count to do that.  What is DeleteLineList supposed to be doing?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Whiteboard: [sg:critical?] uses freed memory
It's deleting all the frames and lines in the line list. The frame loop is OK because we're doing all lines, not just one line.
> We have a frame in |mLines| with a sibling in the overflow lines

This should not be happening. This is the underlying bug.
(Assignee)

Updated

11 years ago
Attachment #229428 - Attachment is obsolete: true
Attachment #229428 - Flags: superreview?(roc)
Attachment #229428 - Flags: review?(roc)
(Assignee)

Comment 11

11 years ago
The regression window is: 2006-01-20-16  --  2006-01-21-05
More specifically, it's the the nsBlockFrame patch from bug 310638,
I manually reverted that change and it fixed the testcase in this bug,
I'm pretty sure it brings back the old crash though...
Blocks: 310638
Keywords: regression
(Assignee)

Comment 12

11 years ago
Created attachment 230339 [details]
Testcase #3 (nsBlockFrame crash that bug 310638 fixed)
(Assignee)

Updated

11 years ago
Attachment #230339 - Attachment description: Testcase #3 (nsBolckFrame crash that bug 310638 fixed) → Testcase #3 (nsBlockFrame crash that bug 310638 fixed)
(Assignee)

Comment 13

11 years ago
Created attachment 230346 [details]
Frame dump #2

Ok, I've found the real bug. Here's a frame tree to illustrate.
In the first round we're removing the blue frame, which have a
next-in-flow in the overflow line list (this is the case that
bug 310638 fixed). In this particular case though, we have a
next sibling as well (red). This is what trips us later because
we will pick up that sibling and set it as next sibling on our
prev sibling, on line 5863:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.784&root=/cvsroot&mark=5863,5959-5969#5859
which happens to be on the main line list, which will crash
when this frame tree is destroyed later.

When we switch from the main line list to the overflow line list
(on line 5969) we need to null out |prevSibling|...
(Assignee)

Comment 14

11 years ago
Created attachment 230404 [details] [diff] [review]
Patch rev. 2

This patch fixes the crash. "Original testcase" still crash but that
turned out to be bug 334602. After applying Boris' patch for that the
testcase does not crash. There are still a few nasty assertions triggered
by it though but I think we can handle them in separate bugs:

###!!! ASSERTION: overflow list w/o frames: 'mFrames.NotEmpty()',
      file nsInlineFrame.cpp, line 362
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)',
      file nsBlockFrame.cpp, line 916
###!!! ASSERTION: next in flow should have been deleted: '!kidNextInFlow',
      file nsColumnSetFrame.cpp, line 503
WARNING: Content has no document.: 
      file nsTextFrame.cpp, line 5948
WARNING: Reflow of frame failed in nsLineLayout:
      file nsLineLayout.cpp, line 997
Attachment #230404 - Flags: superreview?(roc)
Attachment #230404 - Flags: review?(roc)
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Whiteboard: [sg:critical?] uses freed memory → [sg:critical?] [needs review roc] uses freed memory
Attachment #230404 - Flags: superreview?(roc)
Attachment #230404 - Flags: superreview+
Attachment #230404 - Flags: review?(roc)
Attachment #230404 - Flags: review+
(Assignee)

Comment 15

11 years ago
I filed bug 346405 for the remaining assertions/crash on "Original testcase".

Checked in to trunk at 2006-07-29 02:38 PDT.
(I will ask for 1.8 and 1.8.0 branch approval in a couple of days...)

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.9a1?
Resolution: --- → FIXED
Whiteboard: [sg:critical?] [needs review roc] uses freed memory → [sg:critical?] uses freed memory
Are you going to go for 1.8.1 approval here?
(Assignee)

Updated

11 years ago
Attachment #230404 - Flags: approval1.8.1?
Attachment #230404 - Flags: approval1.8.0.7?

Comment 17

11 years ago
Comment on attachment 230404 [details] [diff] [review]
Patch rev. 2

a=schrep for drivers.  Thanks guys - Land away!
Attachment #230404 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 18

11 years ago
Checked in to MOZILLA_1_8_BRANCH at 2006-08-05 21:18 PDT.
Keywords: fixed1.8.1
Comment on attachment 230404 [details] [diff] [review]
Patch rev. 2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #230404 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 20

11 years ago
Checked in to MOZILLA_1_8_0_BRANCH at 2006-08-10 02:57 PDT.
Keywords: fixed1.8.0.7

Comment 21

11 years ago
https://bugzilla.mozilla.org/attachment.cgi?id=229114
ff2b2 debug/nightly windows/linux no crash
verified fixed 1.8
Keywords: fixed1.8.1 → verified1.8.1

Comment 22

11 years ago
v.fixed on both branches with 8/24 nightly builds, no crash with either testcase:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060824 Firefox/1.5.0.7pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2
Keywords: fixed1.8.0.7 → verified1.8.0.7
Group: security
Flags: in-testsuite?
Crash Signature: [@ nsLineBox::DeleteLineList]
(Assignee)

Comment 23

4 years ago
Added crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/308b0c3494ef
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/308b0c3494ef
You need to log in before you can comment on or make changes to this bug.