Closed Bug 408602 Opened 17 years ago Closed 16 years ago

"ASSERTION: reflow roots should never split" and crash [@ nsPropertyTable::PropertyList::Equals] with -moz-column, float, table, rel&abs pos

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9

People

(Reporter: jruderman, Assigned: fantasai.bugs)

References

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files, 3 obsolete files)

testcase
289 bytes, application/xhtml+xml
Details
patch part I (nsPageContentFrame only)
1.69 KB, patch
roc
: review+
roc
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review 
Loading the testcase in a Mac trunk debug build triggers:

###!!! ASSERTION: reflow roots should never split: 'status == NS_FRAME_COMPLETE', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 6195

Closing the testcase triggers a crash [@ nsPropertyTable::PropertyList::Equals] dereferencing 0xddddde09.
Attached file testcase (obsolete) — 

    
  
Whiteboard: [sg:critical?]

    
    

    
  
So it looks like nsViewportFrame just uses the aStatus of its principal child (if there are no fixed kids) or the last fixed-pos kid (if it has any).

In this case, we end up with an aStatus of

  NS_FRAME_OVERFLOW_INCOMPLETE |  NS_FRAME_REFLOW_NEXTINFLOW

Not sure about the former, but the latter seems bad; someone needs to reflow that next-in-flow, right?

    
    

    
  
Fixed-pos kids shouldn't be splitting, they should be getting an unconstrained available height...

    
  
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3

    
    

    
  
In this case, the fixed-pos kid is a <body>.  This has an abs-pos child block (<tbody>) that's got columns.  ExcessOverflowContainers-list for the <body> ends up with a frame for the <tbody> on it.  The <tbody>s first block child of the columnset frame seems to have an overflow list with the <tr> placeholder on it (the <tr> is a floating block).  It also has a OverflowOutOfFlow-list with the AreaFrame for the <tr> on it.  Oh, and that AreaFrame has overflow itself...

In any case, the reason there's splitting to start with is the columnset.

    
    

    
  
Related to bug 399994?
Assignee: nobody → fantasai.bugs

    
  
Priority: P3 → P2

    
    

    
  
Increasing to P1 because this interferes with fuzzing.
Priority: P2 → P1

    
    

    
  
Moving to B4 - fantasai you have time for this?
Target Milestone: --- → mozilla1.9beta4

    
    

    
  
The high-priority bugs I've got are this one, bug 381497, bug 410596, bug 414255, and bug 404215. I'll place this third on the list after 404215 (which is partly done) and bug 381497, which is blocking someone else. Time-wise, I can't do much until later this week. Once I've started.. I have basically no idea how long it takes me to fix a given bug, I don't have much experience. I can let you know how far I've gotten by the end of Friday, though.

    
    

    
  
Fantasi - code freeze for b4 coming up in two days - have you had any time to look at this?

    
    

    
  
Nope, got sidetracked by bug 417676 and 404215 is taking longer than I expected. I'll put it aside and look at this one tonight. If I don't have a fix for you by Monday you should probably assign it to someone else.

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  
Same testcase, using <div> instead of table elements.

        Attachment #293426 -
        Attachment is obsolete: true

    
    

    
  
The columnset is returning aStatus = NS_FRAME_NOT_COMPLETE | NS_FRAME_REFLOW_NEXTINFLOW despite being given availableHeight == NS_UNCONSTRAINEDSIZE.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch part I (obsolete)
        — 
          — Splinter Review
      

    


  
This fixes the assert. The crash is fixed by the first patch in bug 404215, and is therefore a dup of that bug.

    
  
Depends on: 404215

    
    

    
  
Don't think this blocks b4, nor requires beta exposure. Moving to P2. Feel free to nominate the patch for approva1.9b4?, though, once reviewed.
Priority: P1 → P2
Target Milestone: mozilla1.9beta4 → mozilla1.9
Flags: wanted-next+
Flags: blocking1.9-
Flags: tracking1.9+
Flags: wanted1.9.0.x?

    
    

    
  
With the patch in bug 404215 comment 46, I get an assertion, but not the one in comment 0.

###!!! ASSERTION: aFrame must be block frame: 'aFrame->GetType() == nsGkAtoms::blockFrame', file /Users/jruderman/trunk/mozilla/layout/base/nsLayoutUtils.cpp, line 2383


    
    

    
  
That assertion just looks bogus to me.  I commented to that effect in bug 404215.

    
    

    
  


  
nsViewportFrame seems to be reflowing children with a fixed availableHeight for some reason, and the assertion in the previous patch fires just by opening Firefox. Not sure what's going on there.. but this should catch any attempts to split fixed frames in nsPageContentFrames.

        Attachment #305343 -
        Attachment is obsolete: true

    
  

        Attachment #316541 -
        Flags: superreview?(roc)

        Attachment #316541 -
        Flags: review?(roc)

    
    

    
  
+  NS_ASSERTION(fixedStatus == NS_FRAME_COMPLETE, "fixed frames can be truncated, but not incomplete");

So if a fixed frame was truncated, this assertion would fire, right? But it shouldn't. Shouldn't you be asserting NS_FRAME_IS_COMPLETE?

    
    

    
  


  
Good point.

        Attachment #316541 -
        Attachment is obsolete: true

        Attachment #316875 -
        Flags: superreview?(roc)

        Attachment #316875 -
        Flags: review?(roc)

        Attachment #316541 -
        Flags: superreview?(roc)

        Attachment #316541 -
        Flags: review?(roc)

        Attachment #316875 -
        Flags: superreview?(roc)

        Attachment #316875 -
        Flags: superreview+

        Attachment #316875 -
        Flags: review?(roc)

        Attachment #316875 -
        Flags: review+

    
  

        Attachment #316875 -
        Flags: approval1.9?

    
    

    
  
Note to approver: "Part II" of the fix (fixing the crash) was checked in as the patch for bug 404215. Also, the testcase is already checked in as a crashtest.
Flags: in-testsuite+

    
    

    
  
Comment on attachment 316875 [details] [diff] [review]
patch part I (nsPageContentFrame only)

a1.9+=damons

        Attachment #316875 -
        Flags: approval1.9? → approval1.9+

    
  
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: wanted1.9.0.x?
Crash Signature: [@ nsPropertyTable::PropertyList::Equals]
Group: core-security

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: