Bugzilla@Mozilla – Bug 513394
"ASSERTION: Some PresArena objects were not freed" with -moz-column, list-item, float, :after
Last modified: 2009-12-06 17:06:21 PST
Summon comment box
Created attachment 397408 [details] testcase (triggers the assertion but not the crash) ###!!! ASSERTION: Some PresArena objects were not freed: 'mAllocCount == 0', file /Users/jruderman/central/layout/base/nsPresArena.cpp, line 169 Appears to be exploitable.
The <div style="height: 20px; display: inline-block;"> doesn't get deleted because at the time of destruction the mLines of its block parent has it as the second child, but the mNextSibling link of the first child is null. At some point we do an invalid mutation of the frame tree.
Created attachment 398095 [details] [review] patch When we pull lines after calling ReflowLine in ReflowDirtyLines we use mPrevChild of the nsBlockReflowState in order to properly set the next sibling of our last child to the start of the pulled line. That is, as long as it is not null. Bug 476357 added an early return to ReflowBlockFrame just before we set mPrevChild.
Comment on attachment 398095 [details] [review] patch r=dbaron
Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/075a261bb1a5
Comment on attachment 398095 [details] [review] patch Need this on 1.9.1 and 1.9.2.
Jesse, it sounded like you have a crashing testcase for this? Would be good to include in the crashtests once we have this fixed on the branches.
Created attachment 398221 [details] [review] followup Add an assertion to catch the problem of this bug. nsBlockFrame::PullFrameFrom calls SetNextSibling twice, doing what should be the same thing twice. Fix the comment above nsBlockFrame::PullFrameFrom.
Probably need a roll-up branch patch that includes the followup once it's reviewed.
The followup is mostly just cleanup, I was thinking it would be trunk-only.
Not sure you wanted to nuke the blocking+ flag...
Whoops, no I didn't. For some reason it was showing as --- for me.
Comment on attachment 398221 [details] [review] followup Between here: >- aLine->LastChild()->SetNextSibling(frame); >+ NS_ASSERTION(aState.mPrevChild == aLine->LastChild(), >+ "mPrevChild should be the LastChild of the line we are adding to"); >+ // The frame is being pulled from a next-in-flow; therefore we >+ // need to add it to our sibling list. >+ frame->SetNextSibling(nsnull); >+ if (nsnull != aState.mPrevChild) { >+ aState.mPrevChild->SetNextSibling(frame); >+ } And here: >- // The frame is being pulled from a next-in-flow; therefore we >- // need to add it to our sibling list. >- frame->SetNextSibling(nsnull); >- if (nsnull != aState.mPrevChild) { >- aState.mPrevChild->SetNextSibling(frame); >- } >- There's the following code: if (0 != --fromLineChildCount) { // Mark line dirty now that we pulled a child fromLine->SetChildCount(fromLineChildCount); fromLine->MarkDirty(); fromLine->mFirstChild = frame->GetNextSibling(); } which you're going to break since it will now set fromLine->mFirstChild to null instead of to the correct frame. (I'm also not sure I'm the best reviewer for this; I think roc and fantasai know this code better than I do.)
Does the followup need to happen in a security-sensitive bug?
I guess we can move the followup to a new bug. I don't think anyone can glean anything more from the followup+discussion then from the patch that is already committed. Any objections?
Filed bug 514634 for the followup.
Does this bug affect 1.9.0? I'd hate to fix it in 3.5.4 and not in the accompanying 3.0.x if so.
Comment on attachment 398095 [details] [review] patch Approved for 1.9.1.4, a=dveditz for release-drivers
(In reply to comment #16) > Does this bug affect 1.9.0? I'd hate to fix it in 3.5.4 and not in the > accompanying 3.0.x if so. No; it's a regression from bug 476357.
Pushed: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5c27d5405847 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/df891eff2eff
No assertion visible when loading the given testcase with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5pre) Gecko/20091008 Shiretoko/3.5.5pre ID:20091008213747. Marking as verified fixed on 1.9.1.
Crashtest checked in: http://hg.mozilla.org/mozilla-central/rev/a86c5f1d3fef