Closed Bug 815315 Opened 7 years ago Closed 7 years ago
Table Frame doesn't deal with row-group continuations very well
940 bytes, patch
|Details | Diff | Splinter Review|
4.31 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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.
You need to log in before you can comment on or make changes to this bug.