Revert "Simplify implementation of FileSystemOperationRunner."

This reverts commit d3c0d246.

Reason for revert: Speculative revert for issue 864351, which started
on Canary channel the day after this was landed. The tests in the
FileSystemURLLoaderFactoryTest suite remain disabled, because they
hit a NetworkService feature check now, under Cast, if re-enabled.

Original change's description:
> 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}

TBR=wez@chromium.org,rockot@chromium.org,alemate@chromium.org,noel@chromium.org,fukino@chromium.org,cmumford@chromium.org,tzik@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 846985, 860547, 864351
Change-Id: Ib858c73226785d9fbef3459d49e6cc3fd670d040
Reviewed-on: https://chromium-review.googlesource.com/1179782
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584152}

chrome/browser/chromeos/file_manager/fileapi_util.cc

@@ -406,25 +406,6 @@ 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;
@@ -567,7 +548,9 @@ void CheckIfDirectoryExists(
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&CheckIfDirectoryExistsOnIoThread, file_system_context,
+      base::BindOnce(base::IgnoreResult(
+                         &storage::FileSystemOperationRunner::DirectoryExists),
+                     file_system_context->operation_runner()->AsWeakPtr(),
                      internal_url, google_apis::CreateRelayCallback(callback)));
 }
 
@@ -586,9 +569,10 @@ void GetMetadataForPath(
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&GetMetadataForPathOnIoThread, file_system_context,
-                     internal_url, fields,
-                     google_apis::CreateRelayCallback(callback)));
+      base::BindOnce(
+          base::IgnoreResult(&storage::FileSystemOperationRunner::GetMetadata),
+          file_system_context->operation_runner()->AsWeakPtr(), internal_url,
+          fields, google_apis::CreateRelayCallback(callback)));
 }
 
 storage::FileSystemURL CreateIsolatedURLFromVirtualPath(

extensions/browser/api/file_system/file_system_api.cc

@@ -825,14 +825,12 @@ 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),
-            base::Unretained(context->operation_runner()), url,
+            context->operation_runner()->AsWeakPtr(), url,
             storage::FileSystemOperation::GET_METADATA_FIELD_IS_DIRECTORY,
             base::Bind(
                 &PassFileInfoToUIThread,

storage/browser/fileapi/file_system_operation_impl.cc

@@ -380,8 +380,6 @@ FileSystemOperationImpl::FileSystemOperationImpl(
       async_file_util_(nullptr),
       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());
@@ -409,7 +407,7 @@ void FileSystemOperationImpl::GetUsageAndQuotaThenRunTask(
   quota_manager_proxy->quota_manager()->GetUsageAndQuota(
       url.origin(), FileSystemTypeToQuotaStorageType(url.type()),
       base::BindOnce(&FileSystemOperationImpl::DidGetUsageAndQuotaAndRunTask,
-                     weak_ptr_, task, error_callback));
+                     weak_factory_.GetWeakPtr(), task, error_callback));
 }
 
 void FileSystemOperationImpl::DidGetUsageAndQuotaAndRunTask(
@@ -437,7 +435,7 @@ void FileSystemOperationImpl::DoCreateFile(
       base::BindOnce(
           exclusive ? &FileSystemOperationImpl::DidEnsureFileExistsExclusive
                     : &FileSystemOperationImpl::DidEnsureFileExistsNonExclusive,
-          weak_ptr_, callback));
+          weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DoCreateDirectory(
@@ -446,8 +444,8 @@ void FileSystemOperationImpl::DoCreateDirectory(
     bool exclusive, bool recursive) {
   async_file_util_->CreateDirectory(
       std::move(operation_context_), url, exclusive, recursive,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
-                     callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
+                     weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DoCopyFileLocal(
@@ -459,8 +457,8 @@ void FileSystemOperationImpl::DoCopyFileLocal(
   async_file_util_->CopyFileLocal(
       std::move(operation_context_), src_url, dest_url, option,
       progress_callback,
-      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
-                     callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
+                     weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DoMoveFileLocal(
@@ -470,8 +468,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_ptr_,
-                     callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
+                     weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DoCopyInForeignFile(
@@ -480,8 +478,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_ptr_,
-                     callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
+                     weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DoTruncate(const FileSystemURL& url,
@@ -489,8 +487,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_ptr_,
-                     callback));
+      base::BindOnce(&FileSystemOperationImpl::DidFinishOperation,
+                     weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DoOpenFile(const FileSystemURL& url,
@@ -498,7 +496,8 @@ 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_ptr_, callback));
+      base::BindOnce(&DidOpenFile, file_system_context_,
+                     weak_factory_.GetWeakPtr(), callback));
 }
 
 void FileSystemOperationImpl::DidEnsureFileExistsExclusive(
@@ -560,8 +559,8 @@ void FileSystemOperationImpl::DidDeleteRecursively(
     DCHECK(!recursive_operation_delegate_);
     recursive_operation_delegate_.reset(new RemoveOperationDelegate(
         file_system_context(), url,
-        base::Bind(&FileSystemOperationImpl::DidFinishOperation, weak_ptr_,
-                   callback)));
+        base::Bind(&FileSystemOperationImpl::DidFinishOperation,
+                   weak_factory_.GetWeakPtr(), callback)));
     recursive_operation_delegate_->RunRecursively();
     return;
   }

storage/browser/fileapi/file_system_operation_impl.h

@@ -198,7 +198,6 @@ 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

@@ -33,7 +33,8 @@ 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 {
+class STORAGE_EXPORT FileSystemOperationRunner
+    : public base::SupportsWeakPtr<FileSystemOperationRunner> {
  public:
   using GetMetadataCallback = FileSystemOperation::GetMetadataCallback;
   using ReadDirectoryCallback = FileSystemOperation::ReadDirectoryCallback;
@@ -239,39 +240,50 @@ 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 OperationID id,
+  void DidFinish(const OperationHandle& handle,
                  const StatusCallback& callback,
                  base::File::Error rv);
-  void DidGetMetadata(const OperationID id,
+  void DidGetMetadata(const OperationHandle& handle,
                       const GetMetadataCallback& callback,
                       base::File::Error rv,
                       const base::File::Info& file_info);
-  void DidReadDirectory(const OperationID id,
+  void DidReadDirectory(const OperationHandle& handle,
                         const ReadDirectoryCallback& callback,
                         base::File::Error rv,
                         std::vector<filesystem::mojom::DirectoryEntry> entries,
                         bool has_more);
-  void DidWrite(const OperationID id,
+  void DidWrite(const OperationHandle& handle,
                 const WriteCallback& callback,
                 base::File::Error rv,
                 int64_t bytes,
                 bool complete);
-  void DidOpenFile(const OperationID id,
+  void DidOpenFile(const OperationHandle& handle,
                    const OpenFileCallback& callback,
                    base::File file,
                    base::OnceClosure on_close_callback);
   void DidCreateSnapshot(
-      const OperationID id,
+      const OperationHandle& handle,
       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 OperationID id,
+  void OnCopyProgress(const OperationHandle& handle,
                       const CopyProgressCallback& callback,
                       FileSystemOperation::CopyProgressType type,
                       const FileSystemURL& source_url,
@@ -282,7 +294,8 @@ class STORAGE_EXPORT FileSystemOperationRunner {
   void PrepareForRead(OperationID id, const FileSystemURL& url);
 
   // These must be called at the beginning and end of any async operations.
-  OperationID BeginOperation(std::unique_ptr<FileSystemOperation> operation);
+  OperationHandle BeginOperation(std::unique_ptr<FileSystemOperation> operation,
+                                 base::WeakPtr<BeginOperationScoper> scope);
   // 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);
@@ -295,12 +308,6 @@ 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>;
@@ -312,9 +319,6 @@ 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);
 };