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

RESOLVED FIXED in Firefox 20

Status

()

Core
Layout: Tables
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({sec-other, testcase})

Trunk
mozilla20
sec-other, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox19 affected, firefox20 fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main20+])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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),
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
Created attachment 685910 [details] [diff] [review]
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.
Attachment #685910 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 685911 [details] [diff] [review]
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
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+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/6e5da7fd6740
https://hg.mozilla.org/mozilla-central/rev/aabc7c735fb0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox20: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

5 years ago
status-firefox-esr10: --- → unaffected
status-firefox-esr17: --- → unaffected

Updated

5 years ago
Keywords: sec-other
Whiteboard: sec-other

Updated

5 years ago
status-b2g18: --- → unaffected
bug 812879 was 19 affected. Can I assume that this bug affected the shipped 19?
(Assignee)

Comment 11

5 years ago
(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.
status-firefox19: --- → affected
Whiteboard: [adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.