catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.91k stars 562 forks source link

Telemetry dies with an exception if a results label has a slash character in it #4605

Open pcc opened 5 years ago

pcc commented 5 years ago

I invoked catapult with a command like this:

tools/perf/run_benchmark --browser=exact --browser-executable=out/and_scs/apks/MonochromePublic.apk --device=SERIAL --results-label=out/and_scs --pageset-repeat=5 --output-dir=bm-android2 system_health.memory_mobile

The benchmark ran as normal, but at the end of the output there was a stack trace that looked like this:

Traceback (most recent call last):
  <module> at /path/to/c/src/tools/perf/run_benchmark:27
    sys.exit(main())
  main at /path/to/c/src/tools/perf/run_benchmark:23
    return benchmark_runner.main(config, [trybot_command.Trybot])
  main at /path/to/c/src/third_party/catapult/telemetry/telemetry/benchmark_runner.py:446
    return command_instance.Run(options)
  Run at /path/to/c/src/third_party/catapult/telemetry/telemetry/benchmark_runner.py:316
    return min(255, b.Run(args))
  Run at /path/to/c/src/third_party/catapult/telemetry/telemetry/benchmark.py:105
    return story_runner.RunBenchmark(self, finder_options)
  RunBenchmark at /path/to/c/src/third_party/catapult/telemetry/telemetry/internal/story_runner.py:423
    results.PrintSummary()
  PrintSummary at /path/to/c/src/third_party/catapult/telemetry/telemetry/internal/results/page_test_results.py:580
    self._SerializeTracesToDirPath()
  _SerializeTracesToDirPath at /path/to/c/src/third_party/catapult/telemetry/telemetry/internal/results/page_test_results.py:620
    fh = value.Serialize()
  Serialize at /path/to/c/src/third_party/catapult/telemetry/telemetry/value/trace.py:134
    shutil.copy(self._temp_file.GetAbsPath(), self._file_path)
  copy at /usr/lib/python2.7/shutil.py:119
    copyfile(src, dst)
  copyfile at /usr/lib/python2.7/shutil.py:83
    with open(dst, 'wb') as fdst:
IOError: [Errno 2] No such file or directory: '/path/to/c/src/bm-android2/background_social_facebook_out/and_scs_2018-11-21_07-34-21_9064.html'

And the results.html file was empty, causing me to lose several hours of benchmark results.

benshayden commented 5 years ago

@nedn @CalebRouleau Want to add either some validation checks or string escaping?

CalebRouleau commented 5 years ago

Either of those sounds good to me. pcc, want to contribute a fix for this? We appreciate the help since we're understaffed right now.

Here's a starting point: https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/results/results_options.py?q=results-label&sq=package:chromium&g=0&l=88