Last Comment Bug 513394 - "ASSERTION: Some PresArena objects were not freed" with -moz-column, list-item, float, :after
: "ASSERTION: Some PresArena objects were not freed" with -moz-column, list-ite...
Status: RESOLVED FIXED
: [sg:critical?]
: assertion, crash, mlk, regression, testcase, verified1.9.1
Product: Core
Classification: Components
Component: Layout
: Trunk
: x86 Mac OS X
: P2 critical (vote)
: mozilla1.9.3a1
Assigned To: Timothy Nikkel (:tn)
: layout
:
:
: 306939 framedest 476357
  Show dependency treegraph
 
Reported: 2009-08-28 18:20 PDT by Jesse Ruderman
Modified: 2009-12-06 17:06 PST (History)
10 users (show)
roc: blocking1.9.2+
samuel.sidler+old: wanted1.9.0.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .4+
  .4-fixed


Attachments
testcase (triggers the assertion but not the crash) (576 bytes, text/html)
2009-08-28 18:20 PDT, Jesse Ruderman
no flags Details
patch (1.43 KB, patch)
2009-09-02 01:59 PDT, Timothy Nikkel (:tn)
dbaron: review+
roc: approval1.9.2+
dveditz: approval1.9.1.4+
Details | Diff | Splinter Review
followup (3.78 KB, patch)
2009-09-02 14:23 PDT, Timothy Nikkel (:tn)
dbaron: review-
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2009-08-28 18:20:12 PDT
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.
Comment 1 Timothy Nikkel (:tn) 2009-08-29 04:33:22 PDT
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.
Comment 2 Timothy Nikkel (:tn) 2009-09-02 01:59:06 PDT
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 3 David Baron [:dbaron] 2009-09-02 06:22:39 PDT
Comment on attachment 398095 [details] [review]
patch

r=dbaron
Comment 4 Boris Zbarsky (:bz) 2009-09-02 08:36:58 PDT
Pushed that patch as http://hg.mozilla.org/mozilla-central/rev/075a261bb1a5
Comment 5 Boris Zbarsky (:bz) 2009-09-02 08:37:22 PDT
Comment on attachment 398095 [details] [review]
patch

Need this on 1.9.1 and 1.9.2.
Comment 6 Timothy Nikkel (:tn) 2009-09-02 13:06:32 PDT
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.
Comment 7 Timothy Nikkel (:tn) 2009-09-02 14:23:41 PDT
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.
Comment 8 Daniel Veditz 2009-09-02 16:09:52 PDT
Probably need a roll-up branch patch that includes the followup once it's reviewed.
Comment 9 Timothy Nikkel (:tn) 2009-09-02 16:22:24 PDT
The followup is mostly just cleanup, I was thinking it would be trunk-only.
Comment 10 Boris Zbarsky (:bz) 2009-09-02 16:41:13 PDT
Not sure you wanted to nuke the blocking+ flag...
Comment 11 Timothy Nikkel (:tn) 2009-09-02 16:46:07 PDT
Whoops, no I didn't. For some reason it was showing as --- for me.
Comment 12 David Baron [:dbaron] 2009-09-03 05:39:54 PDT
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.)
Comment 13 Jesse Ruderman 2009-09-03 10:38:18 PDT
Does the followup need to happen in a security-sensitive bug?
Comment 14 Timothy Nikkel (:tn) 2009-09-03 13:41:38 PDT
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?
Comment 15 Timothy Nikkel (:tn) 2009-09-04 02:19:26 PDT
Filed bug 514634 for the followup.
Comment 16 Daniel Veditz 2009-09-04 10:26:20 PDT
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 17 Daniel Veditz 2009-09-04 10:26:40 PDT
Comment on attachment 398095 [details] [review]
patch

Approved for 1.9.1.4, a=dveditz for release-drivers
Comment 18 David Baron [:dbaron] 2009-09-04 11:06:33 PDT
(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.
Comment 20 Henrik Skupin (:whimboo) 2009-10-08 13:43:54 PDT
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.
Comment 21 Jesse Ruderman 2009-12-06 17:06:21 PST
Crashtest checked in: http://hg.mozilla.org/mozilla-central/rev/a86c5f1d3fef

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