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"
 #include "base/callback.h"
 #include "base/files/file.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 using EntryList = storage::AsyncFileUtil::EntryList;
 
 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(
     const mojom::DocumentPtr& document) {
   base::FilePath::StringType filename = document->display_name;
 
   // Replace path separators appearing in the file name.
   // Chrome OS is POSIX and kSeparators is "/".
   base::ReplaceChars(filename, base::FilePath::kSeparators, "_", &filename);
 
@@ skipping 60 matching lines @@
 
 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)) {
     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,
                                              const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   auto iter = path_to_watcher_id_.find(path);
   if (iter == path_to_watcher_id_.end()) {
     callback.Run(base::File::FILE_ERROR_FAILED);
     return;
   }
@@ skipping 18 matching lines @@
   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;
+    entry.second = kOrphanWatcherId;
 }
 
 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 63 matching lines @@
     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,
     const WatcherCallback& watcher_callback,
-    const StatusCallback& callback,
     const std::string& document_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (document_id.empty()) {
-    callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+
+  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()) {
+    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,
                                                 bool success) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   callback.Run(success ? base::File::FILE_OK : base::File::FILE_ERROR_FAILED);
 }
 
 void ArcDocumentsProviderRoot::ResolveToContentUrlWithDocumentId(
     const ResolveToContentUrlCallback& callback,
@@ skipping 104 matching lines @@
 
     mapping[filename] =
         ThinDocument{document->document_id,
                      document->mime_type == kAndroidDirectoryMimeType};
   }
 
   callback.Run(base::File::FILE_OK, std::move(mapping));
 }
 
 }  // namespace arc