Last Comment Bug 493649 - Crash [@ nsPropertyTable::PropertyList::Equals]
: Crash [@ nsPropertyTable::PropertyList::Equals]
Status: RESOLVED FIXED
[sg:critical?]
: 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)
Mentors:
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: ---
.2+
.2-fixed


Attachments
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 Jesse Ruderman 2009-05-18 16:06:31 PDT
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.
Comment 1 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 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 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 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 Boris Zbarsky [:bz] (still a bit busy) 2009-06-05 15:27:03 PDT
Alternately, do that last reflow with aUnboundedLastColumn true.
Comment 6 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 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 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 1.9.1.0.

We should also take this for 1.9.0.12.
Comment 9 Daniel Veditz [:dveditz] 2009-06-08 14:20:43 PDT
If this makes 1.9.1 we'll take it in 1.9.0.12, otherwise 1.9.0.13 would match up with 1.9.1.1
Comment 10 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 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 Damon Sicore (:damons) 2009-06-08 15:50:38 PDT
What's the risk?
Comment 13 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 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 1.9.1.1.  Sadly, I see no way to nominate for 1.9.0.13.
Comment 15 Samuel Sidler (old account; do not CC) 2009-06-10 08:30:34 PDT
Boris: When we're fully done with 1.9.0.11, the 1.9.0.13 flag will be created.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2009-06-11 10:13:51 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/740fcf842fab as usual without test...
Comment 17 Daniel Veditz [:dveditz] 2009-06-12 10:18:30 PDT
1.9.0.13 flags created, you can move the approval request now.
Comment 18 Daniel Veditz [:dveditz] 2009-07-10 11:30:23 PDT
Comment on attachment 381890 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.13, a=dveditz for release-drivers
Comment 19 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 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 1.9.1.2, we'd take this patch for that release, so a=beltzner for mozilla-1.9.1 landing ASAP.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2009-07-22 12:52:16 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/92b0a8e3aab4
Comment 22 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 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:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Comment 24 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 1.9.0.13.
Comment 25 Mats Palmgren (:mats) 2013-05-19 09:36:20 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df654c0503a3
Comment 26 Phil Ringnalda (:philor) 2013-05-19 17:48:58 PDT
https://hg.mozilla.org/mozilla-central/rev/df654c0503a3

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