Closed Bug 1361951 Opened 3 years ago Closed 3 years ago

Crash in ComputeAnimationValue

Categories

(Core :: DOM: Animation, defect, critical)

55 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: marcia, Assigned: boris)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
I already moved ComputeAnimationValue into StyleAnimationValue and added an "if" check before using |doc|, so I guess this should be fixed in Bug 1346052.
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?
Flags: needinfo?(boris.chiou)
(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.
Flags: needinfo?(boris.chiou)
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.
(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 on attachment 8864727 [details]
Bug 1361951 - Add null check for nsIDocument in ComputeAnimationDistance.

https://reviewboard.mozilla.org/r/136380/#review139468
Attachment #8864727 - Flags: review?(bbirtles) → review+
Thanks!
Assignee: nobody → boris.chiou
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ec331cdc006
Add null check for nsIDocument in ComputeAnimationDistance. r=birtles
https://hg.mozilla.org/mozilla-central/rev/9ec331cdc006
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.