509569 - "ASSERTION: canStoreInRuleTree must be false for reset structs if any properties were specified as inherit" with -moz-border-start

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: canStoreInRuleTree must be false for reset structs if any properties were specified as inherit: '!canStoreInRuleTree || aRuleDetail == eRuleNone || aRuleDetail == eRulePartialReset || aRuleDetail == eRuleFullReset', file /Users/jruderman/central/layout/style/nsRuleNode.cpp, line 4589

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the issue here is that the |border| shorthand doesn't reset the value for eCSSProperty_border_start_style_value (which therefore remains inherit, even though it's not used because the sources are both set to physical).  So we end up in ComputeBorderData with aRuleDetail == eRulePartialMixed, but never set aCanStoreInRuleTree to false (which I think is correct in this case; the inherit values are never going to be used, since you can't set the *_source values to "logical" without also resetting the style_value.

Obvious fixes:

1)  Loosen up the assert for the mixed cases (possibly only for border/padding
    /margin structs).
2)  Make parsing reset more stuff somehow.
3)  Make AdjustLogicalBoxProp set aCanStoreInRuleTree to false if any of the
    value cssvalues are inherit, not just the ones it ends up using).

Need to think a bit more about whether there's a correctness issue here, or just assert-avoidance.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 2)  Make parsing reset more stuff somehow.

This is not an option; for example this style:

  "-moz-border-start: inherit; border-left: none; border-right: none"

would trigger the assert.  Further, it would trigger the assert even if the declarations were scattered over multiple rules.  There's no way the parser can deal with this.

Now in terms of correctness...  The assert only fires if both the ltr source and rtl source for a box prop are "physical" and the relevant values are not parent-dependent while one of the logical value values is inherit.  If we cache such a struct in the ruletree, I think we're ok, since it in fact doesn't contain any information that depends on the parent.

We could make the rule detail correct by using Check*Callback functions and passing the inherited/total/etc numbers from nsRuleNode::CheckSpecifiedProperties to the callbacks and then having the callback detect the case when both sources are physical and adjust the inherited number as needed.  But in practice, I'd think this situation is rare; it's simpler to do item 3 from above.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and since there's no correctness issue, I don't think we need to worry about this for 1.9.2.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

This is the simple thing to do, but note comment 2.  This is just "fixing" the assert by disallowing an otherwise valid optimization....

Comment 5

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

Comment on attachment 413128 [details] [diff] [review]
Possible fix

I have mixed feelings about whether we should do this at all, but I suppose it's a very rare case, so r=dbaron.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/57f4d921cd32