WebAnimation should never prevent entering the back/forward cache

[Chris Dumez]

WebAnimation should never prevent entering the back/forward cache.

[Radar WebKit Bug Importer]

<rdar://problem/56374249>

[Antoine Quint]

So to fix we need to:

- remove WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED()
- subclass ActiveDOMObject::suspend() and ActiveDOMObject::resume()
- queue events between calls to suspend() and resume() or use a WindowEventLoop (though I'm not sure that is always possible since a WebAnimation may not be associated with a Document directly)
- don't resolve promises while the document is suspended

We might need something similar for DocumentTimeline, although it's possible that already happens.

[Chris Dumez]

(In reply to Antoine Quint from comment #2)
> So to fix we need to:
> 
> - remove WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED()
> - subclass ActiveDOMObject::suspend() and ActiveDOMObject::resume()
> - queue events between calls to suspend() and resume() or use a
> WindowEventLoop (though I'm not sure that is always possible since a
> WebAnimation may not be associated with a Document directly)

ActiveDOMObject need a script execution context when constructed. So presumably WebAnimation has one on construction.

If you cannot use WindowEventLoop, we have GenericEventQueue & SuspendableTaskQueue.
In your case, since you have both promises and events, SuspendableTaskQueue may be better. You can add such data member and then simply enqueue lambdas that either dispatch events or resolve promises. Those data types are PageCache aware and will delay things during suspension for you.

> - don't resolve promises while the document is suspended
> 
> We might need something similar for DocumentTimeline, although it's possible
> that already happens.

[Antoine Quint]

(In reply to Chris Dumez from comment #3)
> (In reply to Antoine Quint from comment #2)
> > So to fix we need to:
> > 
> > - remove WebAnimation::shouldPreventEnteringBackForwardCache_DEPRECATED()
> > - subclass ActiveDOMObject::suspend() and ActiveDOMObject::resume()
> > - queue events between calls to suspend() and resume() or use a
> > WindowEventLoop (though I'm not sure that is always possible since a
> > WebAnimation may not be associated with a Document directly)
> 
> ActiveDOMObject need a script execution context when constructed. So
> presumably WebAnimation has one on construction.
> 
> If you cannot use WindowEventLoop, we have GenericEventQueue &
> SuspendableTaskQueue.
> In your case, since you have both promises and events, SuspendableTaskQueue
> may be better. You can add such data member and then simply enqueue lambdas
> that either dispatch events or resolve promises. Those data types are
> PageCache aware and will delay things during suspension for you.

Right, we have a Document at creation time. It may not necessarily match the animation's associated document, but for the purpose of the back/forward cache, that should be fine since they will all be associated with the same top-level navigation.

I will look into SuspendableTaskQueue, it seems to be the most appropriate for my use. Thanks for the tips.

[Antoine Quint]

Created attachment 381558 [details]
Patch

[Chris Dumez]

Comment on attachment 381558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381558&action=review

> Source/WebCore/ChangeLog:11
> +        We remove the Web Animation subclass of the deprecated method ActiveDOMObject::shouldPreventEnteringBackForwardCache_DEPRECATED()

s/subclass/override

> Source/WebCore/animation/WebAnimation.cpp:-616
> -        callOnMainThread([this, pendingActivity = makePendingActivity(*this), event = WTFMove(event)]() {

You likely want to add `|| m_taskQueue->hasPendingTasks()` to WebAnimation::hasPendingActivity(), to make sure that your JS wrapper does not get collected until the event actually fires.
You used to enforce this behavior via a `pendingActivity = makePendingActivity(*this)` here.

> Source/WebCore/animation/WebAnimation.cpp:618
> +            this->dispatchEvent(event);

this-> should not be needed.

> Source/WebCore/animation/WebAnimation.cpp:1163
> +    ActiveDOMObject::suspend(reasonForSuspension);

Not needed.

> Source/WebCore/animation/WebAnimation.cpp:1169
> +    ActiveDOMObject::resume();

Not needed.

> LayoutTests/webanimations/animation-page-cache.html:52
> +        // Load a new page, and let it go back after 500ms.

This says 500ms but your next line says 1 second.
Either way, this seems like a pretty slow test, it'd be good to shorten this if possible.

[Antoine Quint]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 381558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381558&action=review
> 
> > LayoutTests/webanimations/animation-page-cache.html:52
> > +        // Load a new page, and let it go back after 500ms.
> 
> This says 500ms but your next line says 1 second.
> Either way, this seems like a pretty slow test, it'd be good to shorten this
> if possible.

It is tricky, there needs to be some real delay if we want to have a guarantee that we navigate forward and backward while the animation is live. I think we can shorten it a bit, which I will do in the next patch, but it is necessary for some time to elapse.

[Antoine Quint]

Created attachment 381566 [details]
Patch

[Antoine Quint]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 381558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381558&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        We remove the Web Animation subclass of the deprecated method ActiveDOMObject::shouldPreventEnteringBackForwardCache_DEPRECATED()
> 
> s/subclass/override
> 
> > Source/WebCore/animation/WebAnimation.cpp:-616
> > -        callOnMainThread([this, pendingActivity = makePendingActivity(*this), event = WTFMove(event)]() {
> 
> You likely want to add `|| m_taskQueue->hasPendingTasks()` to
> WebAnimation::hasPendingActivity(), to make sure that your JS wrapper does
> not get collected until the event actually fires.
> You used to enforce this behavior via a `pendingActivity =
> makePendingActivity(*this)` here.


I'll have to research this some more, but adding this check makes webanimations/leak-document-with-web-animation.html regress.

[Chris Dumez]

I found the bug.

[Chris Dumez]

Created attachment 382212 [details]
Patch

[Chris Dumez]

Comment on attachment 382212 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382212&action=review

> Source/WebCore/platform/SuspendableTaskQueue.h:61
> +    bool hasPendingTasks() const { return !m_pendingTasks.isEmpty(); }

This was the bug, wasn't your fault :)

[WebKit Commit Bot]

Comment on attachment 382212 [details]
Patch

Clearing flags on attachment: 382212

Committed r251742: <https://trac.webkit.org/changeset/251742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Quint]

Thanks for wrapping this up Chris!