Closed Bug 339130 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8.1

People

(Reporter: MatsPalmgren_bugz, Assigned: bernd_mozilla)

References

Details

(4 keywords, Whiteboard: [sg:critical] fixed rolled into 346980)

Crash Data

Attachments

(4 files, 1 obsolete file)

stack
11.88 KB, text/plain
Details
stack #2
9.52 KB, text/plain
Details
testcase
842 bytes, text/html
Details
revised patch
11.14 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Attached file stack — 
A few of the assertions leading up to the crash, the stack and some data.
Attached file stack #2 — 
The same call stack as above, but 'row' = 0xdadadada
Summary: Crash in [@ nsVoidArray::Count] → Crash in [@ nsVoidArray::Count] called by nsCellMap::RebuildConsideringRows
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
Attached file testcase — 

    
    

    
  
<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.

    
  
Whiteboard: [sg:critical?]

    
    

    
  
I will develop the patch open inside bug 328017. The print rtest work is bug 339500 and  bug 339521.
Depends on: 328017

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED

    
    

    
  
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)

    
    

    
  
   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)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        revised patch
        — 
          — Splinter Review
      

    


  

        Attachment #224446 -
        Attachment is obsolete: true

        Attachment #224446 -
        Flags: superreview?(bzbarsky)

        Attachment #224446 -
        Flags: review?(bzbarsky)

    
    

    
  
Bernd, do you still want review on the "revised patch"?

    
    

    
  
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 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+

    
    

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

    
  
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]

    
  
Flags: blocking1.8.1? → blocking1.8.1+
Flags: blocking1.8.0.6? → blocking1.8.0.6+

    
  
Blocks: 346980

    
    

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

    
    

    
  
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.  :(

    
    

    
  
fixed by the patch for bug 346980 on branch
Keywords: fixed1.8.1

    
    

    
  
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

    
    

    
  
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+

    
    

    
  
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

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: