Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=haraken
BUG=610855
Committed: https://crrev.com/6d735f78334721b46d37f97ad8efb12d6e329e75
Cr-Commit-Position: refs/heads/master@{#392919}

[sof, 2016-05-11 11:35:51]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2016-05-11 11:35:51]

please take a look.

[sof, 2016-05-11 11:36:59]

Using PersistentHeapHashSet<> is an alternative that would also work (for
obscure reasons), but I don't think we need to resort to persistents for this
one.

[haraken, 2016-05-11 11:53:23]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-11 11:53:23]

LGTM

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

https://codereview.chromium.org/1968683003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:1225:
DEFINE_STATIC_LOCAL(HashSet<UntracedMember<SVGSMILElement>>, loopBreaker, ());

Maybe worth adding a comment why the untraced members are safe.

[sof, 2016-05-11 12:11:20]

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

https://codereview.chromium.org/1968683003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:1225:
DEFINE_STATIC_LOCAL(HashSet<UntracedMember<SVGSMILElement>>, loopBreaker, ());
On 2016/05/11 11:53:23, haraken wrote:
> 
> Maybe worth adding a comment why the untraced members are safe.

yes, done.

[sof, 2016-05-11 12:11:30]

Description was changed from

==========
Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=
BUG=610855
==========

to

==========
Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=haraken
BUG=610855
==========

[sof, 2016-05-11 12:11:34]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-05-11 12:11:34]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1968683003/#ps20001
(title: "add explanatory comment re UntracedMember<>")

[commit-bot: I haz the power, 2016-05-11 12:11:45]

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

[commit-bot: I haz the power, 2016-05-11 14:39:41]

Description was changed from

==========
Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=haraken
BUG=610855
==========

to

==========
Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=haraken
BUG=610855
==========

[commit-bot: I haz the power, 2016-05-11 14:39:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-11 14:41:28]

Description was changed from

==========
Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=haraken
BUG=610855
==========

to

==========
Simplify SVGSMILElement::notifyDependentsIntervalChanged loop breaker.

To catch out recursive notifications, notifyDependentsIntervalChanged()
keeps track of the SVGSMILElements that are on the stack and being
notified, so as to bail early in case of loops.

There's no need for that set of SVGSMILElements to be recorded using
a persistent static local as the objects are stack reachable should
a conservative GC be needed, so an 'ordinary' hash set will do.

Not using a persistent reference also addresses a bad interaction with
LSan (Blink has to release all static persistents before shutting
down to prevent false leaks w/ LSan enabled), but SVGImages containing
animations may end up in this code path as part of an image resource
being finalized. Which would then encounter an empty persistent
static reference and fail (see associated bug and stack trace.)

R=haraken
BUG=610855

Committed: https://crrev.com/6d735f78334721b46d37f97ad8efb12d6e329e75
Cr-Commit-Position: refs/heads/master@{#392919}
==========

[commit-bot: I haz the power, 2016-05-11 14:41:28]

Patchset 2 (id:??) landed as
https://crrev.com/6d735f78334721b46d37f97ad8efb12d6e329e75
Cr-Commit-Position: refs/heads/master@{#392919}