commit bc1bccbe18c24e7161293b36dced5ea830d5debb
author Dominic Farolino <dom@chromium.org> Fri Jun 11 07:33:10 2021
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Jun 11 07:33:10 2021
tree beb0c63a9489c1bba6295a2d3a087ab3aaa5c237
parent a5b2626f1052127439e7a6b1ea6f99a5fe6ff1c8

Add ChildProcess::FlushIOThreadForTesting()

This is an attempt to fix the flaky RenderThreadImplBrowserTest
infrastructure (see the bug that this CL is associated with). The last
attempt at fixing the flaky tests there was https://crrev.com/c/2913006,
but this did not work because there are still in-process-test usages of
ChildProcess::io_task_runner() that are causing the thread to post tasks
back to the main thread (where the BrowserTaskEnvironment is housed) and
thus hitting this CHECK() [1] (see [2] where this is described
entirely).

Therefore, https://crrev.com/c/2914499 was posted to explore removing
the remaining direct usages of ChildProcess::io_task_runner() from
ChildThreadImpl, however this seems impossible to follow-through on
given the test failures there. Plus, even if we resolve these failures,
it is technically possible that this IO thread can get used in other
ways, so that is not the best approach to this problem.

So a more maintainable solution would not be to avoid using this thread
in in-process browser tests, but instead properly flush the thread like
the CHECK() mentioned above recommends. This CL introduces
ChildProcess::FlushIOThreadForTesting() that
RenderThreadImplBrowserTest::TearDown() calls to ensure that the thread
is flushed and does not post any tasks to the
BrowserTaskEnvironment-owned TaskRunners anymore. This is necessary
because when the BrowserTaskEnvironment destructs, it CHECK()s that
after it stops its threads, no other threads (that it is not aware of)
are still posting tasks to the task environment.

So now, after this CL the order of events should be as follow:
 1.) A test finishes
 2.) The TearDown() method is called, flushing all tasks on the
     ChildProcess's owned IO base::Thread
 3.) SetUp() is called for the next test, which overwrites
     `RenderThreadImplBrowserTest::browser_threads_`, which is a
     `BrowserTaskEnvironment`.
 4.) The dtor is called on the old |browser_threads_| object, causing us
     to verify that no tasks are posted after shutdown (the CHECK()
     mentioned above). This should be true now (aka we won't trip the
     CHECK()) because the separate base::Thread that is owned by
     ChildProcess is fully flushed.

[1]: https://source.chromium.org/chromium/chromium/src/+/main:content/public/test/browser_task_environment.cc;l=140;drc=f3243dc72e9d290b9ce1d8db978e0b59e9395dff
[2]: https://docs.google.com/document/d/1lzyEc8g5BcfV67SDgBylJhl1_5HAVGjcXRuiD_ydZEw/edit

R=haraken@chromium.org

Bug: 1126157
Change-Id: I747bf400a6a07507ff33353cbfc95cbc0de6501b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2954844
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#891557}