Crash [@ nsCachedStyleData::GetStyleData] called within nsTableColFrame::GetStyleWidth

RESOLVED FIXED

Status

()

Core
Layout: Tables
--
critical
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: Bernd)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] uses freed memory, crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Loading the testcase in a ***debug*** build of Firefox (on Mac) causes a crash in nsCachedStyleData.  The crash involves accessing memory at address 0xddddddfd, so this is probably a security hole.
(Reporter)

Comment 1

12 years ago
Created attachment 223427 [details]
testcase
(Reporter)

Comment 2

12 years ago
Created attachment 223428 [details]
stack trace
(Reporter)

Comment 3

12 years ago
Thread 0 Crashed:
nsCachedStyleData::GetStyleData(nsStyleStructID const&)
nsStyleContext::GetStyleData(nsStyleStructID)
nsIFrame::GetStyleData(nsStyleStructID) const
nsIFrame::GetStylePosition() const
nsTableColFrame::GetStyleWidth() const
BasicTableLayoutStrategy::AssignNonPctColumnWidths
BasicTableLayoutStrategy::Initialize
nsTableFrame::ReflowTable
nsTableFrame::Reflow
...
(Reporter)

Comment 4

12 years ago
A nightly (non-debug) build crashes on a slightly testcase in TableBackgroundPainter::PaintCell.  So bug 339165 is probably the same as this one.
Blocks: 339165
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

12 years ago
Status: REOPENED → NEW
(Reporter)

Updated

12 years ago
Blocks: 339170
(Assignee)

Comment 5

12 years ago
Created attachment 224595 [details] [diff] [review]
patch
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 6

12 years ago
Created attachment 224710 [details] [diff] [review]
expanded version

this is functional identical to attachment 224595 [details] [diff] [review] but it is less a hack.
Attachment #224710 - Flags: superreview?(bzbarsky)
Attachment #224710 - Flags: review?(bzbarsky)
So what does this patch actually change?
Summary: Crash [@ nsCachedStyleData::GetStyleData] called within nsTableColFrame::GetStyleWidth → Crash [@ nsCachedStyleData::GetStyleData] called within nsTableColFrame::GetStyleWidth
(Assignee)

Comment 8

12 years ago
Every column has its column index, which indicates where it is in table. Every column group has a start col index which shows the first index a column inside this colgroup should have. If we add or remove a col we need to adjust the colindizes of the following columns. There are two cases we need to shift the index from a given column and all following cols in that colgroup or we need to shift all columns in the colgroup. The problem that we encounter without the patch is that regardless whether we pass a column where we need to start or not we treat the first colgroup special. This is not a issue if we insert cols but hits if remove cols. The patch has one function if we adjust a whole rowgroup (2 args) and one function if we shift inside a rowgroup (additional col frame arg where to start).
I guess what I really don't understand are:

1)  Why are we no longer caling SetStartColumnIndex() on any colgroups?  Is this
    what actually fixes the bug?
2)  Since the two functions are basically identical except for what the first
    value of colFrame is, why are they separate functions instead of a single
    function with a null-check?
(Assignee)

Updated

12 years ago
Attachment #224710 - Attachment is obsolete: true
Attachment #224710 - Flags: superreview?(bzbarsky)
Attachment #224710 - Flags: superreview+
Attachment #224710 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #224710 - Flags: superreview+
(Assignee)

Comment 10

12 years ago
Comment on attachment 224595 [details] [diff] [review]
patch

asking for review on the short version.
Attachment #224595 - Flags: superreview?(bzbarsky)
Attachment #224595 - Flags: review?(bzbarsky)
Comment on attachment 224595 [details] [diff] [review]
patch

Fix the comment to explain what we're doing and why, ok?
Attachment #224595 - Flags: superreview?(bzbarsky)
Attachment #224595 - Flags: superreview+
Attachment #224595 - Flags: review?(bzbarsky)
Attachment #224595 - Flags: review+
(Assignee)

Comment 12

12 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:critical] uses freed memory
Affects 1.8 branches also

Updated

12 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Flags: blocking1.8.0.6? → blocking1.8.0.6+
(Assignee)

Comment 14

12 years ago
Created attachment 231420 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH
(Assignee)

Comment 15

12 years ago
Comment on attachment 231420 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH

thats the patch with comments that got checked into trunk
Attachment #231420 - Flags: approval1.8.1?
Attachment #231420 - Flags: approval1.8.0.7?

Updated

12 years ago
Attachment #231420 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 16

12 years ago
fixed on 1.8.1
Keywords: fixed1.8.1
Comment on attachment 231420 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #231420 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 18

12 years ago
fix checked into 1.8.0 branch
Keywords: fixed1.8.0.7
https://bugzilla.mozilla.org/attachment.cgi?id=223427
ff2b2 release|debug windows/linux/macppc no crash, 
verified fixed 1.8
Keywords: fixed1.8.1 → verified1.8.1

Comment 20

12 years ago
v.fixed on both branches with 8/24 nightly builds, no crash with testcase:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060824 Firefox/1.5.0.7pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2
Keywords: fixed1.8.0.7 → verified1.8.0.7

Comment 21

12 years ago
attachment 231420 [details] [diff] [review] alone does not fix this testcase nor the testcase from bug 339170 on 1.0.x branch.

I found that the cellmap patch (bug 346980) subsumes this patch, thus backported it completely, but without success.

Bernd et al, any idea what other dependency I might be missing here? The backtrace still looks similar:

#5  0xa5d69990 in nsCachedStyleData::GetStyleData (this=0xddddddf9, aSID=@0xafe59a04)
    at nsRuleNode.h:211
#6  0xa5d8082b in nsStyleContext::GetStyleData (this=0xdddddddd,
    aSID=eStyleStruct_Position) at nsStyleContext.cpp:248
#7  0xa5af191e in nsIFrame::GetStyleData (this=0x8945530, aSID=eStyleStruct_Position)
    at nsIFrame.h:600
#8  0xa5b027f9 in nsIFrame::GetStylePosition (this=0x8945530) at nsStyleStructList.h:82
#9  0xa5c28dab in nsTableColFrame::GetStyleWidth (this=0x8945530)
    at nsTableColFrame.cpp:110
#10 0xa5c193ba in BasicTableLayoutStrategy::AssignNonPctColumnWidths (this=0x8990808,
    aMaxWidth=18960, aReflowState=@0xafe59e10) at BasicTableLayoutStrategy.cpp:1051
#11 0xa5c16d2d in BasicTableLayoutStrategy::Initialize (this=0x8990808,
    aReflowState=@0xafe59e10) at BasicTableLayoutStrategy.cpp:127
#12 0xa5c30165 in nsTableFrame::ReflowTable (this=0x89404a4, aPresContext=0x890f6a0,
    aDesiredSize=@0xafe5a060, aReflowState=@0xafe59e10, aAvailHeight=1073741824,
    aReason=eReflowReason_Resize, aLastChildReflowed=@0xafe59ca4,
    aDoCollapse=@0xafe59d00, aDidBalance=@0xafe59cac, aStatus=@0xafe5a58c)
    at nsTableFrame.cpp:2218
#13 0xa5c2fa4e in nsTableFrame::Reflow (this=0x89404a4, aPresContext=0x890f6a0,
    aDesiredSize=@0xafe5a060, aReflowState=@0xafe59e10, aStatus=@0xafe5a58c)
    at nsTableFrame.cpp:2076
#14 0xa5b0fd0b in nsContainerFrame::ReflowChild (this=0x894034c, aKidFrame=0x89404a4,
    aPresContext=0x890f6a0, aDesiredSize=@0xafe5a060, aReflowState=@0xafe59e10, aX=0,
    aY=0, aFlags=3, aStatus=@0xafe5a58c) at nsContainerFrame.cpp:966
#15 0xa5c451ea in nsTableOuterFrame::OuterReflowChild (this=0x894034c,
    aPresContext=0x890f6a0, aChildFrame=0x89404a4, aOuterRS=@0xafe5a5c0,
    aMetrics=@0xafe5a060, aAvailWidth=18960, aDesiredSize=@0xafe5a0e0,
    aMargin=@0xafe5a0d0, aMarginNoAuto=@0xafe5a0c0, aPadding=@0xafe5a0b0,
    aReflowReason=eReflowReason_Incremental, aStatus=@0xafe5a58c,
    aNeedToReflowCaption=0xafe5a05c) at nsTableOuterFrame.cpp:1333

Group: security
Flags: in-testsuite?
(Reporter)

Comment 22

11 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCachedStyleData::GetStyleData]
You need to log in before you can comment on or make changes to this bug.