Closed Bug 355986 Opened 18 years ago Closed 18 years ago

ASSERTION: internal error: 'NS_STYLE_DISPLAY_TABLE_ROW == rowFrame->GetStyleDisplay()->mDisplay'

Categories

(Core :: MathML, defect)

Product:
Core
Component:
MathML
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: rbs)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 1 obsolete file)

testcase
443 bytes, application/xhtml+xml
Details
patch
4.71 KB, patch
bernd_mozilla
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
The testcase triggers:

###!!! ASSERTION: internal error: 'NS_STYLE_DISPLAY_TABLE_ROW == rowFrame->GetStyleDisplay()->mDisplay', file /Users/admin/trunk/mozilla/layout/mathml/base/src/nsMathMLmtableFrame.cpp, line 253

###!!! ASSERTION: bad caller: 'aContent->Tag() == nsMathMLAtoms::mstyle_ || aContent->Tag() == nsMathMLAtoms::mtable_', file /Users/admin/trunk/mozilla/layout/mathml/base/src/nsMathMLFrame.cpp, line 90
Attached file testcase — 

    
    

    
  
This is not a regression from bug 322625, for what it's worth.  What's going on here is that:

body * { 
  display: table-footer-group; 
  overflow: -moz-scrollbars-vertical; 
}

Means in particular that the <mi> is a scrollable rowgroup (which we allow, in general).  Then we get to:

nsMathMLmtableFrame::SetInitialChildList which calls MapAllAttributesIntoCSS which has:

MapAllAttributesIntoCSS(nsIFrame* aTableFrame)
{
  // mtable is simple and only has one (pseudo) row-group
  nsIFrame* rowgroupFrame = aTableFrame->GetFirstChild(nsnull);
  if (!rowgroupFrame) return;

  nsIFrame* rowFrame = rowgroupFrame->GetFirstChild(nsnull);
  for ( ; rowFrame; rowFrame = rowFrame->GetNextSibling()) {
    DEBUG_VERIFY_THAT_FRAME_IS(rowFrame, TABLE_ROW);

which is bogus.  GetFirstChild() on aTableFrame can return _either_ a rowgroup or a scrollframe containing the rowgroup.  This code needs to deal with both cases.

    
    

    
  
"<mi> is a scrollable rowgroup" (if that means any thing) is not worth caring about, as nobody expects that to work in MathML.

So for MathML purposes, I am just fine to have an early return if the firstchild isn't a rowgroup. (That is, don't bother mapping any attributes on such contrived situations.)

    
    

    
  
As long as it doesn't crash or assert, I don't really care what this code does, sure.

    
    

    
  
>nsIFrame* rowgroupFrame = aTableFrame->GetFirstChild(nsnull);
assuming that this a rowgroup is common error which sooner or later leads to a crash. One has to call nsTableFrame::GetRowGroupFrame  http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableFrame.cpp#1228
on it.





    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch in progress (obsolete)
        — 
          — Splinter Review
      

    


  
Remains to understand why the other assertion is firing and resolve that as needed.

    
    

    
  
Here it the frame tree (...I mean the forest...) when the assertion happens. Given this, there is no point for <mtable> to try to be too clever and a half. The early returns are just fine. Also the second assertion can be fixed by not calling the function when the tag isn't <mtable>. I will attach an updated patch.

Inline(math))
 TableOuter(math)
  Table(math)
   HTMLScroll(mtable)
    ScrollbarFrame(scrollbar)
     ButtonBoxFrame(scrollbarbutton)
     SliderFrame(slider)
      ButtonBoxFrame(thumb)
       Box(gripper)
      >
     >
     ButtonBoxFrame(scrollbarbutton)
    >
    TableRowGroup(mtable)
     TableRow(mtable)
      TableCell(mtable)
       Block(mtable)
        line 04D3818C:
         TableOuter(mtable)
          Table(mtable)
           HTMLScroll(mtr)
            ScrollbarFrame(scrollbar)
             ButtonBoxFrame(scrollbarbutton)
             SliderFrame(slider)
              ButtonBoxFrame(thumb)
               Box(gripper)
              >
             >
             ButtonBoxFrame(scrollbarbutton)
            >
            TableRowGroup(mtr)
             TableRow(mtr)
              TableCell(mtr)
               Block(mtr)
                line 04D382C0:
                 TableOuter(mtr)
                  Table(mtr)
                   HTMLScroll(mtd)
                    ScrollbarFrame(scrollbar)
                     ButtonBoxFrame(scrollbarbutton)
                     SliderFrame(slider)
                      ButtonBoxFrame(thumb)
                       Box(gripper)
                      >
                     >
                     ButtonBoxFrame
                    >
                    TableRowGroup(mtd)
                     TableRow(mtd)
                      TableCell(mtd)
                       Block(mtd)
                        line 04D383F4:
                         TableOuter(mtd)
                          Table(mtd)
                           HTMLScroll(mi)
                            ScrollbarFrame(scrollbar
                             ButtonBoxFrame(scrollb
                             SliderFrame(slider)
                              ButtonBoxFrame(thumb)
                               Box(gripper)(
                              >
                             >
                             ButtonBoxFrame(scrollb
                            >
                            TableRowGroup(mi)
                             TableRow(mi)
                              TableCell(mi)
                               Block(mi)
                                line 04D38528:
                                 Text
                                  ""
                                 >
                                >
                               >
                              >
                             >
                            >
                           >
                          >
                          ColGroup-list<
                           TableColGroup(mtd)
                            TableCol(mtd)
                           >
                          >
                         >
                        >
                       >
                      >
                     >
                    >
                   >
                  >
                  ColGroup-list<
                   TableColGroup(mtr)
                    TableCol(mtr)
                   >
                  >
                 >
                >
               >
              >
             >
            >
           >
          >
          ColGroup-list<
           TableColGroup(mtable)
            TableCol(mtable)
           >
          >
         >
        >
       >
      >
     >
    >
   >
  >
  ColGroup-list<
   TableColGroup(math)
    TableCol(math)
   >
  >
 >
>


    
    

    
  
Is there a reason why you don't go the way of  nsTableFrame::GetRowGroupFrame ?

    
    

    
  
It is just that the MathML attributes become irrelevant in such cases. So we may as well avoid doing MathML-specific things and default to the default table behavior.

(Also if we dare say in mozilla.dev.tech.mathml that we are supporting such things, people will stare at us in disbelief, and consider them a waste of time and energy, and possibly an abuse. It is not buying us much to have such extended features in MathML.)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  

        Attachment #241675 -
        Attachment is obsolete: true

        Attachment #241751 -
        Flags: superreview?(bzbarsky)

        Attachment #241751 -
        Flags: review?(bernd_mozilla)

    
    

    
  
Comment on attachment 241751 [details] [diff] [review]
patch

>Index: layout/mathml/base/src/nsMathMLmtableFrame.cpp
>+  if (mContent->Tag() == nsMathMLAtoms::mtable_)
>+    nsMathMLFrame::FindAttrDisplaystyle(mContent, mPresentationData);

As a note, both this and the assert it's avoiding don't play nice with XBL.

Looks ok modulo that.

        Attachment #241751 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
Comment on attachment 241751 [details] [diff] [review]
patch

+// useful to debug StirDOM bugs 
please remove this comment line

r+ with this

<offtopic>
I am only curios, what MapAllAttributesIntoCSS has to do inside layout, doesn't that belong to content? Does MathML build its own style resolution/attribute mapping
</offtopic>

        Attachment #241751 -
        Flags: review?(bernd_mozilla) → review+

    
    

    
  
> doesn't that belong to content?

See bug 69409 and especially bug 276267.  The latter has been waiting on review from rbs for about a year and a half now... It probably needs some updating.  :(

    
    

    
  
Actually, don't assume that bug 276267 solve the problem (which is why I haven't much followed it up yet. I just felt like that patch is way too big for what it buys us -- at present). The problem is that MathML attributes are very different from HTML. Only about 7 simple attributes like mathcolor, mathsize, are very easy to map from the content. The rest is a nebulous entanglement. To give you an idea in <mtable>, consider:

<mtable columnalign="top bottom">
  <mtr rowalign="top bottom">,

This is a multi-value attribute, which means that cells on the first column have "text-align:top", those of the second column have text-align:bottom. Similarly for rows with vertical-align. And there are plenty attributes like that. Not only that there is another catch. The indices. If you want to map the attributes from the content, you have to re-implement the famous (or rather infamous?) cellmap, and deal with cell that may not be there in the middle due to display:none (and what about those pseudo frames that come from nowhere...). 

So MapAllAttributesIntoCSS() sneakly waits until the mighty table code has done its fixups and build its indices, then it just figures out the stylistic effects that MathML wants in the cells.

The MathML spec was not really designed to be an intimate friend with CSS. Major browsers were not open source back then, and the spec guys (most from vendor/commercial companies) seemed to bet on self-contained plug-ins.

Here is an excerpt from someone from the MathPlayer plug-in (Paul Topping, the President of Design Science) just yesterday on mozilla.dev.tech.mathml:

"Finally, math formatting has to
deal with spacing, fonts, character styles, etc. in ways that work for
math. While adding <b> inside <mtext>, for example, may seem to be
innocent enough, the potential complexity of interaction of such tags
with MathML's built-in formatting rules is scary. This is bad enough
already with CSS interacting with MathML."

Even in 2006, the nitfy CSS that we provide and that we allow to mix freely isn't well regarded -- at least by the competition... perhaps because they can't emulate that without writing a complete CSS system, which isn't quite a walk in the park.

    
    

    
  
[<mtable columnalign="top bottom"> should be <mtable columnalign="left right">. Also add to the sauce rowspans and colspans, and it becomes quite tasty.]

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

    
    

    
  
this is already tasty enough:
> This is a multi-value attribute, which means that cells on the first column
> have "text-align:top", those of the second column have text-align:bottom.
A bug 915 fix that sneaked in ;-) Don't tell the affinados there what you have done.

    
    

    
  
> I just felt like that patch is way too big for what it buys us

Telling the patch author that instead of leaving them hanging is good too....  But I can understand feeling this way.

> The MathML spec was not really designed to be an intimate friend with CSS.

Mildly said.  ;)  More like "incompatible with"....

    
    

    
  
Crashtest checked in.
Flags: in-testsuite+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: