Bug 220835 – Send the end XRSessionEvent after the platform-specific steps for session shutdown have completed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebXR

[Ada Chan]

Send the end XRSessionEvent after the platform-specific steps for session shutdown have completed.

Will need a way for the PlatformXR::Device to notify the WebXRSession when the platform has completed the session shutdown.

[Ada Chan]

rdar://72014363

[Ada Chan]

Created attachment 418099 [details]
Patch

[Dean Jackson]

Comment on attachment 418099 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.h:106
> +    void shutdown(InitiatedBySystem);

Should the parameter have a default value?

[Ada Chan]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 418099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418099&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRSession.h:106
> > +    void shutdown(InitiatedBySystem);
> 
> Should the parameter have a default value?

I don't think it's needed as it's a private method in WebXRSession and it's only called twice (once with each value). By leaving out the default value, it forces the caller to think about which value to use when calling it.

Do you think there's a good reason for a default value here?

[Ada Chan]

Created attachment 418209 [details]
Patch

[Ada Chan]

Some minor updates to the last version:
- Added [MayThrowException] to the end() method in WebXRSession.idl.
- A couple of comment changes

[Sam Weinig]

Comment on attachment 418209 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +

Any thoughts on how we could test this?

[Sergio Villar Senin]

Comment on attachment 418209 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +
> 
> Any thoughts on how we could test this?

Maybe using the WebFakeXRDevice we have in WebCore/testing?

I guess we should at least test the following situations
* supports shutdown notification with InitiatedBySystem::Yes and InitiatedBySystem::No
* *does not* support shutdown notification with InitiatedBySystem::Yes and InitiatedBySystem::No

> Source/WebCore/Modules/webxr/WebXRSession.cpp:391
> +        return Exception { InvalidStateError };

I don't remember something like this in the spec. I guess you should report an issue in the specs github along with a test for the WPT test suite.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:415
> +// MARK: - TrackingAndRenderingClient

What's this?

> Source/WebCore/platform/xr/PlatformXR.h:53
> +    // TODO: handle visibility changes

WebKit uses FIXME instead of TODO

> Source/WebCore/platform/xr/PlatformXR.h:77
> +    virtual bool willNotifySessionShutdown() const { return false; }

This is a bit of bikeshedding but willDoXXX() methods in Webkit normally are called before a DoXXX() method, i.e., are used to let clients set up themselves for a later call. In this case I think it's better to use something like supportsSessionShutdownNotification().

[Ada Chan]

(In reply to Sergio Villar Senin from comment #8)
> Comment on attachment 418209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418209&action=review
> 
> >> Source/WebCore/ChangeLog:16
> >> +
> > 
> > Any thoughts on how we could test this?
> 
> Maybe using the WebFakeXRDevice we have in WebCore/testing?
> 
> I guess we should at least test the following situations
> * supports shutdown notification with InitiatedBySystem::Yes and
> InitiatedBySystem::No
> * *does not* support shutdown notification with InitiatedBySystem::Yes and
> InitiatedBySystem::No

I'll take a look at the existing webxr layout tests to see how I can use WebFakeXRDevice to test these situations.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:391
> > +        return Exception { InvalidStateError };
> 
> I don't remember something like this in the spec. I guess you should report
> an issue in the specs github along with a test for the WPT test suite.

This is more of my attempt to simplify the handling of this situation. A more complicated alternative would be to save off the end promise (so we'll end up storing a list of these end promises) and resolve them when the session termination has completed.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:415
> > +// MARK: - TrackingAndRenderingClient
> 
> What's this?

It's a protocol that WebXRSession implements. PlatformXR::Device communicates session updates (such as when the session termination has completed) through this protocol.

> 
> > Source/WebCore/platform/xr/PlatformXR.h:53
> > +    // TODO: handle visibility changes
> 
> WebKit uses FIXME instead of TODO

OK, will fix.

> 
> > Source/WebCore/platform/xr/PlatformXR.h:77
> > +    virtual bool willNotifySessionShutdown() const { return false; }
> 
> This is a bit of bikeshedding but willDoXXX() methods in Webkit normally are
> called before a DoXXX() method, i.e., are used to let clients set up
> themselves for a later call. In this case I think it's better to use
> something like supportsSessionShutdownNotification().

That's a good idea. I'll switch to the method name you suggested.

[Ada Chan]

Created attachment 418451 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Ada Chan]

Created attachment 418478 [details]
Patch

[Ada Chan]

Added new tests under http/wpt/webxr.

[Chris Dumez]

(In reply to Ada Chan from comment #13)
> Added new tests under http/wpt/webxr.

Are these tests coming from upstream web-platform-tests (wpt) or did you write them? If they are imported from upstream wpt, they need to go under LayoutTests/imported/w3c/web-platform-tests/. If you wrote them, you put them in the right place already.

[Ada Chan]

(In reply to Chris Dumez from comment #14)
> (In reply to Ada Chan from comment #13)
> > Added new tests under http/wpt/webxr.
> 
> Are these tests coming from upstream web-platform-tests (wpt) or did you
> write them? If they are imported from upstream wpt, they need to go under
> LayoutTests/imported/w3c/web-platform-tests/. If you wrote them, you put
> them in the right place already.

Yes, I wrote these tests, not from upstream wpt.

[Chris Dumez]

Comment on attachment 418478 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:61
> +    m_device->setTrackingAndRenderingClient(makeWeakPtr((PlatformXR::TrackingAndRenderingClient*)this));

Please don't use C-casts (static_cast<> is preferred). Also, why is this cast needed?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:323
> +    Ref<WebXRSession> protectedThis(*this);

I think we like this pattern more nowadays:
auto protectedThis = makeRef(*this);

> Source/WebCore/Modules/webxr/WebXRSession.cpp:390
> +    if (m_ended)

I am looking at the spec [1] but I don't see this behavior specified anyway. Why should we throw in this case?

[1] https://immersive-web.github.io/webxr/#dom-xrsession-end

> Source/WebCore/Modules/webxr/WebXRSession.cpp:391
> +        return Exception { InvalidStateError };

Please provide a proper error message as second parameter.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:400
> +    m_endPromise = WTF::makeUnique<EndPromise>(WTFMove(promise));

WTF:: should not me needed.

Also why the makeUnique? Cannot we just do m_endPromise = WTFMove(promise) and have the data member use Optional<EndPromise>?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:415
> +// MARK: - TrackingAndRenderingClient

Not sure what that is. We don't do this in WebKit usually :)

> Source/WebCore/Modules/webxr/WebXRSession.h:84
>  

You will likely need something like this to avoid your earlier cast:
    using EventTargetWithInlineData::weakPtrFactory;
    using WeakValueType = EventTargetWithInlineData::WeakValueType;

> Source/WebCore/Modules/webxr/WebXRSession.h:105
> +    enum class InitiatedBySystem { No, Yes };

enum class InitiatedBySystem : bool { No, Yes };

> Source/WebCore/platform/xr/PlatformXR.h:73
> +    void setTrackingAndRenderingClient(WeakPtr<TrackingAndRenderingClient>&& client) { m_trackingAndRenderingClient = client; }

= WTFMove(client)

[Ada Chan]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 418478 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418478&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:61
> > +    m_device->setTrackingAndRenderingClient(makeWeakPtr((PlatformXR::TrackingAndRenderingClient*)this));
> 
> Please don't use C-casts (static_cast<> is preferred). Also, why is this
> cast needed?

Thanks for your later suggestion, I don't need this cast anymore.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:323
> > +    Ref<WebXRSession> protectedThis(*this);
> 
> I think we like this pattern more nowadays:
> auto protectedThis = makeRef(*this);

Got it!

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:390
> > +    if (m_ended)
> 
> I am looking at the spec [1] but I don't see this behavior specified anyway.
> Why should we throw in this case?
> 
> [1] https://immersive-web.github.io/webxr/#dom-xrsession-end

It simplifies the handling of this scenario, and turns out this matches Chromium's behavior too.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:391
> > +        return Exception { InvalidStateError };
> 
> Please provide a proper error message as second parameter.

Done.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:400
> > +    m_endPromise = WTF::makeUnique<EndPromise>(WTFMove(promise));
> 
> WTF:: should not me needed.

Removed.

> 
> Also why the makeUnique? Cannot we just do m_endPromise = WTFMove(promise)
> and have the data member use Optional<EndPromise>?

Yep, I've followed your suggestion.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:415
> > +// MARK: - TrackingAndRenderingClient
> 
> Not sure what that is. We don't do this in WebKit usually :)

Oh I see. It was the strangeness of the comment! I'll remove it.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.h:84
> >  
> 
> You will likely need something like this to avoid your earlier cast:
>     using EventTargetWithInlineData::weakPtrFactory;
>     using WeakValueType = EventTargetWithInlineData::WeakValueType;
> 
> > Source/WebCore/Modules/webxr/WebXRSession.h:105
> > +    enum class InitiatedBySystem { No, Yes };
> 
> enum class InitiatedBySystem : bool { No, Yes };

Fixed.

> 
> > Source/WebCore/platform/xr/PlatformXR.h:73
> > +    void setTrackingAndRenderingClient(WeakPtr<TrackingAndRenderingClient>&& client) { m_trackingAndRenderingClient = client; }
> 
> = WTFMove(client)

Fixed.

Thanks for the feedback!

[Ada Chan]

Created attachment 418489 [details]
Patch

[Chris Dumez]

Comment on attachment 418489 [details]
Patch

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

r=me with changes

> Source/WebCore/Modules/webxr/WebXRSession.cpp:391
> +        return Exception { InvalidStateError, "Cannot end a session more than once" };

return Exception { InvalidStateError, "Cannot end a session more than once"_s };

is slightly more efficient since the second parameter is a String.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:400
> +    m_endPromise = WTFMove(promise);

I would suggest adding the following assertion right before the assignment, just to make sure we never overwrite an existing promise (which would then never get resolved):
ASSERT(!m_endPromise);

> Source/WebCore/Modules/webxr/WebXRSession.h:89
> +    void sessionDidEnd() final;

Does this really need to be public?

> LayoutTests/platform/mac/TestExpectations:2261
> +# --- Start WebXR tests --- #

I would just use:
# WebXR tests

> LayoutTests/platform/mac/TestExpectations:2263
> +http/wpt/webxr/xrSession_end_device_reports_shutdown.https.html [ Pass ]

You likely want to [ Skip ] http/wpt/webxr in LayoutTests/TestExpectations, like we do for imported/w3c/web-platform-tests/webxr already. If the tests are not skipped globally, then it makes no sense to enabled them for specific platforms.

> LayoutTests/platform/mac/TestExpectations:2266
> +# --- End WebXR tests --- #

I don't think we need this end comment.

[Chris Dumez]

Comment on attachment 418489 [details]
Patch

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

> LayoutTests/platform/mac/TestExpectations:2264
> +http/wpt/webxr/xrSession_ended_by_system.https.html [ Pass ]

Do you foresee that we'll add tests to http/wpt/webxr that don't pass on macOS? If not, I'd suggest marking the whole folder as [ Pass ]:
http/wpt/webxr [ Pass ]

[Ada Chan]

(In reply to Chris Dumez from comment #20)
> Comment on attachment 418489 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418489&action=review
> 
> > LayoutTests/platform/mac/TestExpectations:2264
> > +http/wpt/webxr/xrSession_ended_by_system.https.html [ Pass ]
> 
> Do you foresee that we'll add tests to http/wpt/webxr that don't pass on
> macOS? If not, I'd suggest marking the whole folder as [ Pass ]:
> http/wpt/webxr [ Pass ]

That's a good point. I'll change it.

[Ada Chan]

(In reply to Chris Dumez from comment #19)
> Comment on attachment 418489 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418489&action=review
> 
> r=me with changes
> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:391
> > +        return Exception { InvalidStateError, "Cannot end a session more than once" };
> 
> return Exception { InvalidStateError, "Cannot end a session more than
> once"_s };
> 
> is slightly more efficient since the second parameter is a String.

Fixed.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:400
> > +    m_endPromise = WTFMove(promise);
> 
> I would suggest adding the following assertion right before the assignment,
> just to make sure we never overwrite an existing promise (which would then
> never get resolved):
> ASSERT(!m_endPromise);

Good idea. Fixed.

> 
> > Source/WebCore/Modules/webxr/WebXRSession.h:89
> > +    void sessionDidEnd() final;
> 
> Does this really need to be public?

Nope, moved it to private.

> 
> > LayoutTests/platform/mac/TestExpectations:2261
> > +# --- Start WebXR tests --- #
> 
> I would just use:
> # WebXR tests

Fixed.

> 
> > LayoutTests/platform/mac/TestExpectations:2263
> > +http/wpt/webxr/xrSession_end_device_reports_shutdown.https.html [ Pass ]
> 
> You likely want to [ Skip ] http/wpt/webxr in LayoutTests/TestExpectations,
> like we do for imported/w3c/web-platform-tests/webxr already. If the tests
> are not skipped globally, then it makes no sense to enabled them for
> specific platforms.

I see. I added the skip in LayoutTests/TestExpectations.

> 
> > LayoutTests/platform/mac/TestExpectations:2266
> > +# --- End WebXR tests --- #
> 
> I don't think we need this end comment.

OK, fixed.

[Ada Chan]

Created attachment 418493 [details]
Patch

[Sergio Villar Senin]

A couple of comments about the tests. You're adding a lot of resource files from WPT that are already in LayoutTests/imported/w3c/web-platform-tests/webxr/resources. I don't see the point of importing them in a different directory.

Secondly, some of them might indeed be non exportable to WPT as you're testing a WebKit implementation detail but http/ does not seem the best place to me to put those on.

Last but not least, I still believe that if we want to throw exceptions in the end() method then we must send a PR to the specs and the corresponding tests to the WPT test suite. The fact that Chrome does it reinforces that idea.

BTW would you mind adding those tests also to the WPE TestExpectations file?

[Chris Dumez]

(In reply to Sergio Villar Senin from comment #24)
> A couple of comments about the tests. You're adding a lot of resource files
> from WPT that are already in
> LayoutTests/imported/w3c/web-platform-tests/webxr/resources. I don't see the
> point of importing them in a different directory.

It is true that tests in http/wpt/ have access to resources in LayoutTests/imported/w3c/web-platform-tests/webxr/ so there is no need for duplication. IIRC, when the WPT server runs, files under http/wpt/ are exposed under imported/w3c/web-platform-tests/webkit/

> 
> Secondly, some of them might indeed be non exportable to WPT as you're
> testing a WebKit implementation detail but http/ does not seem the best
> place to me to put those on.

I personally don't see an issue with putting browser-specific tests under http/tests/wpt as long as they use the WPT infrastructure.

> 
> Last but not least, I still believe that if we want to throw exceptions in
> the end() method then we must send a PR to the specs and the corresponding
> tests to the WPT test suite. The fact that Chrome does it reinforces that
> idea.

Maybe filing an issue against the spec on GitHub at least?

> 
> BTW would you mind adding those tests also to the WPE TestExpectations file?

[Ada Chan]

(In reply to Chris Dumez from comment #25)
> (In reply to Sergio Villar Senin from comment #24)
> > A couple of comments about the tests. You're adding a lot of resource files
> > from WPT that are already in
> > LayoutTests/imported/w3c/web-platform-tests/webxr/resources. I don't see the
> > point of importing them in a different directory.
> 
> It is true that tests in http/wpt/ have access to resources in
> LayoutTests/imported/w3c/web-platform-tests/webxr/ so there is no need for
> duplication. IIRC, when the WPT server runs, files under http/wpt/ are
> exposed under imported/w3c/web-platform-tests/webkit/

I have removed the files in resources and updated the paths to those files in the tests.

> 
> > 
> > Secondly, some of them might indeed be non exportable to WPT as you're
> > testing a WebKit implementation detail but http/ does not seem the best
> > place to me to put those on.
> 
> I personally don't see an issue with putting browser-specific tests under
> http/tests/wpt as long as they use the WPT infrastructure.
> 
> > 
> > Last but not least, I still believe that if we want to throw exceptions in
> > the end() method then we must send a PR to the specs and the corresponding
> > tests to the WPT test suite. The fact that Chrome does it reinforces that
> > idea.
> 
> Maybe filing an issue against the spec on GitHub at least?

I'll look into it.

> 
> > 
> > BTW would you mind adding those tests also to the WPE TestExpectations file?

I'll add them to wpe/TestExpectations.

[Ada Chan]

Created attachment 418567 [details]
Patch

[Ada Chan]

I've filed an issue against the spec regarding XRSession.end():
https://github.com/immersive-web/webxr/issues/1166

[EWS]

Committed r271988: <https://trac.webkit.org/changeset/271988>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418567 [details].

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/271988/webkit

introduced 3 constantly failing tests:

http/wpt/webxr/xrSession_end_device_reports_shutdown.https.html
http/wpt/webxr/xrSession_reject_multiple_end.https.html
http/wpt/webxr/xrSession_ended_by_system.https.html 

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=http%2Fwpt%2Fwebxr%2FxrSession_end_device_reports_shutdown.https.html&test=http%2Fwpt%2Fwebxr%2FxrSession_ended_by_system.https.html&test=http%2Fwpt%2Fwebxr%2FxrSession_reject_multiple_end.https.html

This did not fail on EWS because these tests only run in Big Sur

[Ada Chan]

Thanks Truitt!

I've filed: https://bugs.webkit.org/show_bug.cgi?id=221092