New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 727969 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug
Proj-XR


Show other hotlists

Hotlists containing this issue:
VR-Automated-Tests


Sign in to add a comment

WebVR: getVRDisplays() promise never resolves when origin trial activated and VR services not installed

Project Member Reported by jsant...@google.com, May 31 2017

Issue description

Steps to reproduce the problem:
1. Navigate to https://webvr.info/samples/03-vr-presentation.html
2. With devtools, execute `navigator.getVRDevices().then(console.log)`
3. Notice it never resolves.

What is the expected behavior?
For the promise to resolve to an empty array.

What went wrong?
The promise never resolves, and I do *not* have Google VR Services installed, so it seems like a regression of  bug 693298  [0]. This is made worse as when activating the polyfill [1] version of the page, since `getVRDisplays` never resolves, the polyfill can't work its magic either, so the user is stuck with an unresponsive experience.

I was also able to reproduce this bug in Chrome Canary 61.0.3115.0

[0] https://bugs.chromium.org/p/chromium/issues/detail?id=693298
[1] https://webvr.info/samples/03-vr-presentation.html?polyfill=1

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 58.03029.83  Channel: stable
OS Version: 6.0.1
Flash Version: 

https://bugs.chromium.org/p/chromium/issues/detail?id=678283
 
Cc: amp@chromium.org
Starting with M58, you should see a prompt to install VR Services when WebVR is used and they are not installed ( issue 670166 ). Do you not see such a prompt? If not, try scrolling to the top of the page.

Comment 2 by amp@chromium.org, May 31 2017

Cc: -amp@chromium.org
Owner: amp@chromium.org
Status: Started (was: Unconfirmed)
This should be unrelated to the prompt.  The fix I put in for  issue 693298  was the last thing we merged back to 58 and so if this is broken in 58 it's for a different reason.

It looks like my fix was reverted (I'm not sure if it was unintentional or trying to resolve something else) in 59 and forward so I'll take a look and see what might be causing it there.

I'm not sure we are going to be able to merge anything back to 59 much less 58 at this point.

Comment 3 by jsant...@google.com, May 31 2017

I do see the prompt for the Google VR Services to install, sorry for not mentioning that! But I did not install it, and would imagine there are other users doing the same when browsing the web. Does  bug 693298  not include not installing the services, only if out of date/unavailable? Unfortunately, unless getVRDisplays() returns an empty array, this results in a much worse experience on Chrome than other mobile browsers

Comment 4 by jsant...@google.com, May 31 2017

Understandably difficult for fixing M58/59 -- getting this fixed for future versions would still have a big impact. Thanks for checking this out!

Comment 5 by amp@chromium.org, May 31 2017

My initial theory was that this was broken by https://codereview.chromium.org/2727873002 (lazy initialization of GVR).  But after further looking at the code it looks like we are properly covering our bases (ie if there is no version of VR Services available then it shouldn't hit those code paths).  It's possible that if you have an old version of VR Services available (ie a 6P where the stock version is there, but not compatible) that this becomes an issue.  I need to debug further (probalby with a full debug build and gdb instead of just looking through the code manually, but I don't have a rooted device that doesn't have VR services already up to date in the OS release.

FYI Code reference, CreateVRDisplayInfo has the callback that must be called for the promise to resolve.  It looks like if the library never loads then the callback never runs:
https://cs.chromium.org/chromium/src/device/vr/android/gvr/gvr_device.cc?l=27


Comment 6 by amp@chromium.org, May 31 2017

Some clarification on the behavior of what is observed with this bug.

On visiting the site the gl canvas is loaded and animated as expected.  The canvas is not responsive to any device movement though (no magic window) as the VR services library is not loaded (so GVR can't give poses) and the polyfill isn't loaded because the promise doesn't resolve (so no device orientation events from the polyfill).

In theory the site could ignore the promise (time out or something) and still oad the polyfill to get back to a responsive site (and show an enter vr button for polyfill binocular version).
Cc: joshcarpenter@chromium.org bajones@chromium.org meganlindsay@chromium.org
Labels: M-60 Proj-VR
amp@ says the promise resolution is NOT currently tied to the prompt. Thus, we should be returning 0 devices. The fact that we do not is a bug that needs to be fixed. The following is a more general discussion of what should happen assuming this specific bug is fixed.

At a high level:
We might not know what value to return in the promise until the user has taken action. Similarly, implementations _could_ prompt the user for permission or download a VR component. Thus, apps should handle the unresolved promise. One way to handle this would be for the polyfill to time out or tell the user to accept/dismiss any prompts.

Looking at the details of the prompt, there are three cases:

1. The user clicks the action button in the prompt and is taken to the Play store and away from Chrome. In the happy case, the user installs/updates GVR and returns to Chrome. Chrome could either have responded to the click by returning 0 devices or poll GVR and resolve the promise once it's installed/updated. In the former case, the user would need to reload the page.
Note: The bug prevents either of these from happening, and thus the promise is not resolved.

2. The user dismisses the dialog ('x'). In this case we can return 0 devices immediately, and the app will work as expected.
Note: The bug prevents this from happening, and thus the promise is not resolved.

3. The user ignores or does not see the dialog. In this case, Chrome is still waiting for the user to take action. (This is also the case for #1 where the user does not successfully install/update GVR, even if it is an installation failure.) Thus, the promise remains unresolved.

Considerations:
* Polling in #1 provides the most seamless experience for the happy case.
  - If we don't poll, we should tell the user to refresh the page after installing/updating.
* For users that ignore/don't see the prompt, immediately returning 0 devices will allow the user to have _an_ experience with the polyfill.
  - However, doing so prevents the seamless experience and users may not know they are getting a degraded polyfill experience. For the latter, it may be best to break the experience, though it's unfortunate that the application doesn't have insight into what's happening. 
* We currently prompt all users to install GVR even if they do not have a VR device. Thus, we may be more likely to encounter #2 and #3. ( Issue 695937  could help address this.)


It appears that the current intended behavior is to return 0 devices immediately in all cases, and there is a bug preventing this. It's not clear that this is the best long-term solution.

Comment 8 by amp@chromium.org, May 31 2017

I think we could do much better in handling this (tieing in the user interaction to the promise), but that also involves more complex wiring and could be error prone.  For now I think we should get the list returning 0 if there are no libraries (resolving the promise and this bug) and file a new bug for getting that functionality to dynamically respond to user interaction and libraries being loaded when they weren't before.

Comment 9 by amp@chromium.org, May 31 2017

Cc: mthiesse@chromium.org
After looking through this more I'm getting confused by all the interactions.  I think we are missing a callback through mojo in the case where we didn't initialize the vr display and that results in the promise hanging, but I can't figure out exactly where things should be fixed.

Brain dumping what I've found so far to gather my thoughts and maybe someone else can make better sense of what is going on here.  (+mthiesse for that).


General flow through the bug:
1. Site requests navigator.GetVRDisplays()
2. That eventually filters down into through to the VRController: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vr/VRController.cpp?l=36
  a. This is where the promise is created.
  b. A callback is also created which can resolve the promise when all displays are synced: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vr/VRController.cpp?l=31
  c. but that is called with the wrong value (more on this later)
  d. eventually the call makes it way to https://cs.chromium.org/chromium/src/device/vr/vr_device_manager.cc?l=86, This is when the VRServiceImpl (the browser side of the mojom VR interface which the navigator interacts with) registers itself with the VR device manager.
3. The GVR delegate provider initializes VrShellDelegate and attempts to load the libraries.
  a. I thought things would fail here, but apparently it all works fine at this level even though the gvr api can not be loaded.
  b. The java parts are confusing to me here and this is where I thought mthiesse could shed some light on what should be happening.
4. The GVR device provider preemptively creates a device and adds it to the list when it is queried for devices (this is where my previous fix used to check for initialization before, but that won't work now because of 3 a): https://cs.chromium.org/chromium/src/device/vr/android/gvr/gvr_device_provider.cc?l=25
5. The device manager then let's the VRService try to connect all the devices that were returned: https://cs.chromium.org/chromium/src/device/vr/vr_device_manager.cc?l=98
and the VRService attempts to create VRDisplayInfo from them
https://cs.chromium.org/chromium/src/device/vr/vr_service_impl.cc?l=52
6. If that is successful a callback comes back to the VRService and checks if it was succesful or not.
  a. I think this is where things break down.
  b. in this bugs case we end up with a non_presenting_delegate_provider which properly returns null because there is no gvr api. https://cs.chromium.org/chromium/src/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc?dr=CSs&l=150
  c. I think this is the first point at which we could know the libraries are missing (but I'm not sure)
7. The VRService properly cuts off at this point and doesn't actually create a VRDisplay: https://cs.chromium.org/chromium/src/device/vr/vr_service_impl.cc?l=77
  a. However, it doesn't make a callback as it would if a display was created a few lines below that
8. The VRService finishes registering with the vr device manager and then sets the client for future interaction:
https://cs.chromium.org/chromium/src/device/vr/vr_service_impl.cc?l=44
  a. Note that the callback is sent the number of devices which were just pulled from the device providers (in this case 1 which will never actually be a display because there are no libraries)
9. The VRController never gets a call to OnGetDisplays which would resolve the promise: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vr/VRController.cpp?l=91 as it thinks there is still an outstanding device.

So if we never add a device I think we would be good (but we won't know if the libraries are available early enough to prevent this), or if we properly call back through mojo that there are no displays available then it should resolve the promise (but this might require some new mojo wiring or some other magic I'm not familiar with).

Does this make sense to anyone else?

On the polyfill end, there's a patch up[0] to detect this bug by polyfilling if `getVRDisplays()` does not resolve within 1000ms which should fix the issue for those using the polyfill, but only on the latest version. Hoping we can get this in the next Aframe release atleast for further reach.

[0] https://github.com/googlevr/webvr-polyfill/pull/248

Comment 11 by amp@chromium.org, Jun 1 2017

The interactions between the renderer and browser for WebVR is in need of some reworking, but that will be more fitting to happen as part of enabling multi-platfrom support and the re-architecture of Chromes VR implementation.

For now I've uploaded a change that tries to keep the state consistent between the two.  The comments on the renderer side imply that the call of number of displays synced only happens after all devices are connected, but we had been returning it even before attempting to connect (well likely at the same time).

https://codereview.chromium.org/2915993004
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a7288d743d475fd4f1a903a1d3d9503ba282792

commit 7a7288d743d475fd4f1a903a1d3d9503ba282792
Author: amp <amp@chromium.org>
Date: Fri Jun 02 01:34:11 2017

[WebVR] Only count devices that have successfully connected.

This allows the navigator.GetVRDisplays() promise to resolve when the
underlying VR libraries are not installed or out of date.

BUG= 727969 

Review-Url: https://codereview.chromium.org/2915993004
Cr-Commit-Position: refs/heads/master@{#476516}

[modify] https://crrev.com/7a7288d743d475fd4f1a903a1d3d9503ba282792/device/vr/vr_service_impl.cc
[modify] https://crrev.com/7a7288d743d475fd4f1a903a1d3d9503ba282792/device/vr/vr_service_impl.h

Comment 13 by amp@chromium.org, Jun 2 2017

Once this bakes on canary for a day or two I'll request a merge to 60.  It just missed the canary for Jun 2 so it will be available in next weeks release.

Comment 14 by amp@chromium.org, Jun 2 2017

The change that should get tests to catch future regression went in with https://chromium-review.googlesource.com/522210

Comment 15 by amp@chromium.org, Jun 6 2017

Labels: -Pri-2 Pri-1
I filed 729857 to track the possible behavior change discussed in comment 7.

Comment 17 by amp@chromium.org, Jun 6 2017

Labels: Merge-Request-60
Verified on Canary 61.0.3122.0, requesting merge to 60.
Project Member

Comment 18 by sheriffbot@chromium.org, Jun 6 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 19 by bugdroid1@chromium.org, Jun 6 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4d2ab94fc1760c766538b48abaad800dbc9b575

commit b4d2ab94fc1760c766538b48abaad800dbc9b575
Author: amp <amp@chromium.org>
Date: Tue Jun 06 16:17:27 2017

[WebVR] Only count devices that have successfully connected.

This allows the navigator.GetVRDisplays() promise to resolve when the
underlying VR libraries are not installed or out of date.

BUG= 727969 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2915993004
Cr-Original-Commit-Position: refs/heads/master@{#476516}
Review-Url: https://codereview.chromium.org/2920223007
Cr-Commit-Position: refs/branch-heads/3112@{#185}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/b4d2ab94fc1760c766538b48abaad800dbc9b575/device/vr/vr_service_impl.cc
[modify] https://crrev.com/b4d2ab94fc1760c766538b48abaad800dbc9b575/device/vr/vr_service_impl.h

Comment 20 by amp@chromium.org, Jun 6 2017

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Verified in Chrome Canary 61.0.3123.0
Components: Blink>WebXR

Sign in to add a comment