Closed
Bug 344064
Opened 18 years ago
Closed 17 years ago
Crash [@ DoDeletingFrameSubtree ] [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent] with 1.8.0.5 and 1.8.1 branch
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)
References
Details
(4 keywords, Whiteboard: [sg:critical?] deleted frame, 1.8 branches only)
Crash Data
Attachments
(6 files)
1.38 KB,
application/xhtml+xml
|
Details | |
1.11 KB,
application/xhtml+xml
|
Details | |
355 bytes,
application/xhtml+xml
|
Details | |
3.16 KB,
text/plain
|
Details | |
25.14 KB,
text/html
|
Details | |
2.65 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
dveditz
:
approval1.8.1.8+
dveditz
:
approval1.8.0.14+
|
Details | Diff | Splinter Review |
See upcoming testcase, it crashes FF1.5.0.5RC and FF2, but not current trunk build. I can minimise the testcase, if desired. Talkback ID: TB20766966M 0x00000000 DoDeletingFrameSubtree [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9675] DeletingFrameSubtree [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9754] nsCSSFrameConstructor::ContentRemoved [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9973] nsCSSFrameConstructor::ReinsertContent [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9623] nsCSSFrameConstructor::ContentInserted [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 9526] nsCSSFrameConstructor::RecreateFramesForContent [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11983] nsCSSFrameConstructor::ProcessRestyledFrames [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10447] nsCSSFrameConstructor::RestyleElement [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10519] nsCSSFrameConstructor::ProcessOneRestyle [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13956] nsCSSFrameConstructor::ProcessPendingRestyles [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 14004] nsCSSFrameConstructor::RestyleEvent::HandleEvent [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 14066] 0x778b0c24 0x00200064 0xeed9285e
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Updated•18 years ago
|
Version: Trunk → 1.8 Branch
Assignee | ||
Comment 2•18 years ago
|
||
The testcase also crashes SeaMonkey nightly build 2006-07-10-01 on Linux: 1. load testcase 2. type CTRL++
Reporter | ||
Comment 3•18 years ago
|
||
Hmm, yes, I see it too on windows, I get talkback ID: TB20804616Z nsCachedStyleData::GetStyleData nsRuleNode::ComputeDisplayData nsRuleNode::WalkRuleTree nsRuleNode::GetDisplayData which seems to indicate bug 343206.
Reporter | ||
Comment 4•17 years ago
|
||
The testcase still crashes in the latest 1.8 branch builds, the fix for bug 372576 didn't fix this bug. The crash mentioned in comment 3 and 4 on trunk doesn't seem to be there anymore.
Comment 5•17 years ago
|
||
I caught this in a Mac debug 1.8 branch build, childFrame points to deleted memory at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1110.6.76&mark=9738#9737 .
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Whiteboard: [sg:critical] deleted frame, 1.8 branches only
Updated•17 years ago
|
Whiteboard: [sg:critical] deleted frame, 1.8 branches only → [sg:critical?] deleted frame, 1.8 branches only
Updated•17 years ago
|
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Summary: Crash [@ DoDeletingFrameSubtree ] with 1.8.0.5 and 1.8.1 branch → Crash [@ DoDeletingFrameSubtree ] [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent] with 1.8.0.5 and 1.8.1 branch
Assignee | ||
Comment 6•17 years ago
|
||
Assignee | ||
Comment 7•17 years ago
|
||
Remove the style attribute from: <xul:editor style="float: left;"></xul:editor>
Assignee | ||
Comment 8•17 years ago
|
||
Assignee | ||
Comment 9•17 years ago
|
||
Assignee | ||
Comment 10•17 years ago
|
||
Frame destruction in nsCSSFrameConstructor::ContentRemoved :-( On branch we do if (display->mDisplay == NS_STYLE_DISPLAY_POPUP) ... popup stuff ... else if (display->IsFloating()) remove frame from nsLayoutAtoms::floatList and if it fails remove from nsnull list else if (display->IsAbsolutelyPositioned()) remove frame from fixed/abs-list and if it fails remove from nsnull list else remove from nsnull list http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=10082,10102,10106,10119,10132,10136,10146,10156#10082 Compare trunk: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1343&root=/cvsroot&mark=9523,9538,9547,9557#9522 The problem with the branch code is that removing from ::floatList never fails! http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=5584,5603-5606,5621#5580 and nsBlockFrame::RemoveFloat always Destroys the frame, even when it fails to find it: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=5518#5482
Assignee | ||
Comment 11•17 years ago
|
||
Wrapping the float/position blocks in if (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) is the best I can think of right now (basically what we do on trunk), it fixes all the testcases. Boris, what do you think?
Comment 12•17 years ago
|
||
I think that sounds like a great idea to me. It's the Right Thing To Do, and should be safe for branch...
Comment 13•17 years ago
|
||
mats, can you get reviews to wrap this one up? sounds like boris thinks this is a good fix.
Updated•17 years ago
|
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Comment 14•17 years ago
|
||
Mats, any news here?
Updated•17 years ago
|
Attachment #263836 -
Flags: superreview?(dbaron)
Attachment #263836 -
Flags: review?(dbaron)
Attachment #263836 -
Flags: approval1.8.1.5?
Attachment #263836 -
Flags: approval1.8.0.13?
Is there anything other than popups, floating, or absolutely positioned elements that are out-of-flow? (Note that the indentation at the beginning of the popup bit on the 1.8 branch is very misleading -- it's got an unbraced if with indentation that makes it look like a lot more is inside the if.) I suppose this is ok, but this code has so many error conditions that it seems to accept and try to handle (that I'd think should have been handled much earlier so we'd never create such broken frame trees) that I'm a little worried.
Assignee | ||
Comment 16•17 years ago
|
||
(In reply to comment #15) > Is there anything other than popups, floating, or absolutely positioned > elements that are out-of-flow? No. But what happens in the testcase is that we have a frame that "IsFloating()" but the actual frame isn't NS_FRAME_OUT_OF_FLOW due to some failure(?). > (Note that the indentation at the beginning of > the popup bit on the 1.8 branch is very misleading Good point, I didn't see that, which makes this code even more interesting ;-) The destruction of popups is actually split between the first 'if'-block, for those that are out-of-flow (have placeholder and lives in the popupset frame) and the last 'else' block for normal flow popups. So, the patch is actually correct, but I'll admit that I was a bit lucky there ;-)
Comment 17•17 years ago
|
||
We need to shorten the 1.8.1.5 schedule, this one will have to make the next train.
Flags: blocking1.8.1.5+ → blocking1.8.1.6?
Whiteboard: [sg:critical?] deleted frame, 1.8 branches only → [sg:critical?] deleted frame, 1.8 branches only, need r=dbaron
Updated•17 years ago
|
Attachment #263836 -
Flags: approval1.8.1.5? → approval1.8.1.6?
Comment on attachment 263836 [details] [diff] [review] Wallpaper r+sr=dbaron. But when you land this, could you fix the existing indentation problem (that is, unindent the parts that shouldn't have been indented surrounding your new {} rather than indenting what's inside them)
Attachment #263836 -
Flags: superreview?(dbaron)
Attachment #263836 -
Flags: superreview+
Attachment #263836 -
Flags: review?(dbaron)
Attachment #263836 -
Flags: review+
Updated•17 years ago
|
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
Updated•17 years ago
|
Attachment #263836 -
Flags: approval1.8.0.13? → approval1.8.0.14?
Updated•17 years ago
|
Flags: blocking1.8.1.7?
Flags: blocking1.8.1.7+
Flags: blocking1.8.0.14?
Flags: blocking1.8.0.14+
Whiteboard: [sg:critical?] deleted frame, 1.8 branches only, need r=dbaron → [sg:critical?] deleted frame, 1.8 branches only
Comment 19•17 years ago
|
||
Comment on attachment 263836 [details] [diff] [review] Wallpaper approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers
Attachment #263836 -
Flags: approval1.8.1.7?
Attachment #263836 -
Flags: approval1.8.1.7+
Attachment #263836 -
Flags: approval1.8.0.14?
Attachment #263836 -
Flags: approval1.8.0.14+
Assignee | ||
Comment 20•17 years ago
|
||
MOZILLA_1_8_BRANCH mozilla/layout/base/nsCSSFrameConstructor.cpp 1.1110.6.84 MOZILLA_1_8_0_BRANCH mozilla/layout/base/nsCSSFrameConstructor.cpp 1.1110.6.12.2.60 -> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Keywords: fixed1.8.0.14,
fixed1.8.1.7
Resolution: --- → FIXED
Comment 21•17 years ago
|
||
verified fixed 1.8.1.7 using: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre ID:2007090304 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre)Gecko/2007090308 BonEcho/2.0.0.7pre no crash on the testcases from this bug - adding verified keyword
Keywords: fixed1.8.1.7 → verified1.8.1.7
Updated•17 years ago
|
Group: security
Verified fixed on the 1.8.0.x branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre; I no longer crash with any of the 3 testcases linked here. Replacing fixed1.8.0.14 with verified1.8.0.14
Keywords: fixed1.8.0.14 → verified1.8.0.14
Comment 23•15 years ago
|
||
crash test landed http://hg.mozilla.org/mozilla-central/rev/1851743bd36e
Flags: in-testsuite? → in-testsuite+
Updated•13 years ago
|
Crash Signature: [@ DoDeletingFrameSubtree ]
[@ nsIFrame::GetNextSibling]
[@ nsCSSFrameConstructor::FindFrameWithContent]
You need to log in
before you can comment on or make changes to this bug.
Description
•