Closed
Bug 515811
Opened 15 years ago
Closed 15 years ago
"ASSERTION: Some PresArena objects were not freed" with -moz-column, floats, huge font
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta2-fixed |
status1.9.1 | --- | .6-fixed |
People
(Reporter: jruderman, Assigned: tnikkel)
References
Details
(5 keywords, Whiteboard: [sg:critical?] for 1.9.2 branch and earlier)
Attachments
(2 files)
278 bytes,
text/html
|
Details | |
1.82 KB,
patch
|
fantasai.bugs
:
review+
dbaron
:
superreview+
shaver
:
approval1.9.2+
dveditz
:
approval1.9.1.6+
dveditz
:
approval1.9.0.16+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: Some PresArena objects were not freed: 'mAllocCount == 0', file /Users/jruderman/central/layout/base/nsPresArena.cpp, line 169 Seems exploitable, but I'm not as sure as usual.
Reporter | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 1•15 years ago
|
||
I knew I should have listened to that nagging voice saying "what about the other early exit in ReflowBlockFrame?" from bug 513394. When reflowing (the first continuation of) the div that is the child of the <div style="float: left; -moz-column-count: 3;"> (the div for short) we call ReflowBlockFrame on the <div style="clear: both;"> and it returns early, without setting mPrevChild because of this code if (!treatWithClearance && !applyTopMargin && mightClearFloats && aState.mReflowState.mDiscoveredClearance) { nscoord curY = aState.mY + aState.mPrevBottomMargin.get(); nscoord clearY = aState.ClearFloats(curY, breakType, replacedBlock); if (clearY != curY) { // Looks like that assumption was invalid, we do need // clearance. Tell our ancestor so it can reflow again. It is // responsible for actually setting our clearance flag before // the next reflow. treatWithClearance = PR_TRUE; // Only record the first frame that requires clearance if (!*aState.mReflowState.mDiscoveredClearance) { *aState.mReflowState.mDiscoveredClearance = frame; } // Exactly what we do now is flexible since we'll definitely be // reflowed. return NS_OK; } } And then back in ReflowDirtyLines after the main loop over the lines is finished we check if we should pull lines with PRBool heightConstrained = aState.mReflowState.availableHeight != NS_UNCONSTRAINEDSIZE; PRBool skipPull = willReflowAgain && heightConstrained; except aState.mReflowState.availableHeight has overflowed and is now unconstrained. So we end up pulling frames and mPrevChild is one back of where is should be for the pull. The available height gets set to unconstrained in ReflowBlockFrame where we ComputeCollapsedTopMargin, and the topMargin is unconstrained because the huge font on the child <div id="x" style="margin: 1em 0pt;"> gives a huge margin. And then we do // Now put the Y coordinate back to the top of the top-margin + // clearance, and flow the block. aState.mY -= topMargin; availSpace.y -= topMargin; if (NS_UNCONSTRAINEDSIZE != availSpace.height) { availSpace.height += topMargin; } so the available height becomes unconstrained.
Assignee: nobody → tnikkel
Attachment #400699 -
Flags: review?(fantasai.bugs)
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite?
Comment on attachment 400699 [details] [diff] [review] patch It looks reasonable to me, but I'd rather have dbaron, bz, or roc take a look here.
Attachment #400699 -
Flags: superreview?(dbaron)
Attachment #400699 -
Flags: review?(fantasai.bugs)
Attachment #400699 -
Flags: review+
Reporter | ||
Comment 3•15 years ago
|
||
WFM? I'm suspicious, though, because bug 497495 part 4 moved this assertion.
Assignee | ||
Comment 4•15 years ago
|
||
The moving of the assertion in bug 497495 isn't to blame as I still get the (moved) assertion in a debug build at http://hg.mozilla.org/mozilla-central/rev/c655e28003ef which is after that landed.
Assignee | ||
Comment 5•15 years ago
|
||
Oh, it's due to the landing of bug 512471. At the end of ReflowDirtyLines it no longer depends on mPrevChild being set correctly so it doesn't fail.
Assignee | ||
Comment 6•15 years ago
|
||
I think it's still worth it to do this patch though.
Assignee | ||
Comment 7•15 years ago
|
||
Oh, and bug 512471 is not going to land on branches, so we'll need this patch for branches.
Reporter | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] for 1.9.2 branch and earlier
Comment on attachment 400699 [details] [diff] [review] patch sr=dbaron, although I'm not sure why we're going on and pulling frames (and reflowing more frames?) after we've already discovered we need to reflow again
Attachment #400699 -
Flags: superreview?(dbaron) → superreview+
Assignee | ||
Comment 9•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bb82129563af
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Assignee | ||
Updated•15 years ago
|
Attachment #400699 -
Flags: approval1.9.2?
Attachment #400699 -
Flags: approval1.9.1.6?
Attachment #400699 -
Flags: approval1.9.0.16?
Updated•15 years ago
|
Flags: blocking1.9.2?
Updated•15 years ago
|
Attachment #400699 -
Flags: approval1.9.1.6?
Attachment #400699 -
Flags: approval1.9.1.6+
Attachment #400699 -
Flags: approval1.9.0.16?
Attachment #400699 -
Flags: approval1.9.0.16+
Comment 10•15 years ago
|
||
Comment on attachment 400699 [details] [diff] [review] patch Approved for 1.9.1.6 and 1.9.0.16, a=dveditz for release-drivers
Comment on attachment 400699 [details] [diff] [review] patch a=shaver for 1.9.2, thanks again timothy!
Attachment #400699 -
Flags: approval1.9.2? → approval1.9.2+
Flags: blocking1.9.2? → wanted1.9.2+
Assignee | ||
Comment 12•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e0529f8ec04e
status1.9.2:
--- → beta2-fixed
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/49e937c4a4db
status1.9.1:
--- → .6-fixed
Comment 14•15 years ago
|
||
Checking in nsBlockFrame.cpp; /cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v <-- nsBlockFrame.cpp new revision: 3.967; previous revision: 3.966
Keywords: fixed1.9.0.16
Comment 15•15 years ago
|
||
With debug 1.9.1 from before the fix, I don't see the assertion in comment 0. The only assert that I see that seems related, perhaps, is: ###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/albill/mozilla-191/layout/base/nsPresShell.cpp, line 674 With debug 1.9.0 before the fix, I see no assertion at all. Is this just a defense in depth fix on 1.9.1 and 1.9.0 or were these assertions actually seen on these two branches?
Reporter | ||
Comment 16•15 years ago
|
||
That's the same assertion. Its text changed at some point.
Comment 17•15 years ago
|
||
All right, so it is definitely fixed for 1.9.1. I don't see any assertion during 1.9.0 runs before the fix.
Keywords: verified1.9.1
Updated•14 years ago
|
Group: core-security
Assignee | ||
Comment 18•14 years ago
|
||
Added crashtest to 1.9.1, 1.9.2, and mozilla-central. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/976ec8c4d911 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/914ae05fe0a2 http://hg.mozilla.org/mozilla-central/rev/260d768e55f4
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•