Simplify implementation of FileSystemOperationRunner.

- Use a base::AutoReset instance to manage a single integer tracking
  whether the completion callback for an operation is invoked before
  we have actually returned to the caller.
- Remove the now-unused OperationHandle and BeginOperationScoper.
- Replace SupportsWeakPtr with an internal WeakPtrFactory.
- Replace potentially unsafe use of GetWeakPtr() with a |weak_ptr_|
  member.
- Temporarily disables the recently-added FileSystemURLLoaderFactoryTest
  tests, which have incorrect threading, causing WeakPtr checks to
  fire.

Bug: 846985, 860547
Change-Id: Ia059b1b87ef11f3218aeb6c65d7b8dd62be6c393
Reviewed-on: https://chromium-review.googlesource.com/1074427
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574220}

chrome/browser/chromeos/file_manager/fileapi_util.cc

@@ -406,6 +406,25 @@ class ConvertSelectedFileInfoListToFileChooserFileInfoListImpl {
       ConvertSelectedFileInfoListToFileChooserFileInfoListImpl);
 };
 
+void CheckIfDirectoryExistsOnIoThread(
+    scoped_refptr<storage::FileSystemContext> file_system_context,
+    const storage::FileSystemURL& internal_url,
+    storage::FileSystemOperationRunner::StatusCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  file_system_context->operation_runner()->DirectoryExists(internal_url,
+                                                           std::move(callback));
+}
+
+void GetMetadataForPathOnIoThread(
+    scoped_refptr<storage::FileSystemContext> file_system_context,
+    const storage::FileSystemURL& internal_url,
+    int fields,
+    storage::FileSystemOperationRunner::GetMetadataCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  file_system_context->operation_runner()->GetMetadata(internal_url, fields,
+                                                       callback);
+}
+
 }  // namespace
 
 EntryDefinition::EntryDefinition() = default;
@@ -548,9 +567,7 @@ void CheckIfDirectoryExists(
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(base::IgnoreResult(
-                         &storage::FileSystemOperationRunner::DirectoryExists),
-                     file_system_context->operation_runner()->AsWeakPtr(),
+      base::BindOnce(&CheckIfDirectoryExistsOnIoThread, file_system_context,
                      internal_url, google_apis::CreateRelayCallback(callback)));
 }
 
@@ -569,10 +586,9 @@ void GetMetadataForPath(
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(
-          base::IgnoreResult(&storage::FileSystemOperationRunner::GetMetadata),
-          file_system_context->operation_runner()->AsWeakPtr(), internal_url,
-          fields, google_apis::CreateRelayCallback(callback)));
+      base::BindOnce(&GetMetadataForPathOnIoThread, file_system_context,
+                     internal_url, fields,
+                     google_apis::CreateRelayCallback(callback)));
 }
 
 storage::FileSystemURL CreateIsolatedURLFromVirtualPath(

content/test/BUILD.gn

@@ -732,7 +732,10 @@ test("content_browsertests") {
     "../browser/download/mhtml_generation_browsertest.cc",
     "../browser/download/save_package_browsertest.cc",
     "../browser/fileapi/file_system_browsertest.cc",
-    "../browser/fileapi/file_system_url_loader_factory_browsertest.cc",
+
+    # These tests have incorrect threading (https://crbug.com/860547).
+    #"../browser/fileapi/file_system_url_loader_factory_browsertest.cc",
+
     "../browser/fileapi/fileapi_browsertest.cc",
     "../browser/find_request_manager_browsertest.cc",
     "../browser/frame_host/blocked_scheme_navigation_browsertest.cc",

extensions/browser/api/file_system/file_system_api.cc

@@ -824,12 +824,14 @@ ExtensionFunction::ResponseAction FileSystemRetainEntryFunction::Run() {
             ->CreateVirtualRootPath(filesystem_id)
             .Append(base::FilePath::FromUTF8Unsafe(filesystem_path)));
 
+    // It is safe to use base::Unretained() for operation_runner(), since it
+    // is owned by |context| which will delete it on the IO thread.
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::BindOnce(
             base::IgnoreResult(
                 &storage::FileSystemOperationRunner::GetMetadata),
-            context->operation_runner()->AsWeakPtr(), url,
+            base::Unretained(context->operation_runner()), url,
             storage::FileSystemOperation::GET_METADATA_FIELD_IS_DIRECTORY,
             base::Bind(
                 &PassFileInfoToUIThread,

storage/browser/fileapi/file_system_operation_impl.cc

@@ -380,6 +380,8 @@ FileSystemOperationImpl::FileSystemOperationImpl(
       async_file_util_(NULL),
       pending_operation_(kOperationNone),
       weak_factory_(this) {
+  weak_ptr_ = weak_factory_.GetWeakPtr();
+
   DCHECK(operation_context_.get());
   operation_context_->DetachFromSequence();
   async_file_util_ = file_system_context_->GetAsyncFileUtil(url.type());
@@ -407,7 +409,7 @@ void FileSystemOperationImpl::GetUsageAndQuotaThenRunTask(
   quota_manager_proxy->quota_manager()->GetUsageAndQuota(
       url.origin(), FileSystemTypeToQuotaStorageType(url.type()),
       base::BindOnce(&FileSystemOperationImpl::DidGetUsageAndQuotaAndRunTask,
-                     weak_factory_.GetWeakPtr(), task, error_callback));
+                     weak_ptr_, task, error_callback));
 }
 
 void FileSystemOperationImpl::DidGetUsageAndQuotaAndRunTask(
@@ -435,7 +437,7 @@ void FileSystemOperationImpl::DoCreateFile(
       base::BindOnce(
           exclusive ? &FileSystemOperationImpl::DidEnsureFileExistsExclusive
                     : &FileSystemOperationImpl::DidEnsureFileExistsNonExclusive,
-          weak_factory_.GetWeakPtr(), callback));
+          weak_ptr_, callback));
 }
 
 void FileSystemOperationImpl::DoCreateDirectory(
@@ -444,8 +446,8 @@ void FileSystemOperationImpl::DoCreateDirectory(
     bool exclusive, bool recursive) {
   async_file_util_->CreateDirectory(
       std::move(operation_context_), url, exclusive, recursive,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
-                     weak_factory_.GetWeakPtr(), callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
+                     callback));
 }
 
 void FileSystemOperationImpl::DoCopyFileLocal(
@@ -457,8 +459,8 @@ void FileSystemOperationImpl::DoCopyFileLocal(
   async_file_util_->CopyFileLocal(
       std::move(operation_context_), src_url, dest_url, option,
       progress_callback,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
-                     weak_factory_.GetWeakPtr(), callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
+                     callback));
 }
 
 void FileSystemOperationImpl::DoMoveFileLocal(
@@ -468,8 +470,8 @@ void FileSystemOperationImpl::DoMoveFileLocal(
     const StatusCallback& callback) {
   async_file_util_->MoveFileLocal(
       std::move(operation_context_), src_url, dest_url, option,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
-                     weak_factory_.GetWeakPtr(), callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
+                     callback));
 }
 
 void FileSystemOperationImpl::DoCopyInForeignFile(
@@ -478,8 +480,8 @@ void FileSystemOperationImpl::DoCopyInForeignFile(
     const StatusCallback& callback) {
   async_file_util_->CopyInForeignFile(
       std::move(operation_context_), src_local_disk_file_path, dest_url,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
-                     weak_factory_.GetWeakPtr(), callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
+                     callback));
 }
 
 void FileSystemOperationImpl::DoTruncate(const FileSystemURL& url,
@@ -487,8 +489,8 @@ void FileSystemOperationImpl::DoTruncate(const FileSystemURL& url,
                                          int64_t length) {
   async_file_util_->Truncate(
       std::move(operation_context_), url, length,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
-                     weak_factory_.GetWeakPtr(), callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
+                     callback));
 }
 
 void FileSystemOperationImpl::DoOpenFile(const FileSystemURL& url,
@@ -496,8 +498,7 @@ void FileSystemOperationImpl::DoOpenFile(const FileSystemURL& url,
                                          int file_flags) {
   async_file_util_->CreateOrOpen(
       std::move(operation_context_), url, file_flags,
-      base::BindOnce(&DidOpenFile, file_system_context_,
-                     weak_factory_.GetWeakPtr(), callback));
+      base::BindOnce(&DidOpenFile, file_system_context_, weak_ptr_, callback));
 }
 
 void FileSystemOperationImpl::DidEnsureFileExistsExclusive(
@@ -557,11 +558,10 @@ void FileSystemOperationImpl::DidDeleteRecursively(
   if (rv == base::File::FILE_ERROR_INVALID_OPERATION) {
     // Recursive removal is not supported on this platform.
     DCHECK(!recursive_operation_delegate_);
-    recursive_operation_delegate_.reset(
-        new RemoveOperationDelegate(
-            file_system_context(), url,
-            base::Bind(&FileSystemOperationImpl::DidFinishOperation,
-                       weak_factory_.GetWeakPtr(), callback)));
+    recursive_operation_delegate_.reset(new RemoveOperationDelegate(
+        file_system_context(), url,
+        base::Bind(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
+                   callback)));
     recursive_operation_delegate_->RunRecursively();
     return;
   }

storage/browser/fileapi/file_system_operation_impl.h

@@ -198,6 +198,7 @@ class STORAGE_EXPORT FileSystemOperationImpl : public FileSystemOperation {
   // A flag to make sure we call operation only once per instance.
   OperationType pending_operation_;
 
+  base::WeakPtr<FileSystemOperationImpl> weak_ptr_;
   base::WeakPtrFactory<FileSystemOperationImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperationImpl);

storage/browser/fileapi/file_system_operation_runner.h

@@ -37,8 +37,7 @@ class FileSystemContext;
 // operation fails, in addition to dispatching the callback with an error
 // code (therefore in most cases the caller does not need to check the
 // returned operation ID).
-class STORAGE_EXPORT FileSystemOperationRunner
-    : public base::SupportsWeakPtr<FileSystemOperationRunner> {
+class STORAGE_EXPORT FileSystemOperationRunner {
  public:
   using GetMetadataCallback = FileSystemOperation::GetMetadataCallback;
   using ReadDirectoryCallback = FileSystemOperation::ReadDirectoryCallback;
@@ -246,50 +245,39 @@ class STORAGE_EXPORT FileSystemOperationRunner
                                         base::FilePath* platform_path);
 
  private:
-  class BeginOperationScoper;
-
-  struct OperationHandle {
-    OperationID id;
-    base::WeakPtr<BeginOperationScoper> scope;
-
-    OperationHandle();
-    OperationHandle(const OperationHandle& other);
-    ~OperationHandle();
-  };
-
   friend class FileSystemContext;
   explicit FileSystemOperationRunner(FileSystemContext* file_system_context);
 
-  void DidFinish(const OperationHandle& handle,
+  void DidFinish(const OperationID id,
                  const StatusCallback& callback,
                  base::File::Error rv);
-  void DidGetMetadata(const OperationHandle& handle,
+  void DidGetMetadata(const OperationID id,
                       const GetMetadataCallback& callback,
                       base::File::Error rv,
                       const base::File::Info& file_info);
-  void DidReadDirectory(const OperationHandle& handle,
+  void DidReadDirectory(const OperationID id,
                         const ReadDirectoryCallback& callback,
                         base::File::Error rv,
                         std::vector<filesystem::mojom::DirectoryEntry> entries,
                         bool has_more);
-  void DidWrite(const OperationHandle& handle,
+  void DidWrite(const OperationID id,
                 const WriteCallback& callback,
                 base::File::Error rv,
                 int64_t bytes,
                 bool complete);
-  void DidOpenFile(const OperationHandle& handle,
+  void DidOpenFile(const OperationID id,
                    const OpenFileCallback& callback,
                    base::File file,
                    base::OnceClosure on_close_callback);
   void DidCreateSnapshot(
-      const OperationHandle& handle,
+      const OperationID id,
       const SnapshotFileCallback& callback,
       base::File::Error rv,
       const base::File::Info& file_info,
       const base::FilePath& platform_path,
       scoped_refptr<storage::ShareableFileReference> file_ref);
 
-  void OnCopyProgress(const OperationHandle& handle,
+  void OnCopyProgress(const OperationID id,
                       const CopyProgressCallback& callback,
                       FileSystemOperation::CopyProgressType type,
                       const FileSystemURL& source_url,
@@ -300,8 +288,7 @@ class STORAGE_EXPORT FileSystemOperationRunner
   void PrepareForRead(OperationID id, const FileSystemURL& url);
 
   // These must be called at the beginning and end of any async operations.
-  OperationHandle BeginOperation(std::unique_ptr<FileSystemOperation> operation,
-                                 base::WeakPtr<BeginOperationScoper> scope);
+  OperationID BeginOperation(std::unique_ptr<FileSystemOperation> operation);
   // Cleans up the FileSystemOperation for |id|, which may result in the
   // FileSystemContext, and |this| being deleted, by the time the call returns.
   void FinishOperation(OperationID id);
@@ -314,6 +301,12 @@ class STORAGE_EXPORT FileSystemOperationRunner
   OperationID next_operation_id_ = 1;
   Operations operations_;
 
+  // Used to detect synchronous invocation of completion callbacks by the
+  // back-end, to re-post them to be notified asynchronously. Note that some
+  // operations are recursive, so this may already be true when BeginOperation
+  // is called.
+  bool is_beginning_operation_ = false;
+
   // We keep track of the file to be modified by each operation so that
   // we can notify observers when we're done.
   using OperationToURLSet = std::map<OperationID, FileSystemURLSet>;
@@ -325,6 +318,9 @@ class STORAGE_EXPORT FileSystemOperationRunner
   // Callbacks for stray cancels whose target operation is already finished.
   std::map<OperationID, StatusCallback> stray_cancel_callbacks_;
 
+  base::WeakPtr<FileSystemOperationRunner> weak_ptr_;
+  base::WeakPtrFactory<FileSystemOperationRunner> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperationRunner);
 };