Use TaskScheduler instead of WorkerPool in polling_proxy_config_service.cc.

net/url_request/url_request_context_builder_unittest.cc

Index: net/url_request/url_request_context_builder_unittest.cc
diff --git a/net/url_request/url_request_context_builder_unittest.cc b/net/url_request/url_request_context_builder_unittest.cc
index cd8f21dd4ff5acca21b8122a8dd4d4eb6df87fbf..dabd8c5627e382951b09357899123cf81bfc6956 100644
--- a/net/url_request/url_request_context_builder_unittest.cc
+++ b/net/url_request/url_request_context_builder_unittest.cc
@@ -7,7 +7,9 @@
 #include <memory>
 
 #include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_task_scheduler.h"
 #include "build/build_config.h"
 #include "net/base/request_priority.h"
 #include "net/http/http_auth_challenge_tokenizer.h"
@@ -58,7 +60,8 @@ class MockHttpAuthHandlerFactory : public HttpAuthHandlerFactory {
 
 class URLRequestContextBuilderTest : public PlatformTest {
  protected:
-  URLRequestContextBuilderTest() {
+  URLRequestContextBuilderTest()
+      : scoped_task_scheduler_(base::MessageLoop::current()) {
     test_server_.AddDefaultHandlers(
         base::FilePath(FILE_PATH_LITERAL("net/data/url_request_unittest")));
 #if defined(OS_LINUX) || defined(OS_ANDROID)
@@ -69,6 +72,9 @@ class URLRequestContextBuilderTest : public PlatformTest {
 
   EmbeddedTestServer test_server_;
   URLRequestContextBuilder builder_;
+
+ private:
+  base::test::ScopedTaskScheduler scoped_task_scheduler_;

[eroman, 2017/02/13 23:14:52]

Same question that I asked on this code review
(https://codereview.chromium.org/2602243002/) -- what is the formula used for
including these?

With this approach it is not obvious at all where these need to be added, since
it depends on the transitive set of dependencies on what is run right?

Can we have a single scopedtaskscheduler at the NetSuite level?

In what manner to tests fail when this is not added?

[fdoray, 2017/02/16 21:41:00]

1. Upload a patch set.
2. Send it to the CQ dry run.
3. Add a base::test::ScopedTaskScheduler to tests that failed because a task was
posted without a registered TaskScheduler.
base::WorkerPool is lazy-initialized the first time that it is used in a
process. Tasks posted to it can continue to run after the test from which they
were posted has ended.

base/task_scheduler/post_task.h functions crash if they are called when there is
no TaskScheduler registered in the process. A TaskScheduler can be registered in
the process by instantiating a  base::test::Scoped(Async)TaskScheduler. While it
can't be annoying to add a base::test::Scoped(Async)TaskScheduler to every test
that uses TaskScheduler, we like the fact that it prevents tasks from running
after the end of the test from which they were posted.
Not a crazy idea. Caveats:
- Tests can no longer choose to instantiate a ScopedTaskScheduler vs. a
ScopedAsyncTaskScheduler. A ScopedTaskScheduler facilitates debugging and makes
test more deterministic by running everything on the main thread. However, some
tests (e.g.
https://cs.chromium.org/chromium/src/net/disk_cache/disk_cache_test_base.h?ty...)
really need multi-threaded execution.
- We plan to add methods to control time on Scoped(Async)TaskScheduler (useful
when you post delayed tasks). I it better if these methods are called on a
member than on a global variable.
- There is a TaskScheduler in tests that don't need it (unnecessary complexity).
DCHECK failure when DCHECK_IS_ON(), crash otherwise.

[eroman, 2017/02/16 22:14:02]

Thanks for the explanation.

This pattern still feels fragile to me, since it isn't an explicit dependency of
the API, and has all the issues related to that:

 * May require changing tests to have a fixture
 * Don't know until runtime if it has such a dependency
 * Can lead to a proliferation of ScopedTaskScheduler in confusing places (for
instance I was surprised to see it in this file -- in fact I still don't really
know why we need it here, and why it isn't in other unit-test files. Perhaps by
coincidence the other locations depending on this already had a
ScopedTaskRunner?).
  * When modifying code, it is hard to know when to *delete* a
ScopedTaskScheduler. Whereas you may rely on a runtime crash to determine when
to add it, what is the mechanism to know when it is no longer being used and
should be deleted?


Have you considered making it a default dependency, but optional to override? As
in, the high level test suite can initialize and set a default async scheduler
so code using it will work by default.

However interested tests can optionally replace it with the deterministic
version. This doesn't address your concern of ensuring that tasks do not run
after the test has completed, however that model is what we relied on previously
anyway.

 };
 
 TEST_F(URLRequestContextBuilderTest, DefaultSettings) {