Closed Bug 815315 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout: Tables, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- affected
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: sec-other, testcase, Whiteboard: [adv-main20+])

Attachments

(2 files)

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),
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.
Assignee: nobody → matspal
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.
Attachment #685910 - Flags: review?(bzbarsky)
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
Attachment #685911 - Flags: review?(bzbarsky)
Comment on attachment 685910 [details] [diff] [review]
Avoid crashing if the last row-group child frame was removed

r=me
Attachment #685910 - Flags: review?(bzbarsky) → review+
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)?
Attachment #685911 - Flags: review?(bzbarsky) → review+
(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.
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.
Whiteboard: sec-other
https://hg.mozilla.org/mozilla-central/rev/6e5da7fd6740
https://hg.mozilla.org/mozilla-central/rev/aabc7c735fb0
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Keywords: sec-other
Whiteboard: sec-other
bug 812879 was 19 affected. Can I assume that this bug affected the shipped 19?
(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.
Whiteboard: [adv-main20+]
Group: core-security

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

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