Replace BrowserThread::FILE in safe browsing with task scheduler.

Replace BrowserThread::FILE in safe browsing with task scheduler.

BUG=689520
Review-Url: https://codereview.chromium.org/2887843002
Cr-Commit-Position: refs/heads/master@{#477791}
Committed: https://chromium.googlesource.com/chromium/src/+/3eef4a0999119b7e1aa6f948bb7ff71d5ebac91f

[Sigurður Ásgeirsson, 2017-05-16 17:22:42]

siggi@chromium.org changed reviewers:
+ fdoray@chromium.org, gab@chromium.org

[Sigurður Ásgeirsson, 2017-05-16 17:22:43]

https://codereview.chromium.org/2887843002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/download_feedback_service_unittest.cc (right):

https://codereview.chromium.org/2887843002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/download_feedback_service_unittest.cc:428:
base::RunLoop().RunUntilIdle();
This is now racy, as the before/after tests are racing with the posted tasks.

[Sigurður Ásgeirsson, 2017-05-16 17:31:22]

Two more cases of the same pattern in this directory.

https://codereview.chromium.org/2887843002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_feedback_unittest.cc (right):

https://codereview.chromium.org/2887843002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_feedback_unittest.cc:191:
base::RunLoop().RunUntilIdle();
Also racy.

https://codereview.chromium.org/2887843002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_feedback_unittest.cc:223:
base::RunLoop().RunUntilIdle();
Also racy.

[gab, 2017-06-01 13:48:05]

Hey Siggi, so you need to replace those base::RunLoop().RunUntilIdle(); with
content::RunAllBlockingPoolTasksUntilIdle() from
content/public/test/test_utils.h. Then it should work. Ultimately
base::RunLoop().RunUntilIdle() should only be to run the main thread (as it is
in product code).

Thanks!

[Sigurður Ásgeirsson, 2017-06-05 17:54:13]

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

[commit-bot: I haz the power, 2017-06-05 17:54:57]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-05 18:14:20]

Hey Gab, Francois,

please take a look - seems everything's more or less passing :O.

Siggi

[commit-bot: I haz the power, 2017-06-05 19:10:22]

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

[commit-bot: I haz the power, 2017-06-05 19:10:23]

Dry run: This issue passed the CQ dry run.

[gab, 2017-06-05 19:33:40]

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_feedback_service_unittest.cc (right):

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_feedback_service_unittest.cc:135: :
file_task_runner_(base::CreateSingleThreadTaskRunnerWithTraits(
Can this use CreateSequencedTaskRunnerWithTraits? We want to avoid
single-threading as much as possible. If it hits a DCHECK in some obscure leaf
please let me know and add a TODO here with appropriate bug# (ping me to find
which)

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_feedback_unittest.cc (right):

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_feedback_unittest.cc:109: :
file_task_runner_(base::CreateSingleThreadTaskRunnerWithTraits(
ditto

[Sigurður Ásgeirsson, 2017-06-06 13:47:46]

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

[commit-bot: I haz the power, 2017-06-06 13:48:01]

Dry run: CQ is trying da patch.

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

[Sigurður Ásgeirsson, 2017-06-06 13:52:00]

Thanks - looks like a SequencedTaskRunner works just fine.

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_feedback_service_unittest.cc (right):

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_feedback_service_unittest.cc:135: :
file_task_runner_(base::CreateSingleThreadTaskRunnerWithTraits(
On 2017/06/05 19:33:39, gab wrote:
> Can this use CreateSequencedTaskRunnerWithTraits? We want to avoid
> single-threading as much as possible. If it hits a DCHECK in some obscure leaf
> please let me know and add a TODO here with appropriate bug# (ping me to find
> which)

Done.

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_feedback_unittest.cc (right):

https://codereview.chromium.org/2887843002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_feedback_unittest.cc:109: :
file_task_runner_(base::CreateSingleThreadTaskRunnerWithTraits(
On 2017/06/05 19:33:39, gab wrote:
> ditto

Done.

[gab, 2017-06-06 14:34:53]

lgtm, thanks

https://codereview.chromium.org/2887843002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(left):

https://codereview.chromium.org/2887843002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:255:
std::unique_ptr<content::TestBrowserThread> file_thread_;
I'm surprised this test doesn't need to add a ScopedTaskEnvironment (well
actually to replace |browser_thread| with a TestBrowserThreadBundle)... Guess
the |file_thread_| was unused.

[commit-bot: I haz the power, 2017-06-06 14:37:03]

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

[commit-bot: I haz the power, 2017-06-06 14:37:03]

Dry run: This issue passed the CQ dry run.

[Sigurður Ásgeirsson, 2017-06-06 14:45:01]

siggi@chromium.org changed reviewers:
+ nparker@chromium.org

[Sigurður Ásgeirsson, 2017-06-06 14:45:01]

Nathan, can you give this the OWNER blessing, please?

https://codereview.chromium.org/2887843002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
(left):

https://codereview.chromium.org/2887843002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:255:
std::unique_ptr<content::TestBrowserThread> file_thread_;
On 2017/06/06 14:34:53, gab wrote:
> I'm surprised this test doesn't need to add a ScopedTaskEnvironment (well
> actually to replace |browser_thread| with a TestBrowserThreadBundle)... Guess
> the |file_thread_| was unused.

Yeah, looks like - I guess this'd start flaking if it was actually used.

[fdoray, 2017-06-06 17:13:41]

lgtm

[Sigurður Ásgeirsson, 2017-06-07 18:15:06]

Nathan, gentle nudge. Is there someone else better suited to reviewing this?

[Nathan Parker, 2017-06-07 22:05:01]

lgtm

[Sigurður Ásgeirsson, 2017-06-07 22:06:05]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2017-06-07 22:06:28]

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-06-07 23:32:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496873165946620,
"parent_rev": "90b8e5b221a2345c27a2e470775ca2d8f5859f42", "commit_rev":
"3eef4a0999119b7e1aa6f948bb7ff71d5ebac91f"}

[commit-bot: I haz the power, 2017-06-07 23:32:34]

Description was changed from

==========
Replace BrowserThread::FILE in safe browsing with task scheduler.

BUG=689520
==========

to

==========
Replace BrowserThread::FILE in safe browsing with task scheduler.

BUG=689520

Review-Url: https://codereview.chromium.org/2887843002
Cr-Commit-Position: refs/heads/master@{#477791}
Committed:
https://chromium.googlesource.com/chromium/src/+/3eef4a0999119b7e1aa6f948bb7f...
==========

[commit-bot: I haz the power, 2017-06-07 23:32:35]

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