Closed Bug 513394 Opened 10 years ago Closed 10 years ago

"ASSERTION: Some PresArena objects were not freed" with -moz-column, list-item, float, :after

Categories

(Core :: Layout, defect, P2, critical)

x86
macOS
defect

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

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [sg:critical?])

Attachments

(2 files, 1 obsolete file)

###!!! ASSERTION: Some PresArena objects were not freed: 'mAllocCount == 0', file /Users/jruderman/central/layout/base/nsPresArena.cpp, line 169

Appears to be exploitable.
Whiteboard: [sg:critical?]
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.
Attached patch patchSplinter Review
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)
Blocks: 476357
Attachment #398095 - Flags: review?(dbaron) → review+
Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/075a261bb1a5
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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?
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.
Attached patch followup (obsolete) — Splinter Review
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)
Probably need a roll-up branch patch that includes the followup once it's reviewed.
blocking1.9.1: --- → .4+
Flags: blocking1.9.2?
The followup is mostly just cleanup, I was thinking it would be trunk-only.
blocking1.9.1: .4+ → ---
Not sure you wanted to nuke the blocking+ flag...
blocking1.9.1: --- → ?
Whoops, no I didn't. For some reason it was showing as --- for me.
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-
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.
Attachment #398221 - Attachment is obsolete: true
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 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.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Attachment #398095 - Flags: approval1.9.2? → approval1.9.2+
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
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
No longer depends on: 521639
Group: core-security
Keywords: regression
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.