Nicholas Nethercote [inactive] Assignee
	Description • 8 years ago

Coverity identified that 1 of the 5 calls to StyleAnimationValue::ComputeDistance() doesn't check the return value. In this case this appears to be legitimate (there's a comment explaining it) but it's a good opportunity to add MOZ_MUST_USE to all the bool return values in that class that indicates success/failure.

	Nicholas Nethercote [inactive] Assignee
	Comment 1 • 8 years ago

birtles, please check the followin in particular:

- the "njn" comment on one case where I wasn't sure what to do;

- the fact that I didn't add MOZ_MUST_USE to UncomputeValue().

	Brian Birtles (:birtles)
	Comment 2 • 8 years ago

Comment on attachment 8785749 [details] [diff] [review]
Use MOZ_MUST_USE in StyleAnimationValue

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

Thanks for doing this. r=me with the following changes.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +563,5 @@
>                 "Must have same unit");
> +  // njn: what to do here?
> +  Unused <<
> +    StyleAnimationValue::Interpolate(aAnimation.property(), aStart, aEnd,
> +                                     aPortion, interpolatedValue);

I think we're not currently expecting this to fail since we only pass transform and opacity values to the compositor and they should never fail to interpolate.

Perhaps an assertion would be appropriate here so that if this ever does happen we know we need to introduce the 50% flip logic we use on the main thread when interpolation fails.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/dom/animation/KeyframeEffect.cpp#406

::: layout/style/StyleAnimationValue.h
@@ +54,5 @@
>     * @return true on success, false on failure.
>     */
> +  static MOZ_MUST_USE bool
> +  Add(nsCSSPropertyID aProperty, StyleAnimationValue& aDest,
> +      const StyleAnimationValue& aValueToAdd, uint32_t aCount) {

Unfortunately, I think our coding style would have us put the return type on the same line as the method name in the method declaration.[1] (Which seems unfortunate because its inconsistent with what we do elsewhere. If you think we can ignore this, I'd be glad to hear it!)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

@@ +219,5 @@
>     * @param [out] aSpecifiedValue The resulting specified value.
>     * @return true on success, false on failure.
> +   *
> +   * These functions are not MOZ_MUST_USE because failing to check the return
> +   * value is common and reasonable.

I think we should actually make UncomputeValue MOZ_MUST_USE.

As far as I can tell, the only two cases where this will fail are:

a. If we pass a StyleAnimationValue with Unit null
b. If we pass a StyleAnimationValue with Unit UnparsedString to one
   of the overloads that does NOT serialize to a string (i.e. serializes
   to an nsCSSValue).

Also, it seems like the only places we *don't* check the result are:

* The debug-only AnimValuesStyleRule::List method

  This could probably reasonably ignore the result (particularly since we are
  using the overload that serializes to a string so it's only the null unit case
  where this could fail)

* The debug-only DumpAnimationProperties function

  Likewise this could also reasonably ignore the result (it too serializes
  to a string)

* CreatePropertyValue in KeyframeEffect.cpp

  In this case I think we don't expect this to fail since we shouldn't have
  a null value at this point (and we are serializing to a string so we shouldn't
  fail in the case when the unit is unparsed string).

  If we do have a failure it's likely a bug. e.g.

  - we added a new type to StyleAnimationValue but forgot to make UncomputeValue
    handle it
  - we added some logic to UncomputeValue that now means it can fail for valid
    units -- if so we should work out how to handle that at call sites
  - we changed the logic of CreatePropertyValue or one of its call sites so we
    can now get null values -- that's probably a bug in itself, or at very least
    we should detect and handle null values

  So I think we should just assert that that the result is true here.

* CSSAnimationBuilder::GetComputedValue

  In this case we're not expecting it to fail since we pass the result of
  ExtractComputedValue to UncomputeValue. I think we should just assert that
  the result is true here, since, like the previous case, if this fails its
  probably a bug.

	Nicholas Nethercote [inactive] Assignee
	Comment 3 • 8 years ago

> > +  // njn: what to do here?
> > +  Unused <<
> > +    StyleAnimationValue::Interpolate(aAnimation.property(), aStart, aEnd,
> > +                                     aPortion, interpolatedValue);
> 
> I think we're not currently expecting this to fail since we only pass
> transform and opacity values to the compositor and they should never fail to
> interpolate.
> 
> Perhaps an assertion would be appropriate here so that if this ever does
> happen we know we need to introduce the 50% flip logic we use on the main
> thread when interpolation fails.[1]

Done.


> > +  static MOZ_MUST_USE bool
> > +  Add(nsCSSPropertyID aProperty, StyleAnimationValue& aDest,
> > +      const StyleAnimationValue& aValueToAdd, uint32_t aCount) {
> 
> Unfortunately, I think our coding style would have us put the return type on
> the same line as the method name in the method declaration.[1] (Which seems
> unfortunate because its inconsistent with what we do elsewhere. If you think
> we can ignore this, I'd be glad to hear it!)

I think we can ignore this. Consider this one:

> static MOZ_MUST_USE bool ExtractComputedValue(nsCSSPropertyID aProperty,
>                                               nsStyleContext* aStyleContext,
>                                               StyleAnimationValue& aComputedValue);

That exceeds 80 lines. So we have to break it somehow, and I think this:

> static MOZ_MUST_USE bool 
> ExtractComputedValue(nsCSSPropertyID aProperty,
>                      nsStyleContext* aStyleContext,
>                      StyleAnimationValue& aComputedValue);

is better than this:

> static MOZ_MUST_USE bool ExtractComputedValue(
>   nsCSSPropertyID aProperty, 
>   nsStyleContext* aStyleContext,
>   StyleAnimationValue& aComputedValue);

I've used this breaking style in other places before and people haven't complained.


> I think we should actually make UncomputeValue MOZ_MUST_USE.

Ok, will do.

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 8 years ago

Updated version, with comments addressed. I will carry over the r+.

	Nicholas Nethercote [inactive] Assignee
	Comment 5 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcfff96e4ffd245631e59cacb187410ef20362c
Bug 1298722 - Use MOZ_MUST_USE in StyleAnimationValue. r=birtles.

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 8 years ago
bugherder

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

	Jonathan Chan [:jyc]
	Comment 7 • 8 years ago

Hello, I was working with related code recently and I noticed that although the comment saying "These functions are not MOZ_MUST_USE" is there, the functions are in fact marked MOZ_MUST_USE. Is this intentional?

For comment see: https://dxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.h?q=StyleAnimationValue&redirect_type=direct#267

	Daniel Holbert [:dholbert]
	Comment 8 • 7 years ago

I just ran into the discrepancy that jyc brought up in comment 7, too (while reviewing a patch that calls this code).

Reading back through this bug, it looks like the comment-vs-code discrepancy exists because:
 - njn *initially* didn't intend to to label these functions (and he admirably included some documentation to explain why they were unlabeled)
 - But, then that changed in comment 2 & comment 3 ("I think we should actually make UncomputeValue MOZ_MUST_USE", "Ok, will do."
 - ...and the original (obsoleted) explanatory code-comment was left in the patch by mistake.

I'll push a trivial followup to clean this up (deleting the comment.)

	Pulsebot
	Comment 9 • 7 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/562b1c41685e
followup: remove obsolete documentation about MOZ_MUST_USE annotations on StyleAnimationValue::UncomputeValue(). (no review, comment-only, DONTBUILD)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/562b1c41685e