Closed
Bug 513394
Opened 15 years ago
Closed 15 years ago
"ASSERTION: Some PresArena objects were not freed" with -moz-column, list-item, float, :after
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
blocking1.9.1 | --- | .4+ |
status1.9.1 | --- | .4-fixed |
People
(Reporter: jruderman, Assigned: tnikkel)
References
Details
(6 keywords, Whiteboard: [sg:critical?])
Attachments
(2 files, 1 obsolete file)
576 bytes,
text/html
|
Details | |
1.43 KB,
patch
|
dbaron
:
review+
roc
:
approval1.9.2+
dveditz
:
approval1.9.1.4+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: Some PresArena objects were not freed: 'mAllocCount == 0', file /Users/jruderman/central/layout/base/nsPresArena.cpp, line 169 Appears to be exploitable.
Reporter | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee: nobody → tnikkel
Attachment #398095 -
Flags: review?(dbaron)
Attachment #398095 -
Flags: review?(dbaron) → review+
Comment on attachment 398095 [details] [diff] [review] patch r=dbaron
Comment 4•15 years ago
|
||
Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/075a261bb1a5
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment 5•15 years ago
|
||
Comment on attachment 398095 [details] [diff] [review] patch Need this on 1.9.1 and 1.9.2.
Attachment #398095 -
Flags: approval1.9.2?
Attachment #398095 -
Flags: approval1.9.1.4?
Assignee | ||
Comment 6•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
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.
Attachment #398221 -
Flags: review?(dbaron)
Comment 8•15 years ago
|
||
Probably need a roll-up branch patch that includes the followup once it's reviewed.
Assignee | ||
Comment 9•15 years ago
|
||
The followup is mostly just cleanup, I was thinking it would be trunk-only.
blocking1.9.1: .4+ → ---
Assignee | ||
Comment 11•15 years ago
|
||
Whoops, no I didn't. For some reason it was showing as --- for me.
Updated•15 years ago
|
blocking1.9.1: ? → .4+
Comment on attachment 398221 [details] [diff] [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.)
Attachment #398221 -
Flags: review?(dbaron) → review-
Reporter | ||
Comment 13•15 years ago
|
||
Does the followup need to happen in a security-sensitive bug?
Assignee | ||
Comment 14•15 years ago
|
||
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?
Assignee | ||
Comment 15•15 years ago
|
||
Filed bug 514634 for the followup.
Assignee | ||
Updated•15 years ago
|
Attachment #398221 -
Attachment is obsolete: true
Comment 16•15 years ago
|
||
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.
Flags: wanted1.9.0.x?
Comment 17•15 years ago
|
||
Comment on attachment 398095 [details] [diff] [review] patch Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #398095 -
Flags: approval1.9.1.4? → approval1.9.1.4+
(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.
Assignee | ||
Updated•15 years ago
|
Flags: wanted1.9.0.x?
Updated•15 years ago
|
Flags: wanted1.9.0.x-
Attachment #398095 -
Flags: approval1.9.2? → approval1.9.2+
Comment 19•15 years ago
|
||
Pushed: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5c27d5405847 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/df891eff2eff
Updated•15 years ago
|
status1.9.2:
--- → beta1-fixed
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Comment 20•15 years ago
|
||
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.
Keywords: verified1.9.1
Target Milestone: --- → mozilla1.9.3a1
Updated•15 years ago
|
Group: core-security
Keywords: regression
Reporter | ||
Comment 21•15 years ago
|
||
Crashtest checked in: http://hg.mozilla.org/mozilla-central/rev/a86c5f1d3fef
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•