mediaview: Hack to avoid Files app to freeze soon after login.

mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current
design assumes operations on changing the current working directory
are serialized. Therefore this change tries to workaround the issue in
a quick way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests

Review-Url: https://codereview.chromium.org/2726373002
Cr-Commit-Position: refs/heads/master@{#456945}
Committed: https://chromium.googlesource.com/chromium/src/+/3f89cb4d2e652514501cd26bcf7fd75f1fda73f8

[Shuhei Takahashi, 2017-03-03 15:50:38]

Description was changed from

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

BUG=chromium:684233
TEST=Files app does not freeze soon after login.
==========

to

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

BUG=chromium:684233
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

[Shuhei Takahashi, 2017-03-03 16:06:19]

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

[commit-bot: I haz the power, 2017-03-03 16:06:30]

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-03-03 16:36:13]

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

[commit-bot: I haz the power, 2017-03-03 16:36:14]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-03-06 02:57:00]

Description was changed from

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

BUG=chromium:684233
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

to

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

[Shuhei Takahashi, 2017-03-09 06:44:41]

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

[Shuhei Takahashi, 2017-03-09 06:44:56]

Patchset #2 (id:20001) has been deleted

[Shuhei Takahashi, 2017-03-09 06:45:02]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-09 06:45:03]

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

[Shuhei Takahashi, 2017-03-09 07:04:17]

Description was changed from

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

to

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current design
assumes operations on changing the current working directory are
serialized. Therefore this change tries to workaround the issue in a quick
way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

[Shuhei Takahashi, 2017-03-09 07:04:36]

Description was changed from

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current design
assumes operations on changing the current working directory are
serialized. Therefore this change tries to workaround the issue in a quick
way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

to

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current
design assumes operations on changing the current working
directory are serialized. Therefore this change tries to workaround
the issue in a quick way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

[Shuhei Takahashi, 2017-03-09 07:04:51]

Description was changed from

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current
design assumes operations on changing the current working
directory are serialized. Therefore this change tries to workaround
the issue in a quick way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

to

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current
design assumes operations on changing the current working directory
are serialized. Therefore this change tries to workaround the issue in
a quick way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

[commit-bot: I haz the power, 2017-03-09 07:14:32]

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

[commit-bot: I haz the power, 2017-03-09 07:14:33]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-03-09 07:15:21]

nya@chromium.org changed reviewers:
+ hidehiko@chromium.org

[Shuhei Takahashi, 2017-03-09 07:15:22]

hidehiko: PTAL

[hidehiko, 2017-03-09 15:28:34]

Found an edge case, so let me share it now for faster iteration.
I'll take another detailed look, tomorrow.

https://codereview.chromium.org/2726373002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2726373002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:136:
callback.Run(base::File::FILE_OK);
Looks there is a regression race and/or leak?
- AddWatcher -> callback here. Mojo(1) is sent.
- RemoveWatcher -> remove the callback. No Mojo is sent.
- Remove the document in container.
- Create another dir at the same position.
- AddWatcher again. -> callback here. Mojo(2) is sent.
- Mojo(1) is returned. <- Here root misunderstands old one is alive.
- DELETE event is sent. -> client is confused.
- Mojo(2) is returned <- ignored. So events to the new dir will be lost.

Similarly,
- AddWatcher -> Mojo sent
- RemoveWatcher -> No Mojo sent.
- Mojo return, but ignored.
Then, watcher in container is (a kind of) leak? (kept registered but unused).

[Shuhei Takahashi, 2017-03-10 04:29:59]

https://codereview.chromium.org/2726373002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2726373002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:136:
callback.Run(base::File::FILE_OK);
I'm afraid I don't understand your point correctly. Please let me chat offline
later.

On 2017/03/09 15:28:34, hidehiko wrote:
> Looks there is a regression race and/or leak?
> - AddWatcher -> callback here. Mojo(1) is sent.
> - RemoveWatcher -> remove the callback. No Mojo is sent.
> - Remove the document in container.
> - Create another dir at the same position.
> - AddWatcher again. -> callback here. Mojo(2) is sent.
> - Mojo(1) is returned. <- Here root misunderstands old one is alive.
> - DELETE event is sent. -> client is confused.
> - Mojo(2) is returned <- ignored. So events to the new dir will be lost.
> 
> Similarly,
> - AddWatcher -> Mojo sent
> - RemoveWatcher -> No Mojo sent.
> - Mojo return, but ignored.
> Then, watcher in container is (a kind of) leak? (kept registered but unused).

[Shuhei Takahashi, 2017-03-10 08:46:03]

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

[commit-bot: I haz the power, 2017-03-10 08:46:14]

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-03-10 09:17:14]

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

[commit-bot: I haz the power, 2017-03-10 09:17:14]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-03-10 09:17:23]

PTAL

https://codereview.chromium.org/2726373002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2726373002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:136:
callback.Run(base::File::FILE_OK);
On 2017/03/10 04:29:59, Shuhei Takahashi wrote:
> I'm afraid I don't understand your point correctly. Please let me chat offline
> later.
> 
> On 2017/03/09 15:28:34, hidehiko wrote:
> > Looks there is a regression race and/or leak?
> > - AddWatcher -> callback here. Mojo(1) is sent.
> > - RemoveWatcher -> remove the callback. No Mojo is sent.
> > - Remove the document in container.
> > - Create another dir at the same position.
> > - AddWatcher again. -> callback here. Mojo(2) is sent.
> > - Mojo(1) is returned. <- Here root misunderstands old one is alive.
> > - DELETE event is sent. -> client is confused.
> > - Mojo(2) is returned <- ignored. So events to the new dir will be lost.
> > 
> > Similarly,
> > - AddWatcher -> Mojo sent
> > - RemoveWatcher -> No Mojo sent.
> > - Mojo return, but ignored.
> > Then, watcher in container is (a kind of) leak? (kept registered but
unused).
> 

After offline chat, we decided to assign IDs to watcher requests not to confuse
different requests.

[hidehiko, 2017-03-10 15:16:02]

Thank you very much for addressing my concern!
Looks much nicer.

https://codereview.chromium.org/2726373002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right):

https://codereview.chromium.org/2726373002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:158: bool
IsWatcherRequestCanceled(const base::FilePath& path,
"const" is missing. Could you write the document of this function?

BTW, this function returns "true" after OnWatcherAdded(), even if it is not
cancelled. Practically, it is not big problem, so could you document it as well?
(or, if you agree the change I proposed below, it is naturally addressed,
IIUC?).

https://codereview.chromium.org/2726373002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:207: //
Invariant: for any |path|, an entry for |path| may exist in
Optional: Instead of having this restriction, how about;

struct WatcherData {  // any better name?
  const int64_t request_id;
  int64_t id;  // watcher ID returned from container.
};
std::map<base::FilePath, WatcherData> path_to_watcher_data_;

etc.?

Then you can simply track Add/RemoveWatcher() calls. WDYT?

[Shuhei Takahashi, 2017-03-14 05:42:47]

PTAL

https://codereview.chromium.org/2726373002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right):

https://codereview.chromium.org/2726373002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:158: bool
IsWatcherRequestCanceled(const base::FilePath& path,
On 2017/03/10 15:16:02, hidehiko wrote:
> "const" is missing. Could you write the document of this function?

Done.


> BTW, this function returns "true" after OnWatcherAdded(), even if it is not
> cancelled. Practically, it is not big problem, so could you document it as
well?
> (or, if you agree the change I proposed below, it is naturally addressed,
> IIUC?).

Yup, let's add comments.

https://codereview.chromium.org/2726373002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:207: //
Invariant: for any |path|, an entry for |path| may exist in
On 2017/03/10 15:16:02, hidehiko wrote:
> Optional: Instead of having this restriction, how about;
> 
> struct WatcherData {  // any better name?
>   const int64_t request_id;
>   int64_t id;  // watcher ID returned from container.
> };
> std::map<base::FilePath, WatcherData> path_to_watcher_data_;
> 
> etc.?
> 
> Then you can simply track Add/RemoveWatcher() calls. WDYT?

Sounds good, let's do that!

[Shuhei Takahashi, 2017-03-14 05:42:52]

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

[commit-bot: I haz the power, 2017-03-14 05:43:05]

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-03-14 06:12:35]

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

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

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-03-14 16:56:13]

One more minor request.

https://codereview.chromium.org/2726373002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2726373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:142: if
(iter->second.inflight_request_id != kInvalidWatcherRequestId) {
I do not think you need individual condition here.
Instead, you can just check watcher_id == kInvalidWatcherId (as L150), which
should cover the inflight case, too.

I personally it looks simpler, because;
1) an entry should be removed from path_to_watcher_data_ anyway (= regardless of
whether inflight or not).
2) Mojo call should be sent to ARC container, iff the valid id is assigned (=
indicates a watcher is installed in the container side).

WDYT?

[Shuhei Takahashi, 2017-03-14 18:36:50]

PTAL

https://codereview.chromium.org/2726373002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2726373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:142: if
(iter->second.inflight_request_id != kInvalidWatcherRequestId) {
On 2017/03/14 16:56:13, hidehiko wrote:
> I do not think you need individual condition here.
> Instead, you can just check watcher_id == kInvalidWatcherId (as L150), which
> should cover the inflight case, too.
> 
> I personally it looks simpler, because;
> 1) an entry should be removed from path_to_watcher_data_ anyway (= regardless
of
> whether inflight or not).
> 2) Mojo call should be sent to ARC container, iff the valid id is assigned (=
> indicates a watcher is installed in the container side).
> 
> WDYT?

I agree.

[Shuhei Takahashi, 2017-03-14 18:36:58]

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

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

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-03-14 19:11:58]

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

[commit-bot: I haz the power, 2017-03-14 19:11:59]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-03-14 19:35:07]

lgtm

[Shuhei Takahashi, 2017-03-15 01:52:12]

The CQ bit was checked by nya@chromium.org

[commit-bot: I haz the power, 2017-03-15 01:53:06]

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-03-15 02:00:49]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489542732215350,
"parent_rev": "a7206b0103ce785d5f5b59a3bfb250611ad635d1", "commit_rev":
"3f89cb4d2e652514501cd26bcf7fd75f1fda73f8"}

[commit-bot: I haz the power, 2017-03-15 02:01:33]

Description was changed from

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current
design assumes operations on changing the current working directory
are serialized. Therefore this change tries to workaround the issue in
a quick way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests
==========

to

==========
mediaview: Hack to avoid Files app to freeze soon after login.

Files app freezes until storage::WatcherManager::AddWatcher() finishes.
To avoid this issue, add a hack to ArcDocumentsProviderRoot so that
AddWatcher() pretends to finish immediately.

Ideally this issue should be fixed in Files app. However its current
design assumes operations on changing the current working directory
are serialized. Therefore this change tries to workaround the issue in
a quick way.

BUG=chromium:698624
TEST=Files app does not freeze soon after login.
TEST=unit_tests

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

[commit-bot: I haz the power, 2017-03-15 02:01:35]

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