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
(deleted), text/html
Details
MacsBug stdlog from testcase crash
(deleted), text/plain
Details
testcase: changes display and color at the same time.
(deleted), text/html
Details
PATCH: prevents recursive ReframeContainingBlock calls when 'special' frames are invloved in a ContentReplaced chain
(deleted), patch
karnaze
: review+
waterson
: superreview+
Details | Diff | Splinter Review
Testcase
(deleted), 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) (deleted) —
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) (deleted) — — Splinter Review
Attached file test case from description (deleted) —
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.
Attached file MacsBug stdlog from testcase crash (deleted) —
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 (deleted) —
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: