Closed Bug 56894 Opened 24 years ago Closed 23 years ago

[PATCH] changing 'display' style along with any other style on a 'special inline-block' element causes asserts and crash

Categories

(Core :: DOM: CSS Object Model, defect, P1)

Product:
Core
Component:
DOM: CSS Object Model
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: bking, Assigned: attinasi_layout)

References

Details

(Keywords: crash, dom1)

Attachments

(5 files, 2 obsolete files)

test case from description
949 bytes, text/html
Details
MacsBug stdlog from testcase crash
30.86 KB, text/plain
Details
testcase: changes display and color at the same time.
863 bytes, text/html
Details
PATCH: prevents recursive ReframeContainingBlock calls when 'special' frames are invloved in a ContentReplaced chain
13.44 KB, patch
karnaze
: review+
waterson
: superreview+
Details | Diff | Splinter Review
Testcase
867 bytes, text/html
Details 
I've included the full source for the problem below. If you load this source 
and then click on hide and the click on the reappear and then the hide again 
you'll see the crash.

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">

<html>
<head>
<title>dhtml fun stuff</title>
<script language="javascript">
function disappeartext(){
	var mm=document.getElementById("makeme");
	mm.style.position='absolute';
	mm.style.visibility='hidden';
}
function appeartext(){
	var mm=document.getElementById("makeme");
	mm.style.position='relative';
	mm.style.visibility='visible';
}
</script>

</head>
<body>
Yeah, we got a magic table trick.<br />
<span id="makeme" style="{visibility: hidden; position: absolute;}"><table 
border="1" summary="cool table">
<tr>
<td>Yeah!</td>
</tr>
</table></span><br />
<form name="clicker" action="dhtmltest1.html" method="post">
<input type="button" name="qer" value="Click to make it disappear" 
onclick="disappeartext();" />
<input type="button" name="qer" value="Click to make it reappear" 
onclick="appeartext();" />
</form>

</body>
</html>
This is crashing in nsBlockFrame::DoRemoveFrame() on a null dereference. 
I'll attach the stack crawl momentarily. Here's the patch to fix the problem:


    // Advance to next flow block if the frame has more continuations
    if (nsnull != aDeletedFrame) {
      flow = (nsBlockFrame*) flow->mNextInFlow;
      NS_ASSERTION(nsnull != flow, "whoops, continuation without a parent");
      if(flow) {
        prevLine = nsnull;
        line = flow->mLines;
        linep = &flow->mLines;
        prevSibling = nsnull;
      }
    }
Attached file stacktrace (obsolete) — 

    
    

    
  
I'll take this one.  Thanks for the patch, Rick.
Assignee: jst → buster
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, 
          
            patch, 
          
            rtm
Priority: P3 → P1

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed fix (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  

    
    

    
  
patch is from rickg.
r=buster
needs an a= from waterson
karnaze, when waterson approves, please nominate for rtm.
Status: NEW → ASSIGNED

    
    

    
  
Defensive code looks fine, a=waterson; let's keep the bug open (you can assign
to me if you want) to figure out why the flow is getting wedged.

    
    

    
  
Adding [rtm+].
Whiteboard: [rtm+] a=waterson, r=buster

    
    

    
  
RTM double plus on the null check.
Please land on branch asap.

Per Waterson's request, please re-assign to him after landing for further
investigation (rather than closing).


    
    

    
  
RTM double plus on the null check.
Please land on branch asap.

Per Waterson's request, please re-assign to him after landing for further
investigation (rather than closing).

Whiteboard: [rtm+] a=waterson, r=buster → [rtm++] a=waterson, r=buster

    
    

    
  
Cool, buster!  XBL can cause this crash to occur too!  YOu just fixed on of my
rtm need info bugs.

Yay.


    
    

    
  
*** Bug 57596 has been marked as a duplicate of this bug. ***

    
    

    
  
10/23 testcase crashes Mac Mozilla installer build 2000102312 nicely while
running Mac OS 9.0.4.  Stdlog follows.

    
    

    
  
patch checked into trunk, even though it only partially fixes this problem.  It
does fix crasher bug 57596.

    
    

    
  
This fix has missed the first N6 candidate build, so we can not take it unless
we respin. This bug is in candidate limbo.  We will reconsider this fix once we
have a candidate in hand, but we can't take this fix before then. PDT marking [rtm+]

Whiteboard: [rtm++] a=waterson, r=buster → [rtm+] a=waterson, r=buster

    
    

    
  
PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Whiteboard: [rtm+] a=waterson, r=buster → [rtm++] a=waterson, r=buster

    
    

    
  
fix checked into branch

Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
marked the wrong bug
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
This is on the talkback topcrash list at #8, adding topcrash keyword and [@ 
nsBlockFrame::DoRemoveFrame] for tracking.
Keywords: topcrash
Summary: mixing position and visibility styles crashes the browser → mixing position and visibility styles crashes the browser [@ nsBlockFrame::DoRemoveFrame]

    
  
Whiteboard: [rtm++] a=waterson, r=buster → [rtm++] a=waterson, r=buster; fixed on branch with 57596

    
    

    
  
Resetting to RTM+, for re-review tomorrow. Given the differences between the 
various versions (buster's machine, the tip and the branch) I'm not certain 
which version he wants to land. 
Status: REOPENED → ASSIGNED
Whiteboard: [rtm++] a=waterson, r=buster; fixed on branch with 57596 → [rtm+] a=waterson, r=buster; fixed on branch with 57596

    
    

    
  
using a netscape 6 branch build on Macintosh, i can't reproduce any crashes with
either of the test cases attached to this bug.  However, I do see a cosmetic bug
where the rendering isn't quite right for the test case attached 10/23/00 16:57
(click a few times back and forth between the buttons).

    
    

    
  
Talked to buster on the phone. we're done with this for RTM, but he's testing
some more before marking fixed.

    
    

    
  
Verifying no crash in testcase using 10-31-14 MN6 candidate build on Win98,
although I do see the cosmetic anomaly Kathy describes.

    
    

    
  
well, the crash is gone, but in debug mode we still see lots of asserts, and I
don't think the display is quite right.  Since the crash can be verified and
documented in bug 57596, I'm going to mutate this bug to cover those other problems.
Severity: critical → normal
Depends on: 57596
Keywords: crash, 
          
            patch, 
          
            rtm, 
          
            topcrash
Priority: P1 → P3
Summary: mixing position and visibility styles crashes the browser [@ nsBlockFrame::DoRemoveFrame] → mixing position and visibility styles causes asserts and bad rendering
Whiteboard: [rtm+] a=waterson, r=buster; fixed on branch with 57596

    
    

    
  
*** Bug 58513 has been marked as a duplicate of this bug. ***
Keywords: dom1
Component: DOM Level 1 → DOM Style

    
    

    
  
Taking QA Contact on all open or unverified DOM Style bugs...
QA Contact: janc → ian

    
    

    
  
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
Status: ASSIGNED → NEW

    
    

    
  
It crashes using cvs trunk build from this morning on WINNT. Clicking the
buttons to disappear then reappear makes it crash.

Adding crash keyword.
Keywords: crash

    
    

    
  
When I load the page testcase when a debug build I get a bunch of assertions
about an "illegal refcnt" 

It crashes with the following stack: (Note the stack goes on and on, looks like
a stack overflow is what causes the crash)


nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
int 3) line 9060 + 3 bytes
nsCSSFrameConstructor::ContentReplaced(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
nsIContent * 0x0521f510, int 3) line 8820 + 28 bytes
nsCSSFrameConstructor::ReframeContainingBlock(nsIPresContext * 0x0405fa60,
nsIFrame * 0x020cdb20) line 13508 + 47 bytes
nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
int 3) line 9186 + 16 bytes
nsCSSFrameConstructor::ContentReplaced(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
nsIContent * 0x0521f510, int 3) line 8820 + 28 bytes
nsCSSFrameConstructor::ReframeContainingBlock(nsIPresContext * 0x0405fa60,
nsIFrame * 0x020cdb20) line 13508 + 47 bytes
nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
int 3) line 9186 + 16 bytes
nsCSSFrameConstructor::ContentReplaced(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
nsIContent * 0x0521f510, int 3) line 8820 + 28 bytes
nsCSSFrameConstructor::ReframeContainingBlock(nsIPresContext * 0x0405fa60,
nsIFrame * 0x020cdb20) line 13508 + 47 bytes
nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
int 3) line 9186 + 16 bytes
nsCSSFrameConstructor::ContentReplaced(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
nsIContent * 0x0521f510, int 3) line 8820 + 28 bytes
nsCSSFrameConstructor::ReframeContainingBlock(nsIPresContext * 0x0405fa60,
nsIFrame * 0x020cdb20) line 13508 + 47 bytes
nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
int 3) line 9186 + 16 bytes
nsCSSFrameConstructor::ContentReplaced(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
nsIContent * 0x0521f510, int 3) line 8820 + 28 bytes
nsCSSFrameConstructor::ReframeContainingBlock(nsIPresContext * 0x0405fa60,
nsIFrame * 0x020cdb20) line 13508 + 47 bytes
nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
int 3) line 9186 + 16 bytes
nsCSSFrameConstructor::ContentReplaced(nsCSSFrameConstructor * const 0x04019de0,
nsIPresContext * 0x0405fa60, nsIContent * 0x040ac0f0, nsIContent * 0x0521f510,
nsIContent * 0x0521f510, int 3) line 8820 + 28 bytes
nsCSSFrameConstructor::ReframeContainingBlock(nsIPresContext * 0x0405fa60,
nsIFrame * 0x020cdb20) line 13508 + 47 bytes
...
...
...
Much more of the same
Target Milestone: --- → mozilla0.9.7

    
    

    
  
Also crashes in N6.2 on winnt.

    
    

    
  
The problem has to do with the way we manage the frames for a 'block inside of
an inline' situation. In the testcase, there is a table inside of a span - this
causes our 'special' frame handling code to kick in. Compounding the complexity
in that code is the fact that when the outer span is getting its position
changed to absolute it causes it to become a block (see EnsureBlockDisplay in
nsRuleNode.cpp).  If you change the SPAN to DIV then all is fine.
Status: NEW → ASSIGNED

    
    

    
  
Interestingly, SplitToContainingBlock is not involved here, however the bug only
happens when the span being manipulated has a block-level element within it.
Because the span is positioned initially, it is treated as a block and thus it
is not a block-in-inline situation. Once we change the absolute span to a
relative span, it becomes an inline again but we are not splitting out the block
within it. This might also explain why the display is totally wrong (before the
recursion up to heaven, or is it down to hell?).

I believe we are not correctly marking (or clearing probably )the
FRAME_IS_SPECIAL bit in the frame state when the frame is switched from inline
to block. This is shown in the testcase by replacing the position:absolute /
position:relative toggle on the span to display:block / display:inline, as in:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">

<html>
<head>
<title>dhtml fun stuff</title>
<script language="javascript">
function disappeartext(){
	var mm=document.getElementById("makeme");
	mm.style.display='block';
	mm.style.visibility='hidden';
}
function appeartext(){
	var mm=document.getElementById("makeme");
	mm.style.display='inline';
	mm.style.visibility='visible';
}
</script>

</head>
<body>
Make it happen<br />
<span id="makeme" style="{visibility: visible; display:block;}">
 <div>Some Text</div>
</span>

<br />

<form name="clicker" action="dhtmltest1.html" method="post">
<input type="button" name="qer" value="Click to make it disappear" 
onclick="disappeartext();" />
<input type="button" name="qer" value="Click to make it reappear" 
onclick="appeartext();" />
</form>

</body>
</html>

Same infinite recursion stack and crash. 

    
    

    
  
Bug 97874 may be an extension of this bug.  I (as a web developer) am running
into that one.  It has a great example link as one of the most recent comments.  

    
    

    
  
Bug 97874 is absolutely related to this one. In both bugs, we are flailing when
there is a block-in-inline that is getting restyled. I am finding lots of
problems along the way. The two most important issues are:
1) when the inline that contains the block is modified, we are not correctly
tearing down the old frames before we build the new ones. I have not identified
the cause of this defect yet.
2) during the creation of the new frames, we are incorrectly causing repeated
reconstruction of the frames. This is a problem in ContentReplaced, which calls
ContentRemoved and ContentAppended, and each of those methods are trying to be
smart about the special block-in-inline situation and are then reframing their
containing block - the same one we are in the middle of reframing. I ahve a fix
for this part, so the hang / crash are covered.

A minor problem is that GetIBContainingBlockFor can erroneously return the same
frame as its own parent. Also, FindPreviousSibling can return itself as the
previous sibling, which also seems very wrong. This happens when the frame you
are trying to find the previous sibling for is the first child of the container
and is also a special inline-block (happens in the testcase).

    
    

    
  
Current top-priority bug -> attinasi_layout
Assignee: attinasi → attinasi_layout
Status: ASSIGNED → NEW
Priority: P3 → P2

    
    

    
  
I believe that the 'display' and 'visibility' properties are seperate problems.
I have a fix for the 'display' property, that fixes the hang/crash too. For the
'visibility' problem I opened a new bug, bug 111238. As such, I am refining this
bug to cover just the 'display' problem.
Status: NEW → ASSIGNED
Summary: mixing position and visibility styles causes asserts and bad rendering → changing 'display' style on a 'special inline-block' element causes asserts and crash

    
    

    
  
Refined summary further, --> P1
Priority: P2 → P1
Summary: changing 'display' style on a 'special inline-block' element causes asserts and crash → changing 'display' style along with any other style on a 'special inline-block' element causes asserts and crash

    
    

    
  


  

        Attachment #17510 -
        Attachment is obsolete: true

        Attachment #17826 -
        Attachment is obsolete: true

    
    

    
  


  
This patch solves much of the problem. 
* It prevents the recursion-till-the-stack-blows crash by letting
ContentRemoved and ContentInserted know that they are being called in a
ContentReplaced context, so they do not need to ReframeContainingBlock for the
special frames. 
* It also corrects an error in GetIBContainingBlock where it used to return the
same frame who's containing block we wanted (added post-condition assertions
too). Looking at the code that this routine replaced, I am pretty sure that
this bug was introduced inadvertently.
* Finally, it does a runtime check for a situation in BlockFrame that was only
asserted before: this is the case where a deleted frame cannot be found in any
line - happens in the first testcase but I don't know why yet - this prevents
yet another hang.
Blocks: 97874

    
    

    
  
*** Bug 100263 has been marked as a duplicate of this bug. ***

    
    

    
  
Patch is about %95...
Summary: changing 'display' style along with any other style on a 'special inline-block' element causes asserts and crash → [PATCH] changing 'display' style along with any other style on a 'special inline-block' element causes asserts and crash

    
    

    
  
Comment on attachment 58734 [details] [diff] [review]
PATCH: prevents recursive ReframeContainingBlock calls when 'special' frames are invloved in a ContentReplaced chain

Was this part of the patch? It doesn't seem related...

>Index: html/base/src/nsBlockFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v
>retrieving revision 3.473
>diff -u -r3.473 nsBlockFrame.cpp
>--- nsBlockFrame.cpp	2001/11/15 07:30:09	3.473
>+++ nsBlockFrame.cpp	2001/11/21 19:06:29
>@@ -4868,6 +4868,8 @@
>     NS_ASSERTION(tmp == aDeletedFrame, "bad prevSibling");
>   }
> #endif
>+  if (line == line_end) 
>+    return NS_ERROR_FAILURE;
> 
>   // Remove frame and all of its continuations
>   while (nsnull != aDeletedFrame) {


> nsCSSFrameConstructor::ContentRemoved(nsIPresContext* aPresContext,
>                                       nsIContent*     aContainer,
>                                       nsIContent*     aChild,
>-                                      PRInt32         aIndexInContainer)
>+                                      PRInt32         aIndexInContainer,
>+                                      PRBool aInContentReplaced)

Nit! Shouldn't aInContentReplaced line up with the other arguments?

>@@ -112,7 +113,8 @@
>   NS_IMETHOD ContentRemoved(nsIPresContext* aPresContext,
>                             nsIContent*     aContainer,
>                             nsIContent*     aChild,
>-                            PRInt32         aIndexInContainer);
>+                            PRInt32         aIndexInContainer,
>+                            PRBool aInContentReplaced);


Ditto!

This looks good: sr=waterson.

        Attachment #58734 -
        Flags: superreview+

    
    

    
  
Chris, that seemingly unrelated part of the patch was mentioned in the third
bullet of my comment: http://bugzilla.mozilla.org/show_bug.cgi?id=56894#c41

Basically, we hit that assertion in one of the testcases and then we end up in
an infinite loop. I have not been able to figure out and fix the
frame-not-found-in-the-line problem, so that runtime check is just a gloss-over
to prevent the infinite loop. I'll put a comment in there about it, referring to
this bug. That is the remaining 5% that is taking me 80% of the time to find ;)

(gotcha on the alignment of the args - thanks.)

    
    

    
  
Missed 0.9.7 --> 0.9.8 (still need the review!)
Target Milestone: mozilla0.9.7 → mozilla0.9.8

    
    

    
  
The patch is missing the changes for content/base/src/nsStyleSet.cpp:


Index: nsStyleSet.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsStyleSet.cpp,v
retrieving revision 3.106
diff -u -r3.106 nsStyleSet.cpp
--- nsStyleSet.cpp	6 Nov 2001 10:04:01 -0000	3.106
+++ nsStyleSet.cpp	14 Dec 2001 09:23:34 -0000
@@ -1403,7 +1403,8 @@
                                             PRInt32         aIndexInContainer)
 {
   return mFrameConstructor->ContentInserted(aPresContext, aContainer,
-                                            aChild, aIndexInContainer, nsnull);
+                                            aChild, aIndexInContainer,
+                                            nsnull, PR_FALSE);
 }
 
 NS_IMETHODIMP StyleSetImpl::ContentReplaced(nsIPresContext* aPresContext,
@@ -1422,7 +1423,8 @@
                                            PRInt32         aIndexInContainer)
 {
   return mFrameConstructor->ContentRemoved(aPresContext, aContainer,
-                                           aChild, aIndexInContainer);
+                                           aChild, aIndexInContainer,
+                                           PR_FALSE);
 }
 
 NS_IMETHODIMP


    
    

    
  
Good catch Alex, thanks. I forgot about diffing the content module :\

    
  
Blocks: 112360

    
  
Blocks: 114303

    
    

    
  
Comment on attachment 58734 [details] [diff] [review]
PATCH: prevents recursive ReframeContainingBlock calls when 'special' frames are invloved in a ContentReplaced chain

r=karnaze, although, I'm not very familar with that code.

        Attachment #58734 -
        Flags: review+

    
    

    
  
Fixes checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase
        — 
      

    


  
The fix isn't enough. Hit left button in this testcase.

This is different from attachment 17827 [details] in two points.
(1) Default style for <span> is relative.
(2) <table> is removed.

See bug 118931.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: