Created attachment 378166 [details] testcase Loading the testcase: ###!!! ASSERTION: reflow roots should never split: 'status == NS_FRAME_COMPLETE', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 7113 Closing window or quitting Firefox: Crash [@ nsPropertyTable::PropertyList::Equals] usually touching 0xddddde09.
When this happens, |target| is the viewport... The CanvasFrame contains only a placeholder; the columnset is on the fixed-list. And the columnset has a next-continuation.
Yeah, so the columnset just reports itself as not complete, then we split it, and have no idea what to do with the result. Given that its available height is unconstrained, should we really be reporting incomplete just because we didn't fit in our column count? roc?
Yes, that's definitely a bug. We should be overflowing to the right.
OK. So we call Reflow() on the columnset. It contains a single block with a single block child with a single line containing a single Text frame containing "\n\n\n\n" in it. We do a ReflowChildren with an unbounded aConfig.mColMaxHeight. It returns true, and a colData.mMaxHeight of 960. The frame tree now has a single child block in the column set (height 960) containing a single block child, containing 4 lines, each of which has a Text frame with "\n" in it. Each of the Text frames is height 960. Note that allFit is true, because aConfig.mColMaxHeight was unconstrained. That's why we return true. We then do a ReflowChildren with a mColMaxHeight of 959. Now the child claims to be incomplete, and the <body> block pushes all but one of the Text lines onto its overflow list. This time we get to a point where columnCount >= aConfig.mBalanceColCount (which is 3), and break out of the loop. feasible comes back false. The last Text frame is on the overflow list of the third blockframe for the <body>. Since (knownInfeasibleHeight >= knownFeasibleHeight - 1) we break out of the balancing loop. Since feasible was false, we do a final ReflowChildren at height 960 (our last knownFeasibleHeight), and with aUnboundedLastColumn = false. But knownFeasibleHeight sort of lied: we don't fit into height 960 while breaking into 3 columns. Or rather, if reflowed with a constrained height of 960, we do not fit into 3 columns. In any case, we do this last reflow, hit columnCount >= 3 again, claim to be incomplete and bail out. We leave that 4th Text frame on the block's overflow list, and put a block on our own overflow list. They stay there permanently, as far as I can see, and then frametree teardown is confused. I don't see any provisions for the columnset ever overflowing to the right here, except in cases where we're not balancing or cases where we balance and find unconstrained feasible height.... Perhaps the right thing to do is that if that last reflow at the known feasible height still returns infeasible (which I believe this one does) we should just fall back on doing one final reflow at the feasible height but with config.mBalanceColCount set to PR_INT32_MAX?
Alternately, do that last reflow with aUnboundedLastColumn true.
Oh, it's balancing, right. Yes, I think doing the last reflow with aUnboundedLastColumn is the right thing to do. The underlying issue is actually related to an issue I discovered in bug 495385. I'll comment more there.
Created attachment 381890 [details] [diff] [review] Proposed fix Patch doesn't include a test, because the bug is security-sensitive... :(
Comment on attachment 381890 [details] [diff] [review] Proposed fix I think it's worth getting this in on the 1.9.1 branch, though I can understand taking it after 188.8.131.52. We should also take this for 184.108.40.206.
If this makes 1.9.1 we'll take it in 220.127.116.11, otherwise 18.104.22.168 would match up with 22.214.171.124
Boris, to be clear, do you think we should hold 1.9.1 for this?
No, but I was hoping to get some eyes on the approval request... It seemed to have gotten lost. ("eyes" can mean a-, obviously; I would totally understand that; it just doesn't make any sense to take it later if we want to take it.)
What's the risk?
I think risk's pretty low; worst case is incorrect layout for columnated blocks where we put too much stuff in the last column instead of creating a new column.... but I can't think of a way that would happen with this patch. Well, I guess the really worst case is we trigger a different exploitable crash in columns code on some other testcase, but that's not much worse than what we have now without this patch. And I _really_ don't see a way that could happen.
Comment on attachment 381890 [details] [diff] [review] Proposed fix Talked to beltzner; we're going to do this in 126.96.36.199. Sadly, I see no way to nominate for 188.8.131.52.
Boris: When we're fully done with 184.108.40.206, the 220.127.116.11 flag will be created.
Pushed http://hg.mozilla.org/mozilla-central/rev/740fcf842fab as usual without test...
18.104.22.168 flags created, you can move the approval request now.
Comment on attachment 381890 [details] [diff] [review] Proposed fix Approved for 22.214.171.124, a=dveditz for release-drivers
Checking in layout/generic/nsColumnSetFrame.cpp; /cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v <-- nsColumnSetFrame.cpp new revision: 3.54; previous revision: 3.53
Comment on attachment 381890 [details] [diff] [review] Proposed fix While this doesn't block 126.96.36.199, we'd take this patch for that release, so a=beltzner for mozilla-1.9.1 landing ASAP.
In my debug 1.9.0 build from 7/6 on OS X, I can't reproduce this crash with the testcase attached. Is this crash 1.9.1 only?
Verified using test case in comment #0. 3.5 crashes. 3.5.2 does not: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:188.8.131.52) Gecko/20090729 Firefox/3.5.2
As I asked on 7/27, is the crash 1.9.1 only? I can't reproduce it with my debug OS X build of 184.108.40.206.