Regression: exiting WebVR presentation breaks magic window mode |
|||||||||||||||
Issue descriptionIn current canary, entering WebVR presentation and exiting often results in a frozen "magic window" mode that doesn't respond to movement. This appears to have been caused by http://crrev.com/c/817545 which fixed duplicate VSync scheduling requests on presentation failure, but introduced a race condition for normal presentation exit. If there's an outstanding presenting VSync request when presentation exits, it must be converted to a normal VSync request for magic window mode, otherwise the callback won't execute if VrShellGl destructs before servicing it.
,
Dec 18 2017
This introduces a different race, where we now occasionally double-up rAF when exiting WebVR presentation. I think we should revert your CL, reland with a fix, then merge back as a single CL.
,
Dec 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb5d8f5edeeda21786277c550c3e9fdafcbf2ea8 commit eb5d8f5edeeda21786277c550c3e9fdafcbf2ea8 Author: Klaus Weidner <klausw@chromium.org> Date: Mon Dec 18 18:27:31 2017 Revert "Fix rAF handoff on exiting WebVR presentation" This reverts commit 9ebbc362883cf325cffc69e9a8d4e38de2fb6996. Reason for revert: Occasional doubled rAF on exiting presentation. Will re-land with additional fix included. Original change's description: > Fix rAF handoff on exiting WebVR presentation > > If there's an outstanding presenting VSync request when presentation exits, > it must be converted to a normal VSync request for magic window mode. > > This fixes a regression introduced by crrev.com/c/817545. > > BUG= 794266 > > Change-Id: I93f0739facb702f68c29d8af7c77b669d6b54bda > Reviewed-on: https://chromium-review.googlesource.com/822985 > Reviewed-by: Bill Orr <billorr@chromium.org> > Reviewed-by: Michael Thiessen <mthiesse@chromium.org> > Commit-Queue: Klaus Weidner <klausw@chromium.org> > Cr-Commit-Position: refs/heads/master@{#523612} TBR=mthiesse@chromium.org,klausw@chromium.org,billorr@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 794266 Change-Id: If0990fa84e7dce6edcd628a92e03792202d94865 Reviewed-on: https://chromium-review.googlesource.com/832926 Reviewed-by: Klaus Weidner <klausw@chromium.org> Commit-Queue: Klaus Weidner <klausw@chromium.org> Cr-Commit-Position: refs/heads/master@{#524744} [modify] https://crrev.com/eb5d8f5edeeda21786277c550c3e9fdafcbf2ea8/third_party/WebKit/Source/modules/vr/VRDisplay.cpp [modify] https://crrev.com/eb5d8f5edeeda21786277c550c3e9fdafcbf2ea8/third_party/WebKit/Source/modules/vr/VRDisplay.h
,
Dec 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/77ae98468cbacdde56496afcf4752bca4333aca3 commit 77ae98468cbacdde56496afcf4752bca4333aca3 Author: Klaus Weidner <klausw@chromium.org> Date: Mon Dec 18 21:19:47 2017 Re-land "Fix rAF handoff on exiting WebVR presentation" If there's an outstanding presenting VSync request when presentation exits, it must be converted to a normal VSync request for magic window mode. This fixes a regression introduced by crrev.com/c/817545. Original change was commit 9ebbc362883cf325cffc69e9a8d4e38de2fb6996, reverted as commit eb5d8f5edeeda21786277c550c3e9fdafcbf2ea8. Additional changes cherry-picked from mthiesse@'s http://crrev.com/832786 "Fix doubling up of WebVR rAF". Bug: 794266 Change-Id: Id97e121ed4a9ca4405acad6ff27383ad2f468cdf Reviewed-on: https://chromium-review.googlesource.com/833126 Commit-Queue: Klaus Weidner <klausw@chromium.org> Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Reviewed-by: Klaus Weidner <klausw@chromium.org> Cr-Commit-Position: refs/heads/master@{#524802} [modify] https://crrev.com/77ae98468cbacdde56496afcf4752bca4333aca3/third_party/WebKit/Source/modules/vr/VRDisplay.cpp [modify] https://crrev.com/77ae98468cbacdde56496afcf4752bca4333aca3/third_party/WebKit/Source/modules/vr/VRDisplay.h
,
Dec 22 2017
Whelp. Not sure if it's this patch that caused it, but on ToT we sometimes drop the rAF loop when exiting presentation :(
,
Jan 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a8a3095847cbf3533474643f568b1fc593bc103 commit 4a8a3095847cbf3533474643f568b1fc593bc103 Author: Klaus Weidner <klausw@chromium.org> Date: Thu Jan 04 07:18:06 2018 Fix WebVR rAF loop terminating erroneously when exiting WebVR. For some reason the window callback we request before entering WebVR is sometimes never fired if it's still pending when WebVR is entered. This may be related to VSync pausing or some other complicated interaction. This CL works around the issue in the simplest way possible for merging back to 64. This CL allows re-requesting of a window callback even if we already had one pending after exiting webVR, and just ignores any outstanding callback if it does happen to fire. Also ensure that a fresh pose gets requested if we defer a magic window VSync animation due to the current pose being stale. This was not the case when transitioning from WebVR to VR Browser magic window mode to 2D magic window mode. Bug: 794266 Change-Id: If40f21a5aa812104ba4e5a1f834491ea9940ce4b Reviewed-on: https://chromium-review.googlesource.com/847833 Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Commit-Queue: Klaus Weidner <klausw@chromium.org> Cr-Commit-Position: refs/heads/master@{#526934} [modify] https://crrev.com/4a8a3095847cbf3533474643f568b1fc593bc103/third_party/WebKit/Source/modules/vr/VRDisplay.cpp [modify] https://crrev.com/4a8a3095847cbf3533474643f568b1fc593bc103/third_party/WebKit/Source/modules/vr/VRDisplay.h
,
Jan 8 2018
,
Jan 8 2018
Requesting merge of the latest two patches into M64: r524802 from comment 4 r526934 from comment 6 This change should be safe since it only affects VRDisplay.{cpp,h} which is only used for WebVR enabled sites. It's fairly high priority since WebVR developers may be tempted to use nasty workarounds if the freezing bug is present in a release build, and this would cause potential compatibility issues down the road. Please let us know if you have further questions about this change. Sorry about the late merge request.
,
Jan 8 2018
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 9 2018
How risky are those changes? How worse can the situation be on M64 stable if something was to go wrong with this merge.
,
Jan 9 2018
cmasso@, I think the risk is very low for Chrome overall since the two patches only affect WebVR code that isn't active on sites not using the WebVR APIs. For WebVR pages, current M64 beta has a known issue where exiting WebVR mode will lead to a frozen view that no longer animates in response to device movement. The two patches are intended to address this issue. For background, the core issue is that WebVR uses a separate animation loop based on the VR device's timing while presenting, and the handoff between the normal 2D browsing animation loop and the presenting animation loop has been tricky. Failing to reschedule animation led to frozen magic window mode, and a duplicated animation request could lead to doubled animation rates. I have fairly high confidence that the fix should be effective based on manual testing and since I think I now understand better what was going wrong. While it's hard to predict, I'd expect any potential problems with this merge to be similar to the corner case issues we saw earlier - restricted to WebVR, and overall not worse than the current M64 beta behavior, so I think it's unlikely that we'd be worse off than without the patch.
,
Jan 9 2018
Thanks! Please merge as soon as possible.
,
Jan 9 2018
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f309e506e1b7fdfc1a5dde54cd0ecdc5554ee8e8 commit f309e506e1b7fdfc1a5dde54cd0ecdc5554ee8e8 Author: Klaus Weidner <klausw@chromium.org> Date: Tue Jan 09 19:18:49 2018 Re-land "Fix rAF handoff on exiting WebVR presentation" If there's an outstanding presenting VSync request when presentation exits, it must be converted to a normal VSync request for magic window mode. This fixes a regression introduced by crrev.com/c/817545. Original change was commit 9ebbc362883cf325cffc69e9a8d4e38de2fb6996, reverted as commit eb5d8f5edeeda21786277c550c3e9fdafcbf2ea8. Additional changes cherry-picked from mthiesse@'s http://crrev.com/832786 "Fix doubling up of WebVR rAF". TBR=klausw@chromium.org (cherry picked from commit 77ae98468cbacdde56496afcf4752bca4333aca3) Bug: 794266 Change-Id: Id97e121ed4a9ca4405acad6ff27383ad2f468cdf Reviewed-on: https://chromium-review.googlesource.com/833126 Commit-Queue: Klaus Weidner <klausw@chromium.org> Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Reviewed-by: Klaus Weidner <klausw@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#524802} Reviewed-on: https://chromium-review.googlesource.com/857323 Cr-Commit-Position: refs/branch-heads/3282@{#463} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/f309e506e1b7fdfc1a5dde54cd0ecdc5554ee8e8/third_party/WebKit/Source/modules/vr/VRDisplay.cpp [modify] https://crrev.com/f309e506e1b7fdfc1a5dde54cd0ecdc5554ee8e8/third_party/WebKit/Source/modules/vr/VRDisplay.h
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/176a4e8e366705f3a603a6e5bdcab650928862f1 commit 176a4e8e366705f3a603a6e5bdcab650928862f1 Author: Klaus Weidner <klausw@chromium.org> Date: Tue Jan 09 20:00:41 2018 Fix WebVR rAF loop terminating erroneously when exiting WebVR. For some reason the window callback we request before entering WebVR is sometimes never fired if it's still pending when WebVR is entered. This may be related to VSync pausing or some other complicated interaction. This CL works around the issue in the simplest way possible for merging back to 64. This CL allows re-requesting of a window callback even if we already had one pending after exiting webVR, and just ignores any outstanding callback if it does happen to fire. TBR=klausw@chromium.org (cherry picked from commit 4a8a3095847cbf3533474643f568b1fc593bc103) Bug: 794266 Change-Id: If40f21a5aa812104ba4e5a1f834491ea9940ce4b Reviewed-on: https://chromium-review.googlesource.com/847833 Reviewed-by: Michael Thiessen <mthiesse@chromium.org> Commit-Queue: Klaus Weidner <klausw@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#526934} Reviewed-on: https://chromium-review.googlesource.com/857898 Reviewed-by: Klaus Weidner <klausw@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#467} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/176a4e8e366705f3a603a6e5bdcab650928862f1/third_party/WebKit/Source/modules/vr/VRDisplay.cpp [modify] https://crrev.com/176a4e8e366705f3a603a6e5bdcab650928862f1/third_party/WebKit/Source/modules/vr/VRDisplay.h
,
Jan 9 2018
Both patches are merged. They passed through the continuous build, and I manually built the branch release and tested various combinations of VR presentation/magic window transitions successfully. Marking as fixed, for verification once an official beta build is available.
,
Jan 9 2018
This was present in M64 and M65 and is now fixed in both.
,
Jan 11 2018
Issue 794640 has been merged into this issue.
,
Jan 12 2018
,
Jan 16 2018
Issue 798447 has been merged into this issue.
,
Jan 16 2018
Issue 797367 has been merged into this issue.
,
Feb 7 2018
,
Feb 7 2018
,
Feb 13 2018
Added Testcase to WebVR Manual Testing Plan document. Use Case: 1) Open https://webvr.info/samples/03-vr-presentation.html 2) Select Enter VR button 3) Close VR via X 4) Verify: that it returns to Magic Window mode and it is not locked. 5) Repeat (2) thru (5) quickly multiple times.
,
Jul 4
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Dec 13 2017