###!!! ASSERTION: internal error: 'NS_STYLE_DISPLAY_TABLE_CELL == cellFrame->GetStyleDisplay()->mDisplay', file nsMathMLmtableFrame.cpp, line 458 458 DEBUG_VERIFY_THAT_FRAME_IS(cellFrame, TABLE_CELL); This assertion was added in bug 348153.

I am not hitting the assertion with my build.

With a fresh build, I can see the assertion.

bz, you might remember us discussing about whether IsSpecialContent() should include <mtable> (with its display check)? Jesse has anwsered the question. This bug clearly shows that it is needed -- otherwise by tracing the code and dumping the frame tree, I see that it does not get wrapped into pseudo-frames when it is out of place like in this bug. (Fortunateley, the wrapper of <mtable> is still attached to the frame tree, so we don't actually get the crash of bug 347355.)

Hmm.... So this is basically trying to replace the code in ConstructFrameByDisplayType and ConstructTableFrame that respectively: 1) Process pseudo-elements and 2) Create pseudos to wrap a table, right? Hacking IsSpecialContent might do the trick, but it seems to me that a better approach (or at least one I'm more likely to be able to convince myself is correct) would be to change AdjustParentFrame to create pseudos as needed if the parent is table-related and the child is DISPLAY_TABLE and to change ConstructMathMLFrame to explicitly process pseudo-frames for mtable. That keeps IsSpecialContent meaning what it means now -- something where frames are not constructed based on the display type. Although one could make that with the block wrapper for mtable it falls into this category.... In any case, I think bernd needs to review this, in addition to me.

>a better approach (or at least one I'm more likely to be able to convince >myself is correct) would be to change AdjustParentFrame to create pseudos as >needed if the parent is table-related and the child is DISPLAY_TABLE and to >change ConstructMathMLFrame to explicitly process pseudo-frames for mtable. Guest what? That's what is already happening. In the present testcase, if <html:table> is appended (instead of <mtable), pseudo-frames are created for it. But <mtable> is missing out because there is one crucial thing that makes <mtable> different. It has those wrappers to make it impersonate an inline-table -- as I indicated in the other bug: nsMathMLmrowFrame ( nsBlockFrame ( nsMathMLmtableFrame ) ) Consequently, when it gets out of CounstructMatMLFrame, it does not hand back a frame with display:table anymore, and therefore we cannot rely on ConstructTableFrame to detect it and perform the two steps you alluded to. Because of this hiding, it must be treated like a regular tag -- like the other tags in IsSpecialContent, and this effectively achieves what you suggested: "change ConstructMathMLFrame to explicitly process pseudo-frames for mtable['s topmost wrapper]". It has become very clear to me in the recent days. [When <mtable> is eventually implemented as inline-table, reversing this will just amount to removing the added check.]

Comment on attachment 234015 [details] [diff] [review] patch > Guest what? That's what is already happening. No, it's not. > In the present testcase, if <html:table> is appended (instead of <mtable), > pseudo-frames are created for it. They're created in ConstructTableFrame, which imo is totally the wrong place to be creating them. If we created them in AdjustParentFrame like I propose, things would work fine for both <mtable> and <html:table>. We'd still need to keep the code in ConstructTableFrame because of the TableProcessChild mess, but... On the other hand, keeping it in both places might mess up the !hasPseudoParent check in ConstructTableFrame. Which is why I'd like bernd to look at this. > But <mtable> is missing out because there is one crucial thing that makes > <mtable> different. Yes, indeed. > and therefore we cannot rely on ConstructTableFrame to detect it More to the point, the parent ConstructTableFrame sees is not the table-row. I still think bernd needs to review this, but I guess this is ok as a band-aid. On trunk, I would really like us to stop patching this hackery and just create a working, simple, design that won't crash at the drop of a hat and most importantly will require minimal work on the part of the various Construct* functions to work correctly. Ideally, no work should be required.

Comment on attachment 234015 [details] [diff] [review] patch I see what you mean -- here is an example to support your suspicion: <mtable> <mtr/> <mtable/> <!-- appendChild() directly inside a parent mtable --> The patch doesn't cater for this case. With the patch, tracing in the debugger showed that what happens is: 1) Are the pseudos created to wrap the child <mtable/>? Yes. Because IsSpecialContent() allows <mtable> (with its display check), we effectively create them, in AdjustParentFrame() -- indirectly through its call to: nsresult rv = GetPseudoCellFrame(*tableCreator, aState, *aParentFrame); So after that, aHasPseudoParent=true, and we have a queue of pseudos awaiting the next step: 2) Are the inferred pseudos processed? No (at least not properly). After the first step, we get into ConstructMathMLFrame() And there the patch does if (IsSpecialContent(aContent, aTag, aNameSpaceID, aStyleContext)) { // process pending pseudo frames if (!aHasPseudoParent && !aState.mPseudoFrames.IsEmpty()) { ProcessPseudoFrames(aState, aFrameItems); } } But since we ended earlier with aHasPseudoParent=true, we don't get to call ProcessPseudoFrames(). While a bit different from what you described, the example clearly shows a limitation. I am retracting my patch. Maybe this is indeed the opportunity to do the right thing (tm) with this pseudo business on the trunk. Rather than sprinkling ProcessPseudoFrames() everywhere, I concur that a better system is worth investigating. So the ball bounces back to bernd's court (bug 243159). Since this assertion is not marked as security sensitive, there is no particular urgency and I will leave things as is, until a lighther re-architecture.

Er... in this case the ProcessPseudoFrames happens later -- in ProcessChildren(), or in ContentInserted/ContentRemoved. So that all actually looks correct to me. Are you sure the assert does not indicate a frame tree that could be exploited?

It indeed does something like that (which is why I said "at least not correctly"); When I dump the frame tree, I am getting different things depending on whether the appended child is <mtable/> or <html:table/>: 1) <mtable> <mtr/> <mtable/> 2) <mtable> <mtr/> <html:table/> I would expect identical frame trees. But I get these: 1) Block(math) Frame(mtable) [=nsMathMLmrowFrame wrapper for the inline-table effect] Block(mtable) [=block wrapper] TableOuter(mtable) Table(mtable) TableRowGroup(mtable) TableRow(mtr)( TableCell(mtd) Block(mtd) Frame(mi) 2) Block(math) Frame(mtable) [=nsMathMLmrowFrame wrapper for the inline-table effect] Block(mtable) [=block wrapper] TableOuter(mtable) Table(mtable) TableRowGroup(mtable) TableRow(mtr) TableCell(mtd) Block(mtd) Frame(mi) TableRowGroup(mtable) TableRow(mtable) TableCell(mtable) Block(mtable) TableOuter(table) Table(table) >Are you sure the assert does not indicate a frame tree that could be exploited? Not sure.

Huh. That's with your patch?

Yes. Without it, we get the assertion. With it, we get the incorrect tree. But I recover the correct tree if I iterate on the patch to emulate what is presently done for <html:table>, where ProcessPseudoFrames() is not based on |!aHasPseudoParent|: case NS_STYLE_DISPLAY_TABLE: { // XXXbz this isn't quite right, but checking for aHasPseudoParent here // would just lead us to ProcessPseudoFrames inside ConstructTableFrame. if (!aState.mPseudoFrames.IsEmpty()) { ProcessPseudoFrames(aState, aFrameItems); } nsIFrame* innerTable; rv = ConstructTableFrame(aState, aContent, aParentFrame, aStyleContext, *tableCreator, PR_FALSE, aFrameItems, newFrame, innerTable); [...] } ============ The iteration for that is: @@ IsSpecialContent() #ifdef MOZ_MATHML if (aNameSpaceID == kNameSpaceID_MathML) return + (aTag == nsMathMLAtoms::mtable_ && + aStyleContext->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_TABLE) || @@ ConstructMathMLFFrame() - if (IsSpecialContent(aContent, aTag, aNameSpaceID, aStyleContext) || - (aTag == nsMathMLAtoms::mtable_ && - disp->mDisplay == NS_STYLE_DISPLAY_TABLE)) { + if (IsSpecialContent(aContent, aTag, aNameSpaceID, aStyleContext)) { // process pending pseudo frames - if (!aHasPseudoParent && !aState.mPseudoFrames.IsEmpty()) { + PRBool isTable = + aTag == nsMathMLAtoms::mtable_ && + disp->mDisplay == NS_STYLE_DISPLAY_TABLE; + if ((isTable || !aHasPseudoParent) && !aState.mPseudoFrames.IsEmpty()) { ProcessPseudoFrames(aState, aFrameItems); } } ===== Do you have a suspicion with this iteration?

In the patch format that can be readily applied for experimentation.

I guess that gets us parity with <html:table>. If we copy that XXX comment too, that might be good enough.

OK, getting back on the review board. I am asking the r+sr again as I am not sure if bernd is back yet? This is so that I can try asking for branch approval.

away till 2006-08-21

sorry, before I can argue what should happen with the tablecreator buisiness, go bugs 347725 and 341858

Comment on attachment 234370 [details] [diff] [review] patch v2b This is OK with me as it syncs correctly what MathML table frame creation does with IsSpecialContent.

Comment on attachment 234370 [details] [diff] [review] patch v2b OK.

Comment on attachment 234370 [details] [diff] [review] patch v2b Guys, I am retracting the patch yet again because I have discovered to my disillusion that I am still on the hook here. (While the pacth might be sound, it doesn't entirely solve the problem.) My earlier testing (comment 10) used a static markup. As I was doing a routine testing to checkin -- based on a dynamic markup, I discovered that it still led to losing pseudo frames (as I described earlier -- but due to a different reason this time). Investigating one thing led to another -- and I now suspect that ProcessPseudoFrames() might be buggy (or more precisely, a calling sequence somewhere -- because I see in the debugger that it overwrites data). I am shortly going to document what I see in the debugger. But after that I will have to attend to an urgent deadline here and might be slow in making further immediate comments.

Testcase -- which unfolds the AppendChild() &c. as seen on to the stack. (Note in general that if/when aFrameItems is cleared, there is no other way to access the pseudos.)

Still happens after the patch for bug 243159.

>Still happens after the patch for bug 243159. I tried very hard not to alter MathML.

WFM, Mac trunk.

Crashtests checked in.