Closed Bug 452157 Opened 12 years ago Closed 12 years ago

Crash [@ nsFrameManager::CaptureFrameStateFor] with -moz-column, float


(Core :: Layout, defect, P3)






(Reporter: jruderman, Assigned: dholbert)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [sg:critical])

Crash Data


(7 files, 1 obsolete file)

Loading the testcase triggers an assertion and a crash.

###!!! ASSERTION: Must only be called on reflowed lines: '!(GetStateBits() & NS_FRAME_IS_DIRTY)', file /Users/jruderman/central/layout/generic/nsFrame.cpp, line 4071

Crash at one of the following:
* [@ nsFrameManager::CaptureFrameStateFor]
* [@ nsContainerFrame::PositionChildViews]
* [@ nsFrameList::CheckForLoops]

Some of the crashes look exploitable.
Whiteboard: [sg:critical]
The assert is coming from a CachedIsEmpty call.  The frame tree is:

        Canvas(html)(-1)@0x142d184 [view=0xcba5570] {0,0,0,0} [state=00002403] [content=0xcacdf30] [sc=0x144c864] pst=:-moz-scrolled-canvas<
          Area(html)(-1)@0x144cb94 {0,0,0,0} [state=00d01401] sc=0x144c98c(i=0,b=1)<
            line 0x144cf90: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4048] {0,0,0,0} <
              Block(body)(1)@0x144cf38 {0,0,0,0} [state=00101401] sc=0x144cae8(i=1,b=0)<
                line 0x144da04: count=3 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0xc000] {0,0,0,0} <
                  Text(0)@0x144cfdc[0,2,T]  next=0x144d2a8 {0,0,0,0} [state=00200000] [content=0xcba5450] sc=0x144ce98 pst=:-moz-non-element<
                  Placeholder(div)(1)@0x144d2a8 {0,0,0,0} [state=00000403] outOfFlowFrame=ColumnSet(div)(1)@0x144d174
                  Text(2)@0x144d9c4[0,2,T]  {0,0,0,0} [state=08000402] [content=0xcba5e90] sc=0x144ce98 pst=:-moz-non-element<
                  ColumnSet(div)(1)@0x144d174 {0,0,0,0} [state=00000523] [content=0xcba49e0] [sc=0x144cee8]<
                    Area(div)(1)@0x144d0c0 next=0x136a238 next-in-flow=0x136a238 {0,0,960,1560} [state=08d00409] [overflow=0,0,960,1920] sc=0x144d1fc(i=0,b=1) pst=:-moz-column-content<
                      line 0x144d99c: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4008] {0,0,960,1560} ca={0,0,960,1920} <
                        Block(div)(0)@0x144d330 next-in-flow=0x144ddb8 {0,0,960,1560} [state=08100409] [overflow=0,0,960,1920] sc=0x144d2e0(i=2,b=1)<
                          line 0x144d924: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8340] {960,0,0,0} ca={0,960,960,960} <
                            Placeholder(div)(0)@0x144d500 {960,0,0,0} outOfFlowFrame=Area(div)(0)@0x144d458
                            Placeholder(div)(1)@0x144d7b0 {960,0,0,0} outOfFlowFrame=Area(div)(1)@0x144d758
                          > floats <
                            placeholder@0x144d500 Area(div)(0) region={0,0,960,2880}
                            placeholder@0x144d7b0 Area(div)(1) region={960,0,0,1920}
                          line 0x144d94c: count=1 state=block,clean,prevmargindirty,not impacted,not wrapped,before:leftbr+rightbr,after:nobr[0x4c0a] {0,0,0,0} <
                            Frame(br)(2)@0x144d8b8 {0,0,0,0} [state=00000400] [content=0xcba5d00]
                          line 0x144d974: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4200] {0,0,0,0} <
                            Text(3)@0x144d8e4[0,1,T]  {0,0,0,0} [state=08600000] [content=0xcba5dc0] sc=0x144d4b0 pst=:-moz-non-element<
                              " "
                            Area(div)(0)@0x144d458 next=0x144d758 {0,960,960,960} [state=00d00100] sc=0x144d3ac(i=1,b=0)<
                              line 0x144d660: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,0,960,960} <
                                Block(span)(0)@0x144d608 {0,0,960,960} [state=00d00000] sc=0x144d55c(i=0,b=0)<
                            Area(div)(1)@0x144d758 {960,960,0,0} [state=00d00100] sc=0x144d6ac(i=0,b=0)<
                    Area(div)(1)@0x136a238 prev-in-flow=0x144d0c0 {1920,0,960,1920} [state=10d00004] sc=0x144d1fc(i=0,b=1) pst=:-moz-column-content<
                      line 0x136a210: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4108] {0,0,960,0} <
                        Block(div)(0)@0x144ddb8 prev-in-flow=0x144d330 {0,0,960,0} [state=10100004] sc=0x144d2e0(i=0,b=0)<

The |this| in CachedIsEmpty() is 0x144d8b8 (the single Frame(br) around).

We then crash in CheckForLoops.  The reason we crash is that we're at nsBlockFrame::SplitLine and aFrame->GetNextSibling() is a deleted frame.  aFrame is the nsPlaceholderFrame at 0x144d7b0.  Its GetNextSibling() pointer points to 0x144d8e4 which is gone from the frame tree by this point.
Flags: blocking1.9.1?
It looks like we do some frame-destroying here from nsBlockFrame::DeleteNextInFlowChild.  The place where we destroy the nsTextFrame is under nsLineBox::DeleteLineList, called from nsBlockFrame::Destroy, called from nsBlockFrame::DeleteNextInFlowChild.
And yes, this looks exploitable.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
The testcase crashes my Linux nightly build from today, too.
Build ID: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20081001 Minefield/3.1b1pre
Crash report: 8a2f2682-900b-11dd-a741-001cc45a2c28

OS/Platform -->All/All
OS: Mac OS X → All
Hardware: PC → All
The testcase also crashes Firefox 3.0.3 (after hanging it for ~15 seconds). Adding 'wanted1.9.0.x?'
Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/2008092515 Ubuntu/8.10 (intrepid) Firefox/3.0.3

No crash on Firefox 2, though.  Adding 'regression' keyword.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20080829 Firefox/
Flags: wanted1.9.0.x?
Keywords: regression
This testcase has it structure simplified a bit, and the style separated out into a <style> block.

I also substituted divs for the original testcase's <span> and <br> (which had "display: inline-block" and "display: inherit" --> "display: block")
Summary: Crash [@ nsFrameManager::CaptureFrameStateFor] with -moz-column, float, inline-block → Crash [@ nsFrameManager::CaptureFrameStateFor] with -moz-column, float
Attachment #341365 - Attachment description: testcase 2 → testcase 2 (crashes Firefox when loaded)
Attachment #335465 - Attachment description: testcase (crashes Firefox when loaded) → testcase 1 (crashes Firefox when loaded)
Just for fun, this testcase has 'position: absolute' on the outermost element instead of 'float: left'.  It still crashes.

(If I make that substitution for the inner floated elements, the crash goes away, though.)
Assignee: nobody → dholbert
This is the frametree at the time of crash (using the same debugging session from the previously-attached backtrace).
So, I've traced this back a little ways.

Basically, we get into a situation where the first part of the frame tree looks like the diagram below:
       Block(div) <
          line <
            Block <
              line <
                Placeholder(div)(1) -- my mNextSibling points to Text(3), below
              line <
              line 0xb04ac290:
                Text(3) <
                  " "

In the diagram above, notice that there's a line between the Placeholder and its mNextSibling.  It contains an empty block, so it looks harmless, but it's actually a problem. 

Specifically, it causes problems at some later point when we move the third line into the Overflow-lines list via "PushLine".  That function clears out the mNextSibling pointer in the _previous line's final frame_.  In this case, that's the wrong mNextSibling pointer to clear -- we'd need to go back *two* lines to get the appropriate mNextSibling pointer.

So, I'm assuming that the situation I've diagrammed above should never happen (i.e. a frame and its mNextSibling should never have an unrelated line separating them). I'm now trying to figure out how we get in that situation.
(In reply to comment #10)
> In the diagram above, notice that there's a line between the Placeholder and
> its mNextSibling.  It contains an empty block, so it looks harmless, but it's
> actually a problem. 

If it's not clear: 
 By "It" in "It contains an empty block", I was referring to the second line -- the line that separates the placeholder from its sibling.
Also, if it's not clear: the crash ends up happening because:
 - PushLine doesn't clear the "correct" mNextSibling pointer, as I described above.
 - This leaves that pointer dangling.
 - We dereference this mNextSibling later on, after the text-frame it points to has died.
Attached patch patch v1 (obsolete) — Splinter Review
So I'm not 100% sure this is correct, but it fixes the problem described in comment 10, and it fixes the crash on all 3 testcases.

Basically, what was happening is this:
Context: We're inside a "Pull data from a next-in-flow" loop @ line 2073.
 1. We pull the first line, and we...
  1a. add its first child as our last child's nextSibling
  1b. append it to our lines list
 2. We pull the second line, and we...
  2a. add its first child as our last child's nextSibling
  2b. append it to our lines list

Step 2a effectively snips the first added line's contents out of the "mNextSibling" chain, while leaving it in the line list, separating sibling frames.  This gives us the situation described in comment 10.

This patch prevents that by updating the aState.mPrevChild pointer, so that in step 2a, we are *append* to the sibling list, rather than *snipping out* the last element as we were doing before.
Attachment #341398 - Flags: superreview?(roc)
Attachment #341398 - Flags: review?(roc)
Comment on attachment 341398 [details] [diff] [review]
patch v1

>+      // Add line to our line list, and set its last child as our new prev-child
>       if (aState.mPrevChild) {
>         aState.mPrevChild->SetNextSibling(toMove->mFirstChild);
>+        aState.mPrevChild = toMove->LastChild();
>       }

Two questions / thoughts:
1. Should the "aState.mPrevChild = toMove->LastChild();" line be moved below the "if" clause, so it happens unconditionally?  I tend to think it should, but I'm not sure if there's anything extra I'd need to do in those situations. (i.e. are there any other variables I'd need to set or tweak when initializing mPrevChild from null to a real value?)

2. Is it possible that toMove is an empty line (meaning LastChild() returns null)?  If so, I definitely wouldn't want to put that null value into aState.mPrevChild.  If that's a possibility, I'd want to store LastChild() in a temporary variable and null-check it before overwriting mPrevChild.
1. Yes, that should be enough.

2. Up above we checked toMove->GetChildCount() > 0, so it can't be empty here.
Attached patch patch v1aSplinter Review
(In reply to comment #14)
> 1. Should the "aState.mPrevChild = toMove->LastChild();" line be moved below
> the "if" clause, so it happens unconditionally?
(In reply to comment #15)
> 1. Yes, that should be enough.

Ok, thanks -- that's fixed in this version.

And thanks for the assurance on #2 -- I figured we were probably already checking for that, but I hadn't verified it yet.
Attachment #341398 - Attachment is obsolete: true
Attachment #341409 - Flags: superreview?(roc)
Attachment #341409 - Flags: review?(roc)
Attachment #341398 - Flags: superreview?(roc)
Attachment #341398 - Flags: review?(roc)
BTW: I'm not including crashtests in the patch so as not to give away sample exploit code while users are vulnerable.

/me toggles "in-testsuite?" as a reminder to land tests down the line.
Flags: in-testsuite?
Attachment #341409 - Flags: superreview?(roc)
Attachment #341409 - Flags: superreview+
Attachment #341409 - Flags: review?(roc)
Attachment #341409 - Flags: review+
Comment on attachment 341409 [details] [diff] [review]
patch v1a

Patch applies cleanly on 1.9.0.x branch -- requesting approval to land there.
Attachment #341409 - Flags: approval1.9.0.4?
Attachment #341409 - Flags: approval1.9.0.4?
Comment on attachment 341409 [details] [diff] [review]
patch v1a

Actually, canceling 1.9.0.x approval request, until after this patch lands on trunk (... after it gets post-1.9.1b1 approval, after such a flag is added to bugzilla. :))
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.4?
Whiteboard: [sg:critical] → [sg:critical][needs trunk landing before approval]
Flags: blocking1.9.0.4? → blocking1.9.0.4+
'patch v1a' pushed to mozilla-central as changeset 0f35e3ef60f1.
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical][needs trunk landing before approval] → [sg:critical]
Comment on attachment 341409 [details] [diff] [review]
patch v1a

Requesting approval to land on branch, now that this is on trunk.
Attachment #341409 - Flags: approval1.9.0.4?
Comment on attachment 341409 [details] [diff] [review]
patch v1a

Approved for, a=dveditz for release-drivers
Attachment #341409 - Flags: approval1.9.0.4? → approval1.9.0.4+
Attached patch crashtests patchSplinter Review
Here are crashtests for testcases 1-3, to land after this has been released on branch.
'patch v1a' landed on 1.9.0.x branch.

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.957; previous revision: 3.956
Verified crashing with Firefox 3.0.3 and the fix with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2008102104 GranParadiso/3.0.4pre.
Group: core-security
Flags: in-testsuite? → in-testsuite+
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090225 Firefox and : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre. No crahes with testcase.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre and  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre.
Crash Signature: [@ nsFrameManager::CaptureFrameStateFor]
You need to log in before you can comment on or make changes to this bug.