Delay SVGImage animations reset while being updated.

Delay SVGImage animations reset while being updated.

When the SVGImage animations timer is serviced, a strong reference
is kept on the stack to the image's underlying ImageResource --
preventing it from being GCed as it isn't stack-reachable
from SVGImageChromeClient.

Additionally strengthen that ImageResource lock and prevent
calls to resetAnimation() on the SVGImage, should those GCs finalize
all clients registered with the ImageResource and the reset operation
then being attempted. Doing so is troublesome for the animations
update that is in-flight (see associated bugs), so we delay the
reset until the update has completed.

BUG=613709, 581546

[sof, 2016-05-23 14:49:20]

Description was changed from

==========
Delay SVGImage animations resetting while the animations are being updated.

R=
BUG=
==========

to

==========
Delay SVGImage animations reset while being updated.

When the SVGImage animations timer is serviced, a strong reference
is kept on the stack to the image's underlying ImageResource --
preventing it from being GCed as it isn't stack-reachable
from SVGImageChromeClient.

Additionally strengthen that ImageResource lock and prevent
calls to resetAnimation() on the SVGImage, should those GCs finalize
all clients registered with the ImageResource and the reset operation
then being attempted. Doing so is troublesome for the animations
update that is in-flight (see associated bugs), so we delay the
reset until the update has completed.

R=
BUG=613709, 581546
==========

[sof, 2016-05-23 14:54:09]

sigbjornf@opera.com changed reviewers:
+ davve@opera.com, fs@opera.com, pdr@chromium.org

[sof, 2016-05-23 14:54:10]

please take a look.

Kind of blunt, but I think it's worthwhile to address this instability in the
shorter-term.

[davve, 2016-05-24 07:21:55]

On 2016/05/23 14:54:10, sof wrote:
> please take a look.
> 
> Kind of blunt, but I think it's worthwhile to address this instability in the
> shorter-term.

While I'm fine with landing this as a short-term fix, I do worry that
it will be hard to fix properly in the future though, since we don't
have a test and it seems the only way to get stacks is to expose the
crash on trunk (as we've done now.)

Long-term I do think the flag in SVGImage is too specific to a
scenario that preferably shouldn't exist. Possibly stupid question:
Couldn't SVGImageChromeClient be an observer of the resource while the
animation is running, to prevent the
ImageResource::allClientsAndObserversRemoved() to be made at all?

[sof, 2016-05-24 07:25:26]

On 2016/05/24 07:21:55, David Vest wrote:
> On 2016/05/23 14:54:10, sof wrote:
> > please take a look.
> > 
> > Kind of blunt, but I think it's worthwhile to address this instability in
the
> > shorter-term.
> 
> While I'm fine with landing this as a short-term fix, I do worry that
> it will be hard to fix properly in the future though, since we don't
> have a test and it seems the only way to get stacks is to expose the
> crash on trunk (as we've done now.)
> 
> Long-term I do think the flag in SVGImage is too specific to a
> scenario that preferably shouldn't exist. Possibly stupid question:
> Couldn't SVGImageChromeClient be an observer of the resource while the
> animation is running, to prevent the
> ImageResource::allClientsAndObserversRemoved() to be made at all?

Considered that, but it doesn't sit too well with ImageObserver being passed to
(SVG)Image upon creation - there's no view of the underlying resource. Adding
one so as to expose the ability to register a fake temporary observer felt
(more) ad-hoc.

[davve, 2016-05-24 07:34:57]

On 2016/05/24 07:25:26, sof wrote:
> On 2016/05/24 07:21:55, David Vest wrote:
...
> > Long-term I do think the flag in SVGImage is too specific to a
> > scenario that preferably shouldn't exist. Possibly stupid question:
> > Couldn't SVGImageChromeClient be an observer of the resource while the
> > animation is running, to prevent the
> > ImageResource::allClientsAndObserversRemoved() to be made at all?
> 
> Considered that, but it doesn't sit too well with ImageObserver being passed
to
> (SVG)Image upon creation - there's no view of the underlying resource. Adding
> one so as to expose the ability to register a fake temporary observer felt
> (more) ad-hoc.

Then I have nothing further. lgtm but fs should probably sign off as well. I
overheard there being discussions about m_resetAnimationAfterUpdate being
necessary.

[fs, 2016-05-24 10:41:54]

Description was changed from

==========
Delay SVGImage animations reset while being updated.

When the SVGImage animations timer is serviced, a strong reference
is kept on the stack to the image's underlying ImageResource --
preventing it from being GCed as it isn't stack-reachable
from SVGImageChromeClient.

Additionally strengthen that ImageResource lock and prevent
calls to resetAnimation() on the SVGImage, should those GCs finalize
all clients registered with the ImageResource and the reset operation
then being attempted. Doing so is troublesome for the animations
update that is in-flight (see associated bugs), so we delay the
reset until the update has completed.

R=
BUG=613709, 581546
==========

to

==========
Delay SVGImage animations reset while being updated.

When the SVGImage animations timer is serviced, a strong reference
is kept on the stack to the image's underlying ImageResource --
preventing it from being GCed as it isn't stack-reachable
from SVGImageChromeClient.

Additionally strengthen that ImageResource lock and prevent
calls to resetAnimation() on the SVGImage, should those GCs finalize
all clients registered with the ImageResource and the reset operation
then being attempted. Doing so is troublesome for the animations
update that is in-flight (see associated bugs), so we delay the
reset until the update has completed.

BUG=613709, 581546
==========

[fs, 2016-05-24 10:52:12]

On 2016/05/24 at 07:34:57, davve wrote:
> On 2016/05/24 07:25:26, sof wrote:
> > On 2016/05/24 07:21:55, David Vest wrote:
> ...
> > > Long-term I do think the flag in SVGImage is too specific to a
> > > scenario that preferably shouldn't exist. Possibly stupid question:
> > > Couldn't SVGImageChromeClient be an observer of the resource while the
> > > animation is running, to prevent the
> > > ImageResource::allClientsAndObserversRemoved() to be made at all?
> > 
> > Considered that, but it doesn't sit too well with ImageObserver being passed
to
> > (SVG)Image upon creation - there's no view of the underlying resource.
Adding
> > one so as to expose the ability to register a fake temporary observer felt
> > (more) ad-hoc.
> 
> Then I have nothing further. lgtm but fs should probably sign off as well. I
overheard there being discussions about m_resetAnimationAfterUpdate being
necessary.

Yes, this is a hole that will probably be very difficult to dig ourselves out
of... Since it's difficult to make a (reliable) test for this, maybe it'd make
sense to reference the bug with the full stacktrace in one of the comments, to
bring the issue closer to the surface? (Reentrancy through the memory allocator
is probably not most people will even consider...)

lgtm

(Tangent: Would it be possible to rig the conservative sweep as to not sweep
objects with pre-finalizers?)

[sof, 2016-05-24 14:02:20]

On 2016/05/24 10:52:12, fs wrote:
> On 2016/05/24 at 07:34:57, davve wrote:
> > On 2016/05/24 07:25:26, sof wrote:
> > > On 2016/05/24 07:21:55, David Vest wrote:
> > ...
> > > > Long-term I do think the flag in SVGImage is too specific to a
> > > > scenario that preferably shouldn't exist. Possibly stupid question:
> > > > Couldn't SVGImageChromeClient be an observer of the resource while the
> > > > animation is running, to prevent the
> > > > ImageResource::allClientsAndObserversRemoved() to be made at all?
> > > 
> > > Considered that, but it doesn't sit too well with ImageObserver being
passed
> to
> > > (SVG)Image upon creation - there's no view of the underlying resource.
> Adding
> > > one so as to expose the ability to register a fake temporary observer felt
> > > (more) ad-hoc.
> > 
> > Then I have nothing further. lgtm but fs should probably sign off as well. I
> overheard there being discussions about m_resetAnimationAfterUpdate being
> necessary.
> 
> Yes, this is a hole that will probably be very difficult to dig ourselves out
> of... Since it's difficult to make a (reliable) test for this, maybe it'd make
> sense to reference the bug with the full stacktrace in one of the comments, to
> bring the issue closer to the surface? (Reentrancy through the memory
allocator
> is probably not most people will even consider...)
> 
> lgtm
> 
> (Tangent: Would it be possible to rig the conservative sweep as to not sweep
> objects with pre-finalizers?)

Thanks both, but based on your feedback and seeing that crashes are also
reported across other kinds of animation updates (e.g., from
SMILTimeContainer::wakeupTimerFired()), I think we need to avoid these
rug-sweeping resetAnimation() calls as part of a (conservative) GC.
https://codereview.chromium.org/2004263003/ does that; i'd like to try that here
instead.

[sof, 2016-07-14 12:57:39]

https://codereview.chromium.org/2004263003/ landed some time ago, closing.