Last Comment Bug 339130 - Crash in [@ nsVoidArray::Count] called by nsCellMap::RebuildConsideringRows
: Crash in [@ nsVoidArray::Count] called by nsCellMap::RebuildConsideringRows
Status: VERIFIED FIXED
[sg:critical] fixed rolled into 346980
: crash, testcase, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.8.1
Assigned To: Bernd
:
Mentors:
Depends on: 424291
Blocks: stirtable 339131 346980
  Show dependency treegraph
 
Reported: 2006-05-24 11:58 PDT by Mats Palmgren (:mats)
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (11.88 KB, text/plain)
2006-05-24 12:00 PDT, Mats Palmgren (:mats)
no flags Details
stack #2 (9.52 KB, text/plain)
2006-05-24 12:51 PDT, Mats Palmgren (:mats)
no flags Details
testcase (842 bytes, text/html)
2006-05-26 07:34 PDT, Jesse Ruderman
no flags Details
patch (10.38 KB, patch)
2006-06-05 10:08 PDT, Bernd
no flags Details | Diff | Splinter Review
revised patch (11.14 KB, patch)
2006-06-07 11:13 PDT, Bernd
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Comment 1 Mats Palmgren (:mats) 2006-05-24 12:00:23 PDT
Created attachment 223207 [details]
stack

A few of the assertions leading up to the crash, the stack and some data.
Comment 2 Mats Palmgren (:mats) 2006-05-24 12:51:51 PDT
Created attachment 223215 [details]
stack #2

The same call stack as above, but 'row' = 0xdadadada
Comment 3 Jesse Ruderman 2006-05-26 07:34:24 PDT
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
Comment 4 Jesse Ruderman 2006-05-26 07:34:49 PDT
Created attachment 223437 [details]
testcase
Comment 5 Bernd 2006-05-27 01:36:03 PDT
<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.
Comment 6 Bernd 2006-05-28 11:56:06 PDT
I will develop the patch open inside bug 328017. The print rtest work is bug 339500 and  bug 339521.
Comment 7 Bernd 2006-06-05 10:08:36 PDT
Created attachment 224446 [details] [diff] [review]
patch
Comment 8 Bernd 2006-06-05 10:13:25 PDT
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.
Comment 9 Bernd 2006-06-05 10:15:35 PDT
   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)
Comment 10 Bernd 2006-06-07 11:13:11 PDT
Created attachment 224723 [details] [diff] [review]
revised patch
Comment 11 Boris Zbarsky [:bz] 2006-06-09 21:42:58 PDT
Bernd, do you still want review on the "revised patch"?
Comment 12 Bernd 2006-06-09 22:15:46 PDT
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 :-( .
Comment 13 Boris Zbarsky [:bz] 2006-06-09 22:34:06 PDT
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
Comment 14 Bernd 2006-06-21 21:59:14 PDT
fixed on trunk, the whole bunch of fixes for 339128 needs serious baking as the changed code is pretty old.
Comment 15 Daniel Veditz [:dveditz] 2006-07-07 10:04:11 PDT
This crash happens on the 1.8 branches also, nominating.
Comment 16 Mike Schroepfer 2006-08-15 12:48:33 PDT
Is this patch suitable for the branch?  Can we get a branch-suitable patch up for approval?
Comment 17 Boris Zbarsky [:bz] 2006-08-15 17:38:13 PDT
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.  :(
Comment 18 Bernd 2006-08-20 12:02:19 PDT
fixed by the patch for bug 346980 on branch
Comment 19 Bernd 2006-08-21 12:00:03 PDT
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 Bob Clary [:bc:] 2006-08-22 00:38:09 PDT
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
Comment 21 Daniel Veditz [:dveditz] 2006-08-23 15:15:14 PDT
Moving to 1.8.0.8 at Bernd's request (regression worries)
Comment 22 Bernd 2006-08-29 12:31:55 PDT
fixed by the cellmap branch patch
Comment 23 alice nodelman [:alice] [:anode] 2006-08-29 16:17:54 PDT
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
Comment 24 Bob Clary [:bc:] 2009-04-24 11:09:54 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/bc81c570de7e

Note You need to log in before you can comment on or make changes to this bug.