Bug 203105 – VRDisplay should not prevent entering the back/forward cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

VRDisplay should not prevent entering the back/forward cache.

[Chris Dumez]

Would someone be able to work on this?

[Sergio Villar Senin]

Chris could you add a little bit more context?

[Chris Dumez]

(In reply to Sergio Villar Senin from comment #2)
> Chris could you add a little bit more context?

We are working on making the back/forward cache (formerly known as the page cache) 100% successful at caching pages. As a result of this, we no longer allow ActiveDOMObjects to prevent caching. ALL ActiveDOMObject (including VRDisplay) need to suspend correctly.

Currently, any page which uses a VRDisplay will not be able to enter the back/forward cache because of this code:
// FIXME: This should never prevent entering the back/forward cache.
bool VRDisplay::shouldPreventEnteringBackForwardCache_DEPRECATED() const
{
    return true;
}

This method needs to be removed to allow the page to enter the back/forward cache. Then you'll likely need to override ActiveDOMObject's suspend() and resume() to implement correct suspension. Looking at VRDisplay, it is likely the following needs to be done:
1. Instead of dispatching events synchronously, append tasks to the WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends while in the cache.
2. Call suspend() / resume() on m_scriptedAnimationController to suspend animations while in the cache
3. Maybe call stopPresenting() on suspension? I don't know much about VRDisplay.

[Sergio Villar Senin]

(In reply to Chris Dumez from comment #3)
> (In reply to Sergio Villar Senin from comment #2)
> > Chris could you add a little bit more context?
> 
> We are working on making the back/forward cache (formerly known as the page
> cache) 100% successful at caching pages. As a result of this, we no longer
> allow ActiveDOMObjects to prevent caching. ALL ActiveDOMObject (including
> VRDisplay) need to suspend correctly.
> 
> Currently, any page which uses a VRDisplay will not be able to enter the
> back/forward cache because of this code:
> // FIXME: This should never prevent entering the back/forward cache.
> bool VRDisplay::shouldPreventEnteringBackForwardCache_DEPRECATED() const
> {
>     return true;
> }

Ah I was not aware of such a thing, it was not there when I added the code.
 
> This method needs to be removed to allow the page to enter the back/forward
> cache. Then you'll likely need to override ActiveDOMObject's suspend() and
> resume() to implement correct suspension. Looking at VRDisplay, it is likely
> the following needs to be done:
> 1. Instead of dispatching events synchronously, append tasks to the
> WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends
> while in the cache.
> 2. Call suspend() / resume() on m_scriptedAnimationController to suspend
> animations while in the cache
> 3. Maybe call stopPresenting() on suspension? I don't know much about
> VRDisplay.

I understand the issue now, I'll come up with something.

[Chris Dumez]

(In reply to Sergio Villar Senin from comment #4)
> (In reply to Chris Dumez from comment #3)
> > (In reply to Sergio Villar Senin from comment #2)
> > > Chris could you add a little bit more context?
> > 
> > We are working on making the back/forward cache (formerly known as the page
> > cache) 100% successful at caching pages. As a result of this, we no longer
> > allow ActiveDOMObjects to prevent caching. ALL ActiveDOMObject (including
> > VRDisplay) need to suspend correctly.
> > 
> > Currently, any page which uses a VRDisplay will not be able to enter the
> > back/forward cache because of this code:
> > // FIXME: This should never prevent entering the back/forward cache.
> > bool VRDisplay::shouldPreventEnteringBackForwardCache_DEPRECATED() const
> > {
> >     return true;
> > }
> 
> Ah I was not aware of such a thing, it was not there when I added the code.

It was there, it was just named differently. It used to be:
bool VRDisplay::canSuspendForPageCache() const
{
    return false;
}

I am pretty sure you added that code and the name is pretty clear about the implications.

[Chris Dumez]

 
> > This method needs to be removed to allow the page to enter the back/forward
> > cache. Then you'll likely need to override ActiveDOMObject's suspend() and
> > resume() to implement correct suspension. Looking at VRDisplay, it is likely
> > the following needs to be done:
> > 1. Instead of dispatching events synchronously, append tasks to the
> > WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends
> > while in the cache.
> > 2. Call suspend() / resume() on m_scriptedAnimationController to suspend
> > animations while in the cache
> > 3. Maybe call stopPresenting() on suspension? I don't know much about
> > VRDisplay.
> 
> I understand the issue now, I'll come up with something.

Thanks for you help.

[Zan Dobersek]

(In reply to Chris Dumez from comment #3)
> 1. Instead of dispatching events synchronously, append tasks to the
> WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends
> while in the cache.

Right now the events are only dispatched against the DOMWindow object. Should the dispatch be asynchronous regardless of the target?

[Chris Dumez]

(In reply to Zan Dobersek from comment #7)
> (In reply to Chris Dumez from comment #3)
> > 1. Instead of dispatching events synchronously, append tasks to the
> > WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends
> > while in the cache.
> 
> Right now the events are only dispatched against the DOMWindow object.
> Should the dispatch be asynchronous regardless of the target?

Yes, I believe so.

[Zan Dobersek]

Created attachment 382822 [details]
WIP

Test pending.

[Sergio Villar Senin]

Created attachment 382824 [details]
Patch

[Chris Dumez]

Comment on attachment 382824 [details]
Patch

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

> Source/WebCore/Modules/webvr/VRDisplay.cpp:216
> +void VRDisplay::dispatchVRDisplayEventInWindowEventLoop(const WTF::AtomString& eventName, Optional<VRDisplayEventReason>&& reason)

Ryosuke already added ActiveDOMObject::queueTaskToDispatchEvent() for this purpose.

> Source/WebCore/Modules/webvr/VRDisplay.cpp:258
> +    stopPresenting();

Note that you may want to do this only if the reason for suspension is BackForwardCache.

[Sergio Villar Senin]

Created attachment 382827 [details]
Dispatch events in DOMWindow

[Chris Dumez]

Comment on attachment 382827 [details]
Dispatch events in DOMWindow

Please see previous review comments.

[Zan Dobersek]

We can use ActiveDOMObject::queueTaskKeepingObjectAlive(). The event-dispatch version doesn't properly handle the target for this case.

[Chris Dumez]

(In reply to Zan Dobersek from comment #14)
> We can use ActiveDOMObject::queueTaskKeepingObjectAlive(). The
> event-dispatch version doesn't properly handle the target for this case.

I did not understand this comment.

[Zan Dobersek]

Comment on attachment 382827 [details]
Dispatch events in DOMWindow

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

Inlining the comment into the patch ...

> Source/WebCore/Modules/webvr/VRDisplay.cpp:224
> +    auto event = VRDisplayEvent::create(eventName, makeRefPtr(this), WTFMove(reason));
> +    document()->eventLoop().queueTask(TaskSource::UserInteraction, *document(), [this, event = WTFMove(event)]() mutable {
> +        if (!document())
> +            return;
> +        if (auto* window = document()->domWindow())
> +            window->dispatchEvent(event);
> +    });

This should use the queueTaskKeepingObjectAlive() function. queueTaskToDispatchEvent() would be an option if the VRDisplay object was the target of these events, but it's not.

[Sergio Villar Senin]

Created attachment 382909 [details]
Patch

[Sergio Villar Senin]

(In reply to Zan Dobersek from comment #16)
> Comment on attachment 382827 [details]
> Dispatch events in DOMWindow
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382827&action=review
> 
> Inlining the comment into the patch ...
> 
> > Source/WebCore/Modules/webvr/VRDisplay.cpp:224
> > +    auto event = VRDisplayEvent::create(eventName, makeRefPtr(this), WTFMove(reason));
> > +    document()->eventLoop().queueTask(TaskSource::UserInteraction, *document(), [this, event = WTFMove(event)]() mutable {
> > +        if (!document())
> > +            return;
> > +        if (auto* window = document()->domWindow())
> > +            window->dispatchEvent(event);
> > +    });
> 
> This should use the queueTaskKeepingObjectAlive() function.
> queueTaskToDispatchEvent() would be an option if the VRDisplay object was
> the target of these events, but it's not.

Right. I guess in the end it's a matter of taste as it does not change essentially anything wrt eventLoop()->queueTask(), but I replaced it in the new patch version.

[Chris Dumez]

Comment on attachment 382909 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        No new tests were added as there is no testing machinery for WebVR so far. It's unclear how

:(

> Source/WebCore/Modules/webvr/VRDisplay.cpp:216
> +void VRDisplay::dispatchVRDisplayEventInEventLoop(const WTF::AtomString& eventName, Optional<VRDisplayEventReason>&& reason)

We likely don't need the WTF::

> Source/WebCore/Modules/webvr/VRDisplay.cpp:261
> +    stopPresenting();

As mentioned earlier, you likely want to stop presenting ONLY if ReasonForSuspension is ReasonForSuspension::BackForwardCache.

> Source/WebCore/Modules/webvr/VRDisplay.h:119
> +    void dispatchVRDisplayEventInEventLoop(const WTF::AtomString& eventName, Optional<VRDisplayEventReason>&&);

WTF:: likely unnecessary.

[Sergio Villar Senin]

Committed r252178: <https://trac.webkit.org/changeset/252178>

[Radar WebKit Bug Importer]

<rdar://problem/56974306>

[Chris Dumez]

Thanks for fixing!