Don't clear 'web animations dirty' flag if we have no rare data

Don't clear 'web animations dirty' flag if we have no rare data

If an SVGElement has an instantiated ElementAnimations object and
animation time has progressed, but no actual animation has been applied
(and hence no SVGElementRareData has been created), we don't need to
clear the dirty bit in the rare data.
The initial trigger for this seems to be the Element.computedName
implementation for a detached node, which tries to compute style in
this case, triggering a DCHECK in Node::containingTreeScope when doing
so.

BUG=686424
Review-Url: https://codereview.chromium.org/2689713003
Cr-Commit-Position: refs/heads/master@{#450689}
Committed: https://chromium.googlesource.com/chromium/src/+/49bd1e63cb077e88db6d89f5a3db801dec55e743

[fs, 2017-02-10 16:00:31]

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

[commit-bot: I haz the power, 2017-02-10 16:00:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 17:54:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 17:54:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fs, 2017-02-13 16:18:43]

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

[commit-bot: I haz the power, 2017-02-13 16:19:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2017-02-13 16:27:06]

Description was changed from

==========
Don't clear 'web animations dirty' flag if we have no rare data

If an SVGElement has an instantiated ElementAnimations object and
animation time has progressed, but no actual animation has been applied
(and hence no SVGElementRareData has been created), we don't need to
clear the dirty bit in the rare data.

BUG=686424
==========

to

==========
Don't clear 'web animations dirty' flag if we have no rare data

If an SVGElement has an instantiated ElementAnimations object and
animation time has progressed, but no actual animation has been applied
(and hence no SVGElementRareData has been created), we don't need to
clear the dirty bit in the rare data.
The initial trigger for this seems to be the Element.computedName
implementation for a detached node, which tries to compute style in
this case, triggering a DCHECK in Node::containingTreeScope when doing
so.

BUG=686424
==========

[commit-bot: I haz the power, 2017-02-13 17:50:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 17:50:03]

Dry run: This issue passed the CQ dry run.

[fs, 2017-02-13 19:16:07]

fs@opera.com changed reviewers:
+ alancutter@chromium.org, dmazzoni@chromium.org

[fs, 2017-02-13 19:16:07]

https://codereview.chromium.org/2689713003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/animations/animval-web-animations-flush-crash.html
(right):

https://codereview.chromium.org/2689713003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/animations/animval-web-animations-flush-crash.html:18:
element.computedName;
This would trigger the DCHECK in Node::containingTreeScope, fixed by the change
to AXObject.cpp. With that fix, the crash does happen though, making the test
mostly useless. Any idea if the same thing (resolving a style without attaching
it to a node I'm assuming is the thing) could be triggered in some other way?

[dmazzoni, 2017-02-13 19:24:54]

Changes lgtm, but I can't think of a way to resolve a style
from a LayoutTest without a node.

Do you maybe want to write a unit test in C++ instead?
They're relatively rare but they do exist...

[fs, 2017-02-13 20:13:30]

On 2017/02/13 at 19:24:54, dmazzoni wrote:
> Changes lgtm, but I can't think of a way to resolve a style
> from a LayoutTest without a node.
> 
> Do you maybe want to write a unit test in C++ instead?
> They're relatively rare but they do exist...

I'm not sure if that has higher utility factor than the current test (tests for
corner-cases discovered by fuzzers tend to be... of limited use.)

[alancutter (OOO until 2018), 2017-02-15 04:11:45]

lgtm with nits.

testharness.js is preferred.

https://codereview.chromium.org/2689713003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/animations/animval-web-animations-flush-crash.html
(right):

https://codereview.chromium.org/2689713003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/animations/animval-web-animations-flush-crash.html:21:
;
I wonder if internals.pauseAnimations(document.timeline.currentTime + 100) would
work instead of a busy wait.

[fs, 2017-02-15 12:46:05]

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

[commit-bot: I haz the power, 2017-02-15 12:46:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 14:04:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 14:04:58]

Dry run: This issue passed the CQ dry run.

[fs, 2017-02-15 14:18:30]

On 2017/02/15 at 04:11:45, alancutter wrote:
...
> testharness.js is preferred.

Test changed.

https://codereview.chromium.org/2689713003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/animations/animval-web-animations-flush-crash.html
(right):

https://codereview.chromium.org/2689713003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/animations/animval-web-animations-flush-crash.html:21:
;
On 2017/02/15 at 04:11:45, alancutter wrote:
> I wonder if internals.pauseAnimations(document.timeline.currentTime + 100)
would work instead of a busy wait.

Sadly, no =(. (Probably because of the lifecycle update in that method.)

[fs, 2017-02-15 14:18:37]

The CQ bit was checked by fs@opera.com

[fs, 2017-02-15 14:18:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2689713003/#ps40001
(title: "Use testharness")

[commit-bot: I haz the power, 2017-02-15 14:18:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 14:24:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487168317641960,
"parent_rev": "69089a6d0b93398de4cae6990808b8af12239354", "commit_rev":
"49bd1e63cb077e88db6d89f5a3db801dec55e743"}

[commit-bot: I haz the power, 2017-02-15 14:25:21]

Description was changed from

==========
Don't clear 'web animations dirty' flag if we have no rare data

If an SVGElement has an instantiated ElementAnimations object and
animation time has progressed, but no actual animation has been applied
(and hence no SVGElementRareData has been created), we don't need to
clear the dirty bit in the rare data.
The initial trigger for this seems to be the Element.computedName
implementation for a detached node, which tries to compute style in
this case, triggering a DCHECK in Node::containingTreeScope when doing
so.

BUG=686424
==========

to

==========
Don't clear 'web animations dirty' flag if we have no rare data

If an SVGElement has an instantiated ElementAnimations object and
animation time has progressed, but no actual animation has been applied
(and hence no SVGElementRareData has been created), we don't need to
clear the dirty bit in the rare data.
The initial trigger for this seems to be the Element.computedName
implementation for a detached node, which tries to compute style in
this case, triggering a DCHECK in Node::containingTreeScope when doing
so.

BUG=686424

Review-Url: https://codereview.chromium.org/2689713003
Cr-Commit-Position: refs/heads/master@{#450689}
Committed:
https://chromium.googlesource.com/chromium/src/+/49bd1e63cb077e88db6d89f5a3db...
==========

[commit-bot: I haz the power, 2017-02-15 14:25:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/49bd1e63cb077e88db6d89f5a3db...