📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/10db86e9140000

📍 Found significant differences after each of 2 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/10db86e9140000

Fetch list of sheriffs in v2spa by benjhayden@chromium.org
https://chromium.googlesource.com/catapult/+/d4ca0bacfdb6d6a0f19091161dd9454ba59b6542
memory:chrome:all_processes:reported_by_os:proportional_resident_size: 9.855e+07 → 9.81e+07 (-4.478e+05)

Roll src/third_party/catapult d18d6c7c7183..5d7bcad36ff4 (2 commits) by chromium-autoroll@skia-public.iam.gserviceaccount.com
https://chromium.googlesource.com/chromium/src/+/ac8ebb246231cdea5872c01e6d596a00462678a3
memory:chrome:all_processes:reported_by_os:proportional_resident_size: 9.81e+07 → 7.841e+07 (-1.97e+07)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Looking at the pinpoint job, this is definitely the issue:

2018-12-06 sky@chromium.org chromeos: close all tabs before running the next test
https://chromium.googlesource.com/catapult.git/+/5d7bcad36ff4605e5d6ef33daf769fb8c3e616f1

Scott, it looks like your CL is closing the tab on Android when it shouldn't. In this benchmark we want to measure memory from the previous story after moving Chrome to the background.

Could you make the tab-closing on that line apply to chrome-os only?

Can you elaborate on why it shouldn't close the tab on android? The reason for adding that code is to ensure each test gets a fresh tab. Without that code, it's possible for a previous test to leave state for the next test.

Exactly. And some (a few) benchmarks do explicitly want that behavior.

For example on memory.top_10_mobile, one test loads a page and measures memory. Then the next page *wants* that tab to be there, pushes the Chrome app to the background, and wants to measure again the memory usage with the tab on the background.

On most benchmarks this is not an issue because (on platforms other than chromeos) the browser is closed all the time after each and every test. So your change is breaking assumptions of benchmarks relying on this API:
https://cs.chromium.org/search/?q=ShouldStopBrowserAfterStoryRun

These benchmarks *want* to keep some state between one test and the next.

+ushesh FYI, this is affecting background measurements for system health measurements.

My initial reaction is each test should get a clean environment, otherwise it has to worry about what might be left over. If a test wants state from the previous test, that should be marked in some way. That said, I'm not particularly familiar with telemetry and only did the change because it resulted in a bunch of tests doing *nothing* on ChromeOS. So, hopefully Caleb and/or Ned can comment on this. 

I agree. The current state is largely due to historical reasons, and the plan indeed is to deprecate this benchmarks doing "funny" stuff like this (issue 761014).

But we're not there yet. And currently ShouldStopBrowserAfterStoryRun *is* the way for benchmarks to mark themselves as "please I need to keep stuff from the previous state".

That API is still there, and we still need to support it.

Sky, could you please send out a new CL that changes the behavior change of https://chromium.googlesource.com/catapult.git/+/5d7bcad36ff4605e5d6ef33daf769fb8c3e616f1 to only apply to ChromeOS as a temporary workaround? You can put a TODO in it pointing to crbug/761014 so that we remember to remove that workaround when that bug is fixed.

I'm sorry this is a complicated area. We are trying to balance supporting legacy behavior with making things less confusing.

I'm about to go OOO, and won't get to this until I'm back (new year).

Did anyone else get to this? Is the change still needed?

Yes, the change is still needed.