347355 - Frames not destroyed (involves <mtable> and <mtd>)

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

*Reloading* this testcase causes my debug build to hit an assertion I added locally, which checks that no frames are "lost" when a FrameArena is destroyed (patch in bug 334514).

Marking as [sg:critical] because I have a slightly larger testcase that causes a debug build of Firefox to crash accessing memory at 0xDADADAF6.  (The testcase I'm attaching does not crash at all; it only triggers the assertion.)

Comment 1

[Jesse Ruderman]

My crashing testcase crashes Firefox 1.5.0.6 (official release build).

Comment 2

[rbs]

Testcase?

Comment 3

[Jesse Ruderman]

Attachment: testcase

Comment 4

[Daniel Veditz [:dveditz]]

If the assertion is only in your local tree, could you attach a different testcase other people could use to verify the fix? The crashing one would be great, you could put it on the security wiki if you don't want to attach it here.

Comment 5

[Jesse Ruderman]

https://intranet.mozilla.org/Security/index.php/Bug_347355

Comment 6

[Daniel Veditz [:dveditz]]

I don't crash using a windows 1.5.0.6 build on the testcase described on the wiki. Is this Mac only? I loaded it from a local file, would it matter if I used http: instead?

Comment 7

[Jesse Ruderman]

Attachment: crashing testcase

Comment 8

[Jesse Ruderman]

Crashes on reload for me with Win32 Firefox15 2006080705.  TB21895290G

Comment 9

[Daniel Veditz [:dveditz]]

Loaded from the bug I crash on reload. It's identical to my hand-edited testcase except for line-endings. I don't crash when I load your attachment 232677 [details] from disk.

The behavior is slightly different when loaded as a file: I get a tiny animated line-art shark next to the letter 't' on the top left side of the page (and then the row is deleted by the testcase). Loaded from the link in the bug I don't see the shark, and the 't' is centered at the top before it's deleted.

Comment 10

[Jesse Ruderman]

When you load it from disk, make sure you are using an extension that causes Firefox to treat it as XML (such as .xhtml) rather than HTML (such as .html).

Comment 11

[rbs]

Attachment: patch

The patch stops invalid <mtd> that are outside <mtr>, and <mtr> that are outside <mtable>, to be given those murky pseudo-frames that nsCSSFrameConstructor has, and which are not handled well by the MathML code. (The MathML 2 spec says that they ought to be inside <mtr> and <mtable>).

Comment 12

[Daniel Veditz [:dveditz]]

I also crash on the 1.8 branch (Bon Echo, both beta 1 and today).

This appears fixed on the trunk sometime between 20060525 and 20060621. Is there some other, better, fix for this crash we should use instead? Using CSS selectors seems like it's got to be fairly slow.

Sorry for the long regression window, July 18 is the earliest nightly available currently, I only had a few random builds on my machine.

Comment 13

[Martijn Wargers (dead)]

(In reply to comment #12)
> This appears fixed on the trunk sometime between 20060525 and 20060621. Is
> there some other, better, fix for this crash we should use instead? Using CSS
> selectors seems like it's got to be fairly slow.

It's not fixed for me on trunk, with the 2006-08-08 trunk build I crash on the 3rd reload with the crashing testcase.
Maybe those css selectors are slow, but it is in mathml.css, so it shouldn't matter for most webpages, only for those which contain mathml.

Comment 14

[rbs]

No worry for the rules. They are really fast (a check of direct parentage), and tag-based rules are the fastest types of rules that the Style System has. Also, it is in mathml.css -- as reminded by martijn. So the impact is negligible either way.

Comment 15

[Jesse Ruderman]

Using CSS rules to work around bugs deep in Gecko scares me.  There are too many ways that could go wrong.  What if...

* the mtd is the root element, and so doesn't have a parent to be checked against :not(mtr)?

* a site finds a way to get around rule using XBL, for example by inserting anonymous non-table stuff between an mtr and an mtd or by making the mtd appear to have no parent?

* a future version of CSS decides that user style sheets override agent style sheets, and someone has a user stylesheet containing the rule "#skipNav { display: inline ! important; }"?

* Mobile Firefox is low on memory and mathml.css fails to load?

* Mobile Firefox is low on memory and parts of mathml.css fail to load?

* the user moved or renamed Firefox.app while it was running, so mathml.css isn't found?  (You can do this on Mac OS X, and it only breaks some things in Firefox!)

* we have a bug where mathml.css only loads if MathML elements are *created* in the correct document?  (Currently, if you call cloneNode on the body of a MathML document and stick the clone in a new document, the new document does not get mathml.css applied.  If you then create a MathML element in that document, mathml.css is suddenly applied to the entire document.  See bug 132844; I imagine there's a plan to make importNode and adoptNode do the right thing once bug 47903 is fixed.)

The patch in bug 307826 scares me for similar reasons, and IMO should be replaced by a fix in C++.

Another problem with this patch is that hiding ":not(mtr) > mtd" is wrong according to MathML 1.  MathML 1 allows both mtr and mtd to be inferred, so an mtd directly inside an mtable is perfectly ok.  (MathML 2 deprecates this inference, but also says "all tools are encouraged to support the old forms as much as possible".)

http://www.w3.org/TR/REC-MathML/chap3_5.html#sec3.5.1
http://www.w3.org/TR/MathML2/chapter3.html#id.3.5.1.1

Comment 16

[rbs]

I agree that it is suboptimal. Perhaps there is a better way.

But your last concern about MathML 1 doesn't bother me that much. I was aware of that.

Comment 17

[rbs]

Of the points that Jesse raised above, the one that troubles me is the XBL one. This is because, in the other cases, if mathml.css isn't loaded then <mtd> is treated as a generic inline span, and so this bug won't surface in the first place.

>* the mtd is the root element, and so doesn't have a parent to be checked
>against :not(mtr)?

I haven't re-checked the rule-matching function of the Style System at this point. But logically, that rule should match if there is no parent, right? CSS gurus? (Besides, as I indicated above, mathml.css is needed to begin with, for <mtd> to get display: table-cell).

>* a future version of CSS decides that user style sheets override agent style
>sheets, and someone has a user stylesheet containing the rule "#skipNav {
>display: inline ! important; }"?

This won't happen, realistically... (If it does, other crucial rules in html.css will be in trouble too.)

--------------
In summary, with further thoughts, this patch still seems to me a decent compromise until a more robust solution is found on the C++ side, essentially because the XBL loophole is still there.

Comment 18

[Jesse Ruderman]

> in the other cases, if mathml.css isn't loaded then <mtd> is
> treated as a generic inline span, and so this bug won't surface in the 
> first place.

A site can easily include copy rules like this from mathml.css in its own stylesheet.  I tried it just now, and turning <mtd>s into table-cells by applying a copy of mathml.css works fine.  Remove the stylesheet, and it reverts to not acting like a table cell.

Comment 19

[rbs]

Yep... that occurred to me after I sent my post... Fixing by C++ is the ideal.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Seems reasonable.

Comment 21

[rbs]

Attachment: c++ patch

This patch enables pseudo-frames created from <mtd> and <mtr> outside <mtr> or <mtable>. With this patch, the <mtd> fish shows up and the crash goes away (without the css rules).

However, I still plan to check in the earlier patch as well to discourage such usages. Even when pseudo-frames show up, they are not spaced nicely because the table/matrix related rules in mathml.css are tag-based. Thus pseudo-frames are not styled properly and this makes Gecko's MathML to look inelegant unecessarily. Another issue is to use the right creator for them and this is been addressed in bug 348153.

Comment 22

[Boris Zbarsky [:bzbarsky]]

So what's the point of this C++ patch given the CSS one?

Comment 23

[Daniel Veditz [:dveditz]]

comment 15 and comment 18

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I imagine there's a plan to make importNode and adoptNode do the right thing 

Not unless someone files bugs on making it happen.  Please request blocking1.9 on said bugs.

> But logically, that rule should match if there is no parent, right?

No.

rbs, can you explain what effect you're trying to achieve with the C++ change?  The pseudo-frame code is notoriously fragile, so ideally bernd would review that change, but he's out of town... :(  I'd like to hear what you're aiming to do before trying to figure out what the patch actually does. 

Comment 25

[rbs]

Technically, the patch is a shortcut for doing:
-----------
  if (aTag == nsMathMLAtoms::mi_ ||
      aTag == nsMathMLAtoms::mn_ ||
      aTag == nsMathMLAtoms::ms_ ||
      aTag == nsMathMLAtoms::mtext_) {
+    if (!aHasPseudoParent && !aState.mPseudoFrames.IsEmpty()) {
+      ProcessPseudoFrames(aState, aFrameItems); 
+    }
    newFrame = NS_NewMathMLTokenFrame(mPresShell, aStyleContext);
  }
  else if (aTag == nsMathMLAtoms::mo_) {
+    if (!aHasPseudoParent && !aState.mPseudoFrames.IsEmpty()) {
+      ProcessPseudoFrames(aState, aFrameItems); 
+    }
    newFrame = NS_NewMathMLmoFrame(mPresShell, aStyleContext);
  }
  else if (aTag == nsMathMLAtoms::mfrac_) {
+    if (!aHasPseudoParent && !aState.mPseudoFrames.IsEmpty()) {
+      ProcessPseudoFrames(aState, aFrameItems); 
+    }
    newFrame = NS_NewMathMLmfracFrame(mPresShell, aStyleContext);
  }
etc...

All the mathml tags are tested in the existing IsSpecialContent(), except <mtable> -- which I am not sure why (and I actually feels there might be another issue here).

Conceptually (as I understand pseudo-frames...), 
------------
the processing aims at closing a "pseudo" table tree that may have been inferred along the way.

When the frame construction code encounters an isolated <mtd> somewhere (or <td> for that matter), it infers/creates (and queues) a pseudo <table> frame and a pseudo <tr> frame. But before it can close the pseudo <tr> frame, it must look-ahead if the *next* frame isn't going to be another <td> frame that will thus remain in the current pseudo <tr> frame. And this is why the clearance comes after checking each of the next tag/displaystyle.

The patch does that -- with the technicalities I described above. The crash was happening because ProcessPseudoFrames wasn't been called, leaving those dangling pseudo frames not added to the frame tree, and thus they turned into zombies at the tear down of the reload.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233219 [details] [diff] [review]
c++ patch

OK, I buy that.  If <mtable> gets a frame based on its tag, it should be listed in IsSpecialContent()... Does it?

Comment 27

[Jesse Ruderman]

Spun off:
* Bug 348413, importNode / adoptNode on MathML should bring in mathml.css.
* Bug 348415, Need C++ fix for bug 307826.

Comment 28

[rbs]

<mtable> gets a table frame based on its tag *and* that it has the right display for the mighty table layout...

  else if (aTag == nsMathMLAtoms::mtable_ &&
           disp->mDisplay == NS_STYLE_DISPLAY_TABLE) {

Does that qualify it for IsSpecialContent() or what?

Comment 29

[Boris Zbarsky [:bzbarsky]]

"what", since the IsSpecialContent check is used for things whose frame doesn't "really" match their display type....

Comment 30

[rbs]

OK, if you think IsSpecialContent() is fine as it is now, that clears up my uncertainty.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Well... "what" means "I'm not sure" in this case.  Again, I'd like to defer to Bernd.

Comment 32

[rbs]

Both patches ckecked in the trunk.

Comment 33

[rbs]

I made a typo in using using // instead of /* */ to delimit the comment in CSS:
+// Don't support mtr without mtable, nor mtd without mtr
+:not(mtable) > mtr,
+:not(mtr) > mtd {
+  display: none !important;
+}

As a result, this CSS declaration is currently *ignored*. Interestingly... this has been a blessing in disguise because it has allowed Jesse to unearth another subtle case in bug 348492.

I will checkin a fix to the typo without further ado. But I will wait until after fixing bug 348492 to guarantee that this pseudo-frame business is really nailed down from the C++ side. When I finally correct the typo, I will add a comment to this bug to say so.

Comment 34

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 233219 [details] [diff] [review]
c++ patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 36

[rbs]

Fixed bug 348492 and corrected typo on the trunk.

fixed1.8.0.7: both patches (with the typo corrected) checked in the 1.8.0 branch.

Comment 37

[rbs]

fixed1.8.1: checked in the 1.8 branch.

Comment 38

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=232501
ff2b2 debug/nightly windows/linux/macppc no crash

https://bugzilla.mozilla.org/attachment.cgi?id=232677
ff2b2 debug/nightly windows/linux/macppc no crash

verified fixed 1.8

Comment 39

[alice nodelman [:alice] [:anode]]

https://bugzilla.mozilla.org/attachment.cgi?id=232677 should not cause the browser to crash

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060821 Firefox/1.5.0.7pre

verified 1.8.0.7

Comment 40

[Boris Zbarsky [:bzbarsky]]

Actually, that patch is wrong, since an <mtable> does NOT in fact create a table frame, so we SHOULD put it in IsSpecialContent.  See bug 374193.

Comment 41

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/73fa608afb0e