Closed Bug 330909 Opened 18 years ago Closed 18 years ago

GetAbsoluteContainingBlock() is broken

Categories

(Core :: Layout: Positioned, defect)

Product:
Core
Component:
Layout: Positioned
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: bernd_mozilla, Assigned: bernd_mozilla)

References

(
URL
)

Details

(Keywords: testcase, verified1.8.0.9, verified1.8.1.1)

Attachments

(1 file, 1 obsolete file)

Updated to tip
1.03 KB, patch
dveditz
: approval1.8.0.8-
dveditz
: approval1.8.0.9+
mtschrep
: approval1.8.1-
dveditz
: approval1.8.1.1+
Details | Diff | Splinter Review 
from bug 329768 comment 7 (bz)

OK, so the setup in comment 6 comes about because in ConstructBlock we push the
block as an abs pos containing block if there isn't one pushed already.  That's
not the real bug here.  The real bug (or the first half) is that
GetAbsoluteContainingBlock does something different -- it returns
mInitialContainingBlock if it finds nothing else.  I was _sure_ we had a bug on
that...

In any case, what happens here is:

1)  We reframe the <html> because of the style change.yet.
2)  Right after that (before we get a chance to reflow) we reframe the
<object>.
3)  We start constructing frames and call GetAbsoluteContainingBlock() to get
    one.  This returns mInitialContainingBlock, which is the outer table
    frame(!).
4)  We construct a frame for the <script>.  It's an abs pos frame, so we put it
    on the abs pos list of our absolute containing block.  But that list is
    empty and the outer table has had no initial reflow yet, so we use
    SetInitialChildList to do that.
5)  nsTableFrame::SetInitialChildList assumes that any aListName that is not
    captionList is null and sticks stuff in the principal list, clobbering
    whatever is there.  It doesn't bother to assert that aListName is null or
    that mFrames is empty...  If it did, debugging this would have taken about
    30 seconds.  :(

So as I see it we have two+ bugs:

1)  GetAbsoluteContainingBlock() is broken.
2a)  nsTableOuterFrame::SetInitialChildList is kinda broken.
2b)  nsTableOuterFrame::Reflow doesn't deal with a null mInnerTableFrame;
     should it?
bug 329768 fixed issue 2a and 2b.
Blocks: 330970
Attached patch insufficient patch (obsolete) — — Splinter Review 
This is still not enough, it applies the same logic as it is done inside GetFloatContaining block but even then the URL still asserts. However the dependent crash is fixed with this. (I have no clue what I am doing here :-()
Blocks: 330981
Blocks: 331679
No longer blocks: 331679
No longer blocks: 330970
Blocks: 330970
Comment on attachment 215599 [details] [diff] [review]
insufficient patch

I think this is the right thing to do, but then again I suggested it... roc, would you review?

Note that this, combined with the change proposed in bug 322625, would make things pretty much completely happy, I think.
Attachment #215599 - Flags: superreview?(roc)
Attachment #215599 - Flags: review?(roc)
Blocks: 322625
Attachment #215599 - Flags: superreview?(roc)
Attachment #215599 - Flags: superreview+
Attachment #215599 - Flags: review?(roc)
Attachment #215599 - Flags: review+
Attached patch Updated to tip — — Splinter Review 

        Attachment #215599 -
        Attachment is obsolete: true
Assignee: nobody → bernd_mozilla

    
    

    
  
Comment on attachment 239949 [details] [diff] [review]
Updated to tip

I think it's worth at least considering this on the branches...  Fixes a number of our table-related crashers.

        Attachment #239949 -
        Flags: approval1.8.1?

        Attachment #239949 -
        Flags: approval1.8.0.8?

    
    

    
  
Checked in on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
    

    
  
Bz - how safe is this for the branches?  I'm a little nervous about touching FrameConstructor without some good trunk testing...

    
    

    
  
I think this is actually pretty safe.  It will only affect pages where the root element is a display:table or is XUL (so "not typical web pages", since there the root element is a block unless the page explicitly styles the <html> element), and it will affect them only by preventing crashes.  What the patch really does is make dynamic DOM changes construct closer to the same frame tree as just parsing the page.

That said, I can understand reluctance to take this for 1.8.1; I have no problem with pushing this out to 1.8.1.1 if we want to get more bake time.

    
    

    
  
Alright - thanks for the detailed explanation - all other things being equal I'd like to play it safe.  So I'll push to 1.8.1.1 for now.   
Flags: blocking1.8.1.1?

    
  

        Attachment #239949 -
        Flags: approval1.8.1? → approval1.8.1-

    
    

    
  
this check-in cause bug 354088 ? 

"position:absolute" seems to be broken.

    
    

    
  
Ugh.  Could be.  I'll investigate tonight.
Depends on: 354088

    
    

    
  
Yeah, this caused bug 354088.  I'll post a patch there.
No longer depends on: 354088
Flags: blocking1.8.0.9?

    
    

    
  
Comment on attachment 239949 [details] [diff] [review]
Updated to tip

We'll take it for 1.8.0.x when 1.8.1.x does

        Attachment #239949 -
        Flags: approval1.8.0.9?

        Attachment #239949 -
        Flags: approval1.8.0.8?

        Attachment #239949 -
        Flags: approval1.8.0.8-
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+

    
    

    
  
Comment on attachment 239949 [details] [diff] [review]
Updated to tip

Approved for 1.8.0/1.8 branches, a=dveditz for drivers

        Attachment #239949 -
        Flags: approval1.8.1.1+

        Attachment #239949 -
        Flags: approval1.8.0.9?

        Attachment #239949 -
        Flags: approval1.8.0.9+

    
    

    
  
Checked in for 1.8.1 and 1.8.0.9.
Keywords: fixed1.8.0.9, 
          
            fixed1.8.1

    
  
Keywords: fixed1.8.1 → fixed1.8.1.1

    
    

    
  
v.fixed on 1.8.0 and 1.8.1 branches with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre 
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

No crash with the testcase URL in this bug, but if there are any other testcases to help us feel good about this change, please let us know.
Keywords: fixed1.8.0.9, 
          
            fixed1.8.1.1 → verified1.8.0.9, 
          
            verified1.8.1.1
Whiteboard: [need testcase]

    
    

    
  
I used the "another (not completely minimised) testcase" from the depending bug 330981: https://bugzilla.mozilla.org/attachment.cgi?id=235173&action=view
That crashes with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061123 Firefox/1.5.0.9pre
But I don't crash with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre
Status: RESOLVED → VERIFIED
Whiteboard: [need testcase]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: