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

RESOLVED FIXED in mozilla1.8.1

Status

()

defect
--
critical
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: jruderman, Assigned: rbs)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla1.8.1
PowerPC
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] deleted nsStyleContext, crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

492 bytes, application/xhtml+xml
Details
3.85 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Reporter

Description

14 years ago
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.
Reporter

Comment 1

14 years ago
Crashes while the status bar counter says 343.
Reporter

Comment 2

14 years ago
TB9201417M
Reporter

Updated

14 years ago
Blocks: stirdom
No longer depends on: stirdom
Assignee

Comment 3

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

Updated

14 years ago
Whiteboard: [sg:fix]
Reporter

Updated

14 years ago
Flags: blocking1.8b5?

Updated

14 years ago
Flags: blocking1.8b5? → blocking1.8b5+
Reporter

Comment 5

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

Comment 6

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

Comment 8

14 years ago
This testcase makes Firefox crash one second after it is loaded.
Reporter

Comment 9

14 years ago
TB9702080M
Keywords: testcase
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?
Flags: blocking1.8b5+ → blocking1.8b5-
Assignee

Comment 11

14 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #198266 - Flags: superreview?(bzbarsky)
Attachment #198266 - Flags: review?(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).
Assignee

Comment 13

14 years ago
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.
> 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.
Assignee

Comment 15

14 years ago
>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... 
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.
Assignee

Comment 17

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

Updated

14 years ago
Summary: StirDOM/MathML crash [@ nsFrameManager::ReResolveStyleContext] → Crash when floated <html:div> is removed from <math:mi> parent [@ nsFrameManager::ReResolveStyleContext]
Reporter

Updated

14 years ago
Whiteboard: [sg:fix] → [sg:fix] [patch]
Comment on attachment 198266 [details] [diff] [review]
patch

We need to actually deal with the error return from AppendFrames...
Attachment #198266 - Flags: superreview?(bzbarsky)
Attachment #198266 - Flags: superreview-
Attachment #198266 - Flags: review?(bzbarsky)
Attachment #198266 - Flags: review-
Assignee

Comment 19

14 years ago
Posted patch patch - fight CSS with CSS (obsolete) — Splinter Review
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.
Attachment #198266 - Attachment is obsolete: true
Attachment #201893 - Flags: superreview?(bzbarsky)
Attachment #201893 - Flags: review?(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?
Either way, that's a pretty inefficient selector.  This stylesheet is only loaded for pages that contain MathML, right?
Assignee

Comment 22

14 years ago
> 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?
Assignee

Comment 23

14 years ago
> Yep. That's okay 

I mean that the original CSS (intention) is okay, and so I won't be changing to ">".
"* > *|*" 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.

Assignee

Comment 25

14 years ago
Posted patch updateSplinter Review
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).
Attachment #201900 - Flags: superreview?(bzbarsky)
Attachment #201900 - Flags: review?(bzbarsky)
Assignee

Updated

14 years ago
Attachment #201893 - Attachment is obsolete: true
Attachment #201893 - Flags: superreview?(bzbarsky)
Attachment #201893 - Flags: review?(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?
Attachment #201900 - Flags: superreview?(bzbarsky)
Attachment #201900 - Flags: superreview+
Attachment #201900 - Flags: review?(bzbarsky)
Attachment #201900 - Flags: review+
Wikipedia uses MathML in HTML IIRC.

/be
Assignee

Comment 28

14 years ago
> 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.
Assignee

Comment 29

14 years ago
Checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Yeah, I think we agreed but were just having issues communicating.  ;)

Updated

14 years ago
Flags: testcase+
Any reason not to take this on the 1.8/1.8.0 branches?
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:fix] [patch] → [sg:critical] deleted nsStyleContext

Updated

13 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Reporter

Updated

13 years ago
Depends on: 348415

Comment 32

13 years ago
Bz/Rbs thoughts on this for the 1.8.1/.0 branches
Target Milestone: --- → mozilla1.8.1
Assignee

Comment 33

13 years ago
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.
Attachment #201900 - Flags: approval1.8.1?
Attachment #201900 - Flags: approval1.8.0.7?
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?
Attachment #201900 - Flags: approval1.8.0.7? → approval1.8.0.7+
UA !important rules override everything else.
Assignee

Comment 37

13 years ago
> 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]
Reporter

Comment 38

13 years ago
Bug 348415 covers developing a better fix.
Assignee

Comment 39

13 years ago
fixed1.8.0.7: checked in the 1.8.0 branch
Keywords: fixed1.8.0.7
This looks reasonably safe to me....
Reporter

Comment 41

13 years ago
<bz> I mean "safe" as in "will not break stuff"
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.
Attachment #201900 - Flags: approval1.8.1? → approval1.8.1+
Assignee

Comment 43

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

Comment 44

13 years ago
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

13 years ago
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.
> 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

13 years ago
(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.
(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
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.
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

Updated

12 years ago
Flags: in-testsuite+ → in-testsuite?
Group: security
Reporter

Comment 51

12 years ago
Crashtest checked in (slightly modified version of testcase 4).
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsFrameManager::ReResolveStyleContext]
You need to log in before you can comment on or make changes to this bug.