Last Comment Bug 344064 - Crash [@ DoDeletingFrameSubtree ] [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent] with 1.8.0.5 and 1.8.1 branch
: Crash [@ DoDeletingFrameSubtree ] [@ nsIFrame::GetNextSibling] [@ nsCSSFrameC...
Status: RESOLVED FIXED
[sg:critical?] deleted frame, 1.8 bra...
: crash, testcase, verified1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 344056
  Show dependency treegraph
 
Reported: 2006-07-09 18:54 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.38 KB, application/xhtml+xml)
2006-07-09 18:55 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Testcase #2, (slightly reduced version of 1st attachment) (1.11 KB, application/xhtml+xml)
2007-05-04 22:19 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #3, minimal (355 bytes, application/xhtml+xml)
2007-05-04 22:21 PDT, Mats Palmgren (:mats)
no flags Details
@GetNextSibling stack (3.16 KB, text/plain)
2007-05-04 22:25 PDT, Mats Palmgren (:mats)
no flags Details
Frame dumps + tracing (25.14 KB, text/html)
2007-05-04 22:28 PDT, Mats Palmgren (:mats)
no flags Details
Wallpaper (2.65 KB, patch)
2007-05-04 23:04 PDT, Mats Palmgren (:mats)
dbaron: review+
dbaron: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-07-09 18:54:40 PDT
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
Comment 1 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-07-09 18:55:09 PDT
Created attachment 228639 [details]
testcase
Comment 2 Mats Palmgren (:mats) 2006-07-10 17:16:24 PDT
The testcase also crashes SeaMonkey nightly build 2006-07-10-01 on Linux:
1. load testcase
2. type CTRL++
Comment 3 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-07-10 17:19:45 PDT
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.
Comment 4 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-04-21 17:20:29 PDT
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 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-05-04 11:09:54 PDT
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 .
Comment 6 Mats Palmgren (:mats) 2007-05-04 22:19:00 PDT
Created attachment 263828 [details]
Testcase #2, (slightly reduced version of 1st attachment)
Comment 7 Mats Palmgren (:mats) 2007-05-04 22:21:30 PDT
Created attachment 263829 [details]
Testcase #3, minimal

Remove the style attribute from:
<xul:editor style="float: left;"></xul:editor>
Comment 8 Mats Palmgren (:mats) 2007-05-04 22:25:18 PDT
Created attachment 263830 [details]
@GetNextSibling stack
Comment 9 Mats Palmgren (:mats) 2007-05-04 22:28:07 PDT
Created attachment 263831 [details]
Frame dumps + tracing
Comment 10 Mats Palmgren (:mats) 2007-05-04 22:59:26 PDT
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
Comment 11 Mats Palmgren (:mats) 2007-05-04 23:04:30 PDT
Created attachment 263836 [details] [diff] [review]
Wallpaper

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 Boris Zbarsky [:bz] 2007-05-06 17:11:34 PDT
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 chris hofmann 2007-05-24 13:47:10 PDT
mats, can you get reviews to wrap this one up?  sounds like boris thinks this is a good fix.
Comment 14 Damon Sicore (:damons) 2007-07-06 15:42:53 PDT
Mats, any news here?
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-09 17:03:28 PDT
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.
Comment 16 Mats Palmgren (:mats) 2007-07-09 22:35:54 PDT
(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 Daniel Veditz [:dveditz] 2007-07-10 14:40:24 PDT
We need to shorten the 1.8.1.5 schedule, this one will have to make the next train.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-31 15:39:30 PDT
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)
Comment 19 Daniel Veditz [:dveditz] 2007-08-28 10:39:19 PDT
Comment on attachment 263836 [details] [diff] [review]
Wallpaper

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers
Comment 20 Mats Palmgren (:mats) 2007-08-30 18:55:21 PDT
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
Comment 21 Carsten Book [:Tomcat] 2007-09-03 15:00:52 PDT
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
Comment 22 Stephen Donner [:stephend] - PTO; back on 5/28 2007-12-10 16:05:01 PST
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
Comment 23 Bob Clary [:bc:] 2009-04-24 11:04:25 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/1851743bd36e

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