815315 - nsTableFrame doesn't deal with row-group continuations very well

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Mats Palmgren (inactive)]

From bug 812879:

ASSERTION: invalid col index: 'Error', file layout/tables/nsTableFrame.cpp
ASSERTION: column frames out of sync with cell map: 'Error', file layout/tables/BasicTableLayoutStrategy.cpp

A related testcase triggers:

ASSERTION: overflow list w/o frames: 'mFrames.NotEmpty()', file layout/generic/nsContainerFrame.cpp
Assertion failure: i < Length() (invalid array index),

Comment 1

[Mats Palmgren (inactive)]

The assertions above are for the testcases in bug 812879.  I'll attach a low-risk
patch there to avoid the underlying issues of the assertions.  I'll use this bug to
fix them properly.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Avoid crashing if the last row-group child frame was removed

This caused the "Assertion failure: i < Length() (invalid array index)"
for "maps.ElementAt(mapIndex)" with mapIndex==-1 in the code below.
Maybe we shouldn't call RemoveFrame with the last row-group child in
the first place, but better safe than sorry.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Make nsTableFrame::RemoveFrame remove all continuations of the removed child frame too

A few notes:
1. make nsTableFrame::RemoveFrame remove all continuations for the
   given child frame too (as nsContainerFrame/BlockFrame does)
2. call DrainSelfOverflowList() for each new parent so that we'll
   use the correct list in DoRemoveFrame
3. 1+2 makes us call MatchCellMapToColCache on each removed
   row-group frame

1+2 is what caused "ASSERTION: overflow list w/o frames" since
we left a row-group on the overflow list.
3 is what caused "ASSERTION: invalid col index" and "ASSERTION:
column frames out of sync with cell map" because the table cellmap
was updated when we removed a row-group, but we left a remaining
row-group continuation in a table having a mismatching number of
frames in mColFrames.

https://tbpl.mozilla.org/?tree=Try&rev=b0f460c804e9

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 685910 [details] [diff] [review]
Avoid crashing if the last row-group child frame was removed

r=me

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 685911 [details] [diff] [review]
Make nsTableFrame::RemoveFrame remove all continuations of the removed child frame too

r=me, but does this make the cellmap sync stuff O(N^2)?

Comment 6

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Comment on attachment 685911 [details] [diff] [review]
> Make nsTableFrame::RemoveFrame remove all continuations of the removed child
> frame too
> 
> r=me, but does this make the cellmap sync stuff O(N^2)?

Yeah, I suspect the block containing "cellMap->Synchronize" can be done once
for each table parent, rather than for each row-group continuation. Part of it
can perhaps be done just once, since there is only one nsTableCellMap for all
table continuations.  I think table continuations in general are quite rare though
(only in Print mode so far) so let's do optimizations as a follow-up bug.
It'd be good to have a few days of baking this to make sure it's correct so far.

Comment 7

[Mats Palmgren (inactive)]

sec-other, since this bug is only hidden to not reveal details about bug 812879,
which fixed the root cause of the security issue.  I don't think we need these
patches on branches.

Comment 8

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5da7fd6740
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabc7c735fb0

Filed bug 817205 to follow-up on optimizing this a bit.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e5da7fd6740
https://hg.mozilla.org/mozilla-central/rev/aabc7c735fb0

Comment 10

[Al Billings [:abillings - ex-MoCo]]

bug 812879 was 19 affected. Can I assume that this bug affected the shipped 19?

Comment 11

[Mats Palmgren (inactive)]

(In reply to Al Billings [:abillings] from comment #10)
> bug 812879 was 19 affected. Can I assume that this bug affected the shipped
> 19?

Yes.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.