Closed Bug 159359 Opened 23 years ago Closed 23 years ago

Will crash almost all later versions of mozilla with simple table/css/form combination [@ nsBlockBandData::Init] Trunk

Categories

(Core :: Layout, defect, P2)

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jmmcmanu, Assigned: karnaze)

References

(
URL
)

Details

(Keywords: crash, testcase, topcrash+)

Crash Data

Attachments

(1 file, 22 obsolete files)

Testcase (will crash mozilla)
175 bytes, text/html
Details
Adding a stylesheet to a table that causes it to be fixed and then putting a form in a non-displayable area (i.e. inbetween the tr and td or not having td's at all) will cause Mozilla to hard crash. Sample code that renders crash: <table style="position: fixed; top: 0px;"> <!-- Crashes Mozilla --> <tr> <form> </form> </tr> </table>
Confirming on linux 2002072422. DDD output: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 1024 (LWP 29882)] 0x41242e70 in nsBlockBandData::Init () from /home/sco/src/mozilla/dist/bin/components/libgklayout.so
Assignee: Matti → attinasi
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout
Ever confirmed: true
QA Contact: asa → petersen
nsSpaceManager::GetTranslation(int & 0, int & 0) line 192 + 13 bytes nsBlockBandData::Init(nsSpaceManager * 0x00000000, const nsSize & {...}) line 74 nsBlockReflowState::nsBlockReflowState(const nsHTMLReflowState & {...}, nsIPresContext * 0x06253f88, nsBlockFrame * 0x0606ba94, const nsHTMLReflowMetrics & {...}, int 0) line 152 nsBlockFrame::Reflow(nsBlockFrame * const 0x0606ba94, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 33420331) line 823 nsContainerFrame::ReflowChild(nsIFrame * 0x0606ba94, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0, unsigned int 0, unsigned int & 33420331) line 807 + 31 bytes nsTableRowFrame::ReflowChildren(nsTableRowFrame * const 0x0606b8e8, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, nsTableFrame & {...}, unsigned int & 0, int 0) line 1120 nsTableRowFrame::Reflow(nsTableRowFrame * const 0x0606b8e8, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) line 1443 + 37 bytes nsContainerFrame::ReflowChild(nsIFrame * 0x0606b8e8, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0, unsigned int 0, unsigned int & 0) line 807 + 31 bytes nsTableRowGroupFrame::ReflowChildren(nsTableRowGroupFrame * const 0x0606b75c, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, nsRowGroupReflowState & {...}, unsigned int & 0, nsTableRowFrame * 0x00000000, int 0, nsTableRowFrame * * 0x00000000, int * 0x0012ed4c) line 444 + 45 bytes nsTableRowGroupFrame::Reflow(nsTableRowGroupFrame * const 0x0606b75c, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) line 1214 + 35 bytes nsContainerFrame::ReflowChild(nsIFrame * 0x0606b75c, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 30, unsigned int 0, unsigned int & 0) line 807 + 31 bytes nsTableFrame::ReflowChildren(nsTableFrame * const 0x0606b5a4, nsIPresContext * 0x06253f88, nsTableReflowState & {...}, int 1, int 0, unsigned int & 0, nsIFrame * & 0x00000000, int * 0x00000000) line 3311 + 53 bytes nsTableFrame::Reflow(nsTableFrame * const 0x0606b5a4, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) line 2007 nsContainerFrame::ReflowChild(nsIFrame * 0x0606b5a4, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0, unsigned int 3, unsigned int & 0) line 807 + 31 bytes nsTableOuterFrame::OuterReflowChild(nsTableOuterFrame * const 0x0606b3dc, nsIPresContext * 0x06253f88, nsIFrame * 0x0606b5a4, const nsHTMLReflowState & {...}, nsHTMLReflowMetrics & {...}, int * 0x00000000, nsSize & {...}, nsMargin & {...}, nsMargin & {...}, nsMargin & {...}, nsReflowReason eReflowReason_Initial, unsigned int & 0) line 1026 + 47 bytes nsTableOuterFrame::Reflow(nsTableOuterFrame * const 0x0606b3dc, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) line 1611 + 72 bytes ViewportFrame::ReflowFixedFrame(nsIPresContext * 0x06253f88, const nsHTMLReflowState & {...}, nsIFrame * 0x0606b3dc, int 1, unsigned int & 0) line 375 + 37 bytes ViewportFrame::IncrementalReflow(nsIPresContext * 0x06253f88, const nsHTMLReflowState & {...}) line 480 ViewportFrame::Reflow(ViewportFrame * const 0x040a9454, nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) line 536 IncrementalReflow::Dispatch(nsIPresContext * 0x06253f88, nsHTMLReflowMetrics & {...}, const nsSize & {...}, nsIRenderingContext & {...}) line 942 PresShell::ProcessReflowCommands(int 1) line 6453 ReflowEvent::HandleEvent() line 6303 HandlePLEvent(ReflowEvent * 0x05fa1b78) line 6317 PL_HandleEvent(PLEvent * 0x05fa1b78) line 596 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00fd6ff0) line 526 + 9 bytes _md_EventReceiverProc(HWND__ * 0x000200d4, unsigned int 49372, unsigned int 0, long 16609264) line 1077 + 9 bytes USER32! 77e01b60() USER32! 77e01cca() USER32! 77e083f1() nsAppShellService::Run(nsAppShellService * const 0x015d5e38) line 452 main1(int 2, char * * 0x00283160, nsISupports * 0x00000000) line 1511 + 32 bytes main(int 2, char * * 0x00283160) line 1871 + 37 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 77e7d326() looks like bug 130251 is not fixed...
Keywords: crash
-> kmcclusk for reassignment Crashed the testecase with this stack: Product ID Gecko1.0 Build ID 2002071708 Operating System Windows NT 5.0 build 2195 Stack Trace nsBlockBandData::Init [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockBandData.cpp, line 72] nsBlockReflowState::nsBlockReflowState [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockReflowState.cpp, line 151] nsBlockFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 766] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 833] nsTableRowFrame::ReflowChildren [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableRowFrame.cpp, line 1116] nsTableRowFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableRowFrame.cpp, line 1457] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 833] nsTableRowGroupFrame::ReflowChildren [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableRowGroupFrame.cpp, line 448] nsTableRowGroupFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableRowGroupFrame.cpp, line 1212] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 833] nsTableFrame::ReflowChildren [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableFrame.cpp, line 3294] nsTableFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableFrame.cpp, line 2004] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 833] nsTableOuterFrame::OuterReflowChild [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableOuterFrame.cpp, line 1028] nsTableOuterFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\table\src\nsTableOuterFrame.cpp, line 1613] ViewportFrame::ReflowFixedFrame [d:\builds\seamonkey\mozilla\layout\html\base\src\nsViewportFrame.cpp, line 383] ViewportFrame::IncrementalReflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsViewportFrame.cpp, line 487] ViewportFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsViewportFrame.cpp, line 555] IncrementalReflow::Dispatch [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 956] PresShell::ProcessReflowCommands [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6536] ReflowEvent::HandleEvent [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6393] PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 597] PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 530] _md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 1078] nsAppShellService::Run [d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 458] main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1474] main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1821] WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1839] WinMainCRTStartup() KERNEL32.DLL + 0x7903 (0x77e87903)
Assignee: attinasi → kmcclusk
Keywords: testcase
*** Bug 159344 has been marked as a duplicate of this bug. ***
reproduced: 1.1 build 2002072514, win2k sp2 I haven't been able to send the talkback incidents. get connection failed errors in talkback. I will report the id as soon as i can get it.
Moiz, the external repeater had a "hiccup" and our servers lost connection. I have restarted the servers now. You should be able to submit your Talkback incident.
Talkback ID: TB8688250K Using Windows 98, build 20070723
Here they are: TB ID: TB8682557X TB ID: TB8682569H
All of the above Talkback incidents have similar stacks with the "nsBlockBandData::Init" signature. We've got a clear tetcase with multiple users crashing. Kevin, should Karnaze get this one?
Keywords: topcrash+
Summary: Will crash almost all later versions of mozilla with simple table/css/form combination → Will crash almost all later versions of mozilla with simple table/css/form combination [@ nsBlockBandData::Init] Trunk
->karnaze
Assignee: kmcclusk → karnaze
Priority: -- → P2
Problem is that nsBlockFrame::Reflow() is called with aReflowState.mSpaceManager=NULL for a frame that has NS_BLOCK_SPACE_MGR=0 (because of the missing TD). Viewport(-1)@0x829e124 nsContainerFrame::ReflowChild for -- GfxScroll(html)(-1)@0x829e3d4 mSpaceManager=(nil) NS_BLOCK_SPACE_MGR=1 Canvas(html)(-1)@0x829e160 nsContainerFrame::ReflowChild for -- Area(html)(-1)@0x82bff38 mSpaceManager=(nil) NS_BLOCK_SPACE_MGR=1 nsBlockFrame::Reflow - Area(html)(-1)@0x82bff38 -- aReflowState.mSpaceManager=(nil) constructed new space manager 0x82c5bf8 (replacing (nil)) Area(html)(-1)@0x82bff38 ReflowBlock Block(body)(1)@0x82c00b8 nsBlockFrame::Reflow - Block(body)(1)@0x82c00b8 -- aReflowState.mSpaceManager=0x82c5bf8 restoring old space manager (nil) ViewportFrame::ReflowFixedFrames reflowState.mSpaceManager=(nil) GetNearestContainingBlock for Placeholder(table)(0)@0x82c0674 is Block(body)(1)@0x82c00b8 TableOuter(table)(0)@0x82c03e0 nsContainerFrame::ReflowChild for -- Table(table)(0)@0x82c05b0 mSpaceManager=(nil) NS_BLOCK_SPACE_MGR=0 Table(table)(0)@0x82c05b0 nsContainerFrame::ReflowChild for -- TableRowGroup(tbody)(3)@0x82c084c mSpaceManager=(nil) NS_BLOCK_SPACE_MGR=0 TableRowGroup(tbody)(3)@0x82c084c nsContainerFrame::ReflowChild for -- TableRow(tr)(0)@0x82c09d8 mSpaceManager=(nil) NS_BLOCK_SPACE_MGR=0 TableRow(tr)(0)@0x82c09d8 nsContainerFrame::ReflowChild for -- Block(form)(1)@0x82c0b24 mSpaceManager=(nil) NS_BLOCK_SPACE_MGR=0 nsBlockFrame::Reflow - Block(form)(1)@0x82c0b24 -- aReflowState.mSpaceManager=(nil) ###!!! ASSERTION: SpaceManager should be set in nsBlockReflowState: 'mSpaceManager', file nsBlockReflowState.cpp, line 91
The source of the bug is a reference to a mSpaceManager whose value is NULL. The bug happens in the initializaton of the reflowstate, wihch will be passed down to a child blockframe for its reflow. mSpaceManager of the reflowstate can be created when reflow a blockframe 1) according to the previous reflow frame's reflowstate's mSpaceManager. 2) if the mState of the blockframe is set NS_BLOCK_SPACE_MGR. As for the testcase of the bug, becuase the mState of the blockframe(form) is not set NS_BLOCK_SPACE_MGR and also the previous reflowstate's mSpaceManager is also NULL,then the crash happened. The inremental reflow process is below: VP 0439CCBC r=1 a=19170,13365 c=19170,13365 cnt=213 tblO 0434F6EC r=0 a=19170,UC c=0,0 cnt=214 tbl 0434F8E4 r=0 a=19170,UC c=UC,UC cnt=215 rowG 0434FA9C r=0 a=UC,UC c=UC,UC cnt=216 row 0434FC28 r=0 a=UC,UC c=UC,UC cnt=217 ******************************** NOT a cell ............. block 0434FD94 r=2 a=0,0 c=UC,UC cnt=218 Block(form)(0)@0434FD94: begin resize reflow availSize=0,0 computedSize=1073741824,1073741824 crash!!!!!! If you add a <TD> in the testcase, there will no crash. the reason that when create of the TableCellInnerframe, its mState is set NS_BLOCK_SPACE_MGR,so the when reflow TableCellInnerframe , the mSpaceManager is created,then when reflow the blockframe(form),the reflowstate's mSpaceManager can be initialized by the previous reflowstate. The inremental reflow process is below: VP 0400B07C r=1 a=19095,13365 c=19095,13365 cnt=197 tblO 033A13E4 r=0 a=19095,UC c=0,0 cnt=198 tbl 033A15DC r=0 a=19095,UC c=UC,UC cnt=199 rowG 033A1794 r=0 a=UC,UC c=UC,UC cnt=200 row 033A1920 r=0 a=UC,UC c=UC,UC cnt=201 cell 033A1A94 r=0 a=UC,UC c=UC,UC cnt=202 block 033A1AF4 r=0 a=UC,UC c=UC,UC cnt=203 Block(td)(0)@033A1AF4: begin initial reflow availSize=1073741824,1073741824 computedSize=1073741824,1073741824 Block(td)(0)@033A1AF4: marking all lines dirty: reason=0 availWidth=1073741824 textAlign=0 Block(td)(0)@033A1AF4: reflowing dirty lines computedWidth=1073741824 line=04014A60 mY=0 dirty=yes oldBounds={0,0,0,0} oldCombinedArea={0,0,0,0} deltaY=0 mPrevBottomMargin=0 block 04014A10 r=0 a=UC,UC c=UC,UC cnt=204 Block(form)(0)@04014A10: begin initial reflow availSize=1073741824,1073741824 computedSize=1073741824,1073741824 OK !!!!!! If you delete the style property of <table>,you will find the reflow command process is different,and when <body> 's blockframe mSpaceManager is created ,so the latter blockframe including <form>'s blockframe 's reflowstate can get mSpaceManager. VP 0419C5A4 r=1 a=19095,13365 c=19095,13365 cnt=232 scroll 0419C854 r=1 a=19095,13365 c=19095,13365 cnt=233 scroll 0419C854 r=1 a=19095,13365 c=19095,13365 cnt=234 canvas 0419C5E0 r=1 a=19095,UC c=19095,13365 cnt=235 area 03433654 r=1 a=19095,UC c=19095,UC cnt=236 Area(html)(-1)@03433654: begin incremental reflow availSize=19095,1073741824 computedSize=19095,1073741824 Area(html)(-1)@03433654: incrementally reflowing dirty lines computedWidth=19095 line=03433824 mY=0 dirty=yes oldBounds={120,0,18855,0} oldCombinedArea={120,0,18855,0} deltaY=0 mPrevBottomMargin=0 block 034337D4 r=1 a=19095,UC c=18855,UC cnt=237 Block(body)(1)@034337D4: begin incremental (ReflowDirty) reflow availSize=19095,1073741824 computedSize=18855,1073741824 Block(body)(1)@034337D4: incrementally reflowing dirty lines: type=ReflowDirty(2) computedWidth=18855 line=03434180 mY=0 dirty=yes oldBounds={0,0,0,0} oldCombinedArea={0,0,0,0} deltaY=0 mPrevBottomMargin=0 tblO 03433AB8 r=0 a=18855,UC c=0,0 cnt=238 tbl 03433C80 r=0 a=18855,UC c=UC,UC cnt=239 rowG 03433E38 r=0 a=UC,UC c=UC,UC cnt=240 row 03433FC4 r=0 a=UC,UC c=UC,UC cnt=241 block 03434130 r=2 a=0,0 c=UC,UC cnt=242 Block(form)(0)@03434130: begin resize reflow availSize=0,0 computedSize=1073741824,1073741824 OK !!!! BTW,the difference of reflow process between the last testcase and the former two,is not related to the style property of <table> directly, just becuase the former two testcase added a diffrent inremental reflowcommand you can find the further explaination according to http://lxr.mozilla.org/seamonkey/source/layout/doc/HLD-SpaceManager.html and http://lxr.mozilla.org/seamonkey/source/layout/doc/DD-SpaceManager.html
see previous comments #13
Depends on: 151860
Attached patch add a new condition (obsolete) — — Splinter Review
after talk with bz, add a new condition
Attachment #94434 - Attachment description: add a new dicision → add a new condition
Attachment #94427 - Attachment is obsolete: true
We (jkeiser, leon, and I) just talked about this for a while... We're thinking that the best fix here would be to not even create this form frame. It's not doing anything useful anyway....
Attachment #94434 - Attachment is obsolete: true
bz, I wonder there should be another testcase for this bug. for with the fix of 151860, this testcase will work fine.
new patch after talk with bz and jkeiser
Comment on attachment 94442 [details] [diff] [review] not create frame whose parent is table or tr Won't this also not build frames for any descendants of the <form>?
Comment on attachment 94442 [details] [diff] [review] not create frame whose parent is table or tr Please remove the existing cases that try to create a pseudo parent frame for the form (MustGeneratePseudoParent, TableProcessChild and ConstructFrameByDisplayType all deal with this). To answer bz's question, children of the <form> are currently placed outside of the <table> entirely--the form is essentially closed and subsequent elements are handled as if they were children of the table or tr. Also, I think the actual code to do this belongs in ConstructHTMLFrame(), and doesn't need to check whether it's block or not--just check if it's form. That way we can remove all this crud.
Attachment #94442 - Flags: needs-work+
Before I think fix of bug 151860 can resolve this bug.So I make it depend on 151860. but when I dig into code, I found that it is different. See http://lxr.mozilla.org/mozilla/source/htmlparser/src/nsElementTable.cpp#114 mozilla treat <form> as child of <tr> So to <table><tr><form>, it should not insert <td> between <tr> and <form>. Although Css spec said that it should do, and HTML4 dtd said that <tr> has only two child <td> and <th>.
also helpful to me. :)
Attachment #94442 - Attachment is obsolete: true
Attachment #94598 - Attachment is obsolete: true
Attachment #94599 - Attachment description: a little modification based previous one → a little modification based on previous one
Attached patch delete useless variable (tag) (obsolete) — — Splinter Review
Attachment #94599 - Attachment is obsolete: true
Comment on attachment 94600 [details] [diff] [review] delete useless variable (tag) r=jkeiser, but keep the braces around { rv = ConstrucTableForeignFrames(...); } ... they are there for consistency of blocking and some portability reason. But karnaze needs to ok this too, please don't check in until he has.
Attachment #94600 - Flags: review+
karnaze is on vacation now, so wait.
// exclude tags // XXX Now that form does not have a special frame, can we remove this? - if ( (nsLayoutAtoms::commentTagName == aTag) || - (nsHTMLAtoms::form == aTag) ) { + if ( (nsLayoutAtoms::commentTagName == aTag) { That XXX comment can go. - { - nsCOMPtr<nsIAtom> tag; - aChildContent->GetTag(*getter_AddRefs(tag)); - // A form doesn't get a psuedo frame parent, but it needs a frame, so just use the current parent - // XXX now that form is a normal frame, do we need to do this? Could we please add an assertion here that the tag is not an HTML form? Something like: #ifdef DEBUG nsCOMPtr<nsINodeInfo> nodeInfo; aChildContent->GetNodeInfo(*getter_AddRefs(nodeInfo)); NS_ASSERTION(!nodeInfo || !nodeInfo->Equals(nsHTMLAtoms::form, kNameSpaceID_None, "How did this HTML form sneak in here? Who broke ConstructHTMLFrame??"); #endif // DEBUG + else if (nsHTMLAtoms::form == aTag) { + if ( ( nsLayoutAtoms::tableRowFrame == parentType ) + || (nsLayoutAtoms::tableFrame == parentType) ) { Where is parentType defined? Also, we want to check that aNameSpaceId == kNameSpaceID_None here, do we not? Otherwise we'll trigger this for <html:form> in random XML documents, and I think we want to treat those as TableForeignFrames. Lose the weird spacing on that inner if, by the way. And combine all the conditions into a single conditional statement (&& and || are meant to be used, after all. ;) )
Also, please put this bug # in that assertion so people aren't mystified as to why it's a Bad Thing :)
Attached patch new version (obsolete) — — Splinter Review
Attachment #94600 - Attachment is obsolete: true
Attached patch a new patch (obsolete) — — Splinter Review
Attachment #94733 - Attachment is obsolete: true
Attached patch patch( new version) (obsolete) — — Splinter Review
Attachment #94734 - Attachment is obsolete: true
Attached patch new version (obsolete) — — Splinter Review
Attachment #94744 - Attachment is obsolete: true
Attached patch a new one (obsolete) — — Splinter Review
BTW, I found that in the line below: + else if (nsHTMLAtoms::form == aTag && kNameSpaceID_None == aNameSpaceID) when access a local testcase m1.html( is same as the testcase of this bug), the value of aNameSpaceID is 3,this means it is a xhtml file (" #define kNameSpaceID_XHTML 3 " ). So i think that "kNameSpaceID_None == aNameSpaceID" should be deleted? in addition, ASSERTION is OK,can display the warning info
Attachment #94749 - Attachment is obsolete: true
I think last part can be: *** else if (nsHTMLAtoms::form == aTag ) { nsCOMPtr<nsIContent> content; aParentFrame->GetContent(getter_AddRefs(content) ); if (content) { nsCOMPtr<nsINodeInfo> parentNodeInfo, childNodeInfo; content->GetNodeInfo(*getter_AddRefs(parentNodeInfo)); aContent->GetNodeInfo(*getter_AddRefs(childNodeInfo)); if ((parentNodeInfo->Equals(nsHTMLAtoms::table, kNameSpaceID_None) || parentNodeInfo->Equals(nsHTMLAtoms::tr, kNameSpaceID_None) )&& *** childNodeInfo->Equals(nsHTMLAtoms::form, kNameSpaceID_None)) { // make <form> be display:none if its parent is <tr> or <table> in html // see bug 159359 nsStyleDisplay* mutdisplay = (nsStyleDisplay*)aStyleContext->GetUniqueStyleData(aPresContext, eStyleStruct_Display); mutdisplay->mDisplay = NS_STYLE_DISPLAY_NONE; aState.mFrameManager->SetUndisplayedContent(aContent, aStyleContext); } } } since the ASSERTION of previous part is effective, modification above is also effective. when test the testcase: <table style="position: fixed; top: 0px;"> <!-- Crashes Mozilla --> <tr> <form> </form> </tr> </table> the values of childNodeInfo->Equals(nsHTMLAtoms::form, kNameSpaceID_None), parentNodeInfo->Equals(nsHTMLAtoms::table, kNameSpaceID_None),parentNodeInfo->Equals(nsHTMLAtoms::tr, kNameSpaceID_None is 1,0,1 .
Leon, that's exactly what it should be. Don't use aNameSpaceID in that function; that value is not reliable. See nsGenericHTMLElement::GetNameSpaceID for the reason.... ;)
Attachment #94756 - Attachment is obsolete: true
Comment on attachment 94804 [details] [diff] [review] a little modification according to comments#35,36 sr=bzbarsky if you get rid of all those tabs and clean up the indentation in the last chunk (the code inside ther innermost block is over-indented and again there are the tabs).
Attachment #94804 - Flags: superreview+
Comment on attachment 94816 [details] [diff] [review] delete tabs and indentation of previous patch r=jkeiser Need if ((parentNodeInfo stuff to line up with the if (<here>
Attachment #94816 - Flags: review+
Comment on attachment 94816 [details] [diff] [review] delete tabs and indentation of previous patch sr=bzbarsky. This still has a tab before the "rv" in + rv = ConstructTableForeignFrame(aPresShell, aPresContext, aState, aChildContent, and the args for that function need to line up with the first line, as well as the whitespace issue jkeiser noted. Please make sure to fix those problems before checking in. If you don't have CVS access yet, let me know and I'll land this.
Attachment #94816 - Flags: superreview+
Comment on attachment 94816 [details] [diff] [review] delete tabs and indentation of previous patch OK. I'm withdrawing my sr, sorry. I applied the patch and ran it on the testcase. It asserted. That would be Bad (and why I asked for the assertion; the implication was that the assertion should not fire). It seems the call to TableProcessChild happens _before_ the display type has been changed. So we need to either: 1) Change dispay earlier or 2) Not call ConstructTableForeignFrame bin cases when that assertion would fire. Again, my apologies for not catching this just by looking at the patch. The flow of control in this code is a little convoluted.
Attachment #94816 - Flags: superreview+ → needs-work+
I think I should set "display:none" in func: ConstructFrameByDisplayType, because when meet <table>, func:ConstructFrameByDisplayType --> ConstructTableFrame-->TableProcessChild --> TableProcessChild ,which will process tags embeded in the <table>/<tr> ,then fire the ASSERTION before set "display:none".
Attached patch new patch (obsolete) — — Splinter Review
Becuase of comments #42 and #43,now set "display:none" in "TableProcessChild".
Attachment #94804 - Attachment is obsolete: true
Attachment #94816 - Attachment is obsolete: true
Comment on attachment 94833 [details] [diff] [review] new patch You shouldn't modify a style struct like this. You also shouldn't cast away const like this, without an explicit new-style NS_CONST_CAST. However, there's no need to do that since you shouldn't be modifying the style struct since it can be shared with other style contexts. You should also prefer the typesafe form of |GetStyleData|, although it doesn't work quite so well with nsCOMPtrs on some platforms, so you might need: const nsStyleDisplay *display; ::GetStyleData(NS_STATIC_CAST(nsIStyleContext*, childStyleContext), &display);
Attachment #94833 - Flags: needs-work+
Comment on attachment 94833 [details] [diff] [review] new patch This patch also looks like it will fail to call |ConstructTableForeignFrame| for anything with 'display: block'. Perhaps all that's needed, though, is the removal of the |break;|, with an explicit comment explaining why you're falling through. Also, why do you have explicit parent checks for the conditions causing the problem? Do we really want to construct the form if the parent is a thead, tfoot, or tbody? (Or col or colgroup, if this function is used for them?) Or is |TableProcessChild| used for other parents as well?
1) about comments#45,we can create a new viarable,get a new "*display". :) (I'm a little lazy at that time:) ) 2) about comments#46, the current plan is: if we find <table> <form> or <table> <tr> <form> in a html file,we will not create frame for <form>. 3) the crash reason is included in comments #13,and a previous resolution is: create a spacemanager for the reflowstate which will be used by (form)blockframe,if the spacemanager is NULL. the first 2 patchs is for this. 4) after discuss with jkeiser and bz,we select current plan. the basic reason is: if not create frame for the block,then the (form)blockframe reflow crash can be avoided.
Basically, we want to avoid constructing a foreign frame (or any frame at all) any time an HTML form is in an HTML table in a place where we cannot allow whitespace. I agree that we should check the behavior of IE for tbody/thead/tfoot in addition to table/tr. As for changing the style context, do we really need to do that? Can we not just not construct a frame? I suppose the context could be shared with things other than <form>s....
next step , i think I should do: 1) delete these code about set styleDisplay->mDisplay, I didn't find it is very necessary,since we do not paln to create a frame for this <form>. 2) judge whether the parent of <form> is <tbody>/<thead>/<tfoot>. 3) delete that "break", becuase of it, other types of file can not displayed correctly. although I have test the code sveral times lat night. :)
Attached patch new version (obsolete) — — Splinter Review
mozilla is ok, when applied to testaces below: testcase1: <table style="position:fixed;"> <tr> <form> </form> </tr> </table> testcase2: <table style="position:fixed;"> <form> </form> </table> testcase3: <div style="position: fixed; top: 50px; display: table"> <div style="display: table-row"> <form> yyyyyyyyyyyyyyyyyyyyy <form> </div> </div> testcase4: <div style="position: fixed; top: 50px; display: table"> <form> aaaa <form> </div>
Attachment #94833 - Attachment is obsolete: true
Attachment #94888 - Attachment is obsolete: true
Attached patch patch (obsolete) — — Splinter Review
> - const nsStyleDisplay* styleDisplay = (const nsStyleDisplay*) > + nsStyleDisplay* styleDisplay = ( nsStyleDisplay*) Why this change? > + // if <form>'s parent is <tr> or <table> in html, This comment does not match the code.... I assume you've tested this in debug mode this time? ;)
Attached patch new one (obsolete) — — Splinter Review
1) recover "const .." part. 2) match comments to code. :)
Attachment #94889 - Attachment is obsolete: true
Comment on attachment 94893 [details] [diff] [review] new one >+ case NS_STYLE_DISPLAY_BLOCK: Why not just do this in the |default| case (like in the current code)? Style rules such as |form { display: inline; }| are likely to be somewhat common in the near future if they're not already, and as long as we're checking that the parent is something that's not allowed to contain form per HTML/XHTML validity, we can almost claim that we're not violating standards. The code to avoid generating the anonymous frames around the "foreign" frame used to kick in in those cases, and generating the anonymous frames might do something unexpected. (Then again, it's not all that important. But if you do decide to leave things as is, you definitely need a clearly visible comment where you're falling through to the |default:| case that the omission of |break;| was intentional.) >+ if (aParentContent) { |aParentContent| is guaranteed never to be null. >+ nsCOMPtr<nsINodeInfo> parentNodeInfo, childNodeInfo; >+ aParentContent->GetNodeInfo(*getter_AddRefs(parentNodeInfo)); >+ aChildContent->GetNodeInfo(*getter_AddRefs(childNodeInfo)); >+ if ((parentNodeInfo->Equals(nsHTMLAtoms::table, kNameSpaceID_None) || >+ parentNodeInfo->Equals(nsHTMLAtoms::tr, kNameSpaceID_None) || >+ parentNodeInfo->Equals(nsHTMLAtoms::tbody, kNameSpaceID_None) || >+ parentNodeInfo->Equals(nsHTMLAtoms::thead, kNameSpaceID_None) || >+ parentNodeInfo->Equals(nsHTMLAtoms::tfoot, kNameSpaceID_None)) && >+ childNodeInfo->Equals(nsHTMLAtoms::form, kNameSpaceID_None)) { These checks seem like they won't work for XHTML, since the namespace will be XHTML. Futhermore, you don't want the None tests if the document isn't an HTML document. In other words, you should use nsIContent::IsContentOfType and then check the tag, I think, rather than using these nodeinfo checks.
Attachment #94893 - Flags: needs-work+
David, the XHTML/HTML part of that is actually correct, in my opinion (it was put in at my insistence). The issue is markup like this: <table> <form> <tr> </tr> </form> </table> In HTML, the <tr> will end up a child of <table> in the content model due to the parser hacking things. We will want to not have a frame for teh <form> because it would break layout on all these crappy pages. In XHTML, the <tr> will be a child of the <form>, since the XML parser is used. We want to make a frame for the <form> because otherwise there will be no frame for the <tr> either.... And people who get extra space from markup like this on an _XHTML_ page (served with a non-text/html mimetype) deserve what they get. If the document is not an HTML document, then the namespace checks will return false because the nodeinfo has the root namespace.
Comment on attachment 94893 [details] [diff] [review] new one In addition, move the ::form check to the beginning of the if() and put the comment on the outside of that whole block starting with nsCOMPtr<nsINodeInfo>. Also, the comment is inaccurate. The point is not that we're not creating a block frame (that was fixed when you removed that code that does ConstructFrame)--the point of that code is to prevent it from creating a psuedoframe below. dbaron has a valid point about XHTML as well as forms with display: inline--this should operate there too. Perhaps a quirks / strict difference is in order instead ... bz?
bz's point about XHTML makes sense to me, so that part of the current patch seems fine, although I don't understand what you mean by the root namespace.
Attachment #94893 - Attachment is obsolete: true
Comment on attachment 95060 [details] [diff] [review] patch according to previous comments :) r/sr=bzbarsky. test well! ;)
Attachment #95060 - Flags: superreview+
Comment on attachment 95060 [details] [diff] [review] patch according to previous comments :) It doesn't really matter, I think, but I'd feel better if you did break; instead of return NS_OK; r=jkeiser with that
Attachment #95060 - Flags: superreview+
Attachment #95060 - Flags: superreview+
Attachment #95060 - Flags: review+
bz, before you check in the patch (attachment 95060 [details] [diff] [review]) tomorrow,pls replace "return NS_OK" with "break",according to what jkeiser said(in comments #60) . thank you.
Fix with "break" instead of "return" checked in. Nicely done, Leon!
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Attachment #95060 - Attachment is obsolete: true
Comment on attachment 95226 [details] [diff] [review] judge whether parentNodeInfo or childNodeInfo is NULL is necessary r=jkeiser
Attachment #95226 - Flags: review+
An nsIContent that corresponds to an element will never have a null nodeinfo. If it does, the content module will have crashed long before we get into this code. If we're constructing a frame for a non-element, someone fucked up and should be fixed. In short, null-checking those nodeinfos is not something we should be doing.
Attachment #95060 - Attachment is obsolete: false
i now crash in nsCSSFrameConstructor::TableProcessChild when going to http://home.c2i.net/dark/linux.html Not sure if it's brand new, but "new'ish" is a good guess (trunk CVS, Linux)
Attached patch Fix the crash (obsolete) — — Splinter Review
OK. I was semi-wrong. The problem is that sometimes aChildContent is a #text node (and unfortunately this is necessary at times). This patch fixes the crash rkaa is seeing by making sure we don't ask said #text nodes for their nodeinfos and such (since they're not forms in any case).
Attachment #95060 - Attachment is obsolete: true
Attachment #95226 - Attachment is obsolete: true
Attached patch -w version for review (obsolete) — — Splinter Review
Attachment #95295 - Attachment is obsolete: true
Attachment #95297 - Attachment is obsolete: true
Comment on attachment 95308 [details] [diff] [review] jst prefers it be done this way, so... r=sicking
Attachment #95308 - Flags: review+
Attachment #95308 - Attachment is obsolete: true
Comment on attachment 95308 [details] [diff] [review] jst prefers it be done this way, so... checked in
No longer depends on: 151860
Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1
Flags: in-testsuite+
Crash Signature: [@ nsBlockBandData::Init]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: