WebXR: refactor session shutdown handling #20051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Nov 1, 2019
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1874824
branch
4 times, most recently
from
0b7ebdf
to
557f47e
Compare
While investigating a flaky test, one of the issues found was that a session 'end' event is triggered directly after issuing the session.end() call, without waiting for device-side session teardown to proceed. As a result, requesting a new session could fail due to the device side thinking there's already an active immersive session. According to https://immersive-web.github.io/webxr/#ended and discussions with Brandon, expected behavior is that the 'end' event and end().then promise resolution should be delayed if needed to ensure that a new session can be started at that time. This CL adds a "ended but not yet shut down" state to XRSession, and adds a new ShutdownSession call to isolated_xr_service's XRRuntime, to enable the expected delay. In case of mojo communication errors, the shutdown happens immediately. Also update WebXrTestFramework's enterSessionWithUserGesture to detect and retry clicks that aren't delivered after session end transitions, and remove sleeps that were previously added to work around flakiness in WebXR VR consent tests. It appears that this flakiness was at least in part caused by prematurely starting a new session while the previous session wasn't fully shut down yet. The test change is included in the same CL since the new shutdown logic by itself left some tests flaky due to clicks not being delivered consistently from the test framework. Change-Id: I6d48b259677c92bac323db0e10803a48718d4a33 Bug: 1014159, 998307 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1874824 Commit-Queue: Klaus Weidner <klausw@chromium.org> Reviewed-by: Dominick Ng <dominickn@chromium.org> Reviewed-by: Alexander Cooper <alcooper@chromium.org> Reviewed-by: Brian Sheedy <bsheedy@chromium.org> Cr-Commit-Position: refs/heads/master@{#712391}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1874824
branch
from
557f47e
to
62efcf7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While investigating a flaky test, one of the issues found was that a session
'end' event is triggered directly after issuing the session.end() call, without
waiting for device-side session teardown to proceed. As a result, requesting a
new session could fail due to the device side thinking there's already an
active immersive session.
According to https://immersive-web.github.io/webxr/#ended and discussions with
Brandon, expected behavior is that the 'end' event and end().then promise
resolution should be delayed if needed to ensure that a new session can
be started at that time.
This CL adds a "ended but not yet shut down" state to XRSession, and adds
a new ShutdownSession call to isolated_xr_service's XRRuntime, to enable
the expected delay. In case of mojo communication errors, the shutdown
happens immediately.
Also update WebXrTestFramework's enterSessionWithUserGesture to detect
and retry clicks that aren't delivered after session end transitions, and
remove sleeps that were previously added to work around flakiness in WebXR
VR consent tests. It appears that this flakiness was at least in part caused
by prematurely starting a new session while the previous session wasn't
fully shut down yet. The test change is included in the same CL since
the new shutdown logic by itself left some tests flaky due to clicks not being
delivered consistently from the test framework.
Change-Id: I6d48b259677c92bac323db0e10803a48718d4a33
Bug: 1014159, 998307
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1874824
Commit-Queue: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712391}