New issue
Advanced search Search tips

Issue 862845 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Aug 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression
Proj-VR
Proj-XR
Proj-XR-VR



Sign in to add a comment

WebVR/WebXR perf tests broken due to faulty profile

Project Member Reported by bsheedy@chromium.org, Jul 12

Issue description

The WebVR and WebXR perf tests have been failing, seemingly due to an issue caused by the VrAssets directory being in the _default_profile profile. Example failure at https://omnibot-swarming-server.appspot.com/task?id=3ea48782e8dc3110&refresh=10&show_raw=1.

The VR assets shouldn't be copied in during these tests, and I honestly don't know how they're ending up in the default profile.

The culprit CL *should* be somewhere in https://chromium.googlesource.com/chromium/src/+log/248b42203b256d2b0e2d310c5dae115a06d72fdd%5E..03f9320605ad419e8ddfcfad0c6436b34e527382?pretty=fuller&n=1000, which is all the changes between the first time this issue appeared and two builds before then. However, nothing looks suspicious.
 
The trailing slash in /data/data/org.chromium.chrome/ seems to also not be handled properly (we end up with double slashes in the path), although not sure off-hand whether that actually causes issues.
These lines look like as if it is trying to copy the component from the default profile to Chrome but the component doesn't exist:

- cp -a ./VrAssets/2.3/* /data/data/org.chromium.chrome//./VrAssets/2.3/*
- cp: bad './VrAssets/2.3/*': No such file or directory

So, the component is not there (which is good) but it is trying to copy it (which is bad). I have no idea why that is.
I SSHed into the bots and poke around on the devices. The directory definitely exists, complete with all the asset files, so maybe permission issues? 
I'm not able to reproduce locally yet. However, adding logging to https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py?q=_default_profile+lang:python&sq=package:chromium&dr=C&l=39 shows that we aren't intentionally trying to copy the VrAssets directory - it doesn't show up in the profile_files_to_copy.

https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/platform/efficient_android_directory_copy.sh seems to just blindly copy everything, so the VrAssets directory must be getting copied somewhere else.
https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/android_browser_finder.py?rcl=39de3d2258563f5ef8c7366c0354adf41fe5a94b&l=59 suggests it is making the host profile dir from some intermediate directory and then copying the profile_files_to_copy into it. Could it be that the intermediate directory is left over from a previous run?
Hhhmm... It shouldn't since the directory should be deleted once the context manager goes out of scope.

I was able to reproduce locally - looks like this might have something to do with running a test without the --profile-dir option (e.g. xr.webvr.static) sometime after running a test with it (e.g. xr.browsing.static).

The VrAssets directory isn't showing up in _default_profile when just running xr.webvr.static after wiping Chromium and _default_profile off the device. However, if I wipe -> run xr.browsing.static -> run xr.webvr.static, xr.webvr.static fails and VrAssets is magically in _default_profile. I'll poke around more to figure out exactly when it's getting copied.
Looks like xr.browsing.static is copying VrAssets into _default_profile as part of its pre-story setup for some reason...
Alright, I think I see the issue. https://chromium.googlesource.com/catapult.git/+/b520f6298c3e149100f869b57b6c19c1f8c0df60 changed the profile pushing behavior so that everything gets pushed to _default_profile, and _default_profile gets copied over unconditionally before each story run. However, _default_profile is never cleaned up on the device, so _default_profile will end up being the union of all profile dirs used in tests on a particular device.

No idea why this took so long to show up, but I'll submit a patch for this.
Great find!
Actually, the issue appears to be that it relies on the delete_device_stale option in PushChangedFiles - which is in fact deleting the asset files as expected. Except that it doesn't delete directories, and apparently trying to copy all contents of an empty directory makes cp sad.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 16

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/0256bd4b17f3a7d3d81cfa24ea9e81a321f0748e

commit 0256bd4b17f3a7d3d81cfa24ea9e81a321f0748e
Author: bsheedy <bsheedy@chromium.org>
Date: Mon Jul 16 17:49:18 2018

Handle stale directories when pushing files

Currently, _GetChangedAndStaleFiles only finds stale files, meaning that
it's possible to end up with empty directories in the profile that
should not be there. This has the potential to cause test failures, so
also include stale directories in the list of stale files so they can
be removed.

Bug:  chromium:862845 
Change-Id: I05ed53674f19f6d8a0c4330c27dd0153ba53e940
Reviewed-on: https://chromium-review.googlesource.com/1135838
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/0256bd4b17f3a7d3d81cfa24ea9e81a321f0748e/devil/devil/android/device_utils.py
[modify] https://crrev.com/0256bd4b17f3a7d3d81cfa24ea9e81a321f0748e/devil/devil/android/device_utils_devicetest.py

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/e8dc05ccba6b7aa0f489fa97ffbf81273766b1ee

commit e8dc05ccba6b7aa0f489fa97ffbf81273766b1ee
Author: Ned Nguyen <nednguyen@google.com>
Date: Wed Jul 18 01:48:51 2018

Revert "Handle stale directories when pushing files"

This reverts commit 0256bd4b17f3a7d3d81cfa24ea9e81a321f0748e.

Reason for revert: suspect causing catapult roll blockage (Android trybots failing on CQ in https://chromium-review.googlesource.com/c/chromium/src/+/1140418)

Original change's description:
> Handle stale directories when pushing files
> 
> Currently, _GetChangedAndStaleFiles only finds stale files, meaning that
> it's possible to end up with empty directories in the profile that
> should not be there. This has the potential to cause test failures, so
> also include stale directories in the list of stale files so they can
> be removed.
> 
> Bug:  chromium:862845 
> Change-Id: I05ed53674f19f6d8a0c4330c27dd0153ba53e940
> Reviewed-on: https://chromium-review.googlesource.com/1135838
> Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
> Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

TBR=perezju@chromium.org,bsheedy@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:862845 
Change-Id: I78cde3f04060aca89273e9c924dad6149fd06662
Reviewed-on: https://chromium-review.googlesource.com/1141364
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/e8dc05ccba6b7aa0f489fa97ffbf81273766b1ee/devil/devil/android/device_utils.py
[modify] https://crrev.com/e8dc05ccba6b7aa0f489fa97ffbf81273766b1ee/devil/devil/android/device_utils_devicetest.py

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 18

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

commit ee1d7d6a10afc8d71253cb849ed508f706cf92a9
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed Jul 18 07:21:37 2018

Roll src/third_party/catapult f5342c4cf3d3..e8dc05ccba6b (9 commits)

https://chromium.googlesource.com/catapult.git/+log/f5342c4cf3d3..e8dc05ccba6b


git log f5342c4cf3d3..e8dc05ccba6b --date=short --no-merges --format='%ad %ae %s'
2018-07-18 nednguyen@google.com Revert "Handle stale directories when pushing files"
2018-07-18 nednguyen@google.com Implement screenshot capturing at system level on Linux
2018-07-17 dtu@chromium.org [pinpoint] Case-insensitive compare for Webview.
2018-07-17 simonhatch@chromium.org Dashboard - Whitelist documentation urls in /add_histograms
2018-07-17 cphlipot0@gmail.com [Systrace] Raise error when specified device is not found
2018-07-17 nednguyen@google.com Add test that verifies the way Telemetry generate skipped entries for filtered stories when benchmark is interrupted
2018-07-17 xiaochengh@chromium.org Disable WebPageReplayGoServerTest.testSmokeStartingWebPageReplayGoServer on Windows
2018-07-16 dtu@chromium.org [pinpoint] Update list of bots available in Pinpoint.
2018-07-16 bsheedy@chromium.org Handle stale directories when pushing files


Created with:
  gclient setdep -r src/third_party/catapult@e8dc05ccba6b

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:862845 , chromium:862996 , chromium:863861 , chromium:860091 ,chromium:864294, chromium:863995 , chromium:862845 
TBR=sullivan@chromium.org

Change-Id: I5cab2a67db2dfee8c71f9ed7e445b82a9df41aec
Reviewed-on: https://chromium-review.googlesource.com/1141465
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#575965}
[modify] https://crrev.com/ee1d7d6a10afc8d71253cb849ed508f706cf92a9/DEPS

Project Member

Comment 14 by bugdroid1@chromium.org, Jul 26

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/933672b6cfa135d177088629349ab14d2dcc363b

commit 933672b6cfa135d177088629349ab14d2dcc363b
Author: bsheedy <bsheedy@chromium.org>
Date: Thu Jul 26 17:27:07 2018

Reland "Handle stale directories when pushing files"

This is a reland of 0256bd4b17f3a7d3d81cfa24ea9e81a321f0748e

Original change's description:
> Handle stale directories when pushing files
> 
> Currently, _GetChangedAndStaleFiles only finds stale files, meaning that
> it's possible to end up with empty directories in the profile that
> should not be there. This has the potential to cause test failures, so
> also include stale directories in the list of stale files so they can
> be removed.
> 
> Bug:  chromium:862845 
> Change-Id: I05ed53674f19f6d8a0c4330c27dd0153ba53e940
> Reviewed-on: https://chromium-review.googlesource.com/1135838
> Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
> Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

Bug:  chromium:862845 
Change-Id: I1cd73a43f33528865c27ffdfa58f16c3d97c7dbf
Reviewed-on: https://chromium-review.googlesource.com/1142008
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/933672b6cfa135d177088629349ab14d2dcc363b/devil/devil/android/device_utils.py
[modify] https://crrev.com/933672b6cfa135d177088629349ab14d2dcc363b/devil/devil/android/device_utils_devicetest.py

Project Member

Comment 15 by bugdroid1@chromium.org, Jul 26

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

commit 9b6bff2c092b99f9311eee3b90c7de2fa2958c27
Author: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Thu Jul 26 20:42:40 2018

Roll src/third_party/catapult 721613b08607..933672b6cfa1 (6 commits)

https://chromium.googlesource.com/catapult.git/+log/721613b08607..933672b6cfa1


git log 721613b08607..933672b6cfa1 --date=short --no-merges --format='%ad %ae %s'
2018-07-26 bsheedy@chromium.org Reland "Handle stale directories when pushing files"
2018-07-26 benjhayden@chromium.org Fix a few typos in Descriptor.
2018-07-26 dtu@chromium.org [pinpoint] Add "Win 10 Perf" to list of Pinpoint bots.
2018-07-26 simonhatch@chromium.org Telemetry - Start xvfb when running snapit tests.
2018-07-26 wangge@google.com Modified Script to Parse Time from Arguments
2018-07-26 dtu@chromium.org [pinpoint] Add Job name field.


Created with:
  gclient setdep -r src/third_party/catapult@933672b6cfa1

The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:862845 , chromium:863995 , chromium:825434 , chromium:863390 , chromium:867722 
TBR=sullivan@chromium.org

Change-Id: I3f2cf3e0fdfe04a7f75b86519204fda3988625e6
Reviewed-on: https://chromium-review.googlesource.com/1151905
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#578430}
[modify] https://crrev.com/9b6bff2c092b99f9311eee3b90c7de2fa2958c27/DEPS

Status: Fixed (was: Available)

Sign in to add a comment