Last Comment Bug 422283 - Crash [@ nsContainerFrame::ReflowOverflowContainerChildren] with -moz-column
: Crash [@ nsContainerFrame::ReflowOverflowContainerChildren] with -moz-column
Status: VERIFIED FIXED
: [sg:critical] post 1.8-branch
: assertion, crash, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: Layout
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Mozilla Corporation)
: layout
:
:
: 331889 422301
  Show dependency treegraph
 
Reported: 2008-03-11 17:36 PDT by Jesse Ruderman
Modified: 2009-02-03 17:39 PST (History)
14 users (show)
roc: blocking1.9.1+
roc: blocking1.9-
dveditz: blocking1.9.0.6+
roc: wanted1.9.0.x+
asac: wanted1.8.1.x-
asac: wanted1.8.0.x-
roc: in‑testsuite+
See Also:
Crash Signature:
[@ nsContainerFrame::ReflowOverflowContainerChildren]


Attachments
testcase (203 bytes, text/html)
2008-03-11 17:36 PDT, Jesse Ruderman
no flags Details
fix (8.09 KB, patch)
2008-07-23 21:38 PDT, Robert O'Callahan (:roc) (Mozilla Corporation)
fantasai.bugs: review-
Details | Diff | Splinter Review
fix v2 (6.47 KB, patch)
2008-08-11 20:42 PDT, Robert O'Callahan (:roc) (Mozilla Corporation)
no flags Details | Diff | Splinter Review
fix v3 (6.46 KB, patch)
2008-08-12 17:32 PDT, Robert O'Callahan (:roc) (Mozilla Corporation)
fantasai.bugs: review+
matspal: superreview+
Details | Diff | Splinter Review
1.9.0 patch (7.39 KB, patch)
2008-12-17 20:33 PST, Robert O'Callahan (:roc) (Mozilla Corporation)
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-03-11 17:36:10 PDT
Created attachment 308768 [details]
testcase

Loading the testcase (in a Mac trunk debug build) triggers:

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 5366

###!!! ASSERTION: can't find deleted frame in lines: 'Error', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 5346

Crash:
* [@ nsContainerFrame::ReflowOverflowContainerChildren] calling 0xdddddddd
* [@ nsOverflowContinuationTracker::StepForward] dereferencing 0xddddde01
Comment 1 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-07-23 20:14:47 PDT
During reflow, one of the body columns ends up in this state, when reflow has moved on to the next column:

            Block(body)(1)@0xcb4c68 next=0xcb4e98 prev-in-flow=0xcb4c10 next-in-flow=0xcb4e98 {2054,0,67,9106} [state=0010000c] [overflow=0,0,320,10152] sc=0xc99ccc(i=0,b=2) pst=:-moz-column-content<
              line 0xcb4e70: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4008] {0,0,67,9000} <
                Block(div)(0)@0xcb4ce8 next=0xcb4f70 prev-in-flow=0xc9a978 next-in-flow=0xcb4dc0 {0,0,67,9000} [state=02100004] sc=0xc99d1c(i=1,b=0)<
                  line 0xc9a3e0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4300] {0,0,0,0} <
                    Inline(span)(3)@0xc9a240 {0,-720,0,960} [content=0x19559260] [sc=0xc99fcc]<
                      Text(0)@0xc9a278[0,1,T]  {0,720,0,0} [state=08600000] [content=0x195592c0] sc=0xc9a078 pst=:-moz-non-element<
                        "\n"
                      >
                    >
                  >
                  Overflow-list<
                    Text(4)@0xc9a2b8[0,2,F]  next-continuation=0xc9a834 {0,96,426,960} [state=13600000] [content=0x195592f0] sc=0xc9a108 pst=:-moz-non-element<
                      "a "
                    >
                  >
                >
              >
              line 0xcb5340: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4108] {0,9000,67,0} ca={0,9000,320,1152} <
                Block(div)(0)@0xcb4f70 prev-in-flow=0xcb4dc0 next-in-flow=0xcb5368 {0,9000,67,0} [state=0210000c] [overflow=0,0,320,1152] sc=0xc99d1c(i=1,b=0)<
                  line 0xc9a878: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x4120] {0,0,320,1152} <
                    Text(4)@0xc9a834[2,2,F]  prev-continuation=0xc9a2b8 next-continuation=0xc9a8a0 {0,96,320,960} [state=13600004] [content=0x195592f0] sc=0xc9a108 pst=:-moz-non-element<
                      "! "
                    >
                  >
                  Overflow-list<
                    Text(4)@0xc9a8a0[4,2,F]  prev-continuation=0xc9a834 next-continuation=0xc9a90c {0,1248,480,960} [state=13600004] [content=0x195592f0] sc=0xc9a108 pst=:-moz-non-element<
                      "b "
                    >
                  >
                >
              >
              ExcessOverflowContainers-list<
                Block(div)(0)@0xcb4dc0 prev-in-flow=0xcb4ce8 next-in-flow=0xcb4f70 {0,0,67,0} [state=0010048c] [overflow=0,0,426,1152] sc=0xc99d1c(i=0,b=0)<
                >
              >
            >

This is bad.
Comment 2 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-07-23 20:59:00 PDT
Actually, getting into that state is quite simple.

Block 0xcb4c68 reflows its child 0xcb4ce8. 0xcb4ce8 is complete but it has overflowing children. So we create an overflow container next-in-flow for it (0xcb4dc0) and put that on our ExcessOverflowContainersList. So far, so good.

But then block 0xcb4c68 reflow keeps going! It looks for more content and in this case it looks through its own next-in-flows, finds block 0xcb4f70 (block 0xcb4dc0's next in flow) and pulls it in and reflows it as a regular child. This is really bad, it should not be touching any of the next-in-flows of 0xcb4dc0 now that that's been made into an overflow container.
Comment 3 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-07-23 21:09:55 PDT
I think we should establish the invariant that an overflow container's next-in-flows are also overflow containers.

One way that might work is, when we make a normal frame into an overflow container, we turn all its next-in-flows into overflow containers and move them to the same ExcessOverflowContainers list as the first one.
Comment 4 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-07-23 21:38:03 PDT
Created attachment 331046 [details] [review]
fix

This does that.

It also includes the fix for bug 422301, modified as fantasai requested.
Comment 5 fantasai 2008-07-25 00:17:00 PDT
Comment on attachment 331046 [details] [review]
fix

Is there any reason the loop to pull in NIFs shouldn't be inside nsOverflowTracker::Insert? Because if not, then it should be there.
Comment 6 Mats Palmgren [:mats] 2008-08-07 03:48:34 PDT
Comment on attachment 331046 [details] [review]
fix

Clearing request, I assume there will be a new patch due to the review-.
Comment 7 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-08-11 20:42:37 PDT
Created attachment 333352 [details] [review]
fix v2

fantasai, is this what you had in mind?
Comment 8 fantasai 2008-08-12 02:45:15 PDT
Yes. That part looks good to me now. I have some concern with this part, though:

+  for (nsIFrame* f = aChild; f; f = f->GetNextInFlow()) {
+    if (f == mSentry) {
+      // Make sure we drop all references if this was the only frame
+      // in the overflow containers list
       ....
+      }
+      break;
     }
   }

I believe that should be

+  for (nsIFrame* f = aChild; f; f = f->GetNextInFlow()) {
+    if (f == mSentry) {
+      // Make sure we drop all references if this was the only frame
+      // in the overflow containers list
       ....
+      }
     }
+    else
+      break;
   }

If I understand the problem Bernd was describing correctly, the loop is to make sure mSentry isn't holding any next-in-flows. Breaking as soon as we match mSentry wouldn't handle the case that the next frame in the list is one of these next-in-flows. As for making sure the next-in-flows later in the list are taken out of the overflow container list when deleted, DeleteNextInFlowChild already handles that. It's only mSentry we need to worry about here.
Comment 9 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-08-12 03:09:35 PDT
Hmm. Then I don't think we should break at all in this loop, unless we reach the part which sets mSentry to null in which case we can be sure we've no more work to do.
Comment 10 fantasai 2008-08-12 04:33:56 PDT
That works for me.
Comment 11 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-08-12 17:32:33 PDT
Created attachment 333482 [details] [review]
fix v3
Comment 12 Mats Palmgren [:mats] 2008-09-04 11:09:15 PDT
Comment on attachment 333482 [details] [review]
fix v3

Looks good to me, fwiw.  sr=mats
Comment 13 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-09-06 01:48:37 PDT
Pushed 23b2ad006a98.
Comment 14 Samuel Sidler (old account; do not CC) 2008-12-17 18:01:27 PST
roc: Can you please attach a 1.9.0 branch patch for this bug?
Comment 15 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-12-17 20:33:26 PST
Created attachment 353617 [details] [review]
1.9.0 patch

I confirmed this fixes the bug on 1.9.0.
Comment 16 Daniel Veditz 2008-12-19 11:44:26 PST
Comment on attachment 353617 [details] [review]
1.9.0 patch

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 17 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-07 17:00:41 PST
Checked in.
Comment 18 Alexander Sack 2009-01-08 06:18:05 PST
seems to be 1.9 and later only. If not flip wanted flag please.
Comment 19 Al Billings [:abillings] 2009-01-14 13:15:11 PST
Verified fixed for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre.
Comment 20 Tony Chung [:tchung] 2009-01-30 17:40:30 PST
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090130 Minefield/3.2a1pre

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