Delay resetting image animation, if possible. (issue 2004263003 by sigbjornf@opera.com)

[sigb...@opera.com]

Reviewers: oilpan-reviews, fs, David Vest
CL: https://codereview.chromium.org/2004263003/

Message:
please take a look.

See https://codereview.chromium.org/2005693002/ for an alternate approach
considered.

Description:
Delay resetting image animation, if possible.

When the last client of an ImageResource removes itself, the animations
of the image is explicitly reset. That resetting can happen either while
finalizing objects after a GC or as part of other explicit removals of
ImageObserver clients. Having that reset happen as part of a garbage
collection is interacting badly with code in the middle of updating
animations (which happen to trigger a GC), so to avoid introducing such
abrupt resets, delay the operation until back at the event loop (and
the animations update step having completed.)

R=
BUG=613709, 581546

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+15, -2 lines):
M third_party/WebKit/Source/core/fetch/ImageResource.h
M third_party/WebKit/Source/core/fetch/ImageResource.cpp


Index: third_party/WebKit/Source/core/fetch/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ImageResource.cpp b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
index 81db13640f8150ed4554bcf6542607cb4925b564..c9b6c20697473fa684d89804ff32a475ee39fcb4 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
@@ -206,10 +206,21 @@ void ImageResource::destroyDecodedDataIfPossible()
}
}

-void ImageResource::allClientsAndObserversRemoved()
+void ImageResource::doResetAnimation()
{
- if (m_image && !errorOccurred())
+ if (m_image)
m_image->resetAnimation();
+}
+
+void ImageResource::allClientsAndObserversRemoved()
+{
+ if (m_image && !errorOccurred()) {
+ // If possible, delay the resetting until back at the event loop.
+ if (!ThreadHeap::willObjectBeLazilySwept(this))
+ Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, bind(&ImageResource::doResetAnimation, CrossThreadWeakPersistentThisPointer<ImageResource>(this)));
+ else
+ m_image->resetAnimation();
+ }
if (m_multipartParser)
m_multipartParser->cancel();
Resource::allClientsAndObserversRemoved();
Index: third_party/WebKit/Source/core/fetch/ImageResource.h
diff --git a/third_party/WebKit/Source/core/fetch/ImageResource.h b/third_party/WebKit/Source/core/fetch/ImageResource.h
index 8ab5b6d18f1cb194d287bbde3fd7ef8540201f5c..0d0910f8c0e0e28b3e7bb8114c983dfe67954af3 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResource.h
+++ b/third_party/WebKit/Source/core/fetch/ImageResource.h
@@ -160,6 +160,8 @@ private:
void checkNotify() override;
void markClientsAndObserversFinished() override;

+ void doResetAnimation();
+
float m_devicePixelRatioHeaderValue;

Member<MultipartImageResourceParser> m_multipartParser;

[f...@opera.com]

On 2016/05/24 at 14:07:31, sigbjornf wrote:
> please take a look.
>
> See https://codereview.chromium.org/2005693002/ for an alternate approach
considered.

Seems reasonable to me (albeit very non-obvious if you don't know what it's
fixing.)

LGTM

https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

On 2016/05/24 14:16:00, fs wrote:
> On 2016/05/24 at 14:07:31, sigbjornf wrote:
> > please take a look.
> >
> > See https://codereview.chromium.org/2005693002/ for an alternate approach
> considered.
>
> Seems reasonable to me (albeit very non-obvious if you don't know what it's
> fixing.)
>
> LGTM

The comment didn't offer very many clues either; expanded it some.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Source/core/fetch/ImageResource.cpp
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Source/core/fetch/ImageResource.cpp#newcode221
third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if
(!ThreadHeap::willObjectBeLazilySwept(this))

Hmm, I'm not really happy about using
ThreadHeap::willObjectBeLazilySwept since it can return a wrong result
(as we discussed previously).

Would it be possible to introduce a flag to ImageResource and set it in
a pre-finalizer? Then we can check the flag here to see if the
ImageResource is going to be collected or not.

https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Source/core/fetch/ImageResource.cpp
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Source/core/fetch/ImageResource.cpp#newcode221
third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if
(!ThreadHeap::willObjectBeLazilySwept(this))

On 2016/05/24 15:40:54, haraken wrote:
>
> Hmm, I'm not really happy about using
ThreadHeap::willObjectBeLazilySwept since
> it can return a wrong result (as we discussed previously).
>
> Would it be possible to introduce a flag to ImageResource and set it
in a
> pre-finalizer? Then we can check the flag here to see if the
ImageResource is
> going to be collected or not.

As this method will be called by another object's prefinalizer, I don't
think that scheme will work.

As long as willBeLazilySwept() returns true while that other object's
prefinalizer is running, that's sufficient. We could add that as a flag
to ThreadState (== is running prefinalizers), I suppose.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

On 2016/05/24 15:47:20, sof wrote:
>
https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Source/core/fetch/ImageResource.cpp
> File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):
>
>
https://codereview.chromium.org/2004263003/diff/20001/third_party/WebKit/Source/core/fetch/ImageResource.cpp#newcode221
> third_party/WebKit/Source/core/fetch/ImageResource.cpp:221: if
> (!ThreadHeap::willObjectBeLazilySwept(this))
> On 2016/05/24 15:40:54, haraken wrote:
> >
> > Hmm, I'm not really happy about using ThreadHeap::willObjectBeLazilySwept
> since
> > it can return a wrong result (as we discussed previously).
> >
> > Would it be possible to introduce a flag to ImageResource and set it in a
> > pre-finalizer? Then we can check the flag here to see if the ImageResource
is
> > going to be collected or not.
>
> As this method will be called by another object's prefinalizer, I don't think
> that scheme will work.

What's the another object? Would it be an option to set a flag on ImageResource
at the very beginning of the another object's pre-finalizer?



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

It would be a hack, I think. CSSFetchImage and whoever else is a client of this
ImageResource.

What's being done here is safe, i.e., ensuring that we don't revive a dying
object during finalization.

More generally, it would be good if you could write out a clear argument why&how
willObjectBeLazilySwept() is unsafe in our implementation -- I don't think I've
seen that. We rely on it quite a bit, timers wouldn't be safe without it. And I
believe they are :)

https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

s/CSSFetchImage/StyleFetchedImage/

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

See https://codereview.chromium.org/1151163002/#msg13.

willObjectBeLazilySwept() is safe only if it's guaranteed that it's not called
when we have a page s.t. a part of the page has already been swept but the
remaining part of the page has not yet been swept. At the end of an event loop
meets the condition, so willObjectBeLazilySwept() in timer is safe. But due to
the subtlety, I want to avoid using willObjectBeLazilySwept() as much as
possible.



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

Thanks for the refresher; call to willObjectBeLazilySwept() in code executed by
non-pre/normal finalizers is what to look out for. Its imprecision wouldn't
matter here, but I accept that you want to avoid its use.

What we're really after here is a way to check if this is executed as part of
the (pre)finalization steps that a conservative GC will perform before returning
from the allocate() call that triggered the conservative GC..but we also need to
determine liveness of the ImageResource. Let me think it over if there's a way
to sidestep willObjectBeLazilySwept() (but not introduce more prefinalization
code, preferably.)

https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

Thinking that over a bit made me realize that willObjectBeLazilySwept(this) will
always return a correct result.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

What happens if ImageResource::allClientsAndObserversRemoved is called by code
other than the pre-finalizer? Does willObjectBeLazilySwept(this) return a
correct result even in the case?

But either way, I think willObjectBeLazilySwept(this) is too complicated to
reason about. If we can set a flag at the beginning of the pre-finalizer and
prevent the animation update, that seems safer to me.



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

We need to fix willObjectBeLazilySwept() to also handle this rare lazy sweeping
case; ps#3 addresses.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

> We need to fix willObjectBeLazilySwept() to also handle this rare lazy
sweeping
> case; ps#3 addresses.

Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer
decreasing the call site rather than making it more correct...

Stepping back, let me ask a couple of questions:

- Do we really need to call resetAnimation when all the clients and the
ImageResource die at the same time?

- You don't delay calling resetAnimation when all the clients are removed but
the ImageResource is still live. This means that style recalc can run in
CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is not
allowed to change object graphs (accurately speaking, it is not allowed to
resurrect any reference), so it looks unsafe to run a complicated thing like
style recalc during a pre-finalizer.



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

On 2016/05/26 04:20:27, haraken wrote:
> > We need to fix willObjectBeLazilySwept() to also handle this rare lazy
> sweeping
> > case; ps#3 addresses.
>
> Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd prefer
> decreasing the call site rather than making it more correct...
>

Don't agree, this is a good solution that I want to go with.

> Stepping back, let me ask a couple of questions:
>
> - Do we really need to call resetAnimation when all the clients and the
> ImageResource die at the same time?
>

That's what we've been doing all along & currently.

> - You don't delay calling resetAnimation when all the clients are removed but
> the ImageResource is still live. This means that style recalc can run in
> CSSStyleImage's pre-finalizer, but isn't it dangerous? A pre-finalizer is not
> allowed to change object graphs (accurately speaking, it is not allowed to
> resurrect any reference), so it looks unsafe to run a complicated thing like
> style recalc during a pre-finalizer.

It's the other way around, when it is dead we cannot delay the call. Yes, we all
would want there not to be that amount of code executed when finalizing, but
that's an old topic.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

I don't see any reason we need to run resetAnimations when both the clients and
the ImageResource at the same time.

If we just need to run resetAnimations only when the clients are dead but the
ImageResource is live, can we just use weak processing to post a task for
resetAnimations? ImageResource can post a task when the weakly observing clients
are gone.



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

resetAnimation() does a lot of work, how would you know that it isn't needed?
This is what ToT does and have been doing for a long time.

iow, I'm looking to address an instability here that has a beta-block label on
it, first & foremost.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

I'm not convinced that this is a right fix...

The problem is that we're calling resetAnimations during a pre-finalizer. This
CL still runs resetAnimations during a pre-finalizer when the clients and the
ImageResource die at the same time. This CL may reduce the crash rate, but the
issue is still there, isn't it?



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

I don't think it is; SVGImage is where this is manifesting itself, where we
explicitly hold on to the ImageResource while executing animations updates.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

Do you mean that it's not possible that allClientsRemoved() is called when the
clients and the ImageResource die at the same time?



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

And just to be clear, the resetAnimation() call isn't unsafe in that accesses
objects that it shouldn't and similar while running finalization code, but that
it upsets an ongoing animations update to have the animation be reset while it
runs (and happens to trigger a conservative GC.) Hence delaying the reset'ing.

https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

On 2016/05/26 08:52:56, haraken wrote:
> On 2016/05/26 08:50:45, sof wrote:
...

> >
> > I don't think it is; SVGImage is where this is manifesting itself, where we
> > explicitly hold on to the ImageResource while executing animations updates.
>

> Do you mean that it's not possible that allClientsRemoved() is called when the
> clients and the ImageResource die at the same time?

It is possible, and it is not a problem for that (lengthy) call to go ahead.
Except for reasons in the context of SVGImage, as detailed in #24.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

On 2016/05/26 08:58:02, sof wrote:
> On 2016/05/26 08:50:45, sof wrote:

> > On 2016/05/26 08:43:33, haraken wrote:
> > > On 2016/05/26 08:20:57, sof wrote:
> > > > On 2016/05/26 08:16:18, haraken wrote:
> > > > > On 2016/05/26 05:19:23, sof wrote:
> > > > > > On 2016/05/26 04:20:27, haraken wrote:
> > > > > > > > We need to fix willObjectBeLazilySwept() to also handle this
rare
> > lazy
> > > > > > > sweeping
> > > > > > > > case; ps#3 addresses.
> > > > > > >
> > > > > > > Hmm. PS3's willObjectBeLazilySwept() looks more correct, but I'd
> > prefer
> > > > > > > decreasing the call site rather than making it more correct...
> > > > > > >
> > > > > >
> > > > > > Don't agree, this is a good solution that I want to go with.
> > > > > >
> > > > > > > Stepping back, let me ask a couple of questions:
> > > > > > >

> > > > > > > - Do we really need to call resetAnimation when all the clients

and
> > the
> > > > > > > ImageResource die at the same time?
> > > > > > >
> > > > > >

> > > > > I don't see any reason we need to run resetAnimations when both the
> > clients
> > > > and

> > > > > the ImageResource at the same time.
> > > > >
> > > > > If we just need to run resetAnimations only when the clients are dead
> but
> > > the
> > > > > ImageResource is live, can we just use weak processing to post a task
> for
> > > > > resetAnimations? ImageResource can post a task when the weakly
observing
> > > > clients
> > > > > are gone.
> > > >
> > > > resetAnimation() does a lot of work, how would you know that it isn't
> > needed?
> > > > This is what ToT does and have been doing for a long time.
> > > >
> > > > iow, I'm looking to address an instability here that has a beta-block
> label
> > on
> > > > it, first & foremost.
> > >
> > > I'm not convinced that this is a right fix...
> > >
> > > The problem is that we're calling resetAnimations during a pre-finalizer.
> This

> > > CL still runs resetAnimations during a pre-finalizer when the clients and
> the

> > > ImageResource die at the same time. This CL may reduce the crash rate, but
> the
> > > issue is still there, isn't it?
> >

> > I don't think it is; SVGImage is where this is manifesting itself, where we
> > explicitly hold on to the ImageResource while executing animations updates.
>

> And just to be clear, the resetAnimation() call isn't unsafe in that accesses
> objects that it shouldn't and similar while running finalization code, but
that
> it upsets an ongoing animations update to have the animation be reset while it
> runs (and happens to trigger a conservative GC.) Hence delaying the reset'ing.

Maybe I'm not fully understanding the real problem here.

> but that
> it upsets an ongoing animations update to have the animation be reset while it
> runs (and happens to trigger a conservative GC.)

^^^ Would you elaborate on this?

Why can a conservative GC be triggered during resetAnimations? GCs should be
forbidden during pre-finalizers.


https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

They don't happen there but while executing the animations update.

Please see https://bugs.chromium.org/p/chromium/issues/detail?id=581546#c3 ,
https://bugs.chromium.org/p/chromium/issues/detail?id=613709#c8 and
https://codereview.chromium.org/2005693002/#msg9

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

Thanks, I now understand the problem well.

Would it be an option to change resetAnimation() to resetAnimationWhenDone()? If
resetAnimationWhenDone() is called during an animation, it just sets a flag. The
animation calls resetAnimation() at the end of the animation if the flag is set.



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

Isn't that what the CL I referred to does (
https://codereview.chromium.org/2005693002/ ) ? See #msg9 why I don't prefer it.

This is taking some time.

https://codereview.chromium.org/2004263003/

[har...@chromium.org]

I'd prefer the approach in https://codereview.chromium.org/2005693002/. Or we
should try to keep alive the clients while doing the animation (If this is
doable, I prefer this the best).

This CL is doing something really hacky:

- When the clients are dead but the ImageResource is live, post a task for
resetAnimation because the animation may be still running at that point.

- When the clients and the ImageResource are dead at the same time, don't post a
task because we cannot post a task (because the ImageResource is already dead).
But this is fine (by accident) because "the clients and the ImageResource are
dead at the same time" should not happen while running the animation.

- I don't want to increase the usage of "mysterious" APIs like
willObjectBeLazilySwept(), which is hard for developers to understand.



https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

i see, don't agree with your assessment(s) here. I won't pursue that approach.

https://codereview.chromium.org/2004263003/

[sigb...@opera.com]

Rebased up past r396798.

https://codereview.chromium.org/2004263003/