1239889 - KeyframeEffectReadOnly constructor crashes when the target element does not have the parent?

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Below codes leads the crash.

    var div = document.createElement('div');
    var effect = new KeyframeEffectReadOnly(div, { opacity: [0, 1] });

 0:04.34 #01: BuildAnimationPropertyListFromPropertyIndexedKeyframes (/home/ikezoe/mozilla-central/dom/animation/KeyframeEffect.cpp:1566)
 0:04.34 #02: mozilla::dom::KeyframeEffectReadOnly::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Element*, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions const&, mozilla::ErrorResult&) (/home/ikezoe/mozilla-central/dom/animation/KeyframeEffect.cpp:1732)
 0:04.40 #03: downcast<mozilla::dom::KeyframeEffectReadOnly> (/home/ikezoe/mozilla-central/obj-firefox/dist/include/mozilla/AlreadyAddRefed.h:138)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Automation test for the crash

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I did not paste the most important line.

 0:02.12 Assertion failure: aTargetElement->GetCurrentDoc() (we should only be able to actively animate nodes that are in a document), at /home/ikezoe/mozilla-central/layout/style/StyleAnimationValue.cpp:2596

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This is likely the cause for bug 1245735 so I think we need to fix this soon. I spoke to Cameron about what is going on and I think the basic problem is that we are computing values in the KeyframeEffectReadOnly constructor. We really should be storing specified values and only computing them when needed since the computed values will change if mTarget is changed or bound to a different part of the tree.

It's probably too expensive to re-compute those every on every frame so we'd need to recompute them every time mTarget changed or BindToTree was called on it. That's a fair bit of work, however.

So, what we can do as an interim step is simply throw if the target element does not have a current doc like we do if mTarget is null.

We *could* just use the style context from the owner doc but that's going to be wrong in some cases.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Filed bug 1245748 for doing the work described in comment 3.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Throw if the animation target does not have a current document

I was thinking of using attachment 8708153 [details] [diff] [review] as the test case and writing a more thoroughgoing test when we fix bug 1245748

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8708153 [details] [diff] [review]
Automation test for the crash

Hi Hiro, I hope you don't mind if I land this test along with the fix.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Sure!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb3381dc5f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b16abc85697

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bb3381dc5f8
https://hg.mozilla.org/mozilla-central/rev/6b16abc85697