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

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 47 matching lines @@
                 extension) == possible_extensions.end()) {
     filename =
         base::FilePath(filename).AddExtension(possible_extensions[0]).value();
   }
 
   return filename;
 }
 
 }  // 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)
     : authority_(authority),
       root_document_id_(root_document_id),
       weak_ptr_factory_(this) {}
 
 ArcDocumentsProviderRoot::~ArcDocumentsProviderRoot() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (observer_wrapper_)
@@ skipping 17 matching lines @@
   ResolveToDocumentId(
       path, base::Bind(&ArcDocumentsProviderRoot::ReadDirectoryWithDocumentId,
                        weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void ArcDocumentsProviderRoot::AddWatcher(
     const base::FilePath& path,
     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;
+  }
   file_system_operation_runner_util::RemoveWatcherOnIOThread(
       watcher_id,
       base::Bind(&ArcDocumentsProviderRoot::OnWatcherRemoved,
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void ArcDocumentsProviderRoot::ResolveToContentUrl(
     const base::FilePath& path,
     const ResolveToContentUrlCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   ResolveToDocumentId(
       path,
       base::Bind(&ArcDocumentsProviderRoot::ResolveToContentUrlWithDocumentId,
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 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(
     const GetFileInfoCallback& callback,
     const std::string& document_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (document_id.empty()) {
     callback.Run(base::File::FILE_ERROR_NOT_FOUND, base::File::Info());
     return;
   }
@@ skipping 62 matching lines @@
   for (const auto& pair : mapping) {
     entry_list.emplace_back(pair.first, pair.second.is_directory
                                             ? storage::DirectoryEntry::DIRECTORY
                                             : storage::DirectoryEntry::FILE);
   }
   callback.Run(base::File::FILE_OK, entry_list, false /* has_more */);
 }
 
 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,
                                                 bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   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) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (document_id.empty()) {
     callback.Run(GURL());
     return;
   }
   callback.Run(BuildDocumentUrl(authority_, document_id));
 }
@@ skipping 96 matching lines @@
 
     mapping[filename] =
         ThinDocument{document->document_id,
                      document->mime_type == kAndroidDirectoryMimeType};
   }
 
   callback.Run(base::File::FILE_OK, std::move(mapping));
 }
 
 }  // namespace arc