346405 - [columns] crash [@ nsColumnSetFrame::GetContentInsertionFrame] and [@ nsLineLayout::TrimTrailingWhiteSpaceIn]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

This is a followup to bug 344557 comment 14.
(Sensitive because I got a crash using freed memory)

In my tree I have bug 344557 fixed and also Boris' patch for bug 334602

STEPS TO REPRODUCE
1. load URL (which is the "Original testcase" from bug 344557).
2. repeatedly type CTRL++ CTRL+0 ... while it is loading

ACTUAL RESULTS
###!!! 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
###!!! ASSERTION: forget-word-frame: '(void*)aFrame == mWordFrames->PeekFront()', file nsLineLayout.cpp, line 3069

and then a couple of crashes:  [@ nsColumnSetFrame::GetContentInsertionFrame]
and [@ nsLineLayout::TrimTrailingWhiteSpaceIn]

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack, nsColumnSetFrame::GetContentInsertionFrame

Comment 2

[Mats Palmgren (inactive)]

Attachment: stack #2, nsLineLayout::TrimTrailingWhiteSpaceIn

Comment 3

[Martijn Wargers (dead)]

Attachment: fairly reduced testcase

Yeah, I already had a file at 'stock' with this type of crash. I wasn't sure it was the same crash as bug 346405 and while minimising, I think I was hitting that bug anyhow, so I wanted to wait before that bug gets fixed before I eventually would file a bug on this.

Comment 4

[Martijn Wargers (dead)]

Attachment: original testcase

Comment 5

[Mats Palmgren (inactive)]

The "fairly reduced testcase" also triggers bug 337412.
I'm fixing that bit over there...

Comment 6

[Mats Palmgren (inactive)]

Loading "fairly reduced testcase" also triggers this assertion:

###!!! ASSERTION: continuing frame had more severe impact than first-in-flow: '!frame->GetPrevContinuation()', file nsFrameManager.cpp, line 1374

Comment 7

[Daniel Veditz [:dveditz]]

Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.

Comment 8

[chris hofmann]

any ideas on a fix? does it look like this might be an easy one?

Comment 9

[Daniel Veditz [:dveditz]]

Robert: please fix this column-related security bug (or assign to one of your minions).

Comment 10

[Martijn Wargers (dead)]

Attachment: not reduced testcase2

The partly reduced and original testcase seem to be worksforme in the latest nightly trunk build.
However, this is an unreduced testcase that still crashes with the same stacktrace, I think.
Talkback ID: TB31479230E

Comment 11

[Jesse Ruderman]

I can reproduce with the newest attachment (and only the newest attachment) on Mac.

Martijn, do you want me to try to reduce it?

Comment 12

[Martijn Wargers (dead)]

(In reply to comment #11)
> Martijn, do you want me to try to reduce it?

Yes, sure, thanks.

Comment 13

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

Attachment: reduced testcase

I reduced this from 262587. It crashes every time on my Mac build.

Comment 14

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

Attachment: fix

This fixes the reduced testcase (#265625). We can call this function to destroy sibling chains, and sometimes we destroy an empty list.

This *doesn't* fix 262587 though, so I need to reduce that *again*. Could really use some testcase reduction love :-)

Comment 15

[Martijn Wargers (dead)]

Attachment: testcase3

This is a testcase that's reasonably minimised and that still crashes in my build that has roc's fix in.

Comment 16

[Jesse Ruderman]

roc says testcase3 (in comment 15) is likely to require an orthogonal fix.  dbaron, it would be great if you could review the patch in comment 14.

Comment 17

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

testcase3 doesn't crash for me :-(

Comment 18

[Martijn Wargers (dead)]

Attachment: testcase4

Yuck, I guess testcase3 is font dependent or something.
Does this testcase4 crash for you?

Comment 19

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

Nope.

Comment 20

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

I need review here ... the patch is small.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 265632 [details] [diff] [review]
fix

r+sr=dbaron.  Sorry for the delay.

Comment 22

[Daniel Veditz [:dveditz]]

I assume this is the right patch for the 1.8 branch too?

Comment 24

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

As I mentioned in comment #14, there is an underlying problem here that I haven't got a fix for. I'll have to minimize the testcase myself since Martijn's minimized testcases don't crash for me. The patch I have here fixes a real issue (stops the testcase in comment #13 from crashing).

Comment 25

[Daniel Veditz [:dveditz]]

That's the testcase it didn't stop crashing on the branch, although it was not a crash-on-load before or after the patch so could be a different crash entirely. In both cases I had to do a lot of quick font-size fiddling (many ctrl-+, occasional Ctrl-0). The crashes seemed to happen on the larger sizes.

In any case, this isn't done enough for the now-truncated 1.8.1.5 release

Comment 26

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

I checked in that patch.

Comment 27

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

All of these testcases no longer crash on trunk, for me. How about you, Martijn? They do take a long time to load and assert like crazy, mostly "ResolveBidi called on non-first continuation".

Comment 28

[Martijn Wargers (dead)]

I'm still crashing with https://bugzilla.mozilla.org/attachment.cgi?id=231217
which is "original testcase" on current trunk build.

http://crash-stats.mozilla.com/report/index/8b35811c-6699-11dc-a846-001a4bd43e5c
0  	nsFloatCacheList::Tail()  	 mozilla/layout/generic/nsLineBox.cpp:849
1 	nsFloatCacheFreeList::Append(nsFloatCacheList&) 	mozilla/layout/generic/nsLineBox.cpp:936
2 	nsLineBox::FreeFloats(nsFloatCacheFreeList&) 	mozilla/layout/generic/nsLineBox.cpp:481
3 	nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) 	mozilla/layout/generic/nsBlockFrame.cpp:3271
4 	nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, int*) 	mozilla/layout/generic/nsBlockFrame.cpp:3188
5 	nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, int*)
etc..

Also, a lot of the testcases are crashing on branch.

Not sure what to do, should I file a new bug for that crasher and morph this bug into a 1.8 branch bug?

Comment 29

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

Sounds good.

Comment 30

[Martijn Wargers (dead)]

Ok, I filed bug 396750 for the crash in comment 28.
This bug is now about the fact that the testcase(s) is (are) crashing on branch.

Comment 31

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

Clearing blocking 1.9 since this is now a branch bug.

Comment 32

[Daniel Veditz [:dveditz]]

Oops: didn't read comment 31, just saw the whiteboard "need trunk landing" and lack of a blocking flag and "fixed" it. Now I'll fix it the other way.

Comment 33

[Daniel Veditz [:dveditz]]

Roc: what's the status of this one for the branch? Looks like the 1.8.1.8 freeze on Oct 3 might not be realistic at this point.

Comment 34

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

I haven't looked at it at all.

Comment 35

[Daniel Veditz [:dveditz]]

Year old+ security hole, pushed off for another 8 wks. Yay us.

Comment 36

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

Current state of the branch:
"not reduced testcase2": we assert and crash in nsColumnSetFrame because it has no child
"reduced testcase", "testcase 3": we assert but don't crash "wrong float parent", not good but not immediately critical)
"testcase 4": no crash or assert

Comment 37

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

Attachment: wallpaper fix

This is a total wallpaper fix, but it's zero risk and fixes the crash and is good for branch IMHO, since trunk already has "the right fix" (or rather, combination of fixes).

Comment 38

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

We're crashing because nsBlockFrame::RenumberLists is calling nsColumnSetFrame::GetContentInsertionFrame during reflow, which can't cope with having no children. It should always have at least one child, of course, but temporarily it doesn't on branch.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But if this is actually called while mFirstChild is null, couldn't this lead to a mangled frame tree?  It seems like this could potentially replace a "safe" null-pointer dereference with something worse.  How do you know it won't?

Comment 40

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

The RenumberLists call site won't cause a problem because it QIs the result to nsBlockFrame.

if this worries you, I'll rewrite it to return null when there's no child, and make sure RenumberLists doesn't crash on a null result. OK?

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think that does sound safer, yes.

Comment 42

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

Attachment: better wallpaper

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 298420 [details] [diff] [review]
better wallpaper

ok, r+sr=dbaron for branch

Comment 44

[Daniel Veditz [:dveditz]]

Comment on attachment 298420 [details] [diff] [review]
better wallpaper

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 45

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

checked in

Comment 47

[Daniel Veditz [:dveditz]]

That's a popular place to crash, 19 other bugs:
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20summary%3A%22nsCachedStyleData::GetStyleData%22

Comment 48

[Al Billings [:abillings - ex-MoCo]]

This is marked as fixed1.8.1.12 but I cannot get any of the test cases to crash in 2.0.0.11 builds. Is there some trick here to doing so?

Comment 49

[Mats Palmgren (inactive)]

"not reduced testcase2" and "reduced testcase" crashes Firefox 2.0.0.11
on Linux if I resize the window.  "testcase3" and "testcase4" does
not crash for me, but comment 18 says they are font/platform(?) dependent.

Comment 50

[Martijn Wargers (dead)]

I can crash with a 2.0.0.11 build, when I load https://bugzilla.mozilla.org/attachment.cgi?id=265682 (testcase 3) and then zoom 3 times in and then 1 time out.
I don't crash with a 1.8 branch build of 2008-01-28, in that way.
I guess that is as good a verification as it gets.

Note that comment 46 still mentions that a crash on current branch is there, but I guess a new bug should be filed on that.

Comment 51

[Al Billings [:abillings - ex-MoCo]]

I finally got it to crash in 2.0.0.11. I'm not sure why it wasn't before. Using the same test case as Martijn.

In 2.0.0.12, it does not crash: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.12) Gecko/2008020121 Firefox/2.0.0.12.

Bug 415827 opened for comment 46.

Comment 52

[Alexander Sack]

Comment on attachment 298420 [details] [diff] [review]
better wallpaper

a=asac for 1.8.0.15

Comment 53

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.729.4.7.2.16; previous revision: 3.729.4.7.2.15
done
Checking in layout/generic/nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.13.10.1; previous revision: 3.13
done