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..8601c08dc2fd194ccd60a872b0cbb4113931e0eb 100644
--- a/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
+++ b/chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
@@ -65,6 +65,15 @@ base::FilePath::StringType GetFileNameForDocument(
 
 }  // namespace
 
+// static
+const int64_t ArcDocumentsProviderRoot::kInvalidWatcherId = -1;
+// static
+const uint64_t ArcDocumentsProviderRoot::kInvalidWatcherRequestId = 0;
+// static
+const ArcDocumentsProviderRoot::WatcherData
+    ArcDocumentsProviderRoot::kInvalidWatcherData = {kInvalidWatcherId,
+                                                     kInvalidWatcherRequestId};
+
 ArcDocumentsProviderRoot::ArcDocumentsProviderRoot(
     const std::string& authority,
     const std::string& root_document_id)
@@ -102,30 +111,45 @@ void ArcDocumentsProviderRoot::AddWatcher(
     const WatcherCallback& watcher_callback,
     const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (path_to_watcher_id_.count(path)) {
+  if (path_to_watcher_data_.count(path)) {
     callback.Run(base::File::FILE_ERROR_FAILED);
     return;
   }
+  uint64_t watcher_request_id = next_watcher_request_id_++;
+  path_to_watcher_data_.insert(
+      std::make_pair(path, WatcherData{kInvalidWatcherId, watcher_request_id}));
   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_request_id,
+                       watcher_callback));
+
+  // 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.
+  callback.Run(base::File::FILE_OK);
 }
 
 void ArcDocumentsProviderRoot::RemoveWatcher(const base::FilePath& path,
                                              const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  auto iter = path_to_watcher_id_.find(path);
-  if (iter == path_to_watcher_id_.end()) {
+  auto iter = path_to_watcher_data_.find(path);
+  if (iter == path_to_watcher_data_.end()) {
     callback.Run(base::File::FILE_ERROR_FAILED);
     return;
   }
-  int64_t watcher_id = iter->second;
-  path_to_watcher_id_.erase(iter);
-  if (watcher_id < 0) {
-    // This is an orphan watcher. Just remove an entry from
-    // |path_to_watcher_id_| and return success.
+  if (iter->second.inflight_request_id != kInvalidWatcherRequestId) {

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

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]

I agree.

+    // There is an in-flight request. Remove the entry to cancel it.
+    path_to_watcher_data_.erase(iter);
+    callback.Run(base::File::FILE_OK);
+    return;
+  }
+  int64_t watcher_id = iter->second.id;
+  path_to_watcher_data_.erase(iter);
+  if (watcher_id == kInvalidWatcherId) {
+    // This is an invalid watcher. No need to send a request to the remote
+    // service.
     callback.Run(base::File::FILE_OK);
     return;
   }
@@ -147,9 +171,9 @@ void ArcDocumentsProviderRoot::ResolveToContentUrl(
 
 void ArcDocumentsProviderRoot::OnWatchersCleared() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  // Mark all watchers orphan.
-  for (auto& entry : path_to_watcher_id_)
-    entry.second = -1;
+  // Mark all watchers invalid.
+  for (auto& entry : path_to_watcher_data_)
+    entry.second = kInvalidWatcherData;
 }
 
 void ArcDocumentsProviderRoot::GetFileInfoWithDocumentId(
@@ -232,52 +256,55 @@ void ArcDocumentsProviderRoot::ReadDirectoryWithNameToThinDocumentMap(
 
 void ArcDocumentsProviderRoot::AddWatcherWithDocumentId(
     const base::FilePath& path,
+    uint64_t watcher_request_id,
     const WatcherCallback& watcher_callback,
-    const StatusCallback& callback,
     const std::string& document_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (IsWatcherInflightRequestCanceled(path, watcher_request_id))
+    return;
+
   if (document_id.empty()) {
-    callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    DCHECK(path_to_watcher_data_.count(path));
+    path_to_watcher_data_[path] = kInvalidWatcherData;
     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, watcher_request_id));
 }
 
 void ArcDocumentsProviderRoot::OnWatcherAdded(const base::FilePath& path,
-                                              const StatusCallback& callback,
+                                              uint64_t watcher_request_id,
                                               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)) {
-    // Multiple watchers have been installed on the same file path in a race.
-    // Following the contract of WatcherManager, we reject all except the first.
+
+  if (IsWatcherInflightRequestCanceled(path, watcher_request_id)) {
     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(path_to_watcher_data_.count(path));
+  path_to_watcher_data_[path] =
+      WatcherData{watcher_id < 0 ? kInvalidWatcherId : watcher_id,
+                  kInvalidWatcherRequestId};
 }
 
-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,
@@ -286,6 +313,14 @@ void ArcDocumentsProviderRoot::OnWatcherRemoved(const StatusCallback& callback,
   callback.Run(success ? base::File::FILE_OK : base::File::FILE_ERROR_FAILED);
 }
 
+bool ArcDocumentsProviderRoot::IsWatcherInflightRequestCanceled(
+    const base::FilePath& path,
+    uint64_t watcher_request_id) const {
+  auto iter = path_to_watcher_data_.find(path);
+  return (iter == path_to_watcher_data_.end() ||
+          iter->second.inflight_request_id != watcher_request_id);
+}
+
 void ArcDocumentsProviderRoot::ResolveToContentUrlWithDocumentId(
     const ResolveToContentUrlCallback& callback,
     const std::string& document_id) {