After way too long trying to bisect/reproduce the issue, it's looking like it's caused by multiple browsing benchmarks being run in a row, and it just so happens that the WPR browsing benchmarks are run after the one non-WPR benchmark.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/build_limited/scripts/slave/+/5915935ce534520f63cf6724ae7abab5f48ee40f

commit 5915935ce534520f63cf6724ae7abab5f48ee40f
Author: bsheedy <bsheedy@google.com>
Date: Wed Jun 06 00:00:49 2018

Finally managed to bisect the issue to this Catapult roll https://chromium-review.googlesource.com/1072149.

Culprit CL is https://chromium.googlesource.com/catapult.git/+/2a4c9a940f81265b2759184c745290285d606d28. Makes sense since the browsing benchmarks use the --profile-dir option, which was affected by the change. Not sure why it's only an issue when it's used in multiple benchmarks in succession, though. csharrison@, any ideas?

I think I can see the regression. We whitelist "lib" but nothing underneath it! Thank you for filing the issue, and I will work on a patch.

Hm now I am not that sure. It implies "lib" is in the host directory sent in by the benchmark which doesn't seem plausible. Still, if "lib" is there I see some potential issues. Namely, if "lib" is a directory in the host profile, we will accidentally change ownership / security context.

bsheedy: where is the logic that specifies --profile-dir for these tests? I couldn't find anything in vr_benchmarks.py

We pass it a directory that's generated here https://cs.chromium.org/chromium/src/tools/perf/contrib/vr_benchmarks/BUILD.gn?q=vr_perf_tests&sq=package:chromium&dr=C&l=50.

It *should* only contain the VR browsing assets (e.g. backgrounds, sound effects) that would normally be downloaded via the component updater, but we pre-install for benchmarks so they're actually available during tests. Looking at the generated directory I've been using to bisect, that's correct - the only contents are .png and .wav files, as well as a .json manifest.

I should also note that I've only been able to repro this on the Pixel XL with N we run the automated tests on. I'm unable to reproduce on my local Pixel 2 with O or the Pixel 2 XL with O I'm in the process of setting up for automated tests.

Here's the log of the relevant chown command that may have caused the failure. Unfortunately, it is truncated:

(INFO) 2018-06-03 02:16:58,924 device_utils.handle_large_command:1068  Large shell command will be run from file: chown 12859.12859 /data/data/org.chromium.chrome/./VrAssets /data/data/org.chromium.chrome/VrAssets/2.3 /data/data/org.chromium.chrome/VrAssets/2.3/incognito_gradient.png /data/data/org.chromium.chrome/VrAssets/2.3/back_button_click.wav /data/data/org.chromium.chrome/VrAssets/2.3/inactive_button_click.wav /data/data/org.chromium.chrome/VrAssets/2.3/button_hover.wav /data/data/org.chromium.chrome/VrAssets/2.3/manifest.json /data/data/org.chromium.chrome/VrAssets/2.3/normal_gradient.png /data/data/org.chromiu ...


It looks like this contains the list of files in the profile though:

(INFO) 2018-06-03 02:15:18,165 cmd_helper._ValidateAndLogCommand:160  [host]> /home/gtvchrome/bot/w/ir/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb -s HT7320200690 shell '( a=/data/local/tmp/md5sum/md5sum_bin;! [[ $(ls -l $a) = *313200* ]]&&exit 2;export LD_LIBRARY_PATH=/data/local/tmp/md5sum;p="/sdcard/profile/vr_assets_profile/VrAssets/2.3/";$a $p"incognito_gradient.png";$a $p"back_button_click.wav";$a $p"inactive_button_click.wav";$a $p"button_hover.wav";$a $p"manifest.json";$a $p"normal_gradient.png";$a $p"background.png";$a $p"fullscreen_gradient.png";$a $p"button_click.wav";: );echo %$?'



The one other behavior difference I can think of in my patch is that I only call chown on the exact set of files in --profile-dir, whereas previously, we generated weird wildcards that could match other things in a profile. This is relevant because when we push a profile to a device, we first push it to /sdcard/profile, _without_ deleting stale files. The /sdcard profile is then copied to the true profile location.

This means that:
1. /sdcard/profile/VRAssets could be populated with files not in --profile-dir (maybe due to previous runs?)
2. When we copy over the /sdcard profile, we don't properly `chown` all the files, since we assume we only care about files in --profile-dir. Maybe we care about other things?

bsheedy: Since you were able to repro, it might help to have the total list of files in /sdcard/profiles/VRAsserts vs the files in /data/data/org.chromium.chrome/. For the latter path, the results of "ls -laR" would probably also help to see file permissions.

Well, I somehow got the device into a state where *all* benchmarks are failing for this reason now... I'll keep trying to get the list of files.

Charlie, do you think there is an easy fix for this? Would reverting the patch help at this point?

I'm not sure of an easy fix, and I think a revert it _should_ help (though I am not confident). Note that we'll need to revert crrev.com/c/1086128 as well since we've since it modifies the same code.

Maybe bsheedy can try locally and see if a revert actually fixes the problem? Otherwise are there instructions for how to run these VR benchmarks locally so I can try to reproduce?

The fact that bsheedy was able to run a bisect indicates a revert would help things. I can try to do a revert today.

Still, it would be nice to have a reliable repro so I can verify a fix.

A revert isn't super necessary - one of the benchmarks this as affecting wasn't actually giving us any useful data (it's a legacy benchmark, and all the metrics we care about are TBMv2).

I did get the output for ls -laRZ for both /sdcard/profile/vr_assets_profile and /data/data/org.chromium.chrome when the issue did and did not reproduce.

At a glance, it looks like the contents are the same in both /data/data/org.chromium.chrome directories, but the security contexts are different. When everything works correctly, the security context is u:object_r:app_data_file:s0. When it doesn't, it's u:object_r:app_data_file:s0:c512,c768.

Deleted: sdcard_profile_good.txt 5.2 KB

sdcard_profile_good.txt 5.2 KB View Download

Deleted: data_profile_good.txt 6.2 KB

data_profile_good.txt 6.2 KB View Download

Deleted: sdcard_profile_bad.txt 5.2 KB

sdcard_profile_bad.txt 5.2 KB View Download

Deleted: data_profile_bad.txt 6.6 KB

data_profile_bad.txt 6.6 KB View Download

The reason the security contexts were different is that I added the logging before they get changed... Attached are logs from after the security contexts get changed. Charlie also submitted a potential fix yesterday, so I'll see if the issue still reproduces on ToT

Deleted: sdcard_profile_good.txt 5.2 KB

sdcard_profile_good.txt 5.2 KB View Download

Deleted: data_profile_good.txt 6.6 KB

data_profile_good.txt 6.6 KB View Download

Deleted: sdcard_profile_bad.txt 5.2 KB

sdcard_profile_bad.txt 5.2 KB View Download

Deleted: data_profile_bad.txt 6.6 KB

data_profile_bad.txt 6.6 KB View Download

Issue appears to be fixed with Charlie's patch on ToT. I'll keep this open until I re-enable the disabled benchmarks.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/build_limited/scripts/slave/+/d8ab1692dc0bc4549cb925019074035ade0ffda0

commit d8ab1692dc0bc4549cb925019074035ade0ffda0
Author: bsheedy <bsheedy@google.com>
Date: Mon Jun 11 16:54:27 2018