1361951 - Crash in ComputeAnimationValue

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-5fb40a4b-b51d-470d-a020-28dc60170504.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170503030212: http://bit.ly/2pK2jvG

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48c0fd9c9ec5d68061ea7b59358874ae8da72572&tochange=82c2d17e74ef9cdf38a5d5ac4eb3ae846ec30ba4

Looks like several bugs landed relation to animation. ni on Brian to see if he has any insight.

Comment 1

[Boris Chiou [:boris]]

I already moved ComputeAnimationValue into StyleAnimationValue and added an "if" check before using |doc|, so I guess this should be fixed in Bug 1346052.

Comment 2

[Brian Birtles (:birtles)]

Hi Boris, did we end up adding a null check for |doc|?

I know that's what we discussed in bug 1346052 comment 19, but looking at nsDOMWindowUtils::ComputeAnimationDistance on trunk (or at least searchfox's cached version), I see just:

  nsIPresShell* shell = element->GetComposedDoc()->GetShell();

Don't we need to check the result of GetComposedDoc() here?

Comment 3

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away May 3~4) from comment #2)
> Hi Boris, did we end up adding a null check for |doc|?
> 
> I know that's what we discussed in bug 1346052 comment 19, but looking at
> nsDOMWindowUtils::ComputeAnimationDistance on trunk (or at least searchfox's
> cached version), I see just:
> 
>   nsIPresShell* shell = element->GetComposedDoc()->GetShell();
> 
> Don't we need to check the result of GetComposedDoc() here?

In nsDOMWindowUtils::ComputeAnimationDistance(), we do the following things:

  v1 = AnimationValue::FromString(start);
  v2 = AnimationValue::FromString(end);

  if (v1.IsNull() || v2.IsNull()) {
    return NS_ERROR_ILLEGAL_VALUE;
  }

  nsIPresShell* shell = element->GetComposedDoc()->GetShell();
  ...

In AnimationValue::FromString(), we check element->GetComposedDoc() first (i.e.
  nsCOMPtr<nsIDocument> doc = aElement->GetComposedDoc();
  if (!doc) {
    return result;
  }
), if it is null, we return a null |AnimationValue|, and if the result of AnimationValue (i.e. |v1| or |v2|) is null, we return NS_ERROR_ILLEGAL_VALUE. Therefore, I think we don't need to check |element->GetComposedDoc()| again.

Comment 4

[Brian Birtles (:birtles)]

I wondered about that. That seems really brittle, don't you think?

We should, at very least, assert that GetComposedDoc is non-null and document that dependency. Better still, though, we should just null check the result of GetComposedDoc before dereferencing it.

Comment 5

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #4)
> I wondered about that. That seems really brittle, don't you think?
> 
> We should, at very least, assert that GetComposedDoc is non-null and
> document that dependency. Better still, though, we should just null check
> the result of GetComposedDoc before dereferencing it.

OK. We can add a null check in this bug. I will upload a patch for it.

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1361951 - Add null check for nsIDocument in ComputeAnimationDistance.

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

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8864727 [details]
Bug 1361951 - Add null check for nsIDocument in ComputeAnimationDistance.

https://reviewboard.mozilla.org/r/136380/#review139468

Comment 8

[Brian Birtles (:birtles)]

Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ec331cdc006
Add null check for nsIDocument in ComputeAnimationDistance. r=birtles

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ec331cdc006