Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

requestSession() should set "pending immersive session" synchronously #643

Closed
ddorwin opened this issue May 14, 2019 · 4 comments · Fixed by #706
Closed

requestSession() should set "pending immersive session" synchronously #643

ddorwin opened this issue May 14, 2019 · 4 comments · Fixed by #706
Assignees
@Manishearth
Labels
fixed by pending PR A PR that is in review will resolve this issue. help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Milestone
June 2019

Comments

@ddorwin
Copy link
Contributor

ddorwin commented May 14, 2019

The requestSession() algorithm currently sets "pending immersive session" to true and false in the asynchronous ("in parallel") part of the algorithm.

I believe this makes it possible to have concurrent outstanding requests (i.e., if two calls are successive). Also, I'm not sure there is any inherent synchronization between runs of "steps in parallel." Thus, the "in parallel" part of the algorithm might actually run at the same time. I believe the right way to handle synchronization is on the main thread, which is the synchronous part of the algorithm.

Thus, "pending immersive session" should be checked and set to true in an early synchronous part of the algorithm, and it should be cleared inside a task queueing a task (to the main thread).
@ddorwin ddorwin mentioned this issue May 14, 2019
Closed
@ddorwin
Copy link
Contributor Author

ddorwin commented May 14, 2019

Addressing #644 will make it easier to address this issue.

@ddorwin
Copy link
Contributor Author

ddorwin commented May 14, 2019

Note that per https://www.w3.org/2001/tag/doc/promises-guide#shorthand-manipulating, "Resolve promise with session" in the "in parallel" steps is shorthand for, among other things, queueing a task. In this case, we probably need to make explicitly queue a task that includes resolving the promise. See, for example, step 10.10 of https://w3c.github.io/encrypted-media/#dom-mediakeysession-generaterequest.

@ddorwin
Copy link
Contributor Author

ddorwin commented May 14, 2019

"pending immersive session" should also be cleared before rejecting the promise. Since the "reject promise with..." steps actually queue a task, the promise will be rejected and the executor may run before "pending immersive session" is cleared. This could lead to unexpected behavior for an application that requests a session in the executor's reject function.

@ddorwin ddorwin mentioned this issue May 14, 2019
Closed
@cwilso cwilso added this to the June 2019 milestone May 17, 2019
@cwilso cwilso added ready for editing Design is decided, issue ready for Editor to fix editor triage Issues marked for triage/re-examination by the editors and removed editor triage Issues marked for triage/re-examination by the editors ready for editing Design is decided, issue ready for Editor to fix labels May 28, 2019
@NellWaliczek NellWaliczek added the help wanted This is a good issue for anyone to pick up and work on filing a PR for. label May 28, 2019
@NellWaliczek NellWaliczek modified the milestones: June 2019, July 2019 May 28, 2019
@Manishearth Manishearth self-assigned this Jun 14, 2019
@Manishearth Manishearth mentioned this issue Jun 14, 2019
Merged
@Manishearth Manishearth added the fixed by pending PR A PR that is in review will resolve this issue. label Jun 14, 2019
@Manishearth
Copy link
Contributor

#706

@toji toji closed this as completed in #706 Jun 17, 2019
@toji toji modified the milestones: July 2019, June 2019 Jun 17, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Manishearth Manishearth

Labels
fixed by pending PR A PR that is in review will resolve this issue. help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Projects
None yet
Milestone
June 2019
Development

Successfully merging a pull request may close this issue.

Move racy parts of requestSession() to the main thread Manishearth/webxr
5 participants
@cwilso @toji @Manishearth @ddorwin @NellWaliczek