Skip to content

Commit

Permalink
mediaview: Support watchers in ArcDocumentsProviderRoot.
Browse files Browse the repository at this point in the history 
Watchers are not always correct by design. See the long comments at
AddWatcher().

BUG=chromium:684233
TEST=trybot

Review-Url: https://codereview.chromium.org/2715473003
Cr-Commit-Position: refs/heads/master@{#454590}
  • Loading branch information
@nya3jp
nya3jp authored and Commit bot committed Mar 3, 2017
1 parent 1cf2cd5 commit 7049abb
Show file tree
Hide file tree
Showing 3 changed files with 291 additions and 3 deletions.
106 changes: 105 additions & 1 deletion chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/strings/stringprintf.h"
#include "base/time/time.h"
#include "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h"
#include "chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h"
#include "content/public/browser/browser_thread.h"
#include "url/gurl.h"

Expand Down Expand Up @@ -75,6 +74,9 @@ ArcDocumentsProviderRoot::ArcDocumentsProviderRoot(

ArcDocumentsProviderRoot::~ArcDocumentsProviderRoot() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (observer_wrapper_)
file_system_operation_runner_util::RemoveObserverOnIOThread(
std::move(observer_wrapper_));
}

void ArcDocumentsProviderRoot::GetFileInfo(
Expand All @@ -95,6 +97,44 @@ void ArcDocumentsProviderRoot::ReadDirectory(
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)) {
callback.Run(base::File::FILE_ERROR_FAILED);
return;
}
ResolveToDocumentId(
path,
base::Bind(&ArcDocumentsProviderRoot::AddWatcherWithDocumentId,
weak_ptr_factory_.GetWeakPtr(), path, watcher_callback,
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;
}
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.
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) {
Expand All @@ -105,9 +145,17 @@ void ArcDocumentsProviderRoot::ResolveToContentUrl(
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;
}

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;
Expand Down Expand Up @@ -182,6 +230,62 @@ void ArcDocumentsProviderRoot::ReadDirectoryWithNameToThinDocumentMap(
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);
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));
}

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)) {
// 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));
return;
}
path_to_watcher_id_.insert(std::make_pair(path, watcher_id));
callback.Run(base::File::FILE_OK);
}

void ArcDocumentsProviderRoot::OnWatcherAddedButRemoved(
const StatusCallback& callback,
bool success) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
callback.Run(base::File::FILE_ERROR_FAILED);
}

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,
const std::string& document_id) {
Expand Down
96 changes: 94 additions & 2 deletions chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,23 @@
#ifndef CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_DOCUMENTS_PROVIDER_ROOT_H_
#define CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_DOCUMENTS_PROVIDER_ROOT_H_

#include <stdint.h>

#include <map>
#include <string>
#include <vector>

#include "base/callback_forward.h"
#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h"
#include "chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h"
#include "components/arc/common/file_system.mojom.h"
#include "storage/browser/fileapi/async_file_util.h"
#include "storage/browser/fileapi/watcher_manager.h"

class GURL;

Expand All @@ -26,16 +32,19 @@ namespace arc {
// All methods must be called on the IO thread.
// If this object is deleted while there are in-flight operations, callbacks
// for those operations will be never called.
class ArcDocumentsProviderRoot {
class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
public:
using GetFileInfoCallback = storage::AsyncFileUtil::GetFileInfoCallback;
using ReadDirectoryCallback = storage::AsyncFileUtil::ReadDirectoryCallback;
using ChangeType = storage::WatcherManager::ChangeType;
using WatcherCallback = base::Callback<void(ChangeType type)>;
using StatusCallback = base::Callback<void(base::File::Error error)>;
using ResolveToContentUrlCallback =
base::Callback<void(const GURL& content_url)>;

ArcDocumentsProviderRoot(const std::string& authority,
const std::string& root_document_id);
~ArcDocumentsProviderRoot();
~ArcDocumentsProviderRoot() override;

// Queries information of a file just like AsyncFileUtil.GetFileInfo().
void GetFileInfo(const base::FilePath& path,
Expand All @@ -46,13 +55,66 @@ class ArcDocumentsProviderRoot {
void ReadDirectory(const base::FilePath& path,
const ReadDirectoryCallback& callback);

// Installs a document watcher.
//
// It is not allowed to install multiple watchers at the same file path;
// if attempted, duplicated requests will fail.
//
// Currently, watchers can be installed only on directories, and only
// directory content changes are notified. The result of installing a watcher
// to a non-directory in unspecified.
//
// NOTES ABOUT CORRECTNESS AND CONSISTENCY:
//
// Document watchers are not always correct and they may miss some updates or
// even notify incorrect update events for several reasons, such as:
//
// - Directory moves: Currently a watcher will misbehave if the watched
// directory is moved to another location. This is acceptable for now
// since we whitelist MediaDocumentsProvider only.
// - Duplicated file name handling in this class: The same reason as
// directory moves. This may happen even with MediaDocumentsProvider,
// but the chance will not be very high.
// - File system operation races: For example, an watcher can be installed
// to a non-directory in a race condition.
// - Broken DocumentsProviders: For example, we get no notification if a
// document provider does not call setNotificationUri().
//
// However, consistency of installed watchers is guaranteed. That is, after
// a watcher is installed on a file path X, an attempt to uninstall a watcher
// at X will always succeed.
//
// Unfortunately it is too difficult (or maybe theoretically impossible) to
// implement a perfect Android document watcher which never misses document
// updates. So the current implementation gives up correctness, but instead
// focuses on following two goals:
//
// 1. Keep the implementation simple, rather than trying hard to catch
// race conditions or minor cases. Even if we return wrong results, the
// worst consequence is just that users do not see the latest contents
// until they refresh UI.
//
// 2. Keep consistency of installed watchers so that the caller can avoid
// dangling watchers.
void AddWatcher(const base::FilePath& path,
const WatcherCallback& watcher_callback,
const StatusCallback& callback);

// Uninstalls a document watcher.
// See the documentation of AddWatcher() above.
void RemoveWatcher(const base::FilePath& path,
const StatusCallback& callback);

// Resolves a file path into a content:// URL pointing to the file
// on DocumentsProvider. Returns URL that can be passed to
// ArcContentFileSystemFileSystemReader to read the content.
// On errors, an invalid GURL is returned.
void ResolveToContentUrl(const base::FilePath& path,
const ResolveToContentUrlCallback& callback);

// ArcFileSystemOperationRunner::Observer overrides:
void OnWatchersCleared() override;

private:
// Thin representation of a document in documents provider.
struct ThinDocument {
Expand Down Expand Up @@ -82,6 +144,17 @@ class ArcDocumentsProviderRoot {
base::File::Error error,
NameToThinDocumentMap mapping);

void AddWatcherWithDocumentId(const base::FilePath& path,
const WatcherCallback& watcher_callback,
const StatusCallback& callback,
const std::string& document_id);
void OnWatcherAdded(const base::FilePath& path,
const StatusCallback& callback,
int64_t watcher_id);
void OnWatcherAddedButRemoved(const StatusCallback& callback, bool success);

void OnWatcherRemoved(const StatusCallback& callback, bool success);

void ResolveToContentUrlWithDocumentId(
const ResolveToContentUrlCallback& callback,
const std::string& document_id);
Expand Down Expand Up @@ -110,6 +183,25 @@ class ArcDocumentsProviderRoot {

const std::string authority_;
const std::string root_document_id_;

// Map from a file path to a watcher ID.
//
// A watcher can be "orphan" when watchers are cleared as notified by
// OnWatchersCleared() callback. Such watchers are still tracked here,
// but they are not known by the remote service, so they are represented
// by the invalid watcher ID (-1).
//
// Note that we do not use a document ID as a key here to guarantee that
// a watch installed by AddWatcher() can be always identified in
// RemoveWatcher() with the same file path specified.
// See the documentation of AddWatcher() for more details.
std::map<base::FilePath, int64_t> path_to_watcher_id_;

// Can be null if this instance is not observing ArcFileSystemOperationRunner.
// Observation is started on the first call of AddWatcher().
scoped_refptr<file_system_operation_runner_util::ObserverIOThreadWrapper>
observer_wrapper_;

base::WeakPtrFactory<ArcDocumentsProviderRoot> weak_ptr_factory_;

DISALLOW_COPY_AND_ASSIGN(ArcDocumentsProviderRoot);
Expand Down
Loading

0 comments on commit 7049abb

Please sign in to comment.