Last Comment Bug 452157 - Crash [@ nsFrameManager::CaptureFrameStateFor] with -moz-column, float
: Crash [@ nsFrameManager::CaptureFrameStateFor] with -moz-column, float
Status: VERIFIED FIXED
: [sg:critical]
: crash, regression, testcase, verified1.9.0.4, verified1.9.1
Product: Core
Classification: Components
Component: Layout
: Trunk
: All All
: P3 critical (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
: layout
:
:
: 306939
  Show dependency treegraph
 
Reported: 2008-08-25 18:20 PDT by Jesse Ruderman
Modified: 2009-02-25 16:55 PST (History)
9 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.4+
samuel.sidler+old: wanted1.9.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
[@ nsFrameManager::CaptureFrameStateFor]


Attachments
testcase 1 (crashes Firefox when loaded) (325 bytes, text/html)
2008-08-25 18:20 PDT, Jesse Ruderman
no flags Details
testcase 2 (crashes Firefox when loaded) (596 bytes, text/html)
2008-10-01 17:00 PDT, Daniel Holbert [:dholbert]
no flags Details
testcase 3 (crashes Firefox when loaded) (602 bytes, text/html)
2008-10-01 17:13 PDT, Daniel Holbert [:dholbert]
no flags Details
backtrace of crash on testcase 3 (8.95 KB, text/plain)
2008-10-01 17:25 PDT, Daniel Holbert [:dholbert]
no flags Details
frametree at crash on testcase 3 (2.23 KB, text/plain)
2008-10-01 17:27 PDT, Daniel Holbert [:dholbert]
no flags Details
patch v1 (944 bytes, patch)
2008-10-01 21:57 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch v1a (944 bytes, patch)
2008-10-02 00:41 PDT, Daniel Holbert [:dholbert]
roc: review+
roc: superreview+
dveditz: approval1.9.0.4+
Details | Diff | Splinter Review
crashtests patch (2.58 KB, patch)
2008-10-13 12:58 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-08-25 18:20:18 PDT
Created attachment 335465 [details]
testcase 1 (crashes Firefox when loaded)

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.
Comment 1 Boris Zbarsky (:bz) 2008-08-28 10:14:15 PDT
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<
                    "\n\n"
                  >
                  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<
                    "\n\n"
                  >
                >
                Float-list<
                  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<
                              " "
                            >
                          >
                          Float-list<
                            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.
Comment 2 Boris Zbarsky (:bz) 2008-08-28 10:16:09 PDT
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.
Comment 3 Boris Zbarsky (:bz) 2008-08-28 10:16:37 PDT
And yes, this looks exploitable.
Comment 4 Daniel Holbert [:dholbert] 2008-10-01 15:54:01 PDT
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
Comment 5 Daniel Holbert [:dholbert] 2008-10-01 16:02:27 PDT
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:1.9.0.3) 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:1.8.1.17) Gecko/20080829 Firefox/2.0.0.17
Comment 6 Daniel Holbert [:dholbert] 2008-10-01 17:00:28 PDT
Created attachment 341365 [details]
testcase 2 (crashes Firefox when loaded)

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")
Comment 7 Daniel Holbert [:dholbert] 2008-10-01 17:13:12 PDT
Created attachment 341370 [details]
testcase 3 (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.)
Comment 8 Daniel Holbert [:dholbert] 2008-10-01 17:25:25 PDT
Created attachment 341371 [details]
backtrace of crash on testcase 3
Comment 9 Daniel Holbert [:dholbert] 2008-10-01 17:27:01 PDT
Created attachment 341372 [details]
frametree at crash on testcase 3

This is the frametree at the time of crash (using the same debugging session from the previously-attached backtrace).
Comment 10 Daniel Holbert [:dholbert] 2008-10-01 21:05:41 PDT
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:
     ColumnSet(div)<
       Block(div) <
          line <
            Block <
              line <
                Placeholder(div)(0)
                Placeholder(div)(1) -- my mNextSibling points to Text(3), below
              >
              line <
                Block(div)(2)
                >
              >
              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.
Comment 11 Daniel Holbert [:dholbert] 2008-10-01 21:07:41 PDT
(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.
Comment 12 Daniel Holbert [:dholbert] 2008-10-01 21:10:40 PDT
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.
Comment 13 Daniel Holbert [:dholbert] 2008-10-01 21:57:50 PDT
Created attachment 341398 [details] [review]
patch v1

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.
Comment 14 Daniel Holbert [:dholbert] 2008-10-01 22:07:14 PDT
Comment on attachment 341398 [details] [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.
Comment 15 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-10-01 22:51:44 PDT
1. Yes, that should be enough.

2. Up above we checked toMove->GetChildCount() > 0, so it can't be empty here.
Comment 16 Daniel Holbert [:dholbert] 2008-10-02 00:41:32 PDT
Created attachment 341409 [details] [review]
patch v1a

(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.
Comment 17 Daniel Holbert [:dholbert] 2008-10-02 00:43:27 PDT
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.
Comment 18 Daniel Holbert [:dholbert] 2008-10-02 04:22:28 PDT
Comment on attachment 341409 [details] [review]
patch v1a

Patch applies cleanly on 1.9.0.x branch -- requesting approval to land there.
Comment 19 Daniel Holbert [:dholbert] 2008-10-02 16:09:08 PDT
Comment on attachment 341409 [details] [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. :))
Comment 20 Daniel Holbert [:dholbert] 2008-10-10 17:27:16 PDT
'patch v1a' pushed to mozilla-central as changeset 0f35e3ef60f1.
Comment 21 Daniel Holbert [:dholbert] 2008-10-10 17:28:37 PDT
Comment on attachment 341409 [details] [review]
patch v1a

Requesting approval to land on branch, now that this is on trunk.
Comment 22 Daniel Veditz 2008-10-13 11:39:02 PDT
Comment on attachment 341409 [details] [review]
patch v1a

Approved for 1.9.0.4, a=dveditz for release-drivers
Comment 23 Daniel Holbert [:dholbert] 2008-10-13 12:58:30 PDT
Created attachment 342927 [details] [review]
crashtests patch

Here are crashtests for testcases 1-3, to land after this has been released on branch.
Comment 24 Daniel Holbert [:dholbert] 2008-10-13 13:37:53 PDT
'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
done
Comment 25 Al Billings [:abillings] 2008-10-21 15:35:02 PDT
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:1.9.0.4pre) Gecko/2008102104 GranParadiso/3.0.4pre.
Comment 26 Marcia Knous [:marcia] 2009-02-25 16:55:39 PST
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.

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