620312 - crash [@ nsTableFrame::InsertCol] because lastColGroup guard did not cover lastColGroup->GetColCount()

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[timeless]

579 void nsTableFrame::InsertCol(nsTableColFrame& aColFrame,

599             if (lastColGroup) {
600               lastColGroup->RemoveChild(*lastCol, PR_FALSE);
601             }
602             // remove the col group if it is empty
603             if (lastColGroup->GetColCount() <= 0) {
604               mColGroups.DestroyFrame((nsIFrame*)lastColGroup);

Comment 1

[timeless]

Attachment: patch

Comment 2

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1593777e94b8

Comment 3

[Mats Palmgren (inactive)]

Hmm, isn't this null check bogus?  How can we a col frame in 'mColFrames'
but no frames in 'mColGroups'?

Comment 4

[Bernd]

mColFrames is a col frame cache on the table frame which is only manually kept in sync with the frame structure.

Comment 5

[Mats Palmgren (inactive)]

Ok, so this is one of the places 'mColFrames' is kept in sync with the
frame tree.

There's only one caller of nsTableFrame::InsertCol and that is
nsTableColGroupFrame::AddColsToTable:
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableColGroupFrame.cpp#116
Since 'this' is non-null, it implies 'mColGroups' has at least one frame
in it, so 'lastColGroup' can never be null (unless 'mColGroups' is also
"out-of-sync" with the frame tree).

Furthermore, when a col-group frame is removed by nsTableFrame::RemoveFrame
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#2277
all col frames for it is explicitly removed from 'mColFrames'.
I added an invariant there that when the last frame is removed from
'mColGroups', 'mColFrames' is also empty -- it passed unit tests.

From reading the code that updates 'mColFrames' and 'mColGroups',
I don't see how they can be out-of-sync with the frame tree, except
temporarily in the methods that deals with inserting/removing frames but
those methods appears to update these members properly.
Am I missing something?