Also org.chromium.chrome.browser.SubresourceFilterTest#resourceFilteredClose
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/113559

This test is Android-specific, removing Chromium sheriff. Other SubresourceFilterTest tests are also flaky (and they started flaking at the same time, so I'm going to merge all bugs into this one.

 Issue 899967  has been merged into this issue.

 Issue 899972  has been merged into this issue.

 Issue 899981  has been merged into this issue.

 Issue 899889  has been merged into this issue.

These tests are heavily flaking on Android CFI.
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=chrome_public_test_apk&builder=chromium.memory%3AAndroid%20CFI&tests=SubresourceFilterTest

csharrison@, could you please take a look?

Yes thanks for putting this on my radar.

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

commit f4c1c0e72255229896f44aad54a3a309e2a5aa3a
Author: Ioana Pandele <ioanap@chromium.org>
Date: Wed Oct 31 14:35:44 2018

Disable failing testcases in SubresourceFilterTest

These have starting flaking heavliy on Android CFI.

TBR=csharrison@chromium.org

Bug: 899903
Change-Id: Iab67f7421ccf576c5e343f56eae333e0adbcae12
Reviewed-on: https://chromium-review.googlesource.com/c/1309745
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604245}
[modify] https://crrev.com/f4c1c0e72255229896f44aad54a3a309e2a5aa3a/chrome/android/javatests/src/org/chromium/chrome/browser/SubresourceFilterTest.java

I think this is the culprit:
https://chromium-review.googlesource.com/c/1283093

Previously, we had a bunch of test logic plumbed through that skipped waiting for startup to be complete for tests. With BEST_EFFORT tasks, we hoped that we could rip all that out. Unfortunately, the behavior was re-introduced.

+eseckler, can you help with next steps here? Last I checked, there was no way to avoid the random delay induced by PostAfterStartupTask (which the new task runner uses to call Start()), even by simulating startup complete.

Moving ericrobinson to own this.

#12: That's true, I guess. The patch affects tasks posted to browser threads with BEST_EFFORT priority. Since priorities are inherited, this also affects tasks posted by other BEST_EFFORT tasks (which may be running on other threads).

Do you know which BEST_EFFORT tasks your test expects to have completed? Are they actually correctly assigned to BEST_EFFORT, or should they really be assigned a higher priority?

We considered removing the random delay in AfterStartupTasks (instead replacing it by chaining the their execution, i.e. post the next task after the former has completed, to avoid executing them all at once), maybe we have to look into this again. +gab FYI.

There's a few tests, which can be found here:
https://chromium-review.googlesource.com/c/chromium/src/+/1255343

Basically, there were a few tests for which we manually indicated startup had been completed for testing purposes to avoid arbitrary delays/timeouts.  I switched our implementation to use a BEST_EFFORT runner for these tasks and removed this special-case assignment.

These are meant to be "background" tasks and have a lower priority, as they involve indexing a new ruleset which is an expensive operation, and shouldn't interfere with other higher-priority tasks.

Ah right, I remember - you switched to BEST_EFFORT tasks before we actually used the priority hint for anything on the browser threads :)

Gab, wdyt? Should we add a way to disable the random AfterStartupTask timeout for tests / replace it?

(In the future, we intend to prioritize these tasks using a more flexible scheduler + SequenceManager on the UI thread, which will replace AfterStartupTasks. That's still a few weeks out though.)

Ha! Yep we probably should have done a bit more due diligence before switching :)

Thanks for looking into this.

 Issue 900606  has been merged into this issue.

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

commit 18c13e77a9fe7285ab173372f5977155be81697b
Author: Clark DuVall <cduvall@chromium.org>
Date: Wed Oct 31 18:23:02 2018

Disable SubresourceFilterTest.resourceNotFiltered

This test is flaking quite a bit:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=chrome_public_test_apk&tests=org.chromium.chrome.browser.SubresourceFilterTest%23resourceNotFiltered

TBR=csharrison@chromium.org

Bug: 899903
Change-Id: I4785f8ac1d517a23696aaba40a9a5e245440f47d
Reviewed-on: https://chromium-review.googlesource.com/c/1310503
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604337}
[modify] https://crrev.com/18c13e77a9fe7285ab173372f5977155be81697b/chrome/android/javatests/src/org/chromium/chrome/browser/SubresourceFilterTest.java

Removing from sheriff's queue. Sounds like this is being actively looked at.

@eseckler, @gab:  Charlie disabled this test, but it would be good to get it re-enabled relatively quickly/not lose track of it.  The three options I see are:
1. Disable the random delay for AfterStartupTasks for testing.
2. Wait on the flexible scheduler/SequenceManger on the UI thread.
3. Re-introduce the testing-only code that forced the tests to consider startup finished.

You can do (1.) by using content::SetBrowserStartupIsCompleteForTesting().

gab: Is that true? It looks like QueueTask still posts with a random delay even if browser startup is complete. I might be missing something...

Hmm, no?

  if (IsBrowserStartupComplete()) {
    ScheduleTask(std::move(queued_task));
    return;
  }

For this, you'd have to call SetBrowserStartupIsCompleteForTesting before any of these tasks gets posted. Is that possible in this case? (Answer might well be yes ;))

I believe it is for many of the cases, but I'll need to check how universal that is.  Will take a look and get back to you/have a CL soon.

gab: If browser startup is complete we run ScheduleTask, but that method still has a random delay:

void ScheduleTask(std::unique_ptr<AfterStartupTask> queued_task) {
  // Spread their execution over a brief time.
  const int kMinDelaySec = 0;
  const int kMaxDelaySec = 10;
  scoped_refptr<base::TaskRunner> target_runner = queued_task->task_runner;
  base::Location from_here = queued_task->from_here;
  target_runner->PostDelayedTask(
      from_here, base::BindOnce(&RunTask, std::move(queued_task)),
      base::TimeDelta::FromSeconds(base::RandInt(kMinDelaySec, kMaxDelaySec)));
}

This is the entry point. It shouldn't even get to QueueTask() looks like.

void AfterStartupTaskUtils::PostTask(
    const base::Location& from_here,
    const scoped_refptr<base::TaskRunner>& destination_runner,
    base::OnceClosure task) {
  if (IsBrowserStartupComplete()) {
    destination_runner->PostTask(from_here, std::move(task));
    return;
  }

Ah I see, that was my misunderstanding! Thanks! That seems like a reasonable solution here.