307826 - Crash when floated <html:div> is removed from <math:mi> parent [@ nsFrameManager::ReResolveStyleContext]

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050908
Firefox/1.6a1

I tried making a simpler testcase and wasn't able to.  I think Firefox gets
itself into a bad/inconsistent state before the crash.

Filing as security-sensitive because the unsimplified testcase uses code from
bug 306663.

Comment 1

[Jesse Ruderman]

Attachment: testcase (not reduced)

Crashes while the status bar counter says 343.

Comment 2

[Jesse Ruderman]

TB9201417M

Comment 3

[rbs]

Please check if it has been fixed in the latest nightly, which now has the fix
for bug 307839.

Comment 5

[Jesse Ruderman]

Still crashes [@ nsFrameManager::ReResolveStyleContext] with a trunk opt build
from a few hours ago.

Comment 6

[Jesse Ruderman]

Attachment: testcase 2: original page, less-random script

I'm attaching this primarily so anyone who is curious can see how I got from
the first testcase to the simplified testcase (coming soon).

Comment 7

[Jesse Ruderman]

Attachment: testcase 3: original page re-serialized with an ID added, much less script

Comment 8

[Jesse Ruderman]

Attachment: testcase 4: simplified

This testcase makes Firefox crash one second after it is loaded.

Comment 9

[Jesse Ruderman]

TB9702080M

Comment 10

[Asa Dotzler [:asa]]

If you come up with a very safe fix in the next couple of days, please request
approval for the patch and we'll evaluate.

RBS, are you still looking into this?

Comment 11

[rbs]

Attachment: patch

Add some checks... MathML has no concept of float/positioned elements and will
never will. It purposely focuses on math typesetting, leaving the other extras
to CSS. So these checks won't do any harm in real life situations.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow this patch.  If MathML cannot be a containing block of
certain type, we should fix nsCSSFrameConstructor accordingly and assert in the
MathML code or something instead of just throwing and hoping callers check rv
correctly (they probably don't, given past experience).

Comment 13

[rbs]

Right. But does it matter? I do get an assert that the frames get lost (and so
will go away with the arena). I agree that it will be nice to block that from
the frame constructor. But I doubt it will be a little patch. Plus, I doubt the
added complexity is worth it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I do get an assert that the frames get lost (and so will go away with the
> arena).

If any of those are animated images that means a crash.

> Plus, I doubt the added complexity is worth it.

It absolutely is, imo.  Especially going forward (for 1.9).  Then again, for 1.9
we should just make MathML be able to be an absolute position frame containing
block.

Comment 15

[rbs]

>we should just make MathML be able to be an absolute position frame containing
>block.

Nope. I don't think so. Nor do I want that.

People do/should not expect to demonstrate their TeX wizardry inside $$...$$. 
Similarly, we don't expect/want people to show off their CSS skills inside 
<math>. They can just wrap the whole lot inside a <div> as style this as they 
see fit.

<mfrac> with a denominator floating over-there on the screen has no meaning. I 
particularly resist over-engineering for things not meant for MathML. All we 
should care here is to make things interact sensibly and not crash on us... 

Comment 16

[Boris Zbarsky [:bzbarsky]]

So the real question at hand is whether MathML is using CSS for layout at all,
right?  If it's not, that's fine as long as we're making that decision and
sticking to it.

But viewed that way, what does it mean to have non-MathML content as a child of
the MathML element?  Should that content even be rendered?  I'd posit that the
answer is "no", since it makes no sense to have an HTML table as the denominator
of a fraction.

Comment 17

[rbs]

bz, MathML uses common CSS things (like "font-style: italic", etc). All that is
good and should be preserved. It also uses other things like a CSS table layout
because a matrix is a table (keep in mind the separation between the HTML
content model and CSS frames that apply to XML in general). We made those
decisions a long time ago since nobody wants to rewrite tens of thousands of
lines to re-invent the wheel.

What is happening here (with this wicked StirDOM thing that could only come from
the inventive brain of jruderman...) is that he is taking a DOM and twisting it
to death, taking one child over there and doing an appendChild() of that child
to another thing over there, repeatedly and randomly. This creates all sorts of
combinations, almost all of which are invalid per the MathML spec (which
thankfully doesn't intend to become as over ambitious as SVG). So the MathML
ends up with strange things which don't make sense, indeed. That's why I think
that we should only try to bail out gracefully, if we can, and not try to bloat
the code unnecessarily.

I will look further at iterating on the patch to see if the frames can be
destroyed (to prevent the crash on animated images that you indicated).

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

We need to actually deal with the error return from AppendFrames...

Comment 19

[rbs]

Attachment: patch - fight CSS with CSS

Some simple CSS to prevent unwarranted/unexpecting things in the first place. It is a _lot_ easier (as these CSS rules show) to prevent frame construction than to kill bad frames that have been created.

The CSS rules are enough the fix this bug and the other evil floating and positioning bugs (bug 314500, bug 314512, bug 314517). I don't even get any assertion. I am leaving the earlier pach in because it cost nothing to have it too.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201893 [details] [diff] [review]
patch - fight CSS with CSS

>Index: content/src/mathml.css

>+/* MathML doesn't permit floating and positioning */
>+*, * *|* {
>+  float: none !important;
>+  position: static !important;

Hmm...  Do we really want to prevent floating on a block child f a mathml element?  That is, should that rule perhaps use the selector "*, * > *|*" instead?

>Index: base/src/nsMathMLContainerFrame.h
>+    if (aListName)
>+      return NS_ERROR_INVALID_ARG;

Given the rules in mathml.css, should those be asserts instead?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Either way, that's a pretty inefficient selector.  This stylesheet is only loaded for pages that contain MathML, right?

Comment 22

[rbs]

> Hmm...  Do we really want to prevent floating on a block child f a mathml
> element?  That is, should that rule perhaps use the selector "*, * > *|*"
> instead?

Yep. That's okay (and is enough). It doesn't happen in real MathML because authors know that it wouldn't be portable to have such markups.

> Given the rules in mathml.css, should those be asserts instead?

OK.

== re: dbaron
> This stylesheet is only loaded for pages that contain MathML, right?

Yep (it is a catalog sheet loaded on demand, indeed). 

Any suggestion for another slick CSS way to achieve the same effect?

Comment 23

[rbs]

> Yep. That's okay 

I mean that the original CSS (intention) is okay, and so I won't be changing to ">".

Comment 24

[Boris Zbarsky [:bzbarsky]]

"* > *|*" is somewhat inefficient, but not that bad (incrementally) given that we already have an "object > *|*" rule in html.css and cache parent style datas...

"* *|*" means that for every node in any document that has MathML somewhere in it, style resolution has to walk to the nearest MathML element or the document root.  The latter would be quite slow.  Given that I would expect most MathML to be used interspersed with other markup, I suggested using the '>' combinator if it achieves the effect you want -- it would be a good deal faster.

Comment 25

[rbs]

Attachment: update

OK, updated per the comments. I tested that it quasted the evil bugs as well.

> Given that I would expect most MathML to be used interspersed with other markup

My expectation is even lower -- that such markups won't happen in real life (except in the contrived and stress testing of bugzilla, or for security exploits which are more worrisome).

Comment 26

[Boris Zbarsky [:bzbarsky]]

> My expectation is even lower

What I meant is that I expect pure mathml documents to be very rare.  I would expect most MathML to be used in an HTML document, in bits with lots of non-mathml text around it...

But maybe that's not how it's used?

Comment 27

[Brendan Eich [:brendan]]

Wikipedia uses MathML in HTML IIRC.

/be

Comment 28

[rbs]

> I would expect most MathML to be used in an HTML document, in bits with lots of
> non-mathml text around it...

Yeah, that's how it is used. We were not taling of the same thing. I instead meant that we don't see HTML's <div>, let alone with float:right (or position:fixed, and the likes), as a child _inside_ the MathML, which is what this bug is made of.

Comment 29

[rbs]

Checked in.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yeah, I think we agreed but were just having issues communicating.  ;)

Comment 31

[Daniel Veditz [:dveditz]]

Any reason not to take this on the 1.8/1.8.0 branches?

Comment 32

[Mike Schroepfer]

Bz/Rbs thoughts on this for the 1.8.1/.0 branches

Comment 33

[rbs]

Comment on attachment 201900 [details] [diff] [review]
update

Albeit not military grade, having the CSS rule has proved quite protective. So asking approval for the 1.8.0 and 1.8 branches.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 201900 [details] [diff] [review]
update

approved for 1.8.0 branch, a=dveditz for drivers

Is this a real fix though? Can an attacking page's own CSS override the default !important rules with its own !important rules?

Comment 35

[Boris Zbarsky [:bzbarsky]]

UA !important rules override everything else.

Comment 37

[rbs]

> Is this a real fix though? 
Yes, but it requires mathtml.css to be effectively loaded (cf. caveat expressed in bug 347355).

> Can an attacking page's own CSS override the default !important rules with its
> own !important rules?
No. The buck stops at the UA's !important rules. The cascading order is:

[least important]
 1. UA normal rules                    = Agent        normal
 2. Presentation hints                 = PresHint     normal
 3. User normal rules                  = User         normal
 4. HTML Presentation hints            = HTMLPresHint normal
 5. Author normal rules                = Document     normal
 6. Override normal rules              = Override     normal
 7. Author !important rules            = Document     !important
 8. Override !important rules          = Override     !important
 9. User !important rules              = User         !important
10. UA !important rules                = Agent        !important
[most important]

Comment 38

[Jesse Ruderman]

Bug 348415 covers developing a better fix.

Comment 39

[rbs]

fixed1.8.0.7: checked in the 1.8.0 branch

Comment 40

[Boris Zbarsky [:bzbarsky]]

This looks reasonably safe to me....

Comment 41

[Jesse Ruderman]

<bz> I mean "safe" as in "will not break stuff"

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 201900 [details] [diff] [review]
update

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.

Comment 43

[rbs]

fixed1.8.1: checked in the 1.8 branch.

Comment 44

[Bob Clary [:bc] (inactive)]

testing with ff2b2:

https://bugzilla.mozilla.org/attachment.cgi?id=195500
Crash winxp TB22357140, Crash ff2b2 linux but no sign of deleted objects.

in ff2b2 winxp debug crash with null |this|, similar stack for linux

>	gklayout.dll!nsIFrame::GetStateBits()  Line 817 + 0xa bytes	C++
 	gklayout.dll!IncrementalReflow::AddCommand(nsPresContext * aPresContext=0x04a14730, nsHTMLReflowCommand * aCommand=0x04e1df18)  Line 938 + 0x8 bytes	C++
 	gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=1)  Line 6896 + 0x1c bytes	C++
...

https://bugzilla.mozilla.org/attachment.cgi?id=197304
winxp, linux: no crash

https://bugzilla.mozilla.org/attachment.cgi?id=197305
winxp, linux: no crash

https://bugzilla.mozilla.org/attachment.cgi?id=197306
winxp, linux: no crash

verified fixed 1.8.

Comment 45

[Jay Patel [:jay]]

v.fixed on both branches with 8/24 nightly builds, no crash with testcases 2, 3, and 4:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060824 Firefox/1.5.0.7pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2

I did see a crash with the original testcase (as did Bob), but I'm assuming that is a different crash, since the simplified testcase is ok.  If I'm wrong, please let me know and revert the verified keywords.

Comment 46

[Daniel Veditz [:dveditz]]

> I did see a crash with the original testcase (as did Bob), but I'm assuming
> that is a different crash, since the simplified testcase is ok.

I guess bug 348415 covers that? Or is it a different crash on the branch needing it's own bug?

In 1.5.0.6 I *only* crash on the first testcase, none of the others, and crash in the same place in 1.5.0.7--what was actually verified? It wasn't a deleted nsStyleContext though, it was due to calling IncrementalReflow::AddCommand with a null frame so I guess that is a different crash.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&mark=938&rev=FIREFOX_1_5_0_6_RELEASE#938

Comment 47

[Bob Clary [:bc] (inactive)]

(In reply to comment #46)

I really don't know anymore. We have bugs with testcases that continue to crash after the fix. The idea seems to be one bug/one stack and if we can't reproduce the exact stack as originally filed or find deleted objects or something really easily exploitable then the bug is "fixed". But that is pretty bogus considering most of these fuzz type bugs show a different stack depending on the phase of the moon, the way you look at them or whatever. Its like whack-a-mole and if a vole appears instead of a mole, we consider the mole dead.  But what if the mole is a shape shifter that just looks different?

I personally would prefer one testcase/one bug and to keep it open until it stops crashing at all.

Comment 48

[Brendan Eich [:brendan]]

(In reply to comment #47)
> I personally would prefer one testcase/one bug and to keep it open until it
> stops crashing at all.

Agreed.  Or one testcase per tracking bug and file and fix dependent bugs, with the same good effect of keeping the bug open until the test works in all phases of moon, etc.

/be

Comment 49

[Boris Zbarsky [:bzbarsky]]

I like brendan's idea; very often a fuzzer testcase will expose multiple back-end bugs, that need fixing in different modules, by different people, etc...  so we really do want one bug per stack, but I agree that it's silly to resolve a bug fixed while the original testcase for it crashes.

Comment 50

[Daniel Veditz [:dveditz]]

I don't mind if a bug is closed based on fixing one particular stack AS LONG AS someone can point at the bug describing the remaining crash(es), which ought to be linked in the dependencies so people can find it. Doesn't appear to be true of this bug yet. (the remaining crash is a null deref, not a potentially critical one like the original problem on the trunk, but still a crash).

Testcases 2 through 4 don't crash for me in 1.5.0.6 or 1.5.0.2

Comment 51

[Jesse Ruderman]

Crashtest checked in (slightly modified version of testcase 4).