1328535 - Assertion failure: aDilutionRatio >= 0.0 && aDilutionRatio <= 1.0 (Dilution ratio should be in [0, 1]), at StyleAnimationValue.cpp:1980

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack from aurora build

Found via bughunter and reproduced on latest windows 7 debug tinderbox trunk build.

Steps to reproduce

-> Load http://divinityhtml.oxygenna.com/genesis/index.html
--> Assertion failure on load

affects trunk to beta according to bughunter data

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

affects debug builds only, a opt build didn't crashed

Comment 2

[Daniel Holbert [:dholbert]]

searchfox link:
http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/layout/style/StyleAnimationValue.cpp#1979-1980

This is an assertion added in bug 1294614. hiro, mind taking a look?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Sure.  An amimation-timing-function for box-shadow animation, that is acually cubic-bezier(0.74, 0.01, 0.35, 1.01), produces values out of range [0, 1].  We don't need to assert there.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1328535 - Remove range-checking assertion for aDilutionRatio, in color animation code.

Some cubic-bezier functions, e.g. cubic-bezier(0,-0.5,0,0), produce values out
of range of [0, 1].

Review commit: https://reviewboard.mozilla.org/r/102336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102336/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8823855 [details]
Bug 1328535 - Remove range-checking assertion for aDilutionRatio, in color animation code.

https://reviewboard.mozilla.org/r/102336/#review102732

r=me with nits addressed.

Two commit message nits:
> Bug 1328535 - Don't assert even if |aDillutionRatio| is out of range of [0, 1]. r?dholbert

1) Typo: you've got an extra "l" in aDillutionRatio there.
2) This doesn't have enough context to suggest what part of the code it's touching. You should mention animation and/or colors, to give a bit more context.

Suggested replacement message:
Bug 1328535 - Remove range-checking assertion for aDilutionRatio, in color animation code. r?dholbert

::: layout/style/test/test_transitions_per_property.html:1672
(Diff revision 1)
>         "shadow-valued property " + prop + " (radius): clamping of negatives");
>      isnot(vals[6], "0px",
>            "shadow-valued property " + prop + " (spread): clamping of negatives");
>    }
> +
> +  // A test case that timing functin produces values greater than 1.0.

typo: s/functin/function/

::: layout/style/test/test_transitions_per_property.html:1688
(Diff revision 1)
> +  // converted to 0.65.
> +  is(cs.getPropertyValue(prop), "rgba(100, 100, 100, 0.65) 0px 0px 0px" + spreadStr,
> +     "shadow-valued property " + prop + ": interpolation of shadows with " +
> +     "timing function which produces values greater than 1.0");
> +
>    div.style.setProperty("transition-timing-function", "linear", "");

Here I think you're intending to restore the original value (which we set elsewhere in this file) -- right?

This seems a little fragile. It'd probably be better to capture the value that you're stomping on (before you stomp on it), and then restore it at the end. Something like this:
====
  var origTimingFunc = div.style.getPropertyValue("transition-timing-function");

 [... your test code here ...]

 // Clean up:
 div.style.setProperty("transition-timing-function", origTimingFunc);
====

How does that sound?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8823855 [details]
Bug 1328535 - Remove range-checking assertion for aDilutionRatio, in color animation code.

https://reviewboard.mozilla.org/r/102336/#review102732

> Here I think you're intending to restore the original value (which we set elsewhere in this file) -- right?
> 
> This seems a little fragile. It'd probably be better to capture the value that you're stomping on (before you stomp on it), and then restore it at the end. Something like this:
> ====
>   var origTimingFunc = div.style.getPropertyValue("transition-timing-function");
> 
>  [... your test code here ...]
> 
>  // Clean up:
>  div.style.setProperty("transition-timing-function", origTimingFunc);
> ====
> 
> How does that sound?

Indeed, it quite makes sense. Thanks!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8823855 [details]
Bug 1328535 - Remove range-checking assertion for aDilutionRatio, in color animation code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102336/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4be17b09b8d
Remove range-checking assertion for aDilutionRatio, in color animation code. r=dholbert

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e4be17b09b8d

Comment 10

[Gerry Chang [:gchang]]

Too late for 51. Mark 51 won't fix.

Comment 11

[Daniel Holbert [:dholbert]]

We might as well wontfix for 52, too. The patch here just removes a bogus assertion -- the only impact is on debug builds. So there's no compelling reason to uplift.

 --> updating 52 status to make that official.