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.

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

Allow session creation to be blocked on required features #423

Closed
toji opened this issue Oct 30, 2018 · 7 comments
Closed

Allow session creation to be blocked on required features #423

toji opened this issue Oct 30, 2018 · 7 comments
Assignees
Labels
fixed by pending PR A PR that is in review will resolve this issue. privacy-and-security Issues related to privacy and security
Milestone

Comments

@toji
Copy link
Member

toji commented Oct 30, 2018

We've previously talked about the need to have a way of doing basic feature detection before creating a session. This is needed because it's a poor user experience to launch into an immersive session, which may involve user-visible transitions and setup steps, only to be immediately told "Oops! Sorry, you can't actually do this." We must balance the desire to avoid that type of user inconvenience, however, with the need to suppress as much fingerprintable information as possible prior to session creation.

The proposed solution so far is to allow the user to pass in a set of "required features" when requesting the session. That way the UA can, prior to performing any user-visible actions, opaquely determine if the requirements can be met and reject early if not. Functionally, this would mean that sites with hard requirements beyond "Do you support AR or VR?" would still add a button to enter immersive mode to their page but upon clicking it the users could ~immediately be presented with a dialog from the page stating "Your system doesn't meet the requirements for this experience."

This can be done in a privacy sensitive way because:

  • Immersive session creation requires user activation, so features cannot be checked prior to user interaction.
  • The session rejection should NOT state which features of the list were not available, making it difficult to determine what was missing in a list of more than one requirement.
  • Because of the user activation requirement, session requests cannot be rapidly spammed with different combinations of features to eventually see what works and what doesn't.
  • The consequences of a successful request are high, in that it will incur user-visible side effects, and thus developers cannot do a "silent" feature check.

We've also discussed in the past that we want to encourage developers to be as responsive as possible to different systems with XR content, and this feature would seem to discourage that in favor of allowing developers to easily block off content to all but a small set of users, but I believe that it has some built-in deterrents:

  • The user experience for getting rejected when you click an "Enter XR" button, while better than if it started incurring full screen transitions and the like, is still relatively poor. Nobody likes to be teased with a feature and then told "Nope! Not for you!" As such, developers are likely to only use this as a gate if they know their experience simply cannot work without a given feature.
  • We can control the list of features that can be tested for, so let's maybe not add a requiresOculusRift enum, yeah?

This general concept has already been introduced in limited form to the spatial tracking explainer with the requiredFrameOfReferenceType dictionary argument. #417 is also highly related. I feel, however, like it should be handled using a more generic mechanism so that any type of requirement we deem necessary in the future (for example: Presence of 6DoF input?) can be tested the same way.

My suggestion is that we have developers pass in an array of requirements (given as enums) to requestSession, like so:

requestSession({
  mode: 'immersive-ar',
  requiredFeatures: ['x-ray-vision', 'levitation']
}).then(/* ... */);

(Example features are intentionally stupid so that nobody thinks I'm proposing anything.)

The benefit to this approach, rather than listing required features with a dictionary, is that there's no mechanism in Javascript to fail if an invalid dictionary entry is passed. (This is a feature, not a bug.) We can, however, enumerate over every entry in an array and reject if any individual item is unsupported or unrecognized. That means that even UAs that haven't been updated to recognize a specific feature can still reliably reject when it's listed as required.

@blairmacintyre
Copy link
Contributor

The benefit to this approach, rather than listing required features with a dictionary, is that there's no mechanism in Javascript to fail if an invalid dictionary entry is passed. (This is a feature, not a bug.) We can, however, enumerate over every entry in an array and reject if any individual item is unsupported or unrecognized. That means that even UAs that haven't been updated to recognize a specific feature can still reliably reject when it's listed as required.

I'm not sure I understand this argument. I can iterate over the keys in a dictionary, right? How is it any different to check x-ray-vision in an array element or x-ray-vision: true as a dictionary? For features with non-boolean values, where we might want to say lightEstimation: "directional", we can't do that with an array.

Having the dictionary be the same as the values passed in to requestSession in #424 seems much easier for programmers to understand.

@toji
Copy link
Member Author

toji commented Oct 31, 2018

Having the dictionary be the same as the values passed in to requestSession in #424 seems much easier for programmers to understand.

Definitely sympathize with this point, but it seems to me like there's technical limitations in play here.

I can only really speak to Chrome's implementation, but I suspect other browsers have similar implementations and I'd like to hear if that's the case or if this is a Chrome-only thing. From the native side when the Blink code receives a dictionary defined in WebIDL it's converted to a class that exposes the defined dictionary keys but has no mechanism for iterating over anything else that may have been passed beyond the keys defined in the IDL. (For example, check out the current XRSessionCreationOptions WebIDL and auto-generated native implementation)

(Okay, fine: I CAN think of at least one way to enumerate the dictionary keys from native code but it's awful and slow and definitely not how it was intended to work. I'm not gonna even try to get it past code review.)

In any case, that means Chrome doesn't have a (realistic) mechanism for checking to see if there are keys in a dictionary that are unexpected, which in turn means that I can't reject a session request that ostensibly requires a feature that Chrome hasn't added yet. For example, if Magic Leap wanted to do early experimentation with environment meshing in Helios, Chrome would happily appear to "support" a session requested with { environmentMeshingML: true }. If the requirements are given as an array, though, the native code CAN iterate through them and reject on values it doesn't recognize.

It does seem like some requirements could benefit from being more than an implied boolean, I'll agree there. I'm just trying to figure out how to make this as "pit-of-success-y" as possible.

One last note: The temptation to make requirements and permissions use the same set of values is super strong, and I'm not completely convinced it's wrong, but I'm currently inclined to say that the number of things we should allow developers to actively block on ought to be pretty small? It's been an ongoing theme in our group discussions that we don't want to encourage developers to block users unnecessarily, so I view this as a fairly drastic tool that should be of limited use.

@toji
Copy link
Member Author

toji commented Oct 31, 2018

Blair reached out to me privately and pointed out that my intended message got lost in the comment above, so I'll state it more concisely: Is the inability to enumerate dictionary keys in native code a Chrome-only limitation (in which case we'll just deal with it) or is that a common limit in other browser as well?

@jdm
Copy link

jdm commented Oct 31, 2018

It's the case in Servo, and I'm pretty sure it's the case in Gecko too.

@kearwood
Copy link
Contributor

I can confirm that Gecko has the same limitations regarding dictionary member iteration.

@NellWaliczek NellWaliczek removed the agenda Request discussion in the next telecon/FTF label Feb 13, 2019
@NellWaliczek NellWaliczek modified the milestones: May 2019, June 2019 May 24, 2019
@NellWaliczek NellWaliczek added the privacy-and-security Issues related to privacy and security label Jun 19, 2019
@NellWaliczek NellWaliczek self-assigned this Jun 24, 2019
@probot-label probot-label bot added the fixed by pending PR A PR that is in review will resolve this issue. label Jun 26, 2019
@probot-label
Copy link

probot-label bot commented Jun 26, 2019

This issue is fixed by PR #739

@NellWaliczek
Copy link
Member

Closed by #739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed by pending PR A PR that is in review will resolve this issue. privacy-and-security Issues related to privacy and security
Projects
None yet
Development

No branches or pull requests

6 participants