Call WatcherManager functions on the IO thread.

Call WatcherManager functions on the IO thread.

Currently it is very difficult to correctly implement WatcherManager
if it needs async operations due to following two facts:

1. WatcherManager functions do not receive
   storage::FileSystemOperationContext, while storage::AsyncFileUtil
   methods do.
2. WatcherManager functions are called on the UI thread, while other
   methods and destructors of its siblings (like
   storage::FileSystemBackend and storage::AsyncFileUtil) are all called
   on the IO thread.

Due to (1), we can't assume WatcherManager and its siblings are alive
after we post tasks to other threads in WatcherManager functions.
Typically we use weak pointers to avoid this kind of race conditions,
but due to (2) it is not possible because WatcherManager is destructed
on the IO thread.

This patch will fix the issue by requiring WatcherManager functions
to be called on the IO thread.

What this patch does are:
- Update private_api_file_system.{cc,h}, the only caller of
  WatcherManager today, to call WatcherManager functions on the IO
  thread.
- Update WatcherManager implementation for File System Providers
  to post tasks to the UI thread.
- Update DCHECK / comments accordingly.

Note that MTPWatcherManager actually expects its functions to be
called on the IO thread and is broken today, so no change is needed.

BUG=chromium:692586
TEST=trybot

Review-Url: https://codereview.chromium.org/2712613002
Cr-Commit-Position: refs/heads/master@{#452768}
Committed: https://chromium.googlesource.com/chromium/src/+/bf45258a17f7f528b652a6b033c606787f8f9a0d

[Shuhei Takahashi, 2017-02-22 07:20:46]

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

[commit-bot: I haz the power, 2017-02-22 07:21:44]

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-22 10:40:43]

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

[commit-bot: I haz the power, 2017-02-22 10:40:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shuhei Takahashi, 2017-02-22 10:42:08]

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

[commit-bot: I haz the power, 2017-02-22 10:42:33]

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-22 11:12:30]

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

[commit-bot: I haz the power, 2017-02-22 11:12:30]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-02-22 11:27:40]

nya@chromium.org changed reviewers:
+ hidehiko@chromium.org, mtomasz@chromium.org, tzik@chromium.org

[Shuhei Takahashi, 2017-02-22 11:27:40]

mtomasz, tzik: PTAL all

hidehiko: PTAL at chrome/browser/chromeos/arc/...

[tzik, 2017-02-23 03:57:18]

looks good

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:425:
base::WeakPtr<file_manager::EventRouter> event_router) {
|event_router| is not used?

[Shuhei Takahashi, 2017-02-23 04:05:57]

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:425:
base::WeakPtr<file_manager::EventRouter> event_router) {
On 2017/02/23 03:57:18, tzik wrote:
> |event_router| is not used?

It is not used in FileManagerPrivateInternalRemoveFileWatchFunction, but it is
used in FileManagerPrivateInternalAddFileWatchFunction. Since the two classes
inherit from the same base class FileWatchFunctionBase, we need the same
signature.

[tzik, 2017-02-23 04:10:37]

lgtm

[mtomasz, 2017-02-23 05:10:05]

lgtm, thanks for fixing!

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc
(right):

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc:78: void
WatcherManager::AddWatcher(
Can we add a comment in watcher_manager.h that all methods have to be called on
IO thead and that callbacks are always called on IO thread?

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc:87:
base::Bind(&CallStatusCallbackOnIOThread, callback),
(We're getting a lot of jumping between threads, but I think it's unavoidable,
as FSP API is primarily done on UI thread, and File API on IO.)

[Shuhei Takahashi, 2017-02-23 06:16:03]

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc
(right):

https://codereview.chromium.org/2712613002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc:78: void
WatcherManager::AddWatcher(
On 2017/02/23 05:10:05, mtomasz wrote:
> Can we add a comment in watcher_manager.h that all methods have to be called
on
> IO thead and that callbacks are always called on IO thread?

Sure, added more comments to watcher_manager.h (in storage/browser/fileapi/).

[Shuhei Takahashi, 2017-02-23 06:16:09]

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

[commit-bot: I haz the power, 2017-02-23 06:16:24]

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-23 06:43:18]

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

[commit-bot: I haz the power, 2017-02-23 06:43:19]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-23 11:05:58]

The direction looks good. Several relatively minor requests.

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:309:
void CallResponseCallbackOnUIThread(
AFAIK, "OnUIThread" suffix is used to represent "this function is running on UI
thread", rather than "This function will post a task to UI thread", in Chromium.

Maybe: PostTaskToUIThread etc.?

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:317:
void CallNotificationCallbackOnUIThread(
Ditto.

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h:91:
using ResponseCallback = base::Callback<void(bool success)>;
#include "base/callback.h" looks missing.

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc:75:
WatcherManager::WatcherManager() {}
s/{}/= default/, please.

[Shuhei Takahashi, 2017-02-24 05:13:25]

PTAL

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:309:
void CallResponseCallbackOnUIThread(
On 2017/02/23 11:05:58, hidehiko wrote:
> AFAIK, "OnUIThread" suffix is used to represent "this function is running on
UI
> thread", rather than "This function will post a task to UI thread", in
Chromium.
> 
> Maybe: PostTaskToUIThread etc.?

Renamed to PostResponseCallbackTaskToUIThread. Does it sound good to you?

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:317:
void CallNotificationCallbackOnUIThread(
On 2017/02/23 11:05:58, hidehiko wrote:
> Ditto.

Done.

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h:91:
using ResponseCallback = base::Callback<void(bool success)>;
On 2017/02/23 11:05:58, hidehiko wrote:
> #include "base/callback.h" looks missing.

Done.

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc:75:
WatcherManager::WatcherManager() {}
On 2017/02/23 11:05:58, hidehiko wrote:
> s/{}/= default/, please.

Done. (FYI, I just left this as-is; see the left side.)

[Shuhei Takahashi, 2017-02-24 05:13:31]

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

[commit-bot: I haz the power, 2017-02-24 05:14:01]

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-24 05:46:24]

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

[commit-bot: I haz the power, 2017-02-24 05:46:25]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-24 05:51:56]

LGTM!

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:309:
void CallResponseCallbackOnUIThread(
On 2017/02/24 05:13:24, Shuhei Takahashi wrote:
> On 2017/02/23 11:05:58, hidehiko wrote:
> > AFAIK, "OnUIThread" suffix is used to represent "this function is running on
> UI
> > thread", rather than "This function will post a task to UI thread", in
> Chromium.
> > 
> > Maybe: PostTaskToUIThread etc.?
> 
> Renamed to PostResponseCallbackTaskToUIThread. Does it sound good to you?

LG. Thanks!

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc
(right):

https://codereview.chromium.org/2712613002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fileapi/watcher_manager.cc:75:
WatcherManager::WatcherManager() {}
On 2017/02/24 05:13:25, Shuhei Takahashi wrote:
> On 2017/02/23 11:05:58, hidehiko wrote:
> > s/{}/= default/, please.
> 
> Done. (FYI, I just left this as-is; see the left side.)

Oh, I didn't find it, and I misunderstood that these are somewhat newly
introduced.
Thank you for pointing it!

[Shuhei Takahashi, 2017-02-24 05:52:49]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2017-02-24 05:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2712613002/#ps80001
(title: "Addressed hidehiko's comments.")

[commit-bot: I haz the power, 2017-02-24 05:53:02]

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-24 05:58:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487915569634560,
"parent_rev": "e03fba3a51966b99375531dc7250a56deffc808c", "commit_rev":
"bf45258a17f7f528b652a6b033c606787f8f9a0d"}

[commit-bot: I haz the power, 2017-02-24 05:58:56]

Description was changed from

==========
Call WatcherManager functions on the IO thread.

Currently it is very difficult to correctly implement WatcherManager
if it needs async operations due to following two facts:

1. WatcherManager functions do not receive
   storage::FileSystemOperationContext, while storage::AsyncFileUtil
   methods do.
2. WatcherManager functions are called on the UI thread, while other
   methods and destructors of its siblings (like
   storage::FileSystemBackend and storage::AsyncFileUtil) are all called
   on the IO thread.

Due to (1), we can't assume WatcherManager and its siblings are alive
after we post tasks to other threads in WatcherManager functions.
Typically we use weak pointers to avoid this kind of race conditions,
but due to (2) it is not possible because WatcherManager is destructed
on the IO thread.

This patch will fix the issue by requiring WatcherManager functions
to be called on the IO thread.

What this patch does are:
- Update private_api_file_system.{cc,h}, the only caller of
  WatcherManager today, to call WatcherManager functions on the IO
  thread.
- Update WatcherManager implementation for File System Providers
  to post tasks to the UI thread.
- Update DCHECK / comments accordingly.

Note that MTPWatcherManager actually expects its functions to be
called on the IO thread and is broken today, so no change is needed.

BUG=chromium:692586
TEST=trybot
==========

to

==========
Call WatcherManager functions on the IO thread.

Currently it is very difficult to correctly implement WatcherManager
if it needs async operations due to following two facts:

1. WatcherManager functions do not receive
   storage::FileSystemOperationContext, while storage::AsyncFileUtil
   methods do.
2. WatcherManager functions are called on the UI thread, while other
   methods and destructors of its siblings (like
   storage::FileSystemBackend and storage::AsyncFileUtil) are all called
   on the IO thread.

Due to (1), we can't assume WatcherManager and its siblings are alive
after we post tasks to other threads in WatcherManager functions.
Typically we use weak pointers to avoid this kind of race conditions,
but due to (2) it is not possible because WatcherManager is destructed
on the IO thread.

This patch will fix the issue by requiring WatcherManager functions
to be called on the IO thread.

What this patch does are:
- Update private_api_file_system.{cc,h}, the only caller of
  WatcherManager today, to call WatcherManager functions on the IO
  thread.
- Update WatcherManager implementation for File System Providers
  to post tasks to the UI thread.
- Update DCHECK / comments accordingly.

Note that MTPWatcherManager actually expects its functions to be
called on the IO thread and is broken today, so no change is needed.

BUG=chromium:692586
TEST=trybot

Review-Url: https://codereview.chromium.org/2712613002
Cr-Commit-Position: refs/heads/master@{#452768}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf45258a17f7f528b652a6b033c6...
==========

[commit-bot: I haz the power, 2017-02-24 05:58:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bf45258a17f7f528b652a6b033c6...