Use TaskScheduler instead of WorkerPool in nss_cert_database.cc.

Use TaskScheduler instead of WorkerPool in nss_cert_database.cc.

Also, remove NSSCertDatabase::SetSlowTaskRunnerForTest(). This method
was used to run slow NSSCertDatabase tasks synchronously in tests. This
is no longer useful since tasks posted through the post_task.h API within
the scope of a ScopedTaskScheduler run synchronously.

The following traits are used to post tasks to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

May Block:
  Tasks posted with MayBlock() may block. This includes but is not
  limited to tasks that wait on synchronous file I/O operations:
  read or write a file from disk, interact with a pipe or a socket,
  rename or delete a file, enumerate files in a directory, etc. This
  trait isn't required for the mere use of locks.

BUG=659191
Review-Url: https://codereview.chromium.org/2603173002
Cr-Commit-Position: refs/heads/master@{#448059}
Committed: https://chromium.googlesource.com/chromium/src/+/982a31fe2c2dc31014dfaf373c6e5da6f094dcc4

[fdoray, 2017-01-01 23:09:35]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-01 23:09:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-01 23:54:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-01 23:54:48]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-01-04 00:44:48]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in nss_cert_database.cc.

The following traits are used:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

May Block:
  Tasks posted with MayBlock() may block. This includes but is not
  limited to tasks that wait on synchronous file I/O operations:
  read or write a file from disk, interact with a pipe or a socket,
  rename or delete a file, enumerate files in a directory, etc. This
  trait isn't required for the mere use of locks.

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in nss_cert_database.cc.

Also, remove NSSCertDatabase::SetSlowTaskRunnerForTest(). This method
was used to run slow NSSCertDatabase tasks synchronously in tests. This
is no longer useful since tasks posted through the post_task.h API within
the scope of a ScopedTaskScheduler run synchronously.

The following traits are used to post tasks to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

May Block:
  Tasks posted with MayBlock() may block. This includes but is not
  limited to tasks that wait on synchronous file I/O operations:
  read or write a file from disk, interact with a pipe or a socket,
  rename or delete a file, enumerate files in a directory, etc. This
  trait isn't required for the mere use of locks.

BUG=659191
==========

[fdoray, 2017-01-04 00:47:12]

fdoray@chromium.org changed reviewers:
+ xunjieli@chromium.org

[fdoray, 2017-01-04 00:47:12]

PTAL. WorkerPool is being deprecated in favor of TaskScheduler.

[xunjieli, 2017-01-04 15:34:29]

xunjieli@chromium.org changed reviewers:
+ rsleevi@chromium.org

[xunjieli, 2017-01-04 15:34:29]

+rsleevi (since I'm completely unfamiliar with this part of the code base.) 

Ryan, since you touched this file recently, could you take this?

[xunjieli, 2017-01-09 16:29:58]

xunjieli@chromium.org changed reviewers:
- xunjieli@chromium.org

[Ryan Sleevi, 2017-01-27 00:51:59]

https://codereview.chromium.org/2603173002/diff/1/chromeos/cert_loader_unitte...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/cert_loader_unitte...
chromeos/cert_loader_unittest.cc:146: base::test::ScopedTaskScheduler
scoped_task_scheduler_;
Why don't you need to bind this to a message loop, as in the other tests?

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
chromeos/network/auto_connect_handler_unittest.cc:83:
scoped_task_scheduler_(&message_loop_) {}
It strikes me as profoundly surprising to see new code depending on
base::MessageLoop as part of its "public" interface - considering how much time
was spent trying to deprecate that.

It's also not clear to me why this one needs to be bound vs the others.

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_ce...
File chromeos/network/network_cert_migrator_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_ce...
chromeos/network/network_cert_migrator_unittest.cc:191:
base::test::ScopedTaskScheduler scoped_task_scheduler_;
Same question - why isn't this bound to a MessageLoop?

[fdoray, 2017-02-01 00:16:17]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 00:17:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 01:23:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 01:23:48]

Dry run: This issue passed the CQ dry run.

[fdoray, 2017-02-02 17:56:32]

PTAnL

https://codereview.chromium.org/2603173002/diff/1/chromeos/cert_loader_unitte...
File chromeos/cert_loader_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/cert_loader_unitte...
chromeos/cert_loader_unittest.cc:146: base::test::ScopedTaskScheduler
scoped_task_scheduler_;
On 2017/01/27 00:51:58, Ryan Sleevi wrote:
> Why don't you need to bind this to a message loop, as in the other tests?

I improved the ScopedTaskScheduler documentation.
https://cs.chromium.org/chromium/src/base/test/scoped_task_scheduler.h

tl;dr ScopedTaskScheduler should be instantiated without arguments most of the
time. When it cohabits with a MessageLoopFor(UI|IO), it needs to be instantiated
with a pointer to it.

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
chromeos/network/auto_connect_handler_unittest.cc:83:
scoped_task_scheduler_(&message_loop_) {}
On 2017/01/27 00:51:59, Ryan Sleevi wrote:
> It strikes me as profoundly surprising to see new code depending on
> base::MessageLoop as part of its "public" interface - considering how much
time
> was spent trying to deprecate that.
> 
> It's also not clear to me why this one needs to be bound vs the others.

New ScopedTaskScheduler documentation addresses this comment.
https://cs.chromium.org/chromium/src/base/test/scoped_task_scheduler.h

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_ce...
File chromeos/network/network_cert_migrator_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_ce...
chromeos/network/network_cert_migrator_unittest.cc:191:
base::test::ScopedTaskScheduler scoped_task_scheduler_;
On 2017/01/27 00:51:59, Ryan Sleevi wrote:
> Same question - why isn't this bound to a MessageLoop?

New ScopedTaskScheduler documentation addresses this comment.
https://cs.chromium.org/chromium/src/base/test/scoped_task_scheduler.h

[Ryan Sleevi, 2017-02-02 22:13:51]

I'm still a little weird-ed out by code switching base::ThreadTaskRunnerHandle
to base::MessageLoop, but it sounds like //base owners are happy with this
approach, so LGTM

[fdoray, 2017-02-03 13:42:02]

On 2017/02/02 22:13:51, Ryan Sleevi wrote:
> I'm still a little weird-ed out by code switching base::ThreadTaskRunnerHandle
> to base::MessageLoop, but it sounds like //base owners are happy with this
> approach, so LGTM

This CL doesn't switch base::ThreadTaskRunnerHandle to base::MessageLoop. At a
few places, this CL initializes a base::ScopedTaskScheduler with a pointer to a
pre-existing base::MessageLoop. This is required to inform the
base::ScopedTaskScheduler that it cohabits with a MessageLoop.

In general, my team discourages using base::MessageLoop. I worked hard to remove
uses of it (https://crbug.com/616447 https://crbug.com/650723
https://crbug.com/650318) so I certainly don't want to introduce new ones :)

[fdoray, 2017-02-03 13:53:05]

fdoray@chromium.org changed reviewers:
+ xiyuan@chromium.org

[fdoray, 2017-02-03 13:53:06]

xiyuan@chromium.org: Please review changes in chromeos/ and
chrome/browser/chromeos/*. Thanks!

[fdoray, 2017-02-03 13:57:11]

fdoray@chromium.org changed reviewers:
+ stevenjb@chromium.org
- xiyuan@chromium.org

[fdoray, 2017-02-03 13:57:11]

stevenjb@chromium.org: Please review changes in chromeos/ and
chrome/browser/chromeos/*. Thanks!

[stevenjb, 2017-02-03 18:01:04]

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
chromeos/network/auto_connect_handler_unittest.cc:133:
message_loop_.task_runner());
Can't we / shouldn't we be able to get rid of this and |message_loop_| here also
now?

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/client_cer...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/client_cer...
chromeos/network/client_cert_resolver_unittest.cc:163:
message_loop_.task_runner());
And here?

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_co...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_co...
chromeos/network/network_connection_handler_unittest.cc:314:
base::MessageLoopForUI message_loop_;
It doesn't look like we use/need this at all here?

[fdoray, 2017-02-03 18:24:44]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 18:25:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-02-03 19:23:07]

stevenjb@: PTAnL

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/auto_conne...
chromeos/network/auto_connect_handler_unittest.cc:133:
message_loop_.task_runner());
On 2017/02/03 18:01:04, stevenjb wrote:
> Can't we / shouldn't we be able to get rid of this and |message_loop_| here
also
> now?

Was taken care of in another CL. Latest patch set reflects it.

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/client_cer...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/client_cer...
chromeos/network/client_cert_resolver_unittest.cc:163:
message_loop_.task_runner());
On 2017/02/03 18:01:04, stevenjb wrote:
> And here?

Same answer.

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_co...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_co...
chromeos/network/network_connection_handler_unittest.cc:314:
base::MessageLoopForUI message_loop_;
On 2017/02/03 18:01:04, stevenjb wrote:
> It doesn't look like we use/need this at all here?

In theory, the test could depend on MessageLoopForUI features (e.g. pumping
input events in RunLoop). However, I removed the MessageLoopForUI in the latest
patch set and bots seem to be happy.

[commit-bot: I haz the power, 2017-02-03 19:27:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 19:27:55]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-02-03 20:04:50]

lgtm

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_co...
File chromeos/network/network_connection_handler_unittest.cc (right):

https://codereview.chromium.org/2603173002/diff/1/chromeos/network/network_co...
chromeos/network/network_connection_handler_unittest.cc:314:
base::MessageLoopForUI message_loop_;
On 2017/02/03 19:23:07, fdoray wrote:
> On 2017/02/03 18:01:04, stevenjb wrote:
> > It doesn't look like we use/need this at all here?
> 
> In theory, the test could depend on MessageLoopForUI features (e.g. pumping
> input events in RunLoop). However, I removed the MessageLoopForUI in the
latest
> patch set and bots seem to be happy.

Yeah, we shouldn't need that any more for this unit test. Cool.

[fdoray, 2017-02-03 20:14:45]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-02-03 20:14:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2603173002/#ps60001
(title: "self-review")

[commit-bot: I haz the power, 2017-02-03 20:15:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 20:34:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486152885632470,
"parent_rev": "c7dda59dad207a6642d7a201e7ed088f3e8bad92", "commit_rev":
"982a31fe2c2dc31014dfaf373c6e5da6f094dcc4"}

[commit-bot: I haz the power, 2017-02-03 20:35:46]

Description was changed from

==========
Use TaskScheduler instead of WorkerPool in nss_cert_database.cc.

Also, remove NSSCertDatabase::SetSlowTaskRunnerForTest(). This method
was used to run slow NSSCertDatabase tasks synchronously in tests. This
is no longer useful since tasks posted through the post_task.h API within
the scope of a ScopedTaskScheduler run synchronously.

The following traits are used to post tasks to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

May Block:
  Tasks posted with MayBlock() may block. This includes but is not
  limited to tasks that wait on synchronous file I/O operations:
  read or write a file from disk, interact with a pipe or a socket,
  rename or delete a file, enumerate files in a directory, etc. This
  trait isn't required for the mere use of locks.

BUG=659191
==========

to

==========
Use TaskScheduler instead of WorkerPool in nss_cert_database.cc.

Also, remove NSSCertDatabase::SetSlowTaskRunnerForTest(). This method
was used to run slow NSSCertDatabase tasks synchronously in tests. This
is no longer useful since tasks posted through the post_task.h API within
the scope of a ScopedTaskScheduler run synchronously.

The following traits are used to post tasks to TaskScheduler:

Priority: Inherited (default)
  The priority is inherited from the calling context (i.e. TaskTraits
  are initialized with the priority of the current task).

Shutdown behavior: CONTINUE_ON_SHUTDOWN
  Tasks posted with this mode which have not started executing before
  shutdown is initiated will never run. Tasks with this mode running at
  shutdown will be ignored (the worker will not be joined).

  Note: Tasks that were previously posted to base::WorkerPool should
  use this shutdown behavior because this is how base::WorkerPool
  handles all its tasks.

May Block:
  Tasks posted with MayBlock() may block. This includes but is not
  limited to tasks that wait on synchronous file I/O operations:
  read or write a file from disk, interact with a pipe or a socket,
  rename or delete a file, enumerate files in a directory, etc. This
  trait isn't required for the mere use of locks.

BUG=659191

Review-Url: https://codereview.chromium.org/2603173002
Cr-Commit-Position: refs/heads/master@{#448059}
Committed:
https://chromium.googlesource.com/chromium/src/+/982a31fe2c2dc31014dfaf373c6e...
==========

[commit-bot: I haz the power, 2017-02-03 20:35:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/982a31fe2c2dc31014dfaf373c6e...

[fdoray, 2017-02-28 01:26:23]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2722733002/ by fdoray@chromium.org.

The reason for reverting is: Using TaskScheduler with NSS is not safe until
TaskScheduler supports dynamically growing its thread pool..