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

VERIFIED FIXED

Status

()

Core
MathML
--
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: rbs)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments)

(Reporter)

Description

11 years ago
*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.)
(Reporter)

Comment 1

11 years ago
My crashing testcase crashes Firefox 1.5.0.6 (official release build).
Flags: blocking1.8.0.7?
Whiteboard: [sg:critical]
(Assignee)

Comment 2

11 years ago
Testcase?
(Reporter)

Comment 3

11 years ago
Created attachment 232501 [details]
testcase
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.
(Reporter)

Comment 5

11 years ago
https://intranet.mozilla.org/Security/index.php/Bug_347355
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?
(Reporter)

Comment 7

11 years ago
Created attachment 232677 [details]
crashing testcase
(Reporter)

Comment 8

11 years ago
Crashes on reload for me with Win32 Firefox15 2006080705.  TB21895290G
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.
(Reporter)

Comment 10

11 years ago
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).
(Assignee)

Comment 11

11 years ago
Created attachment 232980 [details] [diff] [review]
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>).
Attachment #232980 - Flags: superreview?(bzbarsky)
Attachment #232980 - Flags: review?(bzbarsky)
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.
Flags: blocking1.8.1?
(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.
(Assignee)

Comment 14

11 years ago
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.
(Reporter)

Comment 15

11 years ago
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
(Assignee)

Comment 16

11 years ago
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.
(Assignee)

Comment 17

11 years ago
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.
(Reporter)

Comment 18

11 years ago
> 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.
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 19

11 years ago
Yep... that occurred to me after I sent my post... Fixing by C++ is the ideal.
Comment on attachment 232980 [details] [diff] [review]
patch

Seems reasonable.
Attachment #232980 - Flags: superreview?(bzbarsky)
Attachment #232980 - Flags: superreview+
Attachment #232980 - Flags: review?(bzbarsky)
Attachment #232980 - Flags: review+
(Assignee)

Comment 21

11 years ago
Created attachment 233219 [details] [diff] [review]
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.
Attachment #233219 - Flags: superreview?(bzbarsky)
Attachment #233219 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #233219 - Flags: review? → review?(bzbarsky)
So what's the point of this C++ patch given the CSS one?
comment 15 and comment 18
> 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. 
(Assignee)

Comment 25

11 years ago
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 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?
Attachment #233219 - Flags: superreview?(bzbarsky)
Attachment #233219 - Flags: superreview+
Attachment #233219 - Flags: review?(bzbarsky)
Attachment #233219 - Flags: review+
(Reporter)

Comment 27

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

Comment 28

11 years ago
<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?
"what", since the IsSpecialContent check is used for things whose frame doesn't "really" match their display type....
(Assignee)

Comment 30

11 years ago
OK, if you think IsSpecialContent() is fine as it is now, that clears up my uncertainty.
Well... "what" means "I'm not sure" in this case.  Again, I'd like to defer to Bernd.
(Assignee)

Comment 32

11 years ago
Both patches ckecked in the trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 33

11 years ago
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.
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment on attachment 232980 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #232980 - Flags: approval1.8.0.7+
Comment on attachment 233219 [details] [diff] [review]
c++ patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #233219 - Flags: approval1.8.0.7+
Attachment #232980 - Flags: approval1.8.1?
Attachment #233219 - Flags: approval1.8.1?
(Assignee)

Comment 36

11 years ago
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.
Keywords: fixed1.8.0.7

Updated

11 years ago
Attachment #232980 - Flags: approval1.8.1? → approval1.8.1+

Updated

11 years ago
Attachment #233219 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 37

11 years ago
fixed1.8.1: checked in the 1.8 branch.
Keywords: fixed1.8.1

Comment 38

11 years ago
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
Keywords: fixed1.8.1 → verified1.8.1
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7 → verified1.8.0.7
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical] → [sg:critical] Keep private because of #c15 and #c27
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.
Flags: in-testsuite?
Group: core-security
Whiteboard: [sg:critical] Keep private because of #c15 and #c27 → [sg:critical]

Comment 41

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/73fa608afb0e
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.