MathML performs unsafe operations during frame manipulations.

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
MathML
P1
critical
RESOLVED FIXED
11 years ago
7 months ago

People

(Reporter: bz, Assigned: roc)

Tracking

({arch, crash})

Trunk
mozilla1.9beta3
x86
All
arch, crash
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][dbaron-1.9:RsCe])

Attachments

(2 attachments, 12 obsolete attachments)

1.76 KB, application/xhtml+xml
Details
309.46 KB, patch
roc
: review+
Details | Diff | Splinter Review
The following stack in bug 322625 comment 30 says it all:

  nsFrameManager::DebugVerifyStyleTree
  nsMathMLContainerFrame::PropagateScriptStyleFor
  nsMathMLContainerFrame::ReResolveScriptStyle
  nsMathMLContainerFrame::ReLayoutChildren
  nsMathMLmathBlockFrame::RemoveFrame

Where to even start....  First of all, generally speaking the frame tree is not in a consistent state during the RemoveFrame call (e.g. there might be placeholders around pointing to null, or out-of-flows without placeholders or whatever).  So walking the frame tree the way DebugVerifyStyleTree does is not cool.

Second, PropagateScriptStyleFor sets and unsets attributes.  Doing that during frame construction or destruction is just not acceptable; I'm surprised this isn't triggering the frame construction reentry warnings.

Third, PropagateScriptStyleFor calls nsFrameManager::ComputeStyleChangeFor.  Performing style reresolution while in an inconsistent frame tree state is a bad idea.  Then it calls DebugVerifyStyleTree. which is just as bad, but only in debug builds.

What it looks like to me is that all the style munging should happen off an event...  I assume there's a reason that styles are changing when frames are removed, by the way?  If so, perhaps we should talk about how to capture that in a reasonable way in our style system?

Comment 1

11 years ago
You seemed to liked those DebugVerifyStyleTree() calls when you added them, BTW :-)

>Second, PropagateScriptStyleFor sets and unsets attributes.  Doing that during
>frame construction or destruction is just not acceptable; I'm surprised this
>isn't triggering the frame construction reentry warnings.

All these are internal -moz annotations to assist when formatting (with notify=PR_FALSE) -- they don't trigger reentry.

>I assume there's a reason that styles are changing when frames are
>removed, by the way?

[here is an old post, but still informative about the problem]
http://groups.google.com/group/netscape.public.mozilla.mathml/msg/10283d8b17c7a028

>What it looks like to me is that all the style munging should happen off an
>event...

This is what is desirable (it will make the other issues moot).
It is only yesterday that you added the PostRestyle(), and probably that code should be migrated to that.
Er, this should have been critical.  This is probably causing at least some of the StirDOM problems we've been patching around with frame constructor hackery.

> You seemed to liked those DebugVerifyStyleTree() calls when you added them

Yeah, well.  Given what else the code was doing, I didn't even imagine it'd be running during frame construction.  ;)

Agreed that having aNotify false doesn't reenter frame construction per se.  It _does_ fire mutation events on the 1.8 branch, so enters arbitrary javascript.

> It is only yesterday that you added the PostRestyle()

Over two years ago is yesterday?  That said, there's no need to use the PostRestyle thing.  Just use a PLEvent (or nsRunnable on trunk)...  

I'll read the newsgroup post this evening and comment more.

Severity: normal → critical

Comment 3

11 years ago
>Over two years ago is yesterday?

People are not sitting there idle. They also need some sleep, unlike you :-)

>  That said, there's no need to use the
>PostRestyle thing.  Just use a PLEvent (or nsRunnable on trunk)...  

Actually, PostRestyle() or an event might not be needed. It just has be done before the reflow happens.

> I'll read the newsgroup post this evening and comment more.

Yeah, have a look. It is a catch-22. You may be tempted to think that you can do that from the content side. But with some frames (e.g., with display:none) not really there, or having their own CSS settings to be captured on the way, it would be as as if you end up doing all of mathml layout from the content (and the type of frame influences the scriptlevel or compression flags which themselves influence their scriptsizes). Such a full layout from the content istelf makes no sense either.
Group: security
Whiteboard: [sg:critical?]

Comment 4

11 years ago
any more thoughts on how to approach this bug?
URL: arch

Comment 5

11 years ago
Not really.
Uh... why do we need more thoughts?  rbs and I agree that this should happen async, as of comment 1.  The other discussion was him pointing out that this code can't be eliminated entirely; having read his description of the problem, I agree with that.  But we shouldn't be calling this code during frame destruction.
The only problem with that is that this thing sets attributes from inside frame code; as I said that's directly exploitable on the 1.8 branch by anyone who feels like it.  We need to change this code to hand back a list of nodes and attribute values, then loop over the nodes setting the attributes....

rbs, I don't really understand this code enough to do this well.  Do you think you have time to do that?

Comment 8

11 years ago
It doesn't really work as a list. The effect we are trying to emulate is that, it you set the attribute on the first containing parent, say for example:

[-moz-math-font-size="+1"] /* to scale down with { font-size: 71%; } */

then we want to the style system to recalculate the font-sizes in the subtree. The reason is because it is only after the resolution that can we tell whether or not the actual computed values in one of the descendants is above the scriptminsize (we cannot tell in advance because descendants can have their own CSS settings that affect the style resolution). Only after that do we know whether to set the attribute on descendants as we walk down the the tree, discovering the scriptlevel and displaystyle, which themselves depend on the frames (e.g, whether we are inside a fraction or the like).

So it seems like it is going to be a one attribute at a time affair, then re-resolve, then walk down, check and set the next attribute (which is what is happening synchronously at present rather than async). In any case, it seems likely to remain an expensive process as it already is.

What I have in my radar is mostly getting in sync with the reflow branch & cairo/textruns since MathML is broken without them.
Oh, I see.  Yeah, then we'll need to do it one attribute at a time somehow. :(

If we had a mathml element class, we could probably use internal state in that and some sort of pseudo-selector instead, but we don't...

The problem is that we really need to fix this on the branches, not just on trunk.  Right now a sufficiently determined attacker can probably use this to execute arbitrary code on a user's machine.
Flags: blocking1.9?
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Assignee: rbs → roc
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Flags: blocking1.9? → blocking1.9+
I don't have time to do this for 1.8.1.4/1.8.0.12.
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Blocks: 378146

Updated

11 years ago
Target Milestone: --- → mozilla1.9alpha6
Who might have time to work on this? This is blocking our next branch releases. roc, can you reassign this to someone who might be able to work on it?
I don't have time to work on this, especially since I'm not familiar with these issues. bz of course is in the same situation. If rbs can't maintain MathML and no other maintainer steps up then I think we should disable MathML on trunk and branch.

Comment 13

11 years ago
vlad has been making some progess with MathML.  Vlad, can you look at this bug too if you get some extra cycles?
Assignee: roc → vladimir.sukhoy

Comment 14

11 years ago
ok, I'll see what I can do here tomorrow, err... I mean today. Not sure I will be able to do much though, being where I am at the level of familiarity with source code base. I'll try.
Status: NEW → ASSIGNED

Comment 15

11 years ago
Ok, looked at it for a while.. I'm not sure if it will be enough to post an event and do ReLayoutChildren off an event rather than within nsMathMLmathBlockFrame::RemoveFrame (i.e. nsMathMLmathBlockFrame::RemoveFrame posts an event and once it gets to processing that, ReLayoutChildren is called). This seems far too simple to do the job though. I'll try that and see how it works out.

What people were talking about looks like asynchronizing PropagateScriptStyleFor thing, i.e. make it sort of span across a bunch of events, like a continuation or whatever. This is plan b.

BTW there is another routine dealing with frame removal within which there is a call ReLayoutChildren, that is nsMathMLmathInlineFrame::RemoveFrame. 

Comment 16

11 years ago
Created attachment 266166 [details]
test case (crashes non-trunk builds).

It looks like I was able to come up with reproducible crash test case. Mutation event is indeed fired on the dead tree, and an attempt to redelete it once more causes access violation (at least on all of my non-trunk builds).

Comment 17

11 years ago
Tested on MacOS, Windows. It looks like this is everywhere, which is hardly surprising given its nature.
OS: Linux → All

Comment 18

11 years ago
Ok, doing ReLayoutXXX and PropagateScriptStyleFor off an event is not enough. Any kind of setting attribute from within the frame code (as bz mentioned, unfortunately I was far too much out of the source code context to "get" it earlier) can be turned into crash/exploit. Meanwhile, I could not create a test case for this on trunk, I wonder if this is indeed critical on trunk? Mutation events do not propagate over there... Are there any other attack vectors which are feasible on trunk? 
For branches.. I see the following solution to handle the mutation events at least (plan b): split PropagateScriptStyleFor into several stages (actually, 3): before set/unset attribute (code deals with frames), set/unset stage (deals with content), after set/unset (deals with frames again, recursive). Then, the code would post events on stage transitions i.e. the whole "PropagateScriptStyleFor" would complete in three events.. This way the mutation events would still propagate, but at least not from within the frame code.. My prototype for this does not crash the testcase, but if something got broken with MathML... I don't know :).

Comment 19

11 years ago
See bug 378146 for an example that crashes exploitably in trunk debug builds.

Comment 20

11 years ago
What's the difference between branch and trunk that you're seeing with mutation events?

Updated

11 years ago
Attachment #266166 - Attachment mime type: text/plain → application/xhtml+xml

Comment 21

11 years ago
On trunk they don't show up, i.e. not fired at all.

Comment 22

11 years ago
That's strange.  Maybe you're seeing the change in bug 90983?

Comment 23

11 years ago
Most likely. I'm afraid that does not mean that there does not exist an exploitation vector on trunk though, perhaps not as straightforward as in the case attachment 266166 [details]... 

Comment 24

11 years ago
Created attachment 266278 [details] [diff] [review]
synchronous branches patch rev 0

This patch is a start on making PropagateScriptStyleFor asynchronous (patch is valid for both 1_8 and 1_8_0). While attributes are still manipulated within the frame code, extra check performed through nsWeakFrame in the introduced method nsMathMLContainerFrame::PropagateScriptStyleFor_mathml_style eliminates the crash in the test case (attachment 266166 [details]).
A better test case is needed, preferrably the one which makes better use of tree being in the intermediate state..
Do you see the mutation events if you wait till after onload before doing stuff?

Comment 26

11 years ago
Perhaps another option to save us from these headaches might be to have a private/internal setAttribute method that does the setAttribute implementation while skipping over the mutation events calls -- or, perhaps making the aNotify parameter a multivalue flag rather than just a boolean, which a possible being to skip over the mutation events calls.

Comment 27

11 years ago
(In reply to comment #25)
> Do you see the mutation events if you wait till after onload before doing
> stuff?
> 

On trunk - no. On branches - yes.

Comment 28

11 years ago
Created attachment 266332 [details]
Unified testcase (crashes branches, makes trunk unresponsive)

A "unified" testcase, still crashes branches via DOMAttrModified and makes trunk unresponsive (tested MacOS, Windows) via DOMSubtreeModified.
Attachment #266166 - Attachment is obsolete: true

Comment 29

11 years ago
Created attachment 266335 [details] [diff] [review]
corrected version of synchronous patch for branches, rev 1

Just a correction (fixed error which made debug builds fail).
Attachment #266278 - Attachment is obsolete: true

Comment 30

11 years ago
While messing with stuff, found another exploitable flaw, in nsMathMLContainerFrame::ReLayoutChildren there is an iteration over the child list and PropagateScriptStyleFor is invoked for each of the children, and that fires mutation events, and that invokes JavaScript.. so frames may get deleted and then it asks it for the next sibling and that sibling is dead and it calls virtual method on it. I think there is no need to open another bug, it is just that ReLayoutChildren method should be made asynchronous as well as PropagateScriptStyleFor.
I think, technically, this is vulnerable whenever ReLayoutChildren is invoked, not only when ReLayoutChildren is done on RemoveFrame.

Comment 31

11 years ago
Created attachment 266426 [details] [diff] [review]
synchronous patch for branches, rev 2

More enhancements in the synchronous patch, ReLayoutChildren is now split into several routines suitable for calling as an event sequence. 
(with this patch applied, the test case in attachment 266332 [details] no longer crashes branches).
Attachment #266335 - Attachment is obsolete: true

Comment 32

11 years ago
who can review that last patch?

Comment 33

11 years ago
Oh, I think I will post a "true" asynchronous patch before too long, some progress is made. 
The thing is, there are far too many opportunities to exploit the synchronous subscript style propagation, and even if mutation events won't be fired (it is a question, though, if it is ok not to fire them there..) there may be some indirect route towards exploitation.
That last patch does fix some obvious exploitation scenarios, but the issue of messing with inconsistent tree is still there..

Comment 34

11 years ago
Created attachment 266598 [details] [diff] [review]
synchronous patch for branches, rev 3

Synchronous minimal patch for branches (fixes the test cases, but does not do much in terms of the core issue). Should eliminate most "obvious" exploitation routes via mutation events.
A review is requested.
Attachment #266426 - Attachment is obsolete: true

Comment 35

11 years ago
Something like attachment 266598 [details] [diff] [review] for trunk should fix bug 378146. 

I think I'll wait with the asynchronous patch for now (it is pretty large and there are some problems with it, like how do I transfer the presentation context between events in a safe manner? In our case we need to do style resolution on frames after some attributes were modified, "just" passing nsPresContext pointer around somehow does not seem like a great idea).

Also, I think it makes sense to go ahead with disabling mutation events when MathML script style processing happens, it is all internal processing anyway, peculiar to Mozilla implementation of MathML standard..
I wonder if this should be introduced as a part of nsIContent or, rather, a part of custom MathML element class (we need that for DOM, i.e. bug 143842 anyway)?
> (it is pretty large and there are some problems with it, like how do I
> transfer the presentation context between events in a safe manner?

Yeah, don't pass a pointer around. You can get a prescontext from a document via GetPresShellAt(0), so pass strong (nsCOMPtr) references to an nsIDocument instead.
> You can get a prescontext from a document via GetPresShellAt(0)

Actually, that's GetPrimaryShell() now.

And you can also pass around a strong ref to an nsIPresShell, depending on what you want the behavior to be if the document gets a new presentation while the event is posted.

Comment 38

11 years ago
> And you can also pass around a strong ref to an nsIPresShell, depending on what
> you want the behavior to be if the document gets a new presentation while the
> event is posted.
> 
Most likely the behavior is to go on at the old shell, since another script style propagation should probably happen on the new presentation regardless of what was going on at the old one...

How about mutation events disabling though? Since we may have a MathML and non-MathML tag soup and all that can fire mutation, handling it within MathML element does not seem like an option. Should we go ahead and add appropriate code into nsIContent? Should it be another method, another flag or change the meaning of aNotify? Maybe just stop firing mutation events if aNotify is false? (a simple change in nsGenericElement::SetAttr ?).

Comment 39

11 years ago
> just stop firing mutation events if aNotify is false

I would start with this. Failing that (i.e., if it breaks something), then make aNotify multivalue (bitfield).

This requires reviewing call-sites that use aNotify=false, which I think might only actually be happening in MathML and perhaps, at most, a handful of other places. Given these limited call-sites, stopping firing mutation events if aNotify=false does not seem regression-prone to me at first sight.

Another motivation for this is that call-sites that are deliberately using aNotify=false are most certainly not expecting that it would still be leading to mutation events, JavaScript, etc. They only want to set the attribute and bail out. So conceptually, it also makes sense.

Updated

11 years ago
Summary: MathML performs unsafe operations during frame removal → MathML performs unsafe operations during frame manipulations.

Comment 40

11 years ago
Created attachment 266711 [details] [diff] [review]
branches patch: Disable firing mutation events when modifying attributes with aNotify = false

The changes are orthogonal to MathML, moreover not firing DOMAttrModified is what trunk does. 

I think this should be checked in as a security fix (besides all else, there are more exploitable MathML vectors which will get fixed by this, for example http://lxr.mozilla.org/mozilla1.8/source/layout/mathml/base/src/nsMathMLmactionFrame.cpp#431 (this may be deleted within SetAttr, debug only though), or http://lxr.mozilla.org/mozilla1.8/source/layout/mathml/base/src/nsMathMLTokenFrame.cpp#349 (again, "this" may be deleted within SetAttr)..

There are some potential (I think) non-MathML vectors, like in http://lxr.mozilla.org/mozilla1.8/source/layout/forms/nsComboboxControlFrame.cpp#2146 (does SetAttr within frame code, mutation is fired, "this" is destroyed, references member variables afterwards...)

This patch must go in, IMO.
Attachment #266711 - Flags: approval1.8.1.5?
Attachment #266711 - Flags: approval1.8.0.13?
Attachment #266711 - Flags: superreview?(jst)
Attachment #266711 - Flags: review?(bzbarsky)
That patch is a significant behavior change that we didn't take on branch when we took it on trunk because we're not sure how it impacts real-world content (including intranet apps).

I'm not happy with either the idea of making the change on branch without serious investigation of its effects, or the patch as written (e.g. it should skip checking for listeners if aNotify is false if that's what we decide to do).

I also won't have time to review this until at least mid-July.

I'm sure sicking has thoughts on this, since he changed it on trunk, iirc.

Comment 42

11 years ago
Some additional bullet proofing to reduce the worries:
*limit it to MathML for now by checking if the content is in the MathML namespace.
*also change the IID, if still concerned.
You can't change the IID of nsIContent (or anything else, in fact) on branch -- that would break our binary compatibility commitments.

Limiting to MathML might work, yes...  Again, I'd like to hear what Jonas has to say.
Though see comment 38, which implied that there is no reasonable MathML-specific fix?

Comment 45

11 years ago
> You can't change the IID of nsIContent 

Yeah... now that you say it...


> there is no reasonable MathML-specific fix?

I think it might work. We can enforce it on both ways if necessary (although I must confess that I am not immediately seeing why it would be necessary to do this, but that should be the ultimate defense): we could make MathML side do the SetAttr calls only on contents in the MathML namespace, plus the suggested filtering on the content side.

Comment 46

11 years ago
See bug 382568 for an example of non-MathML crash with mutation events on branches (in this case it is <textarea>). From LXR it seems that there are more of these in the codebase (just look for any frame code that does SetAttr or UnsetAttr and then does anything about presentation).
 
While we are at it, it looks like other tree manipulation routines in nsGenericElement do fire mutation events, so if these are used from the layout code they are most likely vulnerable as well. 

Comment 47

11 years ago
I can't access bug 382568. I still don't have access to such bugs. It is getting annoying.

Updated

11 years ago
Blocks: 382754
I do think it's worth stopping firing mutation events when aNotify is false. I think it's fairly unlikely that it'll break any apps, and it's definitely a security problem to leave things as is. Security should trump compatibility in this case I think.

Chofmann, dveditz: Are there similar bugs that we have taken on branches where the fix essentially disables a (rarely used) feature?

However the patch as written is no good. You shouldn't waste time looking for mutation listeners, and you still need to notify XBL that an attribute has changed.

Comment 49

11 years ago
Comment on attachment 266711 [details] [diff] [review]
branches patch: Disable firing mutation events when modifying attributes with aNotify = false

Flags dropped, I will attach a revised version onto bug 382754 (other methods besides SetAttr/UnsetAttr should be taken care of anyway).
Attachment #266711 - Attachment is obsolete: true
Attachment #266711 - Flags: superreview?(jst)
Attachment #266711 - Flags: review?(bzbarsky)
Attachment #266711 - Flags: approval1.8.1.5?
Attachment #266711 - Flags: approval1.8.0.13?

Comment 50

11 years ago
> Chofmann, dveditz: Are there similar bugs that we have taken on branches where
the fix essentially disables a (rarely used) feature?

I think there are, but can't come up with any specific bugs off-hand.

We have also disabled more frequently used features on occasion... ;-)

Updated

11 years ago
No longer blocks: 382754
Depends on: 382754
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Poke.  News on this?

Comment 53

11 years ago
Bug 382754 eliminates mutation event vector off branches here. I could not produce another reliably crashing testcase other than with mutation events. There is a testcase on bug 378146, but it is debug only. I'm not sure about what should be done to make this crash w/o mutation events in non-DEBUG. Boris?
Restyles that trigger XBL constructors would probably do the trick...

Comment 55

11 years ago
Comment on attachment 266598 [details] [diff] [review]
synchronous patch for branches, rev 3

Nothing worth looking at in this patch. Working on new testcase now.
Attachment #266598 - Attachment is obsolete: true
Flags: blocking1.8.1.5+ → blocking1.8.1.6?
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?

Comment 56

10 years ago
I could not produce another testcase here, perhaps my XBL skills were not sufficient. This one needs a new owner.
Assignee: vladimir.sukhoy → rbs
Status: ASSIGNED → NEW
Whiteboard: [sg:critical?] → [sg:critical?][dbaron-1.9:RsCe]
To summarize the current situation (AIUI):

We want to implement effects of the scriptlevel attribute (including implicit
effects from several math element tags) on the font-size style property:
http://www.w3.org/TR/MathML2/chapter3.html#id.3.3.4.2.1 and 3.3.4.2.2.

But the problem with the current implementation is that it calls SetAttr from
frame construction and destruction, which is not safe.

There seem to be 3 options to implementing this safely:

Option 1: Implement this in the content/style system.

  This looks to me like the right place to implement this.
  The style system already handles computed font sizes and this looks like
  just another parameter in the equation.

  The argument against this has been that it is too much logic to do in
  content.  Is that too much work or too much code that we don't want in
  content/style?

  Can anyone persuade me that this is not the best solution?

Option 2: Make SetAttr safe to call from frame construction/destruction.

  This has already been done for mutation events, but I assume such safety for
  XBL notifications is not going to happen.

Option 3: Use asynchronous events posted from frame construction/destruction

  These events would update the style system at some stage between frame
  construction/destruction and reflow.

  I get the impression from comment 35 that this also a requires a fair amount
  of work.

A MathML element from Bug 276267 may be helpful here.

Is all non-MathML layout code that calls SetAttr anonymous content and thus
not exploitable?
>  The argument against this has been that it is too much logic to do in
>  content.

That, and that it doesn't fit well with the CSS model of how life works.  See comment 1.  If you can figure out a way to deal with the issues cited in that newsgroup post, this is by far the best option.

> Option 2: Make SetAttr safe to call from frame construction/destruction.

We could try to do this, if called with aNotify == PR_FALSE...

> Is all non-MathML layout code that calls SetAttr anonymous content and thus
> not exploitable?

We certainly hope so.  And even that, we've been working on eliminating.
Did I miss something? How do script execute when SetAttr is called with
aNotify == false?

Not that I'm a big fan of layout messing around with content anyway...
We might be there already.... But note that "safe" doesn't just mean "doesn't run script" it also means "doesn't enter frame construction or style resolution or layout".  You want examples where setting an attribute with aNotify == PR_FALSE actually ends up notifying?  We've got some, last I checked.
(In reply to comment #58)
> >  The argument against this has been that it is too much logic to do in
> >  content.
> 
> That, and that it doesn't fit well with the CSS model of how life works.  See
> comment 1.  If you can figure out a way to deal with the issues cited in that
> newsgroup post, this is by far the best option.

Here's what I was thinking...
1) Have a pass that runs *before* restyling which traverses the DOM and assigns scriptlevels to elements as some sort of content property. Enable this pass only when the DOM contains MathML elements, or something like that.
2) Whack the style system computation of font-size to check for a scriptlevel on each element and adjust the computed font-size to suit. Again, enable only when MathML is present.
Priority: -- → P3
Priority: P3 → P2
I'm going to take this for myself, because it needs to be fixed soon or all of MathML is at risk.
Assignee: rbs → roc
Priority: P2 → P1
I've done a little bit of research here. I think we can *almost* do this in the style system. The basic idea would be to add style properties -moz-script-level, -moz-script-level-multiplier, and -moz-script-min-size.

-moz-script-level would basically be the same as the MathML property, accepting absolute nonnegative integers and +NNN, -NNN values. We would map the MathML scriptlevel attribute into this CSS property, and put rules for the MathML elements' default scriptlevel effects into html.css*. The style system would then compute an absolute script-level for every style context.

-moz-script-level-multiplier and -moz-script-min-size would just be mapped attributes.

Then in nsRuleNode::SetFont we would consult those properties to compare the scriptlevel of the parent context with the scriptlevel of the current context, and adjust the inherited font-size before applying a font-size rule (if any), taking -moz-script-min-size into account. There's some trickery here to ensure that fonts that go below -moz-script-min-size and then have their scriptlevels decremented Do The Right Thing, but I think that's solvable, because I believe in SetFont we have access to the style context ancestors involved in font-size computation.

* There seem to be a couple of cases that are too difficult to handle easily in the style system:
-- munder/mover/munderover set scriptlevel on their children based on the value of accent and accentunder, which default to inspecting the contents of the overscript or underscript, possibly arbitrarily deep into the DOM if the overscript or underscript is an embellished operator. To solve this I propose that we add a magic attribute to munder/mover/munderover that indicates what the default value of accent and accentunder should be, updated off an event, and checked/invalidated during reflow of the munder/mover/munderover's frame.
-- mfrac sets scriptlevel on its children depending on whether their "displaystyle" is false or not. Again, I propose magic attributes on the mfrac that tell us whether scriptlevel change is needed on the numerator and/or denominator, to be updated off an event and checked/invalidated during reflow of the mfrac frame.

Any thoughts? I'm not sure whether the new properties would go into nsStyleFont or some new nsStyleMathML. I suspect it would be easiest to put them in nsStyleFont.

Comment 64

10 years ago
I am wondering if all that is easier to do if having the MathML element class, like the one from bug 276267.
I don't see how that would help here. Maybe with the mapped-attribute stuff. I'll see.
FWIW, if you want to stick data on content nodes you can use nsINode::GetProperty/SetProperty
Yeah, that's a good idea, better than a magic attribute.
Hmm, but then mapping that data into the style system might be hard. Probably easier to map a magic attribute.
A few thoughts:

 * should changes to the multiplier be retroactive (in terms of DOM nesting), or only apply to scriptlevel changes in descendants?  (This relates to the next question.)

 * consider which of nsStyleFont::mSize and nsStyleFont::mFont.size you want to change

 * Putting this stuff in nsStyleFont is a bit bad since it's not reset by the 'font' property so you'd bloat the rule tree a bit in cases where the 'font' property had previously set everything in the struct; however, doing otherwise is scary since then you'd need to manage cross-struct computed value dependencies.
Only apply to scriptlevel changes in descendants, I think.

I think we'll put this stuff in nsStyleFont but allow 'font' to reset the struct if the mathml.css sheet is not loaded. Then when mathml.css loads, we'll have to blow away the rule tree.
The current MathML attribute-mapping code in nsMathMLFrame is horrible. Rather than build on it I think I'll take the code in bug 276267 and build on that. Thanks Vlad!
Depends on: 404406
I filed bug 404406 with a patch to fix up the attribute mapping to be content-based. That patch also has attribute mapping logic for scriptlevel, scriptminsize and scriptsizemultiplier which I'll build on to fix this bug here.
I just discovered a problem with this approach. munderover, unlike any other element, may need to update the scriptlevel on its second child differently from its third child. I don't think I can do this in a normal CSS rule. Some kind of additional hack will be required.
Blocks: 405271
Blocks: 405187
Created attachment 290198 [details] [diff] [review]
omnibus patch

Okay, my attempt to make this patch separate from the work in bug 404406 didn't really work out due to multiple mistakes on my end. So here's what this patch does:

-- Add a MathMLEnabled flag to nsIDocument. This is set when a MathML element is bound to the document.
-- Add eMATHML to IsNodeOfType.
-- Allow nsPresShellIterator to iterate over hidden presshells.
-- Refactor attribute-mapping logic from nsGenericHTMLElement into a new base
class nsMappedAttributeElement.
-- Add NS_EVENT_STATE_MOZ_MATH_INCREMENT_SCRIPT_LEVEL content state so that we can set it on certain MathML nodes and match on that state in the style system.
-- Create nsMathMLElement derived from nsMappedAttributeElement
-- Move nsPresContext::ClearStyleDataAndReflow to nsCSSFrameConstructor::RebuildStyleData, with a PostRebuildStyleData option which simply sets a flag to trigger RebuildStyleData at the next restyle, and also fires a restyle event to ensure that restyle will happen.
-- nsMathMLElement does a PostRebuildStyleData when MathML is first enabled, to clear up the style system optimizations we're going to do assuming MathML is disabled
-- Implement MathML attribute mapping logic in nsMathMLElement
-- Implement new mapping logic for scriptlevel, scriptminsize,
scriptsizemultiplier
-- The new mapping logic restricts attributes to the elements for which the
MathML REC says they should apply; the old logic did not.
-- Implement an "increment script level" flag in nsMathMLElement that controls the new content state.
-- Remove attribute mapping logic from MathML frames
-- Remove scriptlevel from MathML frame PresentationData, since nsStyleFont has it now
-- For mfrac, munder, mover, munderover, where CSS rules can't determine whether to increment the scriptlevel, we have nsMathMLContainerFrame::SetIncrementScriptLevel which sets the content state on the child whose scriptlevel needs to be incremented. When the state is changed in this way we schedule a post-reflow callback to ensure restyle and re-reflow happens immediately.
-- Only scriptlevel is exposed as a parseable CSS property; scriptsizemultiplier and scriptminsize are only settable via mapped attributes. And scriptlevel can only be set by UA sheets, and only in a "relative" way (it can be set absolutely via mapped attribute).
-- There's a -moz-math-increment-script-level pseudo-class to expose the new content state.
-- An optimization in nsRuleNode::CheckSpecifiedProperties detects when we specified everything except for the three script* properties in the font rule data, and treats that as "fully specified" when MathML is disabled
-- The heart of the patch is ComputeScriptLevelSize, which actually computes the font-size that should be inherited from the parent after adjusting for the change in scriptlevels. The spec is unclear and I think underspecifies how this should work (especially when scriptsizemultiplier, scriptminsize and CSS font-size change along an ancestor chain), but hopefully what I've written makes sense.
-- That gets hooked into nsRuleNode::SetFont's font size calculation
-- nsRuleNode::ComputeFontData computes the script* properties before we need them to compute the font size
-- nsStyleFont has the new properties added to it. The nsStyleFont no-arg constructor is dropped because it's not used, and the nsFont& constructor gets a prescontext parameter so we can set the zoomed font size in the constructor instead of in the caller. I think this isn't strictly needed in this patch but it makes the constructors more consistent.
-- One key new value is mSuppressedScriptScale. This is how the hairy scriptminsize requirements are handled. It records how much more we would have scaled the text by if scriptminsize had not been applied anywhere. I considered various approaches but this seems to be the simplest and most understandable way to achieve the desired results.

It's hard to write tests for this stuff right now because most MathML does not display on trunk. I've got testcases that I've verified using DOM Inspector to check the font size. I think almost all this can ultimately be tested with reftests.
Attachment #290198 - Flags: superreview?(dbaron)
Attachment #290198 - Flags: review?(dbaron)
I don't think we need the presshell iterator change.  We only have hidden presshells when we're in bfcache, and if BindToTree happens we'll be dropped from bfcache, which will destroy the presshell anyway.
Good point. Consider it gone!
Created attachment 290290 [details] [diff] [review]
patch v2

Sorry, that patch was missing some files. This patch has it all, plus Boris's comment.
Attachment #290198 - Attachment is obsolete: true
Attachment #290198 - Flags: superreview?(dbaron)
Attachment #290198 - Flags: review?(dbaron)
Created attachment 290291 [details] [diff] [review]
sorry, this is the right updated patch
Attachment #290290 - Attachment is obsolete: true
Attachment #290291 - Flags: superreview?(dbaron)
Attachment #290291 - Flags: review?(dbaron)
Once this is fixed, there are five other situations where MathML messes with the DOM in potentially evil ways, that I know of:

1) nsMathMLTokenFrame::SetTextStyle sets an attribute with aNotify=false. This gets called during SetInitialChildList. It also gets called during MarkIntrinsicWidthsDirty, and also posts a restyle event in that case. This is fairly easy to replace with one or two new content state bits and CSS pseudoclasses.

2) nsMathMLTokenFrame::SetInitialChildList can also call SetQuotes which calls SetText on anonymous child content with aNotify=false. It should be quite easy to fix by mapping "lquote" and "rquote" to a CSS "quotes" rule via attribute mapping.

3) nsMathMLTokenFrame::Init calls CompressWhitespace to remove leading and trailing whitespace by bashing the DOM text node (aNotify=false). A better way to do this could be to add new state bits to nsTextFrame to indicate "always trim leading space" and "always trim trailing space", with nsMathMLTokenFrame maintaining these bits on its child text frames.

4) nsMathMLmactionFrame::Init can do an UnsetAttr to remove its actiontype attribute for "restyle" actiontypes (aNotify=false). I think we could cut support for all the maction actiontypes; the MathML2 spec says that's still conforming, no-one in their right mind would use them instead of the regular DOM APIs, I don't know who implements them other than us, anything that generates MathML is unlikely to use them, etc. "restyle" in particular isn't even mentioned in the spec so I definitely think we should kill it. (If we absolutely had to keep this actiontype stuff I'd recommend reimplementing maction as an XBL binding.)

5) nsMathMLmtableFrame does horrible things with rowalign, columnalign, rowlines and columnlines attributes on tables (and columnalign and columnlines attributes on rows too). Each of these attributes can contain a *list* of values which get mapped to style on a sequence of columns and rows --- in our implementation, by setting magic attributes. This mapping happens in nsMathMLmtableFrame::SetInitialChildList and also in InsertFrames, AppendFrames etc, although always with aNotify=false. I'm not really sure how to fix this one.

One approach might be to define new CSS property values "-moz-row-sequence(...)" and "-moz-col-sequence(...)", map these multiple-value attributes to those property values, which inherit down, and when we compute style for a table cell, if it has inherited a -moz-row-sequence or -moz-col-sequence value then we get its row or column index and use that to grab the right value from the sequence. I'd appreciate feedback on that idea...

Is aNotify=false SetAttr/SetText during frame construction really still a security issue on trunk? Because I'd hate to fix all this for 1.9 unless it's really necessary.
Whiteboard: [sg:critical?][dbaron-1.9:RsCe] → [sg:critical?][dbaron-1.9:RsCe][needs review]
aNotify=false changes should be pretty safe on trunk, I think.
Okay then, AFAIK we don't need to fix those things for 19. "phew"
*If* we find that it's unsafe to call with aNotify=false, then IMHO *that* is the bug and should be fixed.
PostRebuildStyleData should be called PostRebuildStyleDataEvent
throughout; otherwise it's easy to think it's the function called at the
end of RebuildStyleData rather than a function that posts an event.

I think this is going to make style system mochitests fail, at the very
least because of your mUnsafeRulesEnabled check in nsCSSParser.  You'll
probably have to add an explicit exception for the property in
ListCSSProperties.cpp (see gInaccessibleProperties).  Or is there a way
you see to make the mochitests test this property?

I only see one property added in nsCSSPropList.h, whereas the nsRuleNode
code assumes you're adding three (in the specified+total stuff).  Don't
you need to add all three to nsCSSPropList.h ?

And maybe the ifdef MOZ_MATHML in nsCSSPropList.h should come near the
end, just before the ifdef MOZ_SVG ?

The include guards in nsMappedAttributeElement.h should not begin with
two underscores.  (That's implementation-reserved.)

The constructor for nsMappedAttributeElement does not need explicit
"inline".

NS_EVENT_STATE_MOZ_MATH_INCREMENT_SCRIPT_LEVEL seems like a bit of abuse
of event states, but I guess it works.  However, I'll suggest changing
the name because I think it's too long; we don't generally use MOZ in
our other extension event states, and we may want it a little clearer
that this is the state that the markup calls for incrementing the script
level.  So maybe NS_EVENT_STATE_MATHML_INCREMENT_SCRIPT_LEVEL ?  Then
again, that's only one character shorter.

Are the INCLUDES and REQUIRES in content/mathml/content/src/Makefile.in
really all necessary, or were they just copied from elsewhere?  (And the
use of topsrcdir rather than srcdir for only the last one seems odd.)

Does nsMathMLElement really need its own classinfo ID, or should it just
use Element?  jst or sicking would know, I think.  (If it does, maybe it
should be added at the end, like the comment says?)

I don't think you can add to nsDOMClassInfoID.h without making a
corresponding change to nsDOMClassInfo.cpp (in 2 places), but I don't
see that change, so I expect this will break anything with classinfo IDs
after the one you added (not at the end).

Your nsMathMLElement's QueryInterface method doesn't call
PostQueryInterface, which I think it needs to to avoid breaking XBL's
implements.

The comment in nsMathMLElement::BindToTree should explicitly mention the
code in nsRuleNode::CheckSpecifiedProperties (and vice versa).

Something around CheckSpecifiedProperties needs to assert that the three
mathml properties are not specified if mathml is disabled for the
document.

I'm up to nsMathMLElement::ParseAttribute.
(In reply to comment #83)
> PostRebuildStyleData should be called PostRebuildStyleDataEvent
> throughout; otherwise it's easy to think it's the function called at the
> end of RebuildStyleData rather than a function that posts an event.

Right OK.

> I think this is going to make style system mochitests fail, at the very
> least because of your mUnsafeRulesEnabled check in nsCSSParser.  You'll
> probably have to add an explicit exception for the property in
> ListCSSProperties.cpp (see gInaccessibleProperties).

OK.

> I only see one property added in nsCSSPropList.h, whereas the nsRuleNode
> code assumes you're adding three (in the specified+total stuff).  Don't
> you need to add all three to nsCSSPropList.h ?

Yep, sorry.

> And maybe the ifdef MOZ_MATHML in nsCSSPropList.h should come near the
> end, just before the ifdef MOZ_SVG ?

Yeah OK.

> The include guards in nsMappedAttributeElement.h should not begin with
> two underscores.  (That's implementation-reserved.)

OK.

> The constructor for nsMappedAttributeElement does not need explicit
> "inline".

OK.

> NS_EVENT_STATE_MOZ_MATH_INCREMENT_SCRIPT_LEVEL seems like a bit of abuse
> of event states, but I guess it works.  However, I'll suggest changing
> the name because I think it's too long; we don't generally use MOZ in
> our other extension event states, and we may want it a little clearer
> that this is the state that the markup calls for incrementing the script
> level.  So maybe NS_EVENT_STATE_MATHML_INCREMENT_SCRIPT_LEVEL ?  Then
> again, that's only one character shorter.

NS_EVENT_STATE_INCREMENT_SCRIPT_LEVEL?

> Are the INCLUDES and REQUIRES in content/mathml/content/src/Makefile.in
> really all necessary, or were they just copied from elsewhere?  (And the
> use of topsrcdir rather than srcdir for only the last one seems odd.)

Actually this was part of the patch that I started with from bug 276267, and I forgot to check/update it. I'll fix that up.

> Does nsMathMLElement really need its own classinfo ID, or should it just
> use Element?  jst or sicking would know, I think.  (If it does, maybe it
> should be added at the end, like the comment says?)

I don't really know what the classinfo ID is for. Again I just brought this in from the patch in bug 276267, which did get sr+ from Jonas... So I think I'll keep the ID, just move it to the end.

> I don't think you can add to nsDOMClassInfoID.h without making a
> corresponding change to nsDOMClassInfo.cpp (in 2 places), but I don't
> see that change, so I expect this will break anything with classinfo IDs
> after the one you added (not at the end).

Good point, I'll add that. That was missing from the patch that got sr from Jonas...

> Your nsMathMLElement's QueryInterface method doesn't call
> PostQueryInterface, which I think it needs to to avoid breaking XBL's
> implements.

OK, ditto

> The comment in nsMathMLElement::BindToTree should explicitly mention the
> code in nsRuleNode::CheckSpecifiedProperties (and vice versa).

OK.

> Something around CheckSpecifiedProperties needs to assert that the three
> mathml properties are not specified if mathml is disabled for the
> document.

Good idea.
The sTokenStyles and sEnvironmentStyles arrays need to be
null-terminated (i.e., with { null }).

+  if (tag == nsGkAtoms::ms_ || tag == nsGkAtoms::mi_ || tag == nsGkAtoms::mn_ ||
+      tag == nsGkAtoms::mo_ || tag == nsGkAtoms::mtext_)

The spec (REC-MathML2-20031021) also lists mglyph as supporting these
attributes.  Should we include that as well?  (Do we support it?  Should
we include it even if we don't currently support it?)  Or maybe not; see
http://lists.w3.org/Archives/Public/www-math/2007Nov/0016  (I guess we
should probably leave things as-is.)

Also, could you wrap at something less than 80 characters?  Going to
exactly 80 makes diffs messy.

+  // It doesn't really matter what our tag is here, because only attributes
+  // that satisfy IsAttributeMapped will be stored in the mapped attributes list
+  // and available to the mapping function

Again, could you wrap at something less than 80 characters?  Going to
exactly 80 makes diffs messy.

+  nsAutoString str(aString);
+  str.CompressWhitespace(); //  aString is not a const in this code...

aString is const, str is not.

ParseNumericValue needs parameters for whether it should accept unitless
numbers, since many of the values it's used for parsing don't allow
them.  Otherwise you'll end up with values nsRuleNode doesn't know how
to handle, which leads to inconsistent results due to the rule tree
optimizations.  However, this means it does need to be changed to allow
unitless 0 always.

I also don't think ParseNumericValue should allow a space between the
number and the unit.  It doesn't look like the spec allows it; see
http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter2.html#fund.units
Same for space between the minus sign and the unit.  Actually, the spec
is explicit about this in 
http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter2.html#fund.attval
where it says "Whitespace is not otherwise required, but is permitted
between any of the tokens listed above, except (for compatibility with
CSS) immediately before unit identifiers, between the '-' signs and
digits of negative numbers, or ..."

I also think that ParseNumericValue should make the units
case-insensitive; I don't see anything in the MathML spec either way,
except "The unit identifiers and meanings are taken from CSS." which
would imply case-insensitive.  But I'm not sure, so I asked:
http://lists.w3.org/Archives/Public/www-math/2007Nov/0015

It's worth checking that nsCString::ToFloat returns an error given the
string ".".  (Although it's not entirely clear to me that this is an
error.  Maybe I should email www-math about that too.  So I suppose
nothing to do here, really...)

In nsMathMLElement::MapMathMLAttributesInto:

  the handling of scriptsizemultiplier needs to forbid numbers with
  initial + signs

  the handling of scriptminsize needs to forbid unitless numbers (see
  above)

  the handling of all attributes should check that the value is
  currently null before assigning into it (you currently only check this
  for font size and font family, but I think it's better to be
  consistent in case we add a CSS way to get to the other values later)

  the handling of scriptsizemultiplier and scriptminsize needs to strip
  whitespace from the edges of the value before calling
  ToInteger/ToFloat, unless they accept that whitespace already

  the parsing of fontsize and mathsize should be separated, since small,
  normal, and big are only allowed for the latter.  (It's not even clear
  to me from the spec which should win.)

  both mathsize and fontsize should not allow unitless numbers

  when you set aData->mFontData->mFamily, you should also set
  mFamilyFromHTML to PR_FALSE.

Does the color parsing used for background and mathbackground support
#rgb colors?  (I know it supports #rrggbb.)  It should for both.  It
should also support 'transparent' (maybe for background only, but see
http://lists.w3.org/Archives/Public/www-math/2007Nov/0017 ).  Does it?

I realize some of these issues are in existing code that you moved.  If
necessary, file followup bugs instead of fixing.

Your use of NS_IMPL_ELEMENT_CLONE shouldn't have a semicolon at the end;
that'll probably be a compiler error on newer gccs.

I almost wonder if nsMathMLElement::SetIncrementScriptLevel should take
an aNotify parameter just to document that it does notifications (and
alert callers that they should care).

The include guards for nsMathMLElement shouldn't end with a
double-underscore.  (Per C++ 17.4.3.1.2 [lib.global.names], any name
that contains "__" or begins with "_" followed by a capital letter is
reserved for the implementation for any use; any other name beginning
with underscore is reserved for the implementation for use as a name in
the global namespace.)

In nsMathMLElement.h, there shouldn't be trailing semicolons after the
NS_DECL_ISUPPORTS_INHERITED or any of the NS_FORWARD macros.

In nsMathMLElement.h, GetIncrementScriptLevel should probably be a const
method.

Maybe RebuildStyleData, PostRebuildStyleDataEvent, and mRebuildStyleData
should be *RebuildAllStyleData instead, to avoid confusion with some of
the other restyling things that don't restyle everything?  The name
isn't as dramatic as the old "Clear*"; I think "All" would help.  (Or
maybe "ClearAndRebuild*"?)

In layout/build/Makefile.in, you're inconsistent with the surrounding
code regarding whether to use tabs.

I'm up to the layout/mathml changes.
We don't support mglyph, and probably won't in the near future.
(In reply to comment #85)
> The sTokenStyles and sEnvironmentStyles arrays need to be
> null-terminated (i.e., with { null }).

OK

> +  if (tag == nsGkAtoms::ms_ || tag == nsGkAtoms::mi_ || tag == nsGkAtoms::mn_
> ||
> +      tag == nsGkAtoms::mo_ || tag == nsGkAtoms::mtext_)
> 
> The spec (REC-MathML2-20031021) also lists mglyph as supporting these
> attributes.  Should we include that as well?  (Do we support it?  Should
> we include it even if we don't currently support it?)  Or maybe not; see
> http://lists.w3.org/Archives/Public/www-math/2007Nov/0016  (I guess we
> should probably leave things as-is.)

We don't support mglyph so I left it out. I guess I should add a comment here to explain why.

> Also, could you wrap at something less than 80 characters?  Going to
> exactly 80 makes diffs messy.

OK

> +  nsAutoString str(aString);
> +  str.CompressWhitespace(); //  aString is not a const in this code...
> 
> aString is const, str is not.

Oops

> ParseNumericValue needs parameters for whether it should accept unitless
> numbers, since many of the values it's used for parsing don't allow
> them.  Otherwise you'll end up with values nsRuleNode doesn't know how
> to handle, which leads to inconsistent results due to the rule tree
> optimizations.  However, this means it does need to be changed to allow
> unitless 0 always.

OK

> I also don't think ParseNumericValue should allow a space between the
> number and the unit.  It doesn't look like the spec allows it; see
> http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter2.html#fund.units
> Same for space between the minus sign and the unit.  Actually, the spec
> is explicit about this in 
> http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter2.html#fund.attval
> where it says "Whitespace is not otherwise required, but is permitted
> between any of the tokens listed above, except (for compatibility with
> CSS) immediately before unit identifiers, between the '-' signs and
> digits of negative numbers, or ..."

OK

> I also think that ParseNumericValue should make the units
> case-insensitive; I don't see anything in the MathML spec either way,
> except "The unit identifiers and meanings are taken from CSS." which
> would imply case-insensitive.  But I'm not sure, so I asked:
> http://lists.w3.org/Archives/Public/www-math/2007Nov/0015

OK. I really just moved ParseNumericValue from nsMathMLFrame but I might as well fix these issues while I'm here.

> It's worth checking that nsCString::ToFloat returns an error given the
> string ".".  (Although it's not entirely clear to me that this is an
> error.  Maybe I should email www-math about that too.  So I suppose
> nothing to do here, really...)

Er, ok :-)

> In nsMathMLElement::MapMathMLAttributesInto:
> 
>   the handling of scriptsizemultiplier needs to forbid numbers with
>   initial + signs

OK

>   the handling of scriptminsize needs to forbid unitless numbers (see
>   above)

OK

>   the handling of all attributes should check that the value is
>   currently null before assigning into it (you currently only check this
>   for font size and font family, but I think it's better to be
>   consistent in case we add a CSS way to get to the other values later)

OK

>   the handling of scriptsizemultiplier and scriptminsize needs to strip
>   whitespace from the edges of the value before calling
>   ToInteger/ToFloat, unless they accept that whitespace already

OK

>   the parsing of fontsize and mathsize should be separated, since small,
>   normal, and big are only allowed for the latter.  (It's not even clear
>   to me from the spec which should win.)

OK, but isn't it clear that the math* attributes new in MathML2 should dominate the deprecated attributes from MathML1?

>   both mathsize and fontsize should not allow unitless numbers

OK

>   when you set aData->mFontData->mFamily, you should also set
>   mFamilyFromHTML to PR_FALSE.

OK

> Does the color parsing used for background and mathbackground support
> #rgb colors?

It does,
http://mxr.mozilla.org/seamonkey/source/gfx/src/nsColor.cpp#79

> (I know it supports #rrggbb.)  It should for both.  It
> should also support 'transparent' (maybe for background only, but see
> http://lists.w3.org/Archives/Public/www-math/2007Nov/0017 ).  Does it?

No, I don't think it does. I'll fix that.

> I realize some of these issues are in existing code that you moved.  If
> necessary, file followup bugs instead of fixing.

I'll fix the ones that are easy and file bugs on the rest.

> Your use of NS_IMPL_ELEMENT_CLONE shouldn't have a semicolon at the end;
> that'll probably be a compiler error on newer gccs.

OK

> I almost wonder if nsMathMLElement::SetIncrementScriptLevel should take
> an aNotify parameter just to document that it does notifications (and
> alert callers that they should care).

That's not a bad idea, sure.

> The include guards for nsMathMLElement shouldn't end with a
> double-underscore.  (Per C++ 17.4.3.1.2 [lib.global.names], any name
> that contains "__" or begins with "_" followed by a capital letter is
> reserved for the implementation for any use; any other name beginning
> with underscore is reserved for the implementation for use as a name in
> the global namespace.)

OK

> In nsMathMLElement.h, there shouldn't be trailing semicolons after the
> NS_DECL_ISUPPORTS_INHERITED or any of the NS_FORWARD macros.

OK

> In nsMathMLElement.h, GetIncrementScriptLevel should probably be a const
> method.

Sure

> Maybe RebuildStyleData, PostRebuildStyleDataEvent, and mRebuildStyleData
> should be *RebuildAllStyleData instead, to avoid confusion with some of
> the other restyling things that don't restyle everything?  The name
> isn't as dramatic as the old "Clear*"; I think "All" would help.  (Or
> maybe "ClearAndRebuild*"?)

I'll add "All".

> In layout/build/Makefile.in, you're inconsistent with the surrounding
> code regarding whether to use tabs.

OK, I'll fix.
I wonder whether some of the AttributeChanged methods on the frames are
still needed.

+mroot > * { -moz-script-level:+2; }
+mroot > *:first-child { -moz-script-level:+0; }
+
+msub, msup, msubsup, mmultiscripts > * { -moz-script-level:+1; }
+msub, msup, msubsup, mmultiscripts > *:first-child { -moz-script-level:+0; }

These pairs could probably be combined using :not(:first-child).

Also, I don't think you want to relist "msub, msup, msubsup" in the
last rule, but fixing the previous comment would fix that.  (> has
higher precedence than ,, but I think other than the last line, the
rules are correct as-is.)

In nsCSSParser, you probably need to add the other two properties in a
bunch of places -- at least ParseSingleValueProperty (to not be parsed) 
-- look for compiler warnings for missing cases.

In nsStyleStruct.h, could you add a comment after mScriptMinSize saying
what types of values the nsStyleCoord has?  (Most nsStyleCoord values 
have such comments.)

I still need to look at the nsRuleNode changes.
(In reply to comment #88)
> I wonder whether some of the AttributeChanged methods on the frames are
> still needed.

They are, as far as I can tell.

> +mroot > * { -moz-script-level:+2; }
> +mroot > *:first-child { -moz-script-level:+0; }
> +
> +msub, msup, msubsup, mmultiscripts > * { -moz-script-level:+1; }
> +msub, msup, msubsup, mmultiscripts > *:first-child { -moz-script-level:+0; }
> 
> These pairs could probably be combined using :not(:first-child).

Good idea.

> Also, I don't think you want to relist "msub, msup, msubsup" in the
> last rule, but fixing the previous comment would fix that.  (> has
> higher precedence than ,, but I think other than the last line, the
> rules are correct as-is.)

So I'll need to write
msub > :not(:first-child), msup > :not(:first-child), msubsup > :not(:first-child), mmultiscripts > :not(:first-child) { -moz-script-level:+1; }
right?

> In nsCSSParser, you probably need to add the other two properties in a
> bunch of places -- at least ParseSingleValueProperty (to not be parsed) 
> -- look for compiler warnings for missing cases.

OK

> In nsStyleStruct.h, could you add a comment after mScriptMinSize saying
> what types of values the nsStyleCoord has?  (Most nsStyleCoord values 
> have such comments.)

OK

> I still need to look at the nsRuleNode changes.

That's the most exciting part of all!
(In reply to comment #89)
> So I'll need to write
> msub > :not(:first-child), msup > :not(:first-child), msubsup >
> :not(:first-child), mmultiscripts > :not(:first-child) { -moz-script-level:+1;
> }
> right?

Right.  I was looking at the spec too quickly.
(In reply to comment #85)
> The spec (REC-MathML2-20031021) also lists mglyph as supporting these
> attributes.  Should we include that as well?  (Do we support it?  Should
> we include it even if we don't currently support it?)  Or maybe not; see
> http://lists.w3.org/Archives/Public/www-math/2007Nov/0016  (I guess we
> should probably leave things as-is.)

Ignore this comment (see response on list).

> I also think that ParseNumericValue should make the units
> case-insensitive; I don't see anything in the MathML spec either way,
> except "The unit identifiers and meanings are taken from CSS." which
> would imply case-insensitive.  But I'm not sure, so I asked:
> http://lists.w3.org/Archives/Public/www-math/2007Nov/0015

Same here.
+  } else if (aFont->mSize != incomingParentSize) {
+    // There was no rule affecting the size but the size was affected by the
+    // parent's size via scriptlevel change. So treat this as inherited.
+    // XXX Is this needed?
+    aInherited = PR_TRUE;
+    aFont->mSize = incomingParentSize;

Could you explain why you think this is needed?  I think it's either unneeded or harmful (probably the latter, but not sure).  Or maybe both needed and harmful...
I wasn't sure whether it was needed. If you think it's not needed I'll just take it out.

I wasn't sure what sort of relationship aInherited indicates. There's certainly a dependence of the font-size on the parent style context's font-size here, and it's more complex than just a copy.
(In reply to comment #93)
> I wasn't sure whether it was needed. If you think it's not needed I'll just
> take it out.

Oh, you absolutely need to set aInherited to true if you do that.  I was thinking the comment referred to the whole else.  I need to think more about whether you can do that at all (given that you're setting aInherited to true).
You need to fix CheckFontCallback so that if mScriptLevel is an integer
unit, it does the promotion that already happens for certain values of
'font-size' and 'font-weight'.  You may even want to add a test for
that (i.e., that relative values work correctly, relative to a
non-default value, in a case where all other values in nsStyleFont are
given non-inherited values).

I wonder if you should add an IntegerOffset value to nsCSSValue instead
of using eCSSUnit_Integer for the absolute mScriptLevel values (i.e.,
switch the absolute ones from Number to Integer and put the relative
ones in IntegerOffset).  If so, probably better as a separate patch.

+  } else if (aFont->mSize != incomingParentSize) {
+    // There was no rule affecting the size but the size was affected by the
+    // parent's size via scriptlevel change. So treat this as inherited.
+    // XXX Is this needed?
+    aInherited = PR_TRUE;
+    aFont->mSize = incomingParentSize;

So this will break some cases of rule tree caching.  In particular, if
you have the style rules (or markup equivalent):

element {
  /* specify everything in the 'font' struct to non-inherited values */
  font: large serif;
  -moz-script-min-size: 6px;
  -moz-script-size-multiplier: 0.8;
  -moz-script-level: 0;
}

element#id {
  -moz-script-level: 1;
}

and the element element with id id is not the first element element in
the document, then when you call ComputeStyleFont for the element
element with id id, you'll be computing only the diff from the cached
style data for the element rule, using only the element#id rule.  This
means you'll have no value for font-size, yet you still want to compute
using the value of font-size.


I think for SetGenericFont to work, you need to move the computation of
the new properties into SetFont, since SetGenericFont uses SetFont to do
hypothetical computation given a different generic font (for monospace
vs. non-monospace) and ComputeScriptLevelSize depends on having correct
values of those properties for both aFont and aParent.  And then you
need to make the three places in SetGenericFont that copy font structs
copy the new members (maybe you want to add an operator= to nsStyleFont
for this).  Otherwise script sizes will break pretty badly if the font
switches between generics (or between monospace and non-monospace?).
For that caching issue, aParentFont will be the same as aFont, though, which will start off as a copy of the data we're computing the difference from, so it might not be quite as bad as I think.  Maybe I'll be able to think it through when I'm someplace quieter.
Moving things around the way you suggested sounds OK.

> I wonder if you should add an IntegerOffset value to nsCSSValue instead
> of using eCSSUnit_Integer for the absolute mScriptLevel values (i.e.,
> switch the absolute ones from Number to Integer and put the relative
> ones in IntegerOffset).

I could do that, but is it really worth it? It'll add code.
(In reply to comment #97)
> I could do that, but is it really worth it? It'll add code.

Probably not, although it's not much code.
Comments on ComputeScriptLevelSize:

+    float defaultScriptMinSizeTwips = NS_POINTS_TO_TWIPS(NS_MATHML_DEFAULT_SCRIPT_MIN_SIZE_PT);
+    minScriptSize = aPresContext->TwipsToAppUnits(defaultScriptMinSizeTwips);

Could you put this all in one expression rather than using a temporary
variable so it's clearer you don't actually leave anything around that's
in twips?

ComputeScriptLevelSize needs to assign to aUnconstrainedSize in the
first early-return case.  Otherwise you'll do math on an uninitialized
float, which can crash or produce incorrect results.

In ComputeScriptLevelSize, I'd prefer if you s/aParent/aParentFont/g for
consistency with other functions.

+      // We can't decrease the font size at all, so just stick to no change

Maybe comment that this special case is required because authors are
allowed to explicitly set the font size smaller than the min script
level size.

+    // Apply constraint #2
+    return PR_MIN(scriptLevelSize, *aUnconstrainedSize);

I don't think this is right.  The unconstrained size can still be
smaller than the minimum script size, so you don't want to jump down to
the unconstrained on the first increase.  I think you really want
PR_MIN(scriptLevelSize, PR_MAX(*aUnconstrainedSize, minScriptSize)).

It might be nice to have a comment explaining the interaction of of
scriptlevel changes and font-size changes, which the MathML spec
describes in http://www.w3.org/TR/MathML2/chapter3.html#id.3.3.4.2.2 .
It associates the scriptlevel change with an edge between two nodes.  I
think your code is pretty close, but I think you also need the following
fix to do that correctly:

  All the places in SetFont that set to font-size to a non-relative unit
  (not % or em or ex or inherit) need to set mSuppressedScriptScale to
  1.0f .

+    // Note that font-based length units use the parent's size unadjusted
+    // for scriptlevel changes. A scriptlevel change between us and the parent
+    // is simply ignored.

This isn't clear to me from the spec.  Why 'em' and 'ex' and not '%'?
Or why any of them?  In any case, if that's what you want to pick, then
you probably need to copy mSuppressedScriptScale from the parent in this
case (rather than setting to 1.0f or leaving alone).

I think that's it, although I'm not sure how to fix the bug with rule
tree caching.  (Need to think about it a little more in a bit...)

Tests for various things in this patch would be good.  I'm a bit
concerned about the number of things I've caught during review that
should have been caught by testing.
Attachment #290291 - Flags: superreview?(dbaron)
Attachment #290291 - Flags: superreview-
Attachment #290291 - Flags: review?(dbaron)
Attachment #290291 - Flags: review-
(In reply to comment #99)
> Comments on ComputeScriptLevelSize:
> 
> +    float defaultScriptMinSizeTwips =
> NS_POINTS_TO_TWIPS(NS_MATHML_DEFAULT_SCRIPT_MIN_SIZE_PT);
> +    minScriptSize = aPresContext->TwipsToAppUnits(defaultScriptMinSizeTwips);
> 
> Could you put this all in one expression rather than using a temporary
> variable so it's clearer you don't actually leave anything around that's
> in twips?

OK

> ComputeScriptLevelSize needs to assign to aUnconstrainedSize in the
> first early-return case.  Otherwise you'll do math on an uninitialized
> float, which can crash or produce incorrect results.

OK

> In ComputeScriptLevelSize, I'd prefer if you s/aParent/aParentFont/g for
> consistency with other functions.

OK

> +      // We can't decrease the font size at all, so just stick to no change
> 
> Maybe comment that this special case is required because authors are
> allowed to explicitly set the font size smaller than the min script
> level size.

OK

> +    // Apply constraint #2
> +    return PR_MIN(scriptLevelSize, *aUnconstrainedSize);
> 
> I don't think this is right.  The unconstrained size can still be
> smaller than the minimum script size, so you don't want to jump down to
> the unconstrained on the first increase.  I think you really want
> PR_MIN(scriptLevelSize, PR_MAX(*aUnconstrainedSize, minScriptSize)).

Right.

> It might be nice to have a comment explaining the interaction of of
> scriptlevel changes and font-size changes, which the MathML spec
> describes in http://www.w3.org/TR/MathML2/chapter3.html#id.3.3.4.2.2 .
> It associates the scriptlevel change with an edge between two nodes.  I
> think your code is pretty close, but I think you also need the following
> fix to do that correctly:
> 
>   All the places in SetFont that set to font-size to a non-relative unit
>   (not % or em or ex or inherit) need to set mSuppressedScriptScale to
>   1.0f .

I think the spec is actually unclear here, so I don't think that's provably correct or incorrect. I'm happy to do it though.

> +    // Note that font-based length units use the parent's size unadjusted
> +    // for scriptlevel changes. A scriptlevel change between us and the parent
> +    // is simply ignored.
> 
> This isn't clear to me from the spec.

It's not clear to me either, either way, but this way seemed more useful to authors because it gives them a way to cancel the font size change due to a scriptlevel change.

> Why 'em' and 'ex' and not '%'?

Good question, I probably should add % too.

> Or why any of them?  In any case, if that's what you want to pick, then
> you probably need to copy mSuppressedScriptScale from the parent in this
> case (rather than setting to 1.0f or leaving alone).

OK yeah.

> Tests for various things in this patch would be good.  I'm a bit
> concerned about the number of things I've caught during review that
> should have been caught by testing.

It was difficult to test this because MathML frames don't draw reliably, or at least they didn't as of a couple of weeks ago, and there is no other way to set scriptlevels etc, so I was just setting up some testcases and poking around with DOM Inspector. I suppose I should write some mochitests that check computed style. Sorry about that.
This obviously isn't going to make beta2, but I think that's OK.
Target Milestone: mozilla1.9 M8 → ---
Whiteboard: [sg:critical?][dbaron-1.9:RsCe][needs review] → [sg:critical?][dbaron-1.9:RsCe]
The part of the spec I was thinking of was:

To prevent this problem, MathML renderers should, when decrementing scriptlevel, use as the initial font size the value the font size would have had if it had never been limited by scriptminsize. They should not, however, ignore the effects of explicit settings of fontsize, even to values below scriptminsize.

Without the setting to 1.0 when font-size is set, if you had:

 element E that had a high enough scriptlevel that it would have gone smaller than minScriptSize

 its child C that had a font-size explicitly specified larger than minScriptSize

 child's child G, which decremented scriptlevel

then G would end up with a font size larger than what it would have had had scriptminsize been ignored, since it would additionally be multiplied by an extra ratio that no longer made sense.

(I wonder if the code would be clearer if you carried around the unlimited size instead of the ratio of the unlimited size to the actual size, though.)
I'll try that. At some point I thought there were problems with it, but I can't right now see what they were, and the way I ended up writing the code to compute the scriptlevel effects changed after that.
So I think this is how you should solve the rule tree caching issue.

If aStartStruct is non-null, we know that the start struct has an explicit non-parent-dependent font size, which means that the scriptlevel calculations didn't influence the font size in the start struct.  If aStartStruct is non-null *and* aFontData.mSize.GetUnit() == eCSSUnit_Null, then we can derive from that that scriptlevel calculations don't influence the font size in the data we're computing.  Furthermore, we know that if we (not the start struct) have a parent-dependent font-size, which is the only case where the result of ComputeScriptLevelSize matters, parentData is real.  (These are worth saying in a comment explaining why what you're doing is ok.)  So I think what you need to do is:
  * have that final else in the font size chain have the condition end with && !aStartStruct
  * make sure that you're resetting the suppressed script scale appropriately when the font-size is set explicitly (as I said in comment 99 and comment 102)
  * only assign to aFont->mSuppressedScriptScale right after ComputeScriptLevelSize if !aStartStruct.  You should probably explicitly comment that we even want to skip the clause if aStartStruct && fontData.mScriptLevel.GetUnit() != eCSSUnit_Null, because the presence of aStartStruct *combined* with the absence of any value for font-size in the rules overriding aStartStruct means that the script level doesn't matter at all for the data we're computing (as I said above).

You should check that this makes sense, though. :-)
...so basically r+sr=dbaron once all of the above is fixed.
Target Milestone: --- → mozilla1.9 M11
I ended up just using eDOMClassInfo_Element_id instead of creating a new one for MathML. A new one isn't needed.
> In nsCSSParser, you probably need to add the other two properties in a
> bunch of places -- at least ParseSingleValueProperty (to not be parsed) 
> -- look for compiler warnings for missing cases.

Compiler warnings led me to ParseSingleValueProperty but nowhere else.
I'm afraid I don't understand comment #104. For one thing aStartStruct isn't defined in SetFont.

Actually I think I don't understand rule tree caching at all, since I don't understand the example in comment #95.

> you'll be computing only the diff from the 
> cached style data for the element rule, using only the element#id rule.
> This means you'll have no value for font-size, yet you still want to
> compute using the value of font-size.

Can you explain this a bit more? What precisely do you mean by "no value for font-size"? Certainly the parent nsStyleFont has a valid mSize.
(In reply to comment #108)
> I'm afraid I don't understand comment #104. For one thing aStartStruct isn't
> defined in SetFont.

Ah, right, you need to pass whether you're computing based on an aStartStruct through from ComputeFontData to SetFont.  SetGenericFont's call to SetFont should always act like there was no start struct.

> Actually I think I don't understand rule tree caching at all, since I don't
> understand the example in comment #95.

Does reading
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/style/nsRuleNode.h&rev=1.57&mark=261-300#261
and then
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/style/nsRuleNode.cpp&rev=1.229&mark=1358-1362#1358
help?  The latter is a performance optimization using (1) in the former.

> > you'll be computing only the diff from the 
> > cached style data for the element rule, using only the element#id rule.
> > This means you'll have no value for font-size, yet you still want to
> > compute using the value of font-size.
> 
> Can you explain this a bit more? What precisely do you mean by "no value for
> font-size"? Certainly the parent nsStyleFont has a valid mSize.

No value for font-size in fontData/aFontData, which means you'll hit that |else|.  (due to the startStruct code at the second URL I pasted above)
OK I think I'm finally starting to understand this now!
Here's one problem that arises when we try to track the actual scriptminsize-unconstrained size in nsStyleFont. If we have font-size:2ex, say, then to propagate the unconstrained size properly we have to create a font with that unconstrained size so we can measure its ex-height. With the approach in the previous patch, we were just recording the unconstrained size as a scale factor (mSuppressedScriptScale) relative to the actual size, and so CalcLength could stay the same and we'd just assume the font metrics scaled linearly, and apply mSuppressedScriptScale to the result of CalcLength.

This bug is really horrible, it's draining my will to live!
I guess things would be simpler if we make all relative length and percentage values for 'font-size' reset the scriptminwidth-unconstrained size to the computed size. But that's arguably a violation of the spec's requirement that we compute the unconstrained size as if no scriptminsize effects had taken place. What do you think David?
So CalcLength actually uses the parent font size *with* the user's min-font-size taken into account, which seems a little weird to me.
Created attachment 292909 [details] [diff] [review]
kill the monster

Okay! This patch takes into account all the comments so far. I took the high road with scriptminsize-unconstrained sizes by factoring font size computation into SetFontSize which I can call once for the real font size and once for the unconstrained size. I also added a bunch of reftests covering 1) basic scriptlevel/minscriptsize calculations 2) interaction between scriptlevels and explicit font-size 3) MathML attribute parsing and 4) rule-tree reconstruction when MathML becomes enabled for a document. I verified that the latter fails with the PostRebuildAllStyleDataEvent call commented out of nsMathMLElement::BindToTree.
Attachment #290291 - Attachment is obsolete: true
Attachment #292909 - Flags: superreview?(dbaron)
Attachment #292909 - Flags: review?(dbaron)
Whiteboard: [sg:critical?][dbaron-1.9:RsCe] → [sg:critical?][dbaron-1.9:RsCe][needs review]
Looks like you missed these from comment #95:
> I think for SetGenericFont to work, you need to move the computation of
> the new properties into SetFont, since SetGenericFont uses SetFont to do
> hypothetical computation given a different generic font (for monospace
> vs. non-monospace) and ComputeScriptLevelSize depends on having correct
> values of those properties for both aFont and aParent.  And then you
> need to make the three places in SetGenericFont that copy font structs
> copy the new members (maybe you want to add an operator= to nsStyleFont
> for this).  Otherwise script sizes will break pretty badly if the font
> switches between generics (or between monospace and non-monospace?).
Hmm, yeah.
Created attachment 293623 [details] [diff] [review]
KTM v2

OK, I fixed that and added a reftest for it. The reftest does fail (and asserts too) before the latest changes, now it passes without asserting.
Attachment #292909 - Attachment is obsolete: true
Attachment #293623 - Flags: superreview?(dbaron)
Attachment #293623 - Flags: review?(dbaron)
Attachment #292909 - Flags: superreview?(dbaron)
Attachment #292909 - Flags: review?(dbaron)
Would be nice to see more tests for this at some point -- there's enough
material here for hundreds of tests.  But that's probably too much to
ask right now.

In ListCSSProperties.cpp, comment that -moz-script-level is accepted by
the parser for chrome, but not content.

Could you make nsMathMLElement::ParseNumericValue not have a default
parameter, and file a bug on also adding the length units parameter to
nsMathMLFrame::ParseNumericValue (and thus auditing all its callers)?
(Note that a bunch of callers check cssValue.IsLengthUnit(), which means
they incorrectly ignore unitless 0 both with and without your patch.)

The aErrorCode parameter to nsString::ToInteger and ToFloat is a
PRInt32, not a PRBool.  (But it holds nsresult values, either NS_OK or
NS_ERROR_ILLEGAL_VALUE.

You might actually want to leave mFlags at the start of nsStyleFont, and
put mScriptLevel there as well, since the standard basically requires
nsStyleStruct to have nonzero size (1 byte), so we can fit them in the
remaining three.  (Though I just filed bug 408933.)

Does nsStyleFont::CalcDifference actually need to return a reflow hint
for the changes in the new properties, or is that already handled by
the checks for the changes in the font's size?


I still need to look at the nsRuleNode changes (sound familiar?).
(In reply to comment #118)
> In ListCSSProperties.cpp, comment that -moz-script-level is accepted by
> the parser for chrome, but not content.

OK (it's UA sheets, not chrome sheets, though).

> Could you make nsMathMLElement::ParseNumericValue not have a default
> parameter, and file a bug on also adding the length units parameter to
> nsMathMLFrame::ParseNumericValue (and thus auditing all its callers)?
> (Note that a bunch of callers check cssValue.IsLengthUnit(), which means
> they incorrectly ignore unitless 0 both with and without your patch.)

Sure.

> The aErrorCode parameter to nsString::ToInteger and ToFloat is a
> PRInt32, not a PRBool.  (But it holds nsresult values, either NS_OK or
> NS_ERROR_ILLEGAL_VALUE.

Ugh, OK.

> You might actually want to leave mFlags at the start of nsStyleFont, and
> put mScriptLevel there as well, since the standard basically requires
> nsStyleStruct to have nonzero size (1 byte), so we can fit them in the
> remaining three.  (Though I just filed bug 408933.)

Since dwitte has a patch for that, I hope it's OK to just leave this as-is.

> Does nsStyleFont::CalcDifference actually need to return a reflow hint
> for the changes in the new properties, or is that already handled by
> the checks for the changes in the font's size?

I guess you're right, it doesn't, I'll take that out.

> I still need to look at the nsRuleNode changes (sound familiar?).

:-) We're nearly done here I hope! I'll post a new patch when you've revisited those changes.
Comment on attachment 293623 [details] [diff] [review]
KTM v2

Now for the nsRuleNode changes (all that's left):


Could you add an inline version of CalcLength that doesn't have the
second and third parameters, and just calls the other version with -1
and nsull?  And maybe call the other (complex) version something more
complex like CalcLengthWith ?  The simple version should assert that
aStyleContext is non-null; the full version should assert that
aPresContext is non-null and that aStyleContext or aStyleFont is
non-null.

It seems like nsStyleFont::mScriptMinSize could just be an nscoord; it
need not hold the eStyleUnit_Normal value; you can compute that when
computing the value.

In handling font-size: inherit, you might want to comment that you do
handle script-level change in that case because inherit is the default,
and you don't want the explicit 'inherit' to differ from the default.

+    // sriptminsize-unconstrained size.

typo (needs a "c").

+    // Fast path: we have not been affected by scriptminsize so we don't
+    // need to call SetFontSize again to compute the
+    // sriptminsize-unconstrained size.
+    aFont->mScriptUnconstrainedSize = aFont->mSize;

You should comment that this is ok even if you used a start struct
because the fact that you used a start struct implies that 'font-size'
was set here, which means the scriptminsize has no effect.

+    if (aFont->mScriptSizeMultiplier < 0.0f)
+      aFont->mScriptSizeMultiplier = 0.0f;

Things like this are normally enforced at parse time rather than compute
time.

+  } else {
+    // Set mScriptUnconstrainedSize to its default value
+    aFont->mScriptUnconstrainedSize = scriptLevelAdjustedUnconstrainedParentSize;
+  }

Why do you need to set this before calling SetFontSize?  (It looks like
you set it again after.)

r+sr=dbaron with those addressed/answered
Attachment #293623 - Flags: superreview?(dbaron)
Attachment #293623 - Flags: superreview+
Attachment #293623 - Flags: review?(dbaron)
Attachment #293623 - Flags: review+
(In reply to comment #120)
> Could you add an inline version of CalcLength that doesn't have the
> second and third parameters, and just calls the other version with -1
> and nsull?  And maybe call the other (complex) version something more
> complex like CalcLengthWith ?  The simple version should assert that
> aStyleContext is non-null; the full version should assert that
> aPresContext is non-null and that aStyleContext or aStyleFont is
> non-null.

Sure

> It seems like nsStyleFont::mScriptMinSize could just be an nscoord; it
> need not hold the eStyleUnit_Normal value; you can compute that when
> computing the value.

Right.

> In handling font-size: inherit, you might want to comment that you do
> handle script-level change in that case because inherit is the default,
> and you don't want the explicit 'inherit' to differ from the default.

OK.

> +    // Fast path: we have not been affected by scriptminsize so we don't
> +    // need to call SetFontSize again to compute the
> +    // sriptminsize-unconstrained size.
> +    aFont->mScriptUnconstrainedSize = aFont->mSize;
> 
> You should comment that this is ok even if you used a start struct
> because the fact that you used a start struct implies that 'font-size'
> was set here, which means the scriptminsize has no effect.

Right.

> +    if (aFont->mScriptSizeMultiplier < 0.0f)
> +      aFont->mScriptSizeMultiplier = 0.0f;
> 
> Things like this are normally enforced at parse time rather than compute
> time.

OK I'll change that to parse time.

> +  } else {
> +    // Set mScriptUnconstrainedSize to its default value
> +    aFont->mScriptUnconstrainedSize =
> scriptLevelAdjustedUnconstrainedParentSize;
> +  }
> 
> Why do you need to set this before calling SetFontSize?  (It looks like
> you set it again after.)

You're right, we don't.
(In reply to comment #119)
> > Could you make nsMathMLElement::ParseNumericValue not have a default
> > parameter, and file a bug on also adding the length units parameter to
> > nsMathMLFrame::ParseNumericValue (and thus auditing all its callers)?
> > (Note that a bunch of callers check cssValue.IsLengthUnit(), which means
> > they incorrectly ignore unitless 0 both with and without your patch.)
> 
> Sure.

Filed bug 411227 on that.
Created attachment 295885 [details] [diff] [review]
KTM v3

This patch contains all those changes and is updated to trunk, ready for checkin. Woohoo!
Attachment #293623 - Attachment is obsolete: true
Attachment #295885 - Flags: superreview+
Attachment #295885 - Flags: review+
Created attachment 295888 [details] [diff] [review]
KTM v3.01

Sorry, this is the real one.
Attachment #295885 - Attachment is obsolete: true
Attachment #295888 - Flags: superreview+
Attachment #295888 - Flags: review+

Updated

10 years ago
Whiteboard: [sg:critical?][dbaron-1.9:RsCe][needs review] → [sg:critical?][dbaron-1.9:RsCe]

Updated

10 years ago
URL: arch
Keywords: arch, crash
checked in. Need to watch for perf regressions...
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
I backed this out due to test failure. There's a crash in mochitests like this:

#0  0x15820ec5 in nsIPresShell::FrameConstructor (this=0x0) at ../../dist/include/layout/nsIPresShell.h:204
#1  0x1514a664 in nsPresContext::RebuildAllStyleData (this=0x41b14190) at /Users/roc/mozilla-mathml/layout/base/nsPresContext.cpp:1387
#2  0x1514da7b in nsPresContext::UpdateAfterPreferencesChanged (this=0x41b14190) at /Users/roc/mozilla-mathml/layout/base/nsPresContext.cpp:767
#3  0x1514dae3 in nsPresContext::PrefChangedUpdateTimerCallback (aTimer=0x41f8d7f0, aClosure=0x41b14190) at /Users/roc/mozilla-mathml/layout/base/nsPresContext.cpp:129
#4  0x01370cd2 in nsTimerImpl::Fire (this=0x41f8d7f0) at /Users/roc/mozilla-mathml/xpcom/threads/nsTimerImpl.cpp:400

Presumably the timer is firing after the presentation has been torn down. A simple null check should suffice.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The null check seems to fix mochitests locally, so I relanded with that.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Looks like this one will stick. No problems in perf or unit tests on Linux or Mac. Windows builds going green after some bustage.
Flags: in-testsuite? → in-testsuite+

Updated

10 years ago
Depends on: 411603

Updated

10 years ago
Depends on: 412237

Updated

10 years ago
Blocks: 393760

Updated

10 years ago
Depends on: 413274

Updated

10 years ago
Depends on: 414123
The original 10K-ish patches made me hopeful, is there anything less than the 300K mega patch that will band-aide the problem on the 1.8 branch?
Not as far as I know.

Updated

10 years ago
Depends on: 418007

Updated

10 years ago
Depends on: 431705
Depends on: 427990
Did anyone ever file a bug on that testcase freezing up trunk?
Filed bug 538518.
Please unhide the bug, it was fixed 4 years ago.
Group: core-security
Blocks: 1368607
You need to log in before you can comment on or make changes to this bug.