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

Issue 719013 link

Starred by 4 users

Issue metadata

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



Sign in to add a comment

VRDisplay.requestPresent fails on Samsung S8+

Reported by mckay.da...@gmail.com, May 5 2017

Issue description

Steps to reproduce the problem:
1. On a Samsung Galaxy S8+, visit https://webvr.info/samples/03-vr-presentation.html
2. Press the 'Enter VR' button to trigger the call to vrDevice.requestPresent on line 145 of the html file

What is the expected behavior?
The requestPresent call succeeds and Chrome successfully enters WebVR presentation mode.

The webgl canvas fills the entire screen with left and right eye views of the scene.

What went wrong?
The error callback of the requestPresent promise is returned instead with the following error:

DOMException: Presentation request was denied. {code: 0, name: "NotAllowedError", message: "Presentation request was denied."

Furthermore, Chrome is now `stuck` in landscape mode and cannot be re-oriented to portrait mode.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 58.0.3029.83  Channel: stable
OS Version: 7.0.0; SM-G955U Build/NRD90M
Flash Version: 

The same Chrome build & steps work correctly on a Galaxy S5.

Although the origin-trail token on webvr.info enables WebVR, I've also manually enabled the chrome://flags#enable-vr flag to enabled.

Prior to the requestPresent call, navigator.getVRDisplays is returning a single VRDisplay w/ displayName: "Google, Inc. Default Cardboard"
 
I've also verified the same behavior is present in these builds:

Beta version: 59.0.3071.36
Dev version: 60.0.3087.3
Canary version: 60.0.3089.0
Cc: klausw@chromium.org mthiesse@chromium.org
Labels: -Type-Bug Proj-VR-Daydream VR-Cardboard Type-Bug-Regression
Status: Available (was: Unconfirmed)
Confirmed on an S8 with Beta 59.0.3071.36. (I had to enable the flag due to the OT issue.)
If I refresh the page and then hit Enter VR, I get a red box with the message "ERROR: requestPresent failed. Presentation request was denied."

It works on an S8 on (out of date) Stable 57.0.2987.132.

It appears we have a regression from 57 to 58.

Google VR Services 1.5.153389785
Labels: M-60
Owner: amp@chromium.org
Status: Assigned (was: Available)
It works in Canary 60.0.3089.0 on a 5X
Cc: jdduke@chromium.org
Can you include a bug report or logs?

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

Owner: klausw@chromium.org
Assigning to klausw@ as I wasn't able to get the s8 connecting to my machine.
I've attached a bugreport. Some random observations:

ChromeTabbedActivity and WindowManager seem to be stuck in an update loop, with multiple log messages per second like these:

05-05 16:34:07.116 13494 13494 D ViewRootImpl@3f2bb9[ChromeTabbedActivity]: Relayout returned: oldFrame=[0,0][2960,1440] newFrame=[0,0][2960,1440] result=0x1 surface={isValid=true -552710144} surfaceGenerationChanged=false
05-05 16:34:07.125  1173  2747 V WindowManager: Relayout Window{ed7dcadd0 u0 com.chrome.beta/org.chromium.chrome.browser.ChromeTabbedActivity}: viewVisibility=0 req=2960x1440 WM.LayoutParams{(0,0)(fillxfill) sim=#13 ty=1 fl=#1810180 pfl=0x20000 fmt=-3 wanim=0x1030465 vsysu
i=0x1716 sysuil=true needsMenuKey=2 naviIconColor=0}
05-05 16:34:07.132 13494 13494 D ViewRootImpl@3f2bb9[ChromeTabbedActivity]: Relayout returned: oldFrame=[0,0][2960,1440] newFrame=[0,0][2960,1440] result=0x1 surface={isValid=true -552710144} surfaceGenerationChanged=false
05-05 16:34:07.142  1173  2787 V WindowManager: Relayout Window{ed7dcadd0 u0 com.chrome.beta/org.chromium.chrome.browser.ChromeTabbedActivity}: viewVisibility=0 req=2960x1440 WM.LayoutParams{(0,0)(fillxfill) sim=#13 ty=1 fl=#1810180 pfl=0x20000 fmt=-3 wanim=0x1030465 vsysu
i=0x1716 sysuil=true needsMenuKey=2 naviIconColor=0}

05-05 16:48:30.174  1186  1186 I Ion     : [vr/gvr/capi/src/gvr_core_api_loader_android.cc:174] Successfully loaded GVR library version 1.40.0 from VrCore (target was 1.10.0).
05-05 16:48:30.222  1186  1186 I GVR     : [vr/gvr/capi/src/gvr.cc:96] Initialized GVR version 1.40.0

Also, Javascript seems to be in a crazy multithreaded mode where it keeps animating while allegedly paused in magic window mode, I filed the separate  issue 719101  for that one.
bug-719013-nopresent-b.zip
4.0 MB Download

Comment 7 by jdduke@google.com, May 6 2017

Is Android VR mode being enabled on the S8? You might try not enabling VR mode to see if that makes any difference (either remove the "enableVrMode" manifest entry for your Activity, or don't call Activity.setVrModeEnabled()). Samsung might be doing something behind the scenes which is causing the thrashing.
Correction: #2-#6 were using an S8+.
I see same behavior on S8 (& S8+) as well.
Thru Canary: 60.0.3094.0.

Comment 10 by amp@chromium.org, May 12 2017

Attaching bug report.  I initially thought this was related to the Do not disturb and notification access permissions, which are not granted by default on Samsung devices.  I thought I had seen everything working once I enabled those permissions, but now even with those permissions granted to vr services the issue still repro's on recent versions of Chrome.

It does not repro on Chrome 57 though.  One of the changes that occured around that timeframe was treating the vr device as a virtual display instead of manually overriding sizes directly.  That might have different behavior on a samsung device than on a pixel, but it's only a theory.

I was able to take a bug report of using Chrome canary (see attached).

Some notes as I was trying to look into the permissions:
vr services 1.5.153389785

chrome version 57.0.2987.132
- prompts for permissions about vr core (after first install only).
- works fine in cardboard mode without permissions

chrome 59.0.3071.36
- does not prompt for permissions
- does not transition to presentation (goes into a weird landscape mode and apparently has multiple rAFs going, see  issue 719101 )
- You can add permissions directly (from Settings > Apps > Menu (3 dots) > Special Access > [Do not disturb permission, Notification access] > Toggle and Allow Google Vr Services), but it does not get it working again.

bugreport-2017-05-12-15-37-09.zip
4.2 MB Download
Might be onto something there w.r.t. display handling.. On a S8-proto device I have that seemingly handles the display resolution a bit differently than production devices works normally, entering VR Cardboard successfully.
Stable: 58.0.3029.83 
Canary: 60.0.3097.0
VR services 1.5.153389785

Note that with S8/+ Samsung introduced new screen resolution & aspect ratio, with special handling of reported resolutions to apps (See 'Full screen apps' in settings) which supposed to be transparent but could potentially have side effects in certain special use cases.

(Also though, I swear I observed this landscape+non-present behavior right after got Nougat update for GS7 a couple days ago.. but can no longer reproduce.)

Comment 12 by amp@chromium.org, May 15 2017

The display changes I were thinking of actually went in last Dec, (with https://codereview.chromium.org/2428383006) which was well before the 57 branch point so I don't think those could be the reason things are breaking in 58+.

There may have been some other display changes after later though, so I'll try to add some logging in related areas and see if anything looks suspicious.

Comment 13 by amp@chromium.org, May 16 2017

Owner: amp@chromium.org
Some more digging revealed the following.

We are getting into a loop trying to set the system ui flags and checking to make sure they match what we set (but they never do).

We set them here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java?l=861

and retrieve them here to make sure they are set correctly:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java?l=534

The thing is, on the S8 the flag 0x10 is always set, but it isn't defined in the current SDK: https://developer.android.com/reference/android/view/View.html#getSystemUiVisibility()

I can't figure out why that value is getting set, but if I change the flag to mask only the ones we set, then everything works great.

Perhaps there is a custom Samsung implementation for mActivity.getWindow().getDecorView().getSystemUiVisibility() that returns that flag always set?

Interestingly, the logs show the following which corresponds to the flag which shouldn't be there (but searching for fullscreenStackSysUiFlag got me nowhere):
D WindowManager: set systemUiVisibility of statusbar : systemUiFlags= 0x418 fullscreenStackSysUiFlag= 0x10


The change that causes this to break was the one that added the check mentioned above, https://codereview.chromium.org/2785493006.  This was merged back to 58, but not 57 which explains why it still works on 57 builds (we didn't check the flags there, just set them and trusted they were right).

I'll prepare a fix using the masks, but I'm not sure if we want to push it all the way back to 58 or not.

Comment 14 by amp@chromium.org, May 16 2017

Labels: M-59 M-58
Labels: -M-58
Thanks, amp@! 

This isn't critical enough to merge to stable (M58).
Project Member

Comment 16 by bugdroid1@chromium.org, May 17 2017

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

commit edc6da3999ae141bef700d80b69323849e2290de
Author: amp <amp@chromium.org>
Date: Wed May 17 00:08:21 2017

[VrShell] Mask system ui flags before checking.

This fixes an issue where extraneous flags were being set on some
devices and causing the flag check to fail.

BUG= 719013 

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

[modify] https://crrev.com/edc6da3999ae141bef700d80b69323849e2290de/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java

I'm the original finder / filer of this bug and I'm just chiming in to say *much thanks @amp*! (as well everyone else who worked to find it)

@ddorwin, I must ask why you say: "This isn't critical enough to merge to stable (M58)."?

The risk if the bug remains in stable is that it allows easy authoring of semi-malicious html+js pages.

Any page with an OT, when clicked to enter WebVR on S8 phones (already @ 5 million+ devices), will immediately send Chrome into an infinite loop and lock its UI into landscape mode that the user cannot escape from. This landscape mode seems to persist even after killing Chrome. (Admittedly, the page author would need a Origins trial token to enable this behavior...)

As a developer who is very actively creating WebVR code for my S8, I eagerly await the day when this fix is deployed to the entire fleet of S8s.  :)
Cc: amineer@chromium.org
Labels: -Hotlist-Interop
Thank you for your concern and description of the risk.
The bar for a stable merge is high, and M58 is more than half way through its stable lifetime. In addition to a malicious site requesting an origin token and getting a user to visit and click on the site, the users would need to (at some point) manually install and enable Google VR Services.

+amineer in case he feels differently. Also, FYI, we will be requesting a merge to M59 after this bakes.
Cc: -amineer@chromium.org
Based on a quick skim of the description this is a denial of service attack rather than an escalation of privilege that puts the user at risk, exposes data, etc etc.  While I understand the concern, shipping a new Chrome release means sending bits to more than a billion users - and in a lot of places downloading those bits costs our users cash (think emerging markets) and time.

As such we only respin stable for critical issues and vulnerabilities, and I don't think this meets the bar.  Re-CC me if you have further concerns.
I appreciate the consideration of my merge into 58 request. Those are all very reasonable explanations as to why it's not wise.  Hopefully there are no issues w/ it in M59.

Thanks @all! Looking forward to developing WebVR on the S8+!

Comment 21 by amp@chromium.org, May 17 2017

Labels: Merge-Request-59
Project Member

Comment 22 by sheriffbot@chromium.org, May 17 2017

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
mckay.davis: The fix is now in Canary 60.0.3102.0.
Project Member

Comment 24 by bugdroid1@chromium.org, May 17 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/142aaf16c7ae26acda3ef8c0072520916a390aa8

commit 142aaf16c7ae26acda3ef8c0072520916a390aa8
Author: amp <amp@chromium.org>
Date: Wed May 17 21:12:14 2017

[VrShell] Mask system ui flags before checking.

This fixes an issue where extraneous flags were being set on some
devices and causing the flag check to fail.

BUG= 719013 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2881403003
Cr-Original-Commit-Position: refs/heads/master@{#472257}
Review-Url: https://codereview.chromium.org/2888973002
Cr-Commit-Position: refs/branch-heads/3071@{#611}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/142aaf16c7ae26acda3ef8c0072520916a390aa8/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java

Comment 25 by amp@chromium.org, May 18 2017

Status: Fixed (was: Assigned)
Marking fixed as this has now landed on the 59 branch, but it will still need to be verified in the next beta release.

Comment 26 by amp@chromium.org, May 25 2017

Status: Verified (was: Fixed)
The fix was released to 59 beta as of version 59.0.3071.71 and verified there.

Please let us know if you see anything similar after testing with that version or later.
Components: Blink>WebXR

Sign in to add a comment