New issue
Advanced search Search tips

Issue 787196 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug
Proj-XR



Sign in to add a comment

WebVrTabTest#testPoseDataUnfocusedTab is flaky

Project Member Reported by bsheedy@chromium.org, Nov 21 2017

Issue description

WebVrTabTest#testPoseDataUnfocusedTab is occasionally flaking on the bots (at least the 5X's with M, not sure about others). This is happening due to getFrameData returning false when we expect true.

According to bajones@, if we've called it inside a display's requestAnimationFrame and are passing it a valid VRFrameData object, then it should only ever return false if we've lost tracking, at least according to the spec. That seems unlikely, so there's probably a bug somewhere in the implementation.
 
Update: This is being caused by frame_pose_ being null.
The root issue seems to be a race condition with when getFrameData gets called in relation to the VSync/pose code.

In a failed test run, OnMagicWindowVSync and ProcessScheduledAnimations are called before getFrameData, and a valid pose is obtained, but OnMagicWindowPose is not called until after getFrameData, so frame_pose_ is still null, which causes getFrameData to return false.

In a successful test run, OnMagicWindowPose is called before getFrameData is.

Talking to Bill and Klaus, the cause is that the two callbacks at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/vr/VRDisplay.cpp?sq=package:chromium&dr=CSs&l=210 can fire in either order, when instead, we want to make sure we don't fire rAF until we have a pose.
Firing rAF with a null pose is perfectly valid, should we not just make the test tolerant to this? I don't think we want to delay rAF until we have a pose, especially because magic window inside of VR browsing will always return null poses.
Making the test tolerant would be easy, but talking to Brandon earlier, it sounded like this shouldn't be happening according to the spec. Since he's OOO, I'd be fine with making the test tolerant of this type of failure for now (the sheriffs have been pretty strict about disabling flaky tests recently) and decide on whether to keep that as the permanent solution when he's back.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 21 2017

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

commit 52d21598d6a31b7cc889eb5036937bb5be006e0e
Author: bsheedy <bsheedy@chromium.org>
Date: Tue Nov 21 21:57:31 2017

Fix testPoseDataUnfocusedTab flakiness

Makes testPoseDataUnfocusedTab resilient to flakes caused by poses not
being available on the first requestAnimationFrame.

Bug: 787196
Change-Id: I77f29b040ae35890961221735637740334a255e4
Reviewed-on: https://chromium-review.googlesource.com/782145
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518409}
[modify] https://crrev.com/52d21598d6a31b7cc889eb5036937bb5be006e0e/chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html

Comment 6 by klausw@chromium.org, Nov 22 2017

Proposed patch to avoid null poses here: https://chromium-review.googlesource.com/c/chromium/src/+/783887 . I'll leave that open while waiting for clarification on how it's supposed to behave.
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 28 2017

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

commit af0e7a54376b11198f971255b92b2ee4a24f34e2
Author: Michael Thiessen <mthiesse@chromium.org>
Date: Tue Nov 28 01:11:00 2017

WebVR: Fire display rAF synchronously with window rAF for magic window

We were posting display rAF for magic window when there was no reason
to, which was making comparing window rAF and display rAF performance
difficult. This CL fixes that, and adds a test to make sure display
rAF is equivalent to window rAF in terms of scheduling.

This exposes the same race in getFrameData_oneframeupdate.html that was
found in WebVrTabTest#testPoseDataUnfocusedTab, namely that the pose
can start off being null, racily. The previous behavior of posting the
vrDisplay rAF was hiding this.

Bug:  734747 ,  734208 , 787196
Change-Id: Ib6b657b1966359683b3c7aca16be26be9c3c54dd
Reviewed-on: https://chromium-review.googlesource.com/788240
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519501}
[add] https://crrev.com/af0e7a54376b11198f971255b92b2ee4a24f34e2/third_party/WebKit/LayoutTests/vr/VRDisplay_rAF_fires_with_window_rAF.html
[modify] https://crrev.com/af0e7a54376b11198f971255b92b2ee4a24f34e2/third_party/WebKit/LayoutTests/vr/getFrameData_oneframeupdate.html
[modify] https://crrev.com/af0e7a54376b11198f971255b92b2ee4a24f34e2/third_party/WebKit/Source/modules/vr/VRDisplay.cpp

Owner: klausw@chromium.org
klaus is working on a fix.
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 13 2017

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

commit 28e0c070af0eff724a7e28c8a5196817db028f90
Author: Klaus Weidner <klausw@chromium.org>
Date: Wed Dec 13 02:25:35 2017

Avoid null poses in WebVR magic window mode

There's a race condition where the first magic window VSync could arrive before
ever getting a pose. Add a check and deferred execution to avoid this.

BUG=787196

Change-Id: If804e4d6acc3da7b903ba36108f23c00b1c8fe5b
Reviewed-on: https://chromium-review.googlesource.com/783887
Commit-Queue: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Bill Orr <billorr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523657}
[modify] https://crrev.com/28e0c070af0eff724a7e28c8a5196817db028f90/third_party/WebKit/LayoutTests/vr/getFrameData_oneframeupdate.html
[modify] https://crrev.com/28e0c070af0eff724a7e28c8a5196817db028f90/third_party/WebKit/LayoutTests/vr/getFrameData_samewithinframe.html
[modify] https://crrev.com/28e0c070af0eff724a7e28c8a5196817db028f90/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
[modify] https://crrev.com/28e0c070af0eff724a7e28c8a5196817db028f90/third_party/WebKit/Source/modules/vr/VRDisplay.h

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 13 2017

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

commit c8e3b0c2075d8587e07275ea99d1f1f3572bfb2b
Author: Christos Froussios <cfroussios@chromium.org>
Date: Wed Dec 13 09:13:51 2017

Revert "Avoid null poses in WebVR magic window mode"

This reverts commit 28e0c070af0eff724a7e28c8a5196817db028f90.

Reason for revert: Suspected of breaking vr/VRDisplay_rAF_fires_with_window_rAF.html on Win7 Tests (dbg)
E.g https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/65158

Original change's description:
> Avoid null poses in WebVR magic window mode
> 
> There's a race condition where the first magic window VSync could arrive before
> ever getting a pose. Add a check and deferred execution to avoid this.
> 
> BUG=787196
> 
> Change-Id: If804e4d6acc3da7b903ba36108f23c00b1c8fe5b
> Reviewed-on: https://chromium-review.googlesource.com/783887
> Commit-Queue: Klaus Weidner <klausw@chromium.org>
> Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
> Reviewed-by: Bill Orr <billorr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523657}

TBR=mthiesse@chromium.org,bajones@chromium.org,klausw@chromium.org,billorr@chromium.org

Change-Id: Ic70d3ebdb294e04bf70a0488d21790e05d43e1d5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 787196
Reviewed-on: https://chromium-review.googlesource.com/823903
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523728}
[modify] https://crrev.com/c8e3b0c2075d8587e07275ea99d1f1f3572bfb2b/third_party/WebKit/LayoutTests/vr/getFrameData_oneframeupdate.html
[modify] https://crrev.com/c8e3b0c2075d8587e07275ea99d1f1f3572bfb2b/third_party/WebKit/LayoutTests/vr/getFrameData_samewithinframe.html
[modify] https://crrev.com/c8e3b0c2075d8587e07275ea99d1f1f3572bfb2b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
[modify] https://crrev.com/c8e3b0c2075d8587e07275ea99d1f1f3572bfb2b/third_party/WebKit/Source/modules/vr/VRDisplay.h

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 19 2017

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

commit c3862dab08f64201e0bec159962657897478cefc
Author: Klaus Weidner <klausw@chromium.org>
Date: Tue Dec 19 02:12:12 2017

Re-land "Avoid null poses in WebVR magic window mode"

There's a race condition where the first magic window VSync could arrive before
ever getting a pose. Add a check and deferred execution to avoid this.

Original change was commit 28e0c070af0eff724a7e28c8a5196817db028f90
which was reverted in commit c8e3b0c2075d8587e07275ea99d1f1f3572bfb2b
due to breaking vr/VRDisplay_rAF_fires_with_window_rAF.html on Win7 Tests (dbg).

Re-land includes a modification to the test to ensure there's a non-null pose.

Bug: 787196
Change-Id: I3f09421ad29c3dab49da6d4556ad25a40716c698
Reviewed-on: https://chromium-review.googlesource.com/833327
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@{#524914}
[modify] https://crrev.com/c3862dab08f64201e0bec159962657897478cefc/third_party/WebKit/LayoutTests/vr/VRDisplay_rAF_fires_with_window_rAF.html
[modify] https://crrev.com/c3862dab08f64201e0bec159962657897478cefc/third_party/WebKit/LayoutTests/vr/getFrameData_oneframeupdate.html
[modify] https://crrev.com/c3862dab08f64201e0bec159962657897478cefc/third_party/WebKit/LayoutTests/vr/getFrameData_samewithinframe.html
[modify] https://crrev.com/c3862dab08f64201e0bec159962657897478cefc/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
[modify] https://crrev.com/c3862dab08f64201e0bec159962657897478cefc/third_party/WebKit/Source/modules/vr/VRDisplay.h

Labels: VR-Caught-By-Test
Components: Internals>XR
Removing Internals>VR component and remapping to Internals>XR
Components: -Internals>VR
Labels: -VR-Caught-By-Test XR-Caught-By-Test

Sign in to add a comment