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

chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc

Index: chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
diff --git a/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc b/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
index 6c8b936f1260af4f699518db8174efbc2b458cc8..4d9bdc4252368a14ea830f31bd5e57a51de132fc 100644
--- a/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
+++ b/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
@@ -26,6 +26,27 @@ namespace arc {
 
 namespace {
 
+// A special watcher ID indicating this watcher is orphaned.
+// Though we have an entry in ArcDocumentsProviderRoot, the remote
+// ArcFileSystemService does not have a corresponding watcher. This happens
+// when, for example, AddWatcher() is called for non-existent files or the
+// remote service crashes.
+// We use this value to allow RemoveWatcher() to succeed even after the remote
+// service forgets watchers.
+constexpr int64_t kOrphanWatcherId = -1;
+
+// A special wathcer ID indicating this watcher is being installed.
+// Though we have an entry in ArcDocumentsProviderRoot, we are still in the
+// middle of asking the remote ArcFileSystemService to install a watcher.
+// Once the installation finishes, the entry will be replaced with a correct
+// watcher ID or kOrphanWatcherId if the installation is failed.
+// We use this value to allow AddWatcher() to return immediately.
+//
+// TODO(crbug.com/698624): Remove this hack. It was introduced because Files
+// app freezes until AddWatcher() finishes, but it should be handled in Files
+// app rather than here.
+constexpr int64_t kInstallInProgressWatcherId = -2;
+
 // Computes a file name for a document.
 // TODO(crbug.com/675868): Consolidate with the similar logic for Drive.
 base::FilePath::StringType GetFileNameForDocument(
@@ -106,11 +127,16 @@ void ArcDocumentsProviderRoot::AddWatcher(
     callback.Run(base::File::FILE_ERROR_FAILED);
     return;
   }
+  // HACK: Invoke |callback| immediately.
+  //
+  // TODO(crbug.com/698624): Remove this hack. It was introduced because Files
+  // app freezes until AddWatcher() finishes, but it should be handled in Files
+  // app rather than here.
+  path_to_watcher_id_.insert(std::make_pair(path, kInstallInProgressWatcherId));
+  callback.Run(base::File::FILE_OK);

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

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]

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:

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

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

   ResolveToDocumentId(
-      path,
-      base::Bind(&ArcDocumentsProviderRoot::AddWatcherWithDocumentId,
-                 weak_ptr_factory_.GetWeakPtr(), path, watcher_callback,
-                 callback));
+      path, base::Bind(&ArcDocumentsProviderRoot::AddWatcherWithDocumentId,
+                       weak_ptr_factory_.GetWeakPtr(), path, watcher_callback));
 }
 
 void ArcDocumentsProviderRoot::RemoveWatcher(const base::FilePath& path,
@@ -149,7 +175,7 @@ void ArcDocumentsProviderRoot::OnWatchersCleared() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Mark all watchers orphan.
   for (auto& entry : path_to_watcher_id_)
-    entry.second = -1;
+    entry.second = kOrphanWatcherId;
 }
 
 void ArcDocumentsProviderRoot::GetFileInfoWithDocumentId(
@@ -233,51 +259,59 @@ void ArcDocumentsProviderRoot::ReadDirectoryWithNameToThinDocumentMap(
 void ArcDocumentsProviderRoot::AddWatcherWithDocumentId(
     const base::FilePath& path,
     const WatcherCallback& watcher_callback,
-    const StatusCallback& callback,
     const std::string& document_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  auto iter = path_to_watcher_id_.find(path);
+  if (iter == path_to_watcher_id_.end() ||
+      iter->second != kInstallInProgressWatcherId) {
+    // Multiple watchers have been installed on the same file path in a race.
+    // Following the contract of WatcherManager, we reject all except the first.
+    return;
+  }
+  DCHECK_EQ(kInstallInProgressWatcherId, iter->second);
+
   if (document_id.empty()) {
-    callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    iter->second = kOrphanWatcherId;
     return;
   }
+
   // Start observing ArcFileSystemOperationRunner if we have not.
   if (!observer_wrapper_) {
     observer_wrapper_ =
         new file_system_operation_runner_util::ObserverIOThreadWrapper(this);
     file_system_operation_runner_util::AddObserverOnIOThread(observer_wrapper_);
   }
+
   file_system_operation_runner_util::AddWatcherOnIOThread(
       authority_, document_id, watcher_callback,
       base::Bind(&ArcDocumentsProviderRoot::OnWatcherAdded,
-                 weak_ptr_factory_.GetWeakPtr(), path, callback));
+                 weak_ptr_factory_.GetWeakPtr(), path));
 }
 
 void ArcDocumentsProviderRoot::OnWatcherAdded(const base::FilePath& path,
-                                              const StatusCallback& callback,
                                               int64_t watcher_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (watcher_id < 0) {
-    callback.Run(base::File::FILE_ERROR_FAILED);
-    return;
-  }
-  if (path_to_watcher_id_.count(path)) {
+
+  auto iter = path_to_watcher_id_.find(path);
+  if (iter == path_to_watcher_id_.end() ||
+      iter->second != kInstallInProgressWatcherId) {
     // Multiple watchers have been installed on the same file path in a race.
     // Following the contract of WatcherManager, we reject all except the first.
     file_system_operation_runner_util::RemoveWatcherOnIOThread(
         watcher_id,
         base::Bind(&ArcDocumentsProviderRoot::OnWatcherAddedButRemoved,
-                   weak_ptr_factory_.GetWeakPtr(), callback));
+                   weak_ptr_factory_.GetWeakPtr()));
     return;
   }
-  path_to_watcher_id_.insert(std::make_pair(path, watcher_id));
-  callback.Run(base::File::FILE_OK);
+  DCHECK_EQ(kInstallInProgressWatcherId, iter->second);
+
+  iter->second = watcher_id < 0 ? kOrphanWatcherId : watcher_id;
 }
 
-void ArcDocumentsProviderRoot::OnWatcherAddedButRemoved(
-    const StatusCallback& callback,
-    bool success) {
+void ArcDocumentsProviderRoot::OnWatcherAddedButRemoved(bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  callback.Run(base::File::FILE_ERROR_FAILED);
+  // Ignore |success|.
 }
 
 void ArcDocumentsProviderRoot::OnWatcherRemoved(const StatusCallback& callback,