Reland of "file_manager: Migrate FILE thread to TaskScheduler"

Reland of "file_manager: Migrate FILE thread to TaskScheduler"

The original patch (crrev.com/483916) was reverted due to an ASAN error.
The ASAN error (stack-use-after-return) was caused because the change callback,
that references variables on the stack, was run after leaving the test function.

This patch solved this problem by flushing tasks in the message loop.
Along the way, removed the explicit deletion of FileWatcher.

BUG=689520
TEST=new file in Downloads is detected and displayed instantly (file watching is working as before)
ALSO_TEST=built unit_tests with ASAN enabled locally to ensure that the ASAN error is gone

Review-Url: https://codereview.chromium.org/2970653002
Cr-Commit-Position: refs/heads/master@{#484191}
Committed: https://chromium.googlesource.com/chromium/src/+/42ef90a3a71691ea76ab5875d90d969442e2bd25

[satorux1, 2017-07-04 00:35:21]

Description was changed from

==========
Reland of "file_manager: Migrate FILE thread to TaskScheduler"

The original patch (crrev.com/483916) was reverted due to an ASAN error.
The ASAN error (stack-use-after-return) was caused because the change callback,
that references variables on the stack, was run after leaving the test function.

This patch solved this problem by flushing tasks in the message loop.
Along the way, removed the explicit deletion of FileWatcher.

BUG=689520
TEST=new file in Downloads is detected and displayed instantly (file watching is
working as before)
==========

to

==========
Reland of "file_manager: Migrate FILE thread to TaskScheduler"

The original patch (crrev.com/483916) was reverted due to an ASAN error.
The ASAN error (stack-use-after-return) was caused because the change callback,
that references variables on the stack, was run after leaving the test function.

This patch solved this problem by flushing tasks in the message loop.
Along the way, removed the explicit deletion of FileWatcher.

BUG=689520
TEST=new file in Downloads is detected and displayed instantly (file watching is
working as before)
ALSO_TEST=built unit_tests with ASAN enabled locally to ensure that the ASAN
error is gone
==========

[satorux1, 2017-07-04 00:35:21]

satorux@chromium.org changed reviewers:
+ fukino@chromium.org

[satorux1, 2017-07-04 00:42:08]

Patchset #2 (id:20001) has been deleted

[satorux1, 2017-07-04 00:42:45]

PTAL. PS1 is the original patch. PS2 contains the fix.

[satorux1, 2017-07-04 00:42:50]

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

[commit-bot: I haz the power, 2017-07-04 00:42:57]

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-07-04 01:15:53]

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

[commit-bot: I haz the power, 2017-07-04 01:15:54]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-07-05 04:53:13]

lgtm

[satorux1, 2017-07-05 04:55:08]

The CQ bit was checked by satorux@chromium.org

[commit-bot: I haz the power, 2017-07-05 04:55:19]

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-07-05 05:25:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499230508387980,
"parent_rev": "29fee3f604ab1d4eb63ba4abd22705280efdc0dc", "commit_rev":
"42ef90a3a71691ea76ab5875d90d969442e2bd25"}

[commit-bot: I haz the power, 2017-07-05 05:25:48]

Description was changed from

==========
Reland of "file_manager: Migrate FILE thread to TaskScheduler"

The original patch (crrev.com/483916) was reverted due to an ASAN error.
The ASAN error (stack-use-after-return) was caused because the change callback,
that references variables on the stack, was run after leaving the test function.

This patch solved this problem by flushing tasks in the message loop.
Along the way, removed the explicit deletion of FileWatcher.

BUG=689520
TEST=new file in Downloads is detected and displayed instantly (file watching is
working as before)
ALSO_TEST=built unit_tests with ASAN enabled locally to ensure that the ASAN
error is gone
==========

to

==========
Reland of "file_manager: Migrate FILE thread to TaskScheduler"

The original patch (crrev.com/483916) was reverted due to an ASAN error.
The ASAN error (stack-use-after-return) was caused because the change callback,
that references variables on the stack, was run after leaving the test function.

This patch solved this problem by flushing tasks in the message loop.
Along the way, removed the explicit deletion of FileWatcher.

BUG=689520
TEST=new file in Downloads is detected and displayed instantly (file watching is
working as before)
ALSO_TEST=built unit_tests with ASAN enabled locally to ensure that the ASAN
error is gone

Review-Url: https://codereview.chromium.org/2970653002
Cr-Commit-Position: refs/heads/master@{#484191}
Committed:
https://chromium.googlesource.com/chromium/src/+/42ef90a3a71691ea76ab5875d90d...
==========

[commit-bot: I haz the power, 2017-07-05 05:25:50]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/42ef90a3a71691ea76ab5875d90d...

[gab, 2017-08-07 20:28:56]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2017-08-07 20:28:57]

https://codereview.chromium.org/2970653002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc
(right):

https://codereview.chromium.org/2970653002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc:311:
content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
This should also have been replaced by a TaskScheduler::FlushForTesting()?

(and the tests passing is suspicious...)

[gab, 2017-08-14 21:27:39]

On 2017/08/07 20:28:57, gab wrote:
>
https://codereview.chromium.org/2970653002/diff/40001/chrome/browser/chromeos...
> File
>
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc
> (right):
> 
>
https://codereview.chromium.org/2970653002/diff/40001/chrome/browser/chromeos...
>
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc:311:
> content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
> This should also have been replaced by a TaskScheduler::FlushForTesting()?
> 
> (and the tests passing is suspicious...)

@satorux: ping ^^^

[satorux1, 2017-08-15 08:14:06]

On 2017/08/14 21:27:39, gab wrote:
> On 2017/08/07 20:28:57, gab wrote:
> >
>
https://codereview.chromium.org/2970653002/diff/40001/chrome/browser/chromeos...
> > File
> >
>
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2970653002/diff/40001/chrome/browser/chromeos...
> >
>
chrome/browser/chromeos/extensions/file_manager/file_browser_handler_api_test.cc:311:
> > content::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
> > This should also have been replaced by a TaskScheduler::FlushForTesting()?
> > 
> > (and the tests passing is suspicious...)
> 
> @satorux: ping ^^^

thank you for catching this. i'll have a look tomorrow.