Lock animated property type of result animation during processing

Lock animated property type of result animation during processing

There seems to be a crash due to the animated property type being
cleared too early. To catch this crash at the point of error, lock the
animated property type of the result animation during processing, and
guard for clearing the animation property while the lock is held (in
clearAnimatedType()).

This can potentially be removed when the source of the bug is found.

BUG=581546
Committed: https://crrev.com/cd3dc7b24a97f9dda9e97e803449bde09fe0cb32
Cr-Commit-Position: refs/heads/master@{#394726}

[davve, 2016-05-18 15:53:27]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[davve, 2016-05-18 15:53:39]

The CQ bit was unchecked by davve@opera.com

[davve, 2016-05-18 15:55:26]

davve@opera.com changed reviewers:
+ fs@opera.com, sigbjornf@opera.se

[davve, 2016-05-18 15:55:26]

Something like this you had in mind, sof? Do you think it would catch the
scenario you painted in
https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 ?

[fs, 2016-05-18 16:10:39]

fs@opera.com changed reviewers:
+ sigbjornf@opera.com
- sigbjornf@opera.se

[fs, 2016-05-18 16:10:40]

(fixed sof's email =))

I'd be fine with something like this.

https://codereview.chromium.org/1991513003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h (right):

https://codereview.chromium.org/1991513003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h:113: virtual void
lockAnimatedType() = 0;
Any particular reason we want these to be virtual? I.e why not just put the flag
in SVGSMILElement and add a method to query it in SVGAnimateElement? (Would make
for a slightly smaller patch, but slightly larger elements granted.)

[davve, 2016-05-19 06:35:53]

On 2016/05/18 16:10:40, fs wrote:
> (fixed sof's email =))

doh! Thanks. Conscious effort to not send to sigbjorn@ and all...

[davve, 2016-05-19 06:38:49]

https://codereview.chromium.org/1991513003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h (right):

https://codereview.chromium.org/1991513003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h:113: virtual void
lockAnimatedType() = 0;
On 2016/05/18 16:10:40, fs wrote:
> Any particular reason we want these to be virtual? I.e why not just put the
flag
> in SVGSMILElement and add a method to query it in SVGAnimateElement? (Would
make
> for a slightly smaller patch, but slightly larger elements granted.)

It was an attempt at limiting intrusiveness and keeping the lock close to the
actual m_animatedProperty member. But the simplicity of your proposal convinces
me. Patch updated.

[sof, 2016-05-19 06:45:44]

lgtm to try (to perturb.)

https://codereview.chromium.org/1991513003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp (right):

https://codereview.chromium.org/1991513003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp:291: // later if we
clear the animated type while the lock is held.
mention crbug.com/581546 ?

[davve, 2016-05-19 06:54:27]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 06:54:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1991513003/40001

[fs, 2016-05-19 07:44:00]

lgtm

[davve, 2016-05-19 08:13:33]

The CQ bit was unchecked by davve@opera.com

[davve, 2016-05-19 08:13:33]

The CQ bit was checked by davve@opera.com

[davve, 2016-05-19 08:13:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1991513003/#ps40001
(title: "Update comment and mention bug.")

[commit-bot: I haz the power, 2016-05-19 08:13:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1991513003/40001

[commit-bot: I haz the power, 2016-05-19 10:24:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-19 10:26:56]

Description was changed from

==========
Lock animated property type of result animation during processing

There seems to be a crash due to the animated property type being
cleared too early. To catch this crash at the point of error, lock the
animated property type of the result animation during processing, and
guard for clearing the animation property while the lock is held (in
clearAnimatedType()).

This can potentially be removed when the source of the bug is found.

BUG=581546
==========

to

==========
Lock animated property type of result animation during processing

There seems to be a crash due to the animated property type being
cleared too early. To catch this crash at the point of error, lock the
animated property type of the result animation during processing, and
guard for clearing the animation property while the lock is held (in
clearAnimatedType()).

This can potentially be removed when the source of the bug is found.

BUG=581546

Committed: https://crrev.com/cd3dc7b24a97f9dda9e97e803449bde09fe0cb32
Cr-Commit-Position: refs/heads/master@{#394726}
==========

[commit-bot: I haz the power, 2016-05-19 10:26:57]

Patchset 3 (id:??) landed as
https://crrev.com/cd3dc7b24a97f9dda9e97e803449bde09fe0cb32
Cr-Commit-Position: refs/heads/master@{#394726}