Closed Bug 487899 Opened 11 years ago Closed 11 years ago

nsCellMap::ExpandWithCells is broken if passed-in array has more than one frame

Categories

(Core :: Layout: Tables, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

If one calls InsertFrames on a table row frame and passes in more than one frame in the frame list (as my fix for bug 487449 will do for appends in some cases), the cell map gets screwed up.  Let's consider the simple example of two frames passed in.  Then the array passed to ExpandWithCells will have length 2.  For simplicity, assume there is nothing in the row yet, so that aColIndex == 0, and that both cells have colspan == 1, rowspan == 1.

For the first cell, the current code will compute startColIndex == 0 and endColIndex == 0.  The row is empty, so insertionIndex == row.Length() == 0 <= aColIndex.  So we leave insertionIndex as-is.  Then we make an InsertElementsAt call on the row, passing 0 as the index and 1 as the number of elements to insert to insert a single entry in the array.  We create a celldata for the first cell, and put it in that entry.

For the second cell, startColIndex == 1 and endColIndex == 1.  The row has Length() == 1 > aColIndex == 0, so we set insertionIndex to 0.  We make a call to InsertElementsAt with 0 as the index and 1 - 0 + 1 == 2 as the count.  We create a celldata for the second cell and put it at index 0 in the array, because the colX loop starts at aColIndex != startColIndex.  Then we do nothing at index 1 in the array, because colX == startColIndex at that point.

The result is an array with three entries, not two.  The first entry is for the second cell, the second entry is null, the third entry is for the first cell.  The correct result would be two entries: first for the first cell, second for the second cell.

The fix is pretty simple: use startColIndex instead of aColIndex at all points where we're comparing to endColIndex.  This means the colX loop and the insertionIndex calculation.

Note that if there's only one cell in the array passed in (which is always the case right now for InsertFrames, I believe), this problem never arises, since for the first cell aColIndex == startColIndex in this function.  I bet our gcov data shows the |colx > startColIndex| if body as not covered in tests as a result, but that data seems to have gone away, so I can't confirm it.  :(
Blocks: 487449
Attached patch Proposed fixSplinter Review
No tests here, since I don't think there's any way to trigger the buggy code at the moment, but reftests crash with the patch for bug 487449 without this fix, so it'll be tested just fine.
Attachment #372184 - Flags: superreview?(roc)
Attachment #372184 - Flags: review?(bernd_mozilla)
Attachment #372184 - Flags: superreview?(roc) → superreview+
Comment on attachment 372184 [details] [diff] [review]
Proposed fix

Yes there is no way to test this, as otherwise we would crash. There hasn't been a crash since a long time.
Attachment #372184 - Flags: review?(bernd_mozilla) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/52ddf032257c
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.