Closed
Bug 346405
Opened 18 years ago
Closed 17 years ago
[columns] crash [@ nsColumnSetFrame::GetContentInsertionFrame] and [@ nsLineLayout::TrimTrailingWhiteSpaceIn]
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha8
People
(Reporter: MatsPalmgren_bugz, Assigned: roc)
References
Details
(4 keywords, Whiteboard: [sg:critical?])
Crash Data
Attachments
(7 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
dveditz
:
approval1.8.1.12+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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]
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Comment 2•18 years ago
|
||
Comment 3•18 years ago
|
||
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•18 years ago
|
||
Reporter | ||
Comment 5•18 years ago
|
||
The "fairly reduced testcase" also triggers bug 337412.
I'm fixing that bit over there...
Depends on: 337412
Updated•18 years ago
|
Whiteboard: [sg:critical] uses freed memory
Updated•18 years ago
|
Flags: blocking1.9a1?
Reporter | ||
Comment 6•18 years ago
|
||
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
Keywords: assertion
Updated•18 years ago
|
Flags: blocking1.9a1? → blocking1.9?
Comment 7•18 years ago
|
||
Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.
Assignee: nobody → mats.palmgren
Comment 8•18 years ago
|
||
any ideas on a fix? does it look like this might be an easy one?
Updated•18 years ago
|
Flags: blocking1.9? → blocking1.9+
Comment 9•18 years ago
|
||
Robert: please fix this column-related security bug (or assign to one of your minions).
Assignee: mats.palmgren → roc
Comment 10•18 years ago
|
||
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
Updated•18 years ago
|
Attachment #231217 -
Attachment is obsolete: true
Updated•18 years ago
|
Attachment #231216 -
Attachment is obsolete: true
Comment 11•18 years ago
|
||
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•18 years ago
|
||
(In reply to comment #11)
> Martijn, do you want me to try to reduce it?
Yes, sure, thanks.
Updated•18 years ago
|
Target Milestone: --- → mozilla1.9beta1
Assignee | ||
Comment 13•18 years ago
|
||
I reduced this from 262587. It crashes every time on my Mac build.
Assignee | ||
Comment 14•18 years ago
|
||
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 :-)
Attachment #265632 -
Flags: superreview?(dbaron)
Attachment #265632 -
Flags: review?(dbaron)
Comment 15•18 years ago
|
||
This is a testcase that's reasonably minimised and that still crashes in my build that has roc's fix in.
Comment 16•18 years ago
|
||
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.
Assignee | ||
Comment 17•18 years ago
|
||
testcase3 doesn't crash for me :-(
Comment 18•18 years ago
|
||
Yuck, I guess testcase3 is font dependent or something.
Does this testcase4 crash for you?
Assignee | ||
Comment 19•18 years ago
|
||
Nope.
Updated•18 years ago
|
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Updated•18 years ago
|
Whiteboard: [sg:critical] uses freed memory → [sg:critical] uses freed memory, needs r=dbaron
Assignee | ||
Comment 20•18 years ago
|
||
I need review here ... the patch is small.
Comment 21•18 years ago
|
||
Comment on attachment 265632 [details] [diff] [review]
fix
r+sr=dbaron. Sorry for the delay.
Attachment #265632 -
Flags: superreview?(dbaron)
Attachment #265632 -
Flags: superreview+
Attachment #265632 -
Flags: review?(dbaron)
Attachment #265632 -
Flags: review+
Comment 22•18 years ago
|
||
I assume this is the right patch for the 1.8 branch too?
Whiteboard: [sg:critical] uses freed memory, needs r=dbaron → [sg:critical] uses freed memory, need trunk landing, branch approval
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.6+
Flags: blocking1.8.1.5+
Assignee | ||
Comment 23•18 years ago
|
||
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 24•18 years ago
|
||
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
Assignee | ||
Comment 25•18 years ago
|
||
I checked in that patch.
Updated•17 years ago
|
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Assignee | ||
Comment 26•17 years ago
|
||
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 27•17 years ago
|
||
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?
Version: Trunk → 1.8 Branch
Assignee | ||
Comment 28•17 years ago
|
||
Sounds good.
Comment 29•17 years ago
|
||
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.
Assignee | ||
Comment 30•17 years ago
|
||
Clearing blocking 1.9 since this is now a branch bug.
Flags: blocking1.9+
Updated•17 years ago
|
Flags: in-testsuite?
Updated•17 years ago
|
Flags: blocking1.9?
Comment 31•17 years ago
|
||
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.
Flags: blocking1.9?
Whiteboard: [sg:critical] uses freed memory, need trunk landing, branch approval → [sg:critical?] wfm on trunk, need branch patch
Comment 32•17 years ago
|
||
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.
Assignee | ||
Comment 33•17 years ago
|
||
I haven't looked at it at all.
Comment 34•17 years ago
|
||
Year old+ security hole, pushed off for another 8 wks. Yay us.
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Updated•17 years ago
|
Flags: blocking1.8.0.14+ → blocking1.8.0.15+
Assignee | ||
Comment 35•17 years ago
|
||
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
Assignee | ||
Comment 36•17 years ago
|
||
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).
Attachment #298354 -
Flags: superreview?(dbaron)
Attachment #298354 -
Flags: review?(dbaron)
Assignee | ||
Comment 37•17 years ago
|
||
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 38•17 years ago
|
||
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?
Assignee | ||
Comment 39•17 years ago
|
||
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 40•17 years ago
|
||
I think that does sound safer, yes.
Assignee | ||
Comment 41•17 years ago
|
||
Attachment #298354 -
Attachment is obsolete: true
Attachment #298420 -
Flags: superreview?(dbaron)
Attachment #298420 -
Flags: review?(dbaron)
Attachment #298354 -
Flags: superreview?(dbaron)
Attachment #298354 -
Flags: review?(dbaron)
Comment 42•17 years ago
|
||
Comment on attachment 298420 [details] [diff] [review]
better wallpaper
ok, r+sr=dbaron for branch
Attachment #298420 -
Flags: superreview?(dbaron)
Attachment #298420 -
Flags: superreview+
Attachment #298420 -
Flags: review?(dbaron)
Attachment #298420 -
Flags: review+
Comment 43•17 years ago
|
||
Comment on attachment 298420 [details] [diff] [review]
better wallpaper
approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #298420 -
Flags: approval1.8.1.12+
Updated•17 years ago
|
Whiteboard: [sg:critical?] wfm on trunk, need branch patch → [sg:critical?] need branch patch landing
Assignee | ||
Comment 44•17 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1.12
Resolution: --- → FIXED
Whiteboard: [sg:critical?] need branch patch landing → [sg:critical?]
Comment 45•17 years ago
|
||
That's a popular place to crash, 19 other bugs:
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20summary%3A%22nsCachedStyleData::GetStyleData%22
Comment 46•17 years ago
|
||
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?
Reporter | ||
Comment 47•17 years ago
|
||
"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 48•17 years ago
|
||
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12
Comment 49•17 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Attachment #265625 -
Attachment is private: true
Updated•17 years ago
|
Group: security
Comment 50•17 years ago
|
||
Comment on attachment 298420 [details] [diff] [review]
better wallpaper
a=asac for 1.8.0.15
Attachment #298420 -
Flags: approval1.8.0.15+
Comment 51•17 years ago
|
||
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
Keywords: fixed1.8.0.15
Updated•14 years ago
|
Crash Signature: [@ nsColumnSetFrame::GetContentInsertionFrame]
[@ nsLineLayout::TrimTrailingWhiteSpaceIn]
You need to log in
before you can comment on or make changes to this bug.
Description
•