Closed Bug 493649 Opened 15 years ago Closed 15 years ago

Crash [@ nsPropertyTable::PropertyList::Equals]

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking1.9.1 --- .2+
status1.9.1 --- .2-fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files)

testcase
154 bytes, text/html
Details
Proposed fix
1.46 KB, patch
roc
: review+
roc
: superreview+
beltzner
: approval1.9.1.2+
dveditz
: approval1.9.0.14+
Details | Diff | Splinter Review
Attached file 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.
Whiteboard: [sg:critical?]
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.
Blocks: 474075
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.
Attached patch Proposed fix — — Splinter Review 
Patch doesn't include a test, because the bug is security-sensitive... :(
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #381890 - Flags: superreview?(roc)
Attachment #381890 - Flags: review?(roc)
No longer blocks: 474075
Attachment #381890 - Flags: superreview?(roc)
Attachment #381890 - Flags: superreview+
Attachment #381890 - Flags: review?(roc)
Attachment #381890 - Flags: review+
Attachment #381890 - Flags: approval1.9.1?
Attachment #381890 - Flags: approval1.9.0.12?
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.
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12?
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
Flags: blocking1.9.1?
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.
Flags: blocking1.9.1? → wanted1.9.1+
Attachment #381890 - Flags: approval1.9.1?
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.
Boris: When we're fully done with 1.9.0.11, the 1.9.0.13 flag will be created.
Pushed http://hg.mozilla.org/mozilla-central/rev/740fcf842fab as usual without test...
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: blocking1.9.0.12? → blocking1.9.0.13+
1.9.0.13 flags created, you can move the approval request now.
Attachment #381890 - Flags: approval1.9.0.12? → approval1.9.0.13?
Flags: blocking1.9.1.1?
Comment on attachment 381890 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.13, a=dveditz for release-drivers
Attachment #381890 - Flags: approval1.9.0.13? → approval1.9.0.13+
Attachment #381890 - Flags: approval1.9.1.2?
Checking in layout/generic/nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.54; previous revision: 3.53
Keywords: fixed1.9.0.13
Attachment #381890 - Flags: approval1.9.1.2? → approval1.9.1.1?
blocking1.9.1: --- → .2+
Flags: blocking1.9.1.1? → blocking1.9.1.1-
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.
Attachment #381890 - Flags: approval1.9.1.1? → approval1.9.1.2+
blocking1.9.1: .2+ → .3+
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:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Keywords: verified1.9.1
blocking1.9.1: .3+ → .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 1.9.0.13.
Group: core-security
Crash Signature: [@ nsPropertyTable::PropertyList::Equals]
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df654c0503a3
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: