multiple tfoot cellmap crash [@ nsCellMap::GetCellInfoAt]

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Bernd, Assigned: Bernd)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Windows XP
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

854 bytes, text/html
Details
14.76 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
we access deleted memory the testcase "works" only with latest trunk
(Assignee)

Comment 1

11 years ago
Created attachment 227512 [details]
testcase

lithium is great :-)
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED

Updated

11 years ago
Keywords: crash
Summary: multiple tfoot cellmap crash → multiple tfoot cellmap crash [@ nsCellMap::GetCellInfoAt]
(Assignee)

Comment 2

11 years ago
Created attachment 228063 [details] [diff] [review]
patch

Boris, this another iteration over the patch in bug 339130, I will not promise that it is the last one but the crash frequency is dropping rapidly.
Attachment #228063 - Flags: superreview?(bzbarsky)
Attachment #228063 - Flags: review?(bzbarsky)

Comment 3

11 years ago
I want some documentation about what the new args to this function mean.  I can't even really review this without that documentation...
(Assignee)

Comment 4

11 years ago
Created attachment 231597 [details] [diff] [review]
revised patch
Attachment #228063 - Attachment is obsolete: true
Attachment #231597 - Flags: superreview?(bzbarsky)
Attachment #231597 - Flags: review?(bzbarsky)
Attachment #228063 - Flags: superreview?(bzbarsky)
Attachment #228063 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

11 years ago
The problem is  that a table can have only one footer all other tfoot tags are treated as tbodies. If you remove one tfoot one the previuosly treated as tbody tfoot tags becomes a really footer. This changes its place in the frame list. Every row has a row index which shows its place in the table. If the above described change happens the rowindices become wrong. They need to be reset. This is what ResetRowIndices does. However if we insert a new tfoot tag then we need to find the new rowindices for the insert. When we determine the position of the rows we need the correct order of the rowgroups (OrderRowGroups) the new tfoot will be already in the list of rowgroups but the reset should be done only on the previuosly existing rowgroups. Then we will find the correct startindex and adjust all following rowgroups and their rows.


https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.cpp_sec1
 moves the AdjustRowIndices to the rowgroup where it belongs and makes the return value void.

Further it add two args to  ResetRowIndices that are the frames that should be omitted from the reset as they will be inserted later.


https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.cpp_sec2

Is similiar to the thing that we have done at bug 343778, if we add a cell to the cellmap it is possible that the number of columns shrinks.


https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.cpp_sec3
https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.cpp_sec4

Does what has been described in the intro, we need to reset the rowindices earlier than I previously thought just before we insert the new data into the cellmap. So we split the routine and insert the ResetRowIndices together with a cellmap sync.


https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.cpp_sec5

In order to get the column information right we need to have the correct cellmap information before. As this pretty difficult to get correctly we nuke the cellmap and build it new.


https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.h_sec1
https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableFrame.h_sec2

document the routines and the args


https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableRowGroupFrame.cpp_sec1
https://bugzilla.mozilla.org/attachment.cgi?id=231597&action=diff#mozilla/layout/tables/nsTableRowGroupFrame.h_sec1

shift the routine where it belongs

(Assignee)

Updated

11 years ago
Blocks: 346980

Comment 6

11 years ago
Comment on attachment 231597 [details] [diff] [review]
revised patch

r+sr=bzbarsky.  Thanks for the explanation; that made this much happier!
Attachment #231597 - Flags: superreview?(bzbarsky)
Attachment #231597 - Flags: superreview+
Attachment #231597 - Flags: review?(bzbarsky)
Attachment #231597 - Flags: review+
(Assignee)

Comment 7

11 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 8

11 years ago
By the way, do we need anything like this for thead?
(Assignee)

Comment 9

11 years ago
thead/tfoot are handled similiar when they appear multiple times. So my language is not very precise in this bug but I believe what I did here helps also with multiple theads

Comment 10

11 years ago
Ah, ok.  Most excellent. 

Updated

11 years ago
Blocks: 347796

Updated

11 years ago
No longer blocks: 347796
Depends on: 347796

Updated

11 years ago
Depends on: 347725
Flags: blocking1.8.0.8?
Whiteboard: [sg:critical]
(Assignee)

Comment 11

11 years ago
fixed on branches by the cellmap branch patch
Keywords: fixed1.8.0.7, fixed1.8.1

Comment 12

11 years ago
v.fixed on both branches, no crash with testcase.
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060906 Firefox/1.5.0.7
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060907 BonEcho/2.0b2

However, I also do not crash with 1.5.0.6 or 2.0b1, so will leave it to Bernd to explain why that might be. I think he mentioned the crash depended on other bugs/patches...
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
The 1.8.0.8 blocking nomination got lost in a bugzilla accident, but I think it was left over from when it looked like the cellmap fixes weren't going to make 1.8.0.7. Please re-nominate if I'm missing something
Flags: blocking1.8.0.7+
Group: security
Flags: in-testsuite?

Comment 14

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/34a34a0b550e
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCellMap::GetCellInfoAt]
You need to log in before you can comment on or make changes to this bug.