1354393 - Drop flush from SMILAnimationController::DoSample

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Description

[Brian Birtles (:birtles)]

nsSMILAnimationController::DoSample includes a call to FlushPendingNotifications. This was introduced in bug 592477 because the flush in GetBaseValue was sometimes causing us to crash (see bug 592477 comment 25).

Now that bug 1315874 has removed the flush from GetBaseValue we should, in theory, be able to remove the call to FlushPendingNotifications. This call has been a source of security and performance bugs over the years so it would be nice to remove it.

However, since adding that flush, some code and tests have come to depend on it. If we simply drop it (and with it, the |isStyleFlushNeeded| local variable and |document| kungFuDeathGrip, I suspect) various tests will fail. We need to look through these tests and investigate the cause.

For example, in some cases it might be that the animVal getter for mapped attributes should be doing a flush. In other cases it might be that the test itself is at fault. There might be other cases where we really do need the flush in DoSample, I'm not sure. Nevertheless, it's worth looking into since it might help with performance, it might help avoid future security bugs, and it will probably be necessary to do if we later want to rebase SMIL on top of Web Animations.

[1] http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/smil/nsSMILAnimationController.cpp#443

Comment 1

[Robert Longson [:longsonr]]

Only currently 3 failures, two of which are the same type.

https://treeherder.mozilla.org/jobs?repo=try&revision=a16a5fa481b32c2badf76a81c7e13602da65b32e

/svg/animations/reinserting-svg-into-document.html
layout/reftests/svg/smil/container/deferred-tree-3c.xhtml

layout/reftests/svg/smil/anim-targethref-10.svg

Comment 2

[Brian Birtles (:birtles)]

Yeah, all those seem to be related to updating the animation's target mid-animation so perhaps we could add a more targeted flush somewhere to cover just that scenario.

Comment 3

[Robert Longson [:longsonr]]

Any flush looks likely to stop us from fixing bug 1858792, short of ignoring the reentrancy altogether and just returning from the inner reclone attempt.

Comment 4

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=a02a455e5f86cd82d9f92778a53b3e847546484f

Fixed two of the three issues. Only layout/reftests/svg/smil/anim-targethref-10.svg left and that's intermittent. Since there's no scripting in that one it's a bit harder to see where we'd put a flush.

Comment 5

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=89d3de703e0a5af127772af3e38342a177ef557c&selectedTaskRun=FRN28T19QW6YOZyAqrzQKw.0

Comment 6

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=f9d5e561741a4461d68d19ce84485c34ff11e97e

Comment 7

[Robert Longson [:longsonr]]

Attachment: Bug 1354393 - remove flush in SMILAnimationController::DoSample r=emilio,birtles

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8784930453c1
remove flush in SMILAnimationController::DoSample r=emilio

Comment 9

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/8784930453c1