Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash in [@ nsVoidArray::Count] called by nsCellMap::RebuildConsideringRows

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
Layout: Tables
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Bernd)

Tracking

(Blocks: 1 bug, 4 keywords)

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] fixed rolled into 346980, crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

Created attachment 223207 [details]
stack

A few of the assertions leading up to the crash, the stack and some data.
Created attachment 223215 [details]
stack #2

The same call stack as above, but 'row' = 0xdadadada

Updated

11 years ago
Summary: Crash in [@ nsVoidArray::Count] → Crash in [@ nsVoidArray::Count] called by nsCellMap::RebuildConsideringRows

Comment 3

11 years ago
I'm about to attach a testcase that (on Mac) sometimes crashes [@ nsVoidArray::Count], sometimes crashes [@ nsCellMap::GetRowSpanForNewCell], and sometimes doesn't crash.  It also triggers the following assertion:

###!!! ASSERTION: SetDataAt called with row index > num rows: 'PR_FALSE', file /Users/admin/trunk/mozilla/layout/tables/nsCellMap.cpp, line 2401
Blocks: 339131
Keywords: testcase
OS: Linux → All
Hardware: PC → All

Comment 4

11 years ago
Created attachment 223437 [details]
testcase
(Assignee)

Comment 5

11 years ago
<table id="table">
 <thead></thead>
  <tfoot id="A"><tr><td>td</td></tr></tfoot>
  <tfoot id="B"><tr><td>td</td></tr></tfoot>
  <thead></thead>
</table>

produces the following cellmap

***** START TABLE CELL MAP DUMP ***** 02FEBF00
cols array orig/span-> 02FEBF000=2/0  cols in cache 1
  ***** START GROUP CELL MAP DUMP ***** 02FF5C58
  thead mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 02FF5CC0
  tfoot mapRowCount=1 tableRowCount=1
  row 0 : C0,0  
  C0,0=02FF51C4(0)  
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 02FF5E38
  thead mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 02FF5EA0
  tfoot mapRowCount=1 tableRowCount=1
  row 0 : C0,0  
  C0,0=02FF4EB4(0)  
  ***** END GROUP CELL MAP DUMP *****
***** END TABLE CELL MAP DUMP *****
 ***END TABLE DUMP*** 
 
 the dynamic case:
 
 function boom()
{
  var table = document.getElementById("table");
  table.insertBefore(document.createElement("thead"), document.getElementById("A"));
}
  
</script>
</head>
<body onload="boom();">

<table id="table">
  <tfoot id="A"><tr><td>td</td></tr></tfoot>
  <tfoot id="B"><tr><td>td</td></tr></tfoot>
  <thead></thead>
</table>

produces the following cellmap: Please notice the difference
between the second maps.
 
 
 ***** START TABLE CELL MAP DUMP ***** 02B91FD8
cols array orig/span-> 02B91FD80=2/0  cols in cache 1
  ***** START GROUP CELL MAP DUMP ***** 03017028
  thead mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 03004348
  thead mapRowCount=0 tableRowCount=0
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 030043B0
  tfoot mapRowCount=1 tableRowCount=1
  row 0 : C0,0  
  C0,0=03003374(0)  
  ***** END GROUP CELL MAP DUMP *****

  ***** START GROUP CELL MAP DUMP ***** 02B92190
  tfoot mapRowCount=1 tableRowCount=1
  row 0 : C0,0  
  C0,0=030CAA54(0)  
  ***** END GROUP CELL MAP DUMP *****
***** END TABLE CELL MAP DUMP *****
 ***END TABLE DUMP*** 
 
 as I stated in https://bugzilla.mozilla.org/show_bug.cgi?id=317554#c7
 
> Once the cellmap is out of sync with the row
> group frames, its only a matter of dom transitions till we crash

And this is exactly what is happening again here.


The cure for this is to remove OrderRowGroups 
see https://bugzilla.mozilla.org/show_bug.cgi?id=328017#c6

This however implies a major surgery at table printing. To test 
this one needs to get the printing regression tests running again. 
They got removed from tree together with the viewer removal and need 
to be reimplemented in the layout debugger.

Which is exactly the plan that I had to follow the next weeks.

Updated

11 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 6

11 years ago
I will develop the patch open inside bug 328017. The print rtest work is bug 339500 and  bug 339521.
Depends on: 328017
(Assignee)

Comment 7

11 years ago
Created attachment 224446 [details] [diff] [review]
patch
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 8

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

I did not go for the big solution. This  fixes the synchronization between rowgroups and cellmaps. The sad news is that it does not fix any other of the stirtable crashes. Its rather frustrating.

The good thing is due to the small size the patch is branchable.
Attachment #224446 - Flags: superreview?(bzbarsky)
Attachment #224446 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

11 years ago
   aTableFrame.OrderRowGroups(orderedRowGroups, numRowGroups);
-  NS_ASSERTION(orderedRowGroups.Count() == (PRInt32) numRowGroups,"problem in OrderRowGroups");

the assertion is bogus as the table could have non rowgroup children which would then be after the rowgroups (see nsTableFrame::OrderRowGroups)
(Assignee)

Comment 10

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

Comment 11

11 years ago
Bernd, do you still want review on the "revised patch"?
(Assignee)

Comment 12

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

Yes, I need a review for this. I am only a little bit scared by the low quality that the patches currently have where I ask for review. (see bug 339315 for another example).

The second thing is that due to the various crashes its difficult to test that one does not introduce new crashes. Hopefully the patch for bug 339246 will help, but currently, whichever seed I take, the stirtable crashes within 250 steps :-( .
Attachment #224723 - Flags: superreview?(bzbarsky)
Attachment #224723 - Flags: review?(bzbarsky)

Comment 13

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

>Index: nsCellMap.cpp
>+nsTableCellMap::Synchronize(nsTableFrame* aTableFrame)

>+  maps.Clear();

This isn't needed, right?  Why do it?

>+  aTableFrame->OrderRowGroups(orderedRowGroups, numRowGroups);

If this fails due to OOM, is that an issue?

>+        maps.AppendElement(map);

If this fails due to OOM, is that an issue?

r+sr=bzbarsky
Attachment #224723 - Flags: superreview?(bzbarsky)
Attachment #224723 - Flags: superreview+
Attachment #224723 - Flags: review?(bzbarsky)
Attachment #224723 - Flags: review+
(Assignee)

Comment 14

11 years ago
fixed on trunk, the whole bunch of fixes for 339128 needs serious baking as the changed code is pretty old.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
No longer depends on: 328017
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 342559
(Assignee)

Updated

11 years ago
No longer depends on: 342559
This crash happens on the 1.8 branches also, nominating.
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:critical?] → [sg:critical]

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Flags: blocking1.8.0.6? → blocking1.8.0.6+
(Assignee)

Updated

11 years ago
Blocks: 346980

Comment 16

11 years ago
Is this patch suitable for the branch?  Can we get a branch-suitable patch up for approval?
Target Milestone: --- → mozilla1.8.1

Comment 17

11 years ago
I think the patch in bug 346980 is the branch-suitable version (and includes this fix).  I'm not sure, though, and bernd is away.  :(
(Assignee)

Comment 18

11 years ago
fixed by the patch for bug 346980 on branch
Keywords: fixed1.8.1
(Assignee)

Comment 19

11 years ago
the only bug that should block 1.8.0.7 is bug 346980 which is blocked by the regression 347725

asking for 1.8.0.7- on this

Comment 20

11 years ago
https://bugzilla.mozilla.org/attachment.cgi?id=223437&action=view
ff2b2 windows, linux, macppc no crash; macppc shows 2 tds while windows/linux show 3.
ff2b2 debug windows, linux no crash
verified fixed 1.8
Keywords: fixed1.8.1 → verified1.8.1
Whiteboard: [sg:critical] → [sg:critical] fixed rolled into 346980
Moving to 1.8.0.8 at Bernd's request (regression worries)
Flags: blocking1.8.0.8+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+
(Assignee)

Comment 22

11 years ago
fixed by the cellmap branch patch
Keywords: fixed1.8.0.7
https://bugzilla.mozilla.org/attachment.cgi?id=223437&action=view should not crash browser.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060829 Firefox/1.5.0.7pre

verified 1.8.0.7
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7 → verified1.8.0.7
Flags: blocking1.8.0.7- → blocking1.8.0.7+
Group: security
Flags: in-testsuite?
Depends on: 424291

Comment 24

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