Last Comment Bug 493649 - Crash [@ nsPropertyTable::PropertyList::Equals]
: Crash [@ nsPropertyTable::PropertyList::Equals]
: crash, fixed1.9.0.14, testcase, verified1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Mac OS X
-- critical (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Jet Villegas (:jet)
Depends on:
Blocks: randomstyles
  Show dependency treegraph
Reported: 2009-05-18 16:06 PDT by Jesse Ruderman
Modified: 2013-05-19 17:48 PDT (History)
15 users (show)
roc: wanted1.9.1+
mbeltzner: blocking1.9.1.1-
samuel.sidler+old: wanted1.9.1.x+
dveditz: blocking1.9.0.14+
samuel.sidler+old: wanted1.9.0.x+
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (154 bytes, text/html)
2009-05-18 16:06 PDT, Jesse Ruderman
no flags Details
Proposed fix (1.46 KB, patch)
2009-06-05 18:46 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
mbeltzner: approval1.9.1.2+
dveditz: approval1.9.0.14+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2009-05-18 16:06:31 PDT
Created attachment 378166 [details]

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.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-02 14:34:25 PDT
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.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-03 11:27:33 PDT
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?
Comment 3 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-03 16:22:53 PDT
Yes, that's definitely a bug. We should be overflowing to the right.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-05 15:21:49 PDT
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?
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-05 15:27:03 PDT
Alternately, do that last reflow with aUnboundedLastColumn true.
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-06-05 15:49:09 PDT
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.
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-05 18:46:11 PDT
Created attachment 381890 [details] [diff] [review]
Proposed fix

Patch doesn't include a test, because the bug is security-sensitive... :(
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-05 19:14:30 PDT
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

We should also take this for
Comment 9 User image Daniel Veditz [:dveditz] 2009-06-08 14:20:43 PDT
If this makes 1.9.1 we'll take it in, otherwise would match up with
Comment 10 User image Damon Sicore (:damons) 2009-06-08 15:42:44 PDT
Boris, to be clear, do you think we should hold 1.9.1 for this?
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-08 15:46:54 PDT
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.)
Comment 12 User image Damon Sicore (:damons) 2009-06-08 15:50:38 PDT
What's the risk?
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-08 15:54:38 PDT
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 14 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-10 08:16:07 PDT
Comment on attachment 381890 [details] [diff] [review]
Proposed fix

Talked to beltzner; we're going to do this in  Sadly, I see no way to nominate for
Comment 15 User image Samuel Sidler (old account; do not CC) 2009-06-10 08:30:34 PDT
Boris: When we're fully done with, the flag will be created.
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2009-06-11 10:13:51 PDT
Pushed as usual without test...
Comment 17 User image Daniel Veditz [:dveditz] 2009-06-12 10:18:30 PDT flags created, you can move the approval request now.
Comment 18 User image Daniel Veditz [:dveditz] 2009-07-10 11:30:23 PDT
Comment on attachment 381890 [details] [diff] [review]
Proposed fix

Approved for, a=dveditz for release-drivers
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2009-07-10 23:07:56 PDT
Checking in layout/generic/nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.54; previous revision: 3.53
Comment 20 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-07-21 17:24:33 PDT
Comment on attachment 381890 [details] [diff] [review]
Proposed fix

While this doesn't block, we'd take this patch for that release, so a=beltzner for mozilla-1.9.1 landing ASAP.
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2009-07-22 12:52:16 PDT
Comment 22 User image Al Billings [:abillings] 2009-07-27 13:41:34 PDT
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?
Comment 23 User image juan becerra [:juanb] 2009-07-30 16:18:43 PDT
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: Gecko/20090729 Firefox/3.5.2
Comment 24 User image Al Billings [:abillings] 2009-08-18 17:21:50 PDT
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
Comment 25 User image Mats Palmgren (:mats) 2013-05-19 09:36:20 PDT
Crash test:
Comment 26 User image Phil Ringnalda (:philor) 2013-05-19 17:48:58 PDT

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