1186768 - "Assertion failure: !(mBits & eHaveFontSize) || mFontSize == aCoord" with HTML in MathML

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: !(mBits & eHaveFontSize) || mFontSize == aCoord, at RuleNodeCacheConditions.h:50

This assertion is part of code added in https://hg.mozilla.org/mozilla-central/rev/b7a0c7012a37

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Cameron McCormack (:heycam)]

I can't reproduce this on current m-c (on Linux x64).  Can you still?

Comment 3

[Jesse Ruderman]

Yes, I can still reproduce on Mac.

[TreeHerder, debug, built from https://hg.mozilla.org/mozilla-central/rev/1d4f44ee5166]

Comment 4

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

I see the assertion on Linux.

Comment 5

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

(The call with the assertion has aCoord==511, but mFontSize==640.)

Comment 6

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

The best part, though, is that this is going through a dummy conditions object that isn't even used (created where SetGenericFont calls SetFont).

Comment 7

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

The basic problem is that we make both of these SetFontSize calls:

  SetFontSize(aPresContext, aRuleData, aFont, aParentFont,
              &aFont->mSize,
              systemFont, aParentFont->mSize, scriptLevelAdjustedParentSize,
              aUsedStartStruct, atRoot, aConditions);
  if (aParentFont->mSize == aParentFont->mScriptUnconstrainedSize &&
      scriptLevelAdjustedParentSize == scriptLevelAdjustedUnconstrainedParentSize) {
    // Fast path: we have not been affected by scriptminsize so we don't
    // need to call SetFontSize again to compute the
    // scriptminsize-unconstrained size. This is OK even if we have a
    // start struct, because if we have a start struct then 'font-size'
    // was specified and so scriptminsize has no effect.
    aFont->mScriptUnconstrainedSize = aFont->mSize;
  } else {
    SetFontSize(aPresContext, aRuleData, aFont, aParentFont,
                &aFont->mScriptUnconstrainedSize,
                systemFont, aParentFont->mScriptUnconstrainedSize,
                scriptLevelAdjustedUnconstrainedParentSize,
                aUsedStartStruct, atRoot, aConditions);
  }
  NS_ASSERTION(aFont->mScriptUnconstrainedSize <= aFont->mSize,
               "scriptminsize should never be making things bigger");

the first SetFontSize call has aParentSize=640, aScriptLevelAdjustedParentSize=640; the second one has aParentSize=511, aScriptLevelAdjustedParentSize=511.

I think we probably need to use a nested conditions object for the second SetFontSize call (although we might not need to unconditionally SetUncacheable on the outer one unless the inner one comes back with conditions???).

Comment 8

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

Attachment: patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments

Comment 9

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

Attachment: patch 2 - Crashtest

David Baron confirmed locally that this fails with a fatal assertion
without patch 1 and passes with patch 1.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8667571 [details] [diff] [review]
patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments

Review of attachment 8667571 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +3790,5 @@
>                  scriptLevelAdjustedUnconstrainedParentSize,
> +                aUsedStartStruct, atRoot, unconstrainedConditions);
> +    if (!unconstrainedConditions.CacheableWithoutDependencies()) {
> +      aConditions.SetUncacheable();
> +    }

If we want a single font size value on RuleNodeCacheConditions, and for this second SetFontSize call to share this dependent value if it's the same, then I think we should be checking that mScriptUnconstrainedSize in RuleNodeCacheConditions::Matches is equal to the cached mFontSize value, too.

Though really shouldn't we also be ensuring that the script level adjusted sizes are the same too, since they are inputs into the font-size calculation?  Which would mean we need to store/check other things like mScriptLevel and mScriptSizeMultiplier of the parent nsStyleFont?

I imagine we store nsStyleFonts in the rule tree too often (due to font shorthand rules covering all properties in the struct) to just always decline to cache them.  (Not being a reset struct we'd only store it without conditions anyway.)

Comment 11

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

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #10)
> If we want a single font size value on RuleNodeCacheConditions, and for this
> second SetFontSize call to share this dependent value if it's the same, then
> I think we should be checking that mScriptUnconstrainedSize in
> RuleNodeCacheConditions::Matches is equal to the cached mFontSize value, too.
> 
> Though really shouldn't we also be ensuring that the script level adjusted
> sizes are the same too, since they are inputs into the font-size
> calculation?  Which would mean we need to store/check other things like
> mScriptLevel and mScriptSizeMultiplier of the parent nsStyleFont?

This is already within the else of:
  if (aParentFont->mSize == aParentFont->mScriptUnconstrainedSize &&
      scriptLevelAdjustedParentSize == scriptLevelAdjustedUnconstrainedParentSize) {
so we know there's already a difference in font size at the point where we're calling SetUncacheable.

> I imagine we store nsStyleFonts in the rule tree too often (due to font
> shorthand rules covering all properties in the struct) to just always
> decline to cache them.  (Not being a reset struct we'd only store it without
> conditions anyway.)

I think this is a pretty obscure case -- it's only in the else of the above if, which should only happen for MathML.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #11)
> This is already within the else of:
>   if (aParentFont->mSize == aParentFont->mScriptUnconstrainedSize &&
>       scriptLevelAdjustedParentSize ==
> scriptLevelAdjustedUnconstrainedParentSize) {
> so we know there's already a difference in font size at the point where
> we're calling SetUncacheable.

I'm still not sure why this is relevant.  Why can we ignore the rule "any time you compute values based on values in another style context, you must either record those values on the RuleNodeCacheConditions or set it to uncacheable"?  If we cached an nsStyleFont on a rule node, and when computing that nsStyleFont we happened to fall into the "if" part of the above if statement, wouldn't it not be valid for re-use for another element (matcheing the same rule node) that has a different value for mScriptLevel/mScriptUnconstrainedSize/mScriptMinSize on its parent element?  What am I overlooking?

> > I imagine we store nsStyleFonts in the rule tree too often (due to font
> > shorthand rules covering all properties in the struct) to just always
> > decline to cache them.  (Not being a reset struct we'd only store it without
> > conditions anyway.)
> 
> I think this is a pretty obscure case -- it's only in the else of the above
> if, which should only happen for MathML.

I agree it's obscure, but isn't it needed for correctness?

Comment 13

[Cameron McCormack (:heycam)]

I recognise that this patch doesn't make things any worse than the code pre-bug 804975.  Is that all you're looking to fix here, and we'll handle any correctness issues dealing with inspecting parent mScriptXXX values in another bug (if those correctness issues do indeed exist)?

Comment 14

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

(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #12)
> (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #11)
> > This is already within the else of:
> >   if (aParentFont->mSize == aParentFont->mScriptUnconstrainedSize &&
> >       scriptLevelAdjustedParentSize ==
> > scriptLevelAdjustedUnconstrainedParentSize) {
> > so we know there's already a difference in font size at the point where
> > we're calling SetUncacheable.
> 
> I'm still not sure why this is relevant.  Why can we ignore the rule "any
> time you compute values based on values in another style context, you must
> either record those values on the RuleNodeCacheConditions or set it to
> uncacheable"?

Ah, true.  I missed that you were saying that in the previous comment.

Although I'm not sure that it's actually a problem -- since I think the if statement is actually an optimization -- when the if statement is true, both branches should produce the same result, so we can optimize by not redoing the calculation.

Comment 15

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #14)
> Although I'm not sure that it's actually a problem -- since I think the if
> statement is actually an optimization -- when the if statement is true, both
> branches should produce the same result, so we can optimize by not redoing
> the calculation.

Right.  I think that indicates that if we need to call SetUncacheable() due to inspecting the parent mScriptXXX values, then it needs to be done regardless of which branch we go into.

Comment 16

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

Yeah, I guess we do.  It would be quite unfortunate to have to add an additional dependency just for an obscure MathML feature, though.  And this does fix the fatal assertion we have here.

Comment 17

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

(I was thinking we didn't need it because the if/else isn't actually making a logical distinction based on the parent's data since it's just an optimization, and since the second SetFontSize won't produce different dependencies from the first -- but the problem is that the optimized-away SetFontSize in the if could still produce a font size dependency that could be cached and later applied to a case where we would have gone through the else.)

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8667571 [details] [diff] [review]
patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments

Review of attachment 8667571 [details] [diff] [review]:
-----------------------------------------------------------------

OK, how about we take this then, to resolve the assertions, if you add to the comment to point out how there are still inaccuracies in how we track font-size dependencies, and file a followup for it?

Comment 19

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

I was starting to file that followup bug, and then I realized that caching the font struct in the rule tree is extremely rare anyway, especially when mathML is enabled.  We can condition the whole block on mPresContext->Document()->GetMathMLEnabled(), and just set uncacheable in that condition.  (See the comments in nsRuleNode::CheckSpecifiedProperties, which I'll repeat, explaining why that condition is ok.)

Comment 20

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

Attachment: patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8683468 [details] [diff] [review]
patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments

Review of attachment 8683468 [details] [diff] [review]:
-----------------------------------------------------------------

(Oh, I didn't realise GetMathMLEnabled() returned true only once we'd encountered MathML elements.)

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dae1d7e445d3725191acd97a9ba7def427c342e5
Bug 1186768 patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4101bc25be52731213e53a3529c4be64c57299e
Bug 1186768 patch 2 - Crashtest.  r=heycam

Comment 23

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16843689&repo=mozilla-inbound

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad68ab5f09d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/20719258b21d

Comment 25

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

Attachment: patch 3 - Compute affectedByScriptMinSize sooner to avoid asserting or doing extra work

The assertion added in patch 1 caught a separate bug, which is that we
were computing whether we were affected by scriptMinSize too soon.
Prior to patch 1, this bug meant doing extra work (taking the slow
path).  With patch 1, this changes in documents without MathML to
hitting the assertion instead of taking the slow path.

Comment 26

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27da64b25d76&group_state=expanded

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e92748e429f5938b2488a6e09033b06d191eae1
Bug 1186768 patch 1 - Avoid setting different font-size conditions due to MathML font size adjustments.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf19e565e94468cf0708283665c3fbe19503bbf
Bug 1186768 patch 2 - Crashtest.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/af4f270f9799c4b7c62bc4bac0982ef4899b308e
Bug 1186768 patch 3 - Compute affectedByScriptMinSize sooner to avoid asserting or doing extra work.  r=heycam

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e92748e429f
https://hg.mozilla.org/mozilla-central/rev/3cf19e565e94
https://hg.mozilla.org/mozilla-central/rev/af4f270f9799