Guarantee Close finishes regardless of mojo pipe

Old: FileWriter deleted when mojo pipe severed
New: FileWriter finishes operation regardless of status of mojo pipe

- Formerly static methods made non-static
- Pre-req for autoClose flag

Bug: 1135687
Change-Id: I3ae992a1c4015bc9ce1e19760633a83432825d16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552627
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835888}

content/browser/file_system_access/native_file_system_file_writer_impl.cc

@@ -338,10 +338,26 @@ class BlobReaderClient : public base::SupportsWeakPtr<BlobReaderClient>,
 
 }  // namespace
 
+// Do not call this method if |close_callback_| is not set.
+void NativeFileSystemFileWriterImpl::CallCloseCallbackAndMaybeDeleteThis(
+    blink::mojom::NativeFileSystemErrorPtr result) {
+  std::move(close_callback_).Run(std::move(result));
+
+  if (!receiver_.is_bound()) {
+    // |this| is deleted after this call.
+    manager()->RemoveFileWriter(this);
+  }
+}
+
 void NativeFileSystemFileWriterImpl::OnDisconnect() {
   // TODO(https://crbug.com/1135687): Auto-close file on disconnect if flag is
   // specified.
-  manager()->RemoveFileWriter(this);
+  receiver_.reset();
+
+  if (!close_callback_) {
+    // |this| is deleted after this call.
+    manager()->RemoveFileWriter(this);
+  }
 }
 
 void NativeFileSystemFileWriterImpl::WriteImpl(
@@ -468,87 +484,65 @@ void NativeFileSystemFileWriterImpl::CloseImpl(CloseCallback callback) {
     return;
   }
 
-  // Should the writer be destructed at this point, we want to allow the
-  // close operation to run its course, so we should not purge the swap file.
-  // If the after write check fails, the callback for that will clean up the
-  // swap file even if the writer was destroyed at that point.
+  close_callback_ = std::move(callback);
   state_ = State::kClosePending;
 
   if (!RequireSecurityChecks() || !manager()->permission_context()) {
-    DidPassAfterWriteCheck(std::move(callback));
+    DidAfterWriteCheck(
+        NativeFileSystemPermissionContext::AfterWriteCheckResult::kAllow);
     return;
   }
 
-  ComputeHashForSwapFile(base::BindOnce(
-      &NativeFileSystemFileWriterImpl::DoAfterWriteCheck,
-      weak_factory_.GetWeakPtr(), base::WrapRefCounted(manager()), swap_url(),
-      std::move(callback)));
+  ComputeHashForSwapFile(
+      base::BindOnce(&NativeFileSystemFileWriterImpl::DoAfterWriteCheck,
+                     weak_factory_.GetWeakPtr()));
 }
 
-// static
 void NativeFileSystemFileWriterImpl::DoAfterWriteCheck(
-    base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-    scoped_refptr<NativeFileSystemManagerImpl> manager,
-    const storage::FileSystemURL& swap_url,
-    NativeFileSystemFileWriterImpl::CloseCallback callback,
     base::File::Error hash_result,
     const std::string& hash,
     int64_t size) {
-  if (!file_writer || hash_result != base::File::FILE_OK) {
-    // If writer was deleted, or calculating the hash failed try deleting the
-    // swap file and invoke the callback.
-    manager->operation_runner().PostTaskWithThisObject(
-        FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url));
-    std::move(callback).Run(native_file_system_error::FromStatus(
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (hash_result != base::File::FILE_OK) {
+    // Calculating the hash failed try deleting the swap file and invoke the
+    // callback.
+    manager()->operation_runner().PostTaskWithThisObject(
+        FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url()));
+    CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::FromStatus(
         NativeFileSystemStatus::kOperationAborted,
         "Failed to perform Safe Browsing check."));
     return;
   }
 
-  DCHECK_CALLED_ON_VALID_SEQUENCE(file_writer->sequence_checker_);
-
   auto item = std::make_unique<NativeFileSystemWriteItem>();
-  item->target_file_path = file_writer->url().path();
-  item->full_path = file_writer->swap_url().path();
+  item->target_file_path = url().path();
+  item->full_path = swap_url().path();
   item->sha256_hash = hash;
   item->size = size;
-  item->frame_url = file_writer->context().url;
-  item->has_user_gesture = file_writer->has_transient_user_activation_;
-  file_writer->manager()->permission_context()->PerformAfterWriteChecks(
-      std::move(item), file_writer->context().frame_id,
+  item->frame_url = context().url;
+  item->has_user_gesture = has_transient_user_activation_;
+  manager()->permission_context()->PerformAfterWriteChecks(
+      std::move(item), context().frame_id,
       base::BindOnce(&NativeFileSystemFileWriterImpl::DidAfterWriteCheck,
-                     file_writer, std::move(manager), swap_url,
-                     std::move(callback)));
+                     weak_factory_.GetWeakPtr()));
 }
 
-// static
 void NativeFileSystemFileWriterImpl::DidAfterWriteCheck(
-    base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-    scoped_refptr<NativeFileSystemManagerImpl> manager,
-    const storage::FileSystemURL& swap_url,
-    NativeFileSystemFileWriterImpl::CloseCallback callback,
     NativeFileSystemPermissionContext::AfterWriteCheckResult result) {
-  if (file_writer &&
-      result ==
-          NativeFileSystemPermissionContext::AfterWriteCheckResult::kAllow) {
-    file_writer->DidPassAfterWriteCheck(std::move(callback));
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (result !=
+      NativeFileSystemPermissionContext::AfterWriteCheckResult::kAllow) {
+    // Safe browsing check failed. In this case we should try deleting the swap
+    // file and call the callback to report that close failed.
+    manager()->operation_runner().PostTaskWithThisObject(
+        FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url()));
+    CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::FromStatus(
+        NativeFileSystemStatus::kOperationAborted,
+        "Write operation blocked by Safe Browsing."));
     return;
   }
 
-  // Writer is gone, or safe browsing check failed. In this case we should
-  // try deleting the swap file and call the callback to report that close
-  // failed.
-  manager->operation_runner().PostTaskWithThisObject(
-      FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url));
-  std::move(callback).Run(native_file_system_error::FromStatus(
-      NativeFileSystemStatus::kOperationAborted,
-      "Write operation blocked by Safe Browsing."));
-  return;
-}
-
-void NativeFileSystemFileWriterImpl::DidPassAfterWriteCheck(
-    CloseCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // If the move operation succeeds, the path pointing to the swap file
   // will not exist anymore.
   // In case of error, the swap file URL will point to a valid filesystem
@@ -564,11 +558,11 @@ void NativeFileSystemFileWriterImpl::DidPassAfterWriteCheck(
     result_callback =
         base::BindOnce(&NativeFileSystemFileWriterImpl::DidSwapFileDoQuarantine,
                        weak_factory_.GetWeakPtr(), url(), referrer_url,
-                       std::move(quarantine_remote), std::move(callback));
+                       std::move(quarantine_remote));
   } else {
     result_callback = base::BindOnce(
         &NativeFileSystemFileWriterImpl::DidSwapFileSkipQuarantine,
-        weak_factory_.GetWeakPtr(), std::move(callback));
+        weak_factory_.GetWeakPtr());
   }
   DoFileSystemOperation(
       FROM_HERE, &FileSystemOperationRunner::MoveFileLocal,
@@ -577,7 +571,6 @@ void NativeFileSystemFileWriterImpl::DidPassAfterWriteCheck(
 }
 
 void NativeFileSystemFileWriterImpl::DidSwapFileSkipQuarantine(
-    CloseCallback callback,
     base::File::Error result) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (result != base::File::FILE_OK) {
@@ -585,31 +578,28 @@ void NativeFileSystemFileWriterImpl::DidSwapFileSkipQuarantine(
     DLOG(ERROR) << "Swap file move operation failed source: "
                 << swap_url().path() << " dest: " << url().path()
                 << " error: " << base::File::ErrorToString(result);
-    std::move(callback).Run(native_file_system_error::FromFileError(result));
+    CallCloseCallbackAndMaybeDeleteThis(
+        native_file_system_error::FromFileError(result));
     return;
   }
 
   state_ = State::kClosed;
-  std::move(callback).Run(native_file_system_error::Ok());
+  CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::Ok());
 }
 
-// static
 void NativeFileSystemFileWriterImpl::DidSwapFileDoQuarantine(
-    base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
     const storage::FileSystemURL& target_url,
     const GURL& referrer_url,
     mojo::Remote<quarantine::mojom::Quarantine> quarantine_remote,
-    CloseCallback callback,
     base::File::Error result) {
-  if (file_writer)
-    DCHECK_CALLED_ON_VALID_SEQUENCE(file_writer->sequence_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (result != base::File::FILE_OK) {
-    if (file_writer)
-      file_writer->state_ = State::kCloseError;
+    state_ = State::kCloseError;
     DLOG(ERROR) << "Swap file move operation failed dest: " << target_url.path()
                 << " error: " << base::File::ErrorToString(result);
-    std::move(callback).Run(native_file_system_error::FromFileError(result));
+    CallCloseCallbackAndMaybeDeleteThis(
+        native_file_system_error::FromFileError(result));
     return;
   }
 
@@ -642,7 +632,7 @@ void NativeFileSystemFileWriterImpl::DidSwapFileDoQuarantine(
             ->GetApplicationClientGUIDForQuarantineCheck(),
         mojo::WrapCallbackWithDefaultInvokeIfNotRun(
             base::BindOnce(&NativeFileSystemFileWriterImpl::DidAnnotateFile,
-                           std::move(file_writer), std::move(callback),
+                           weak_factory_.GetWeakPtr(),
                            std::move(quarantine_remote)),
             quarantine::mojom::QuarantineFileResult::ANNOTATION_FAILED));
   } else {
@@ -652,20 +642,16 @@ void NativeFileSystemFileWriterImpl::DidSwapFileDoQuarantine(
         base::BindOnce(&quarantine::SetInternetZoneIdentifierDirectly,
                        target_url.path(), authority_url, referrer_url),
         base::BindOnce(&NativeFileSystemFileWriterImpl::DidAnnotateFile,
-                       std::move(file_writer), std::move(callback),
+                       weak_factory_.GetWeakPtr(),
                        std::move(quarantine_remote)));
 #else
-    if (file_writer) {
-      file_writer->DidAnnotateFile(
-          std::move(callback), std::move(quarantine_remote),
-          quarantine::mojom::QuarantineFileResult::ANNOTATION_FAILED);
-    }
+    DidAnnotateFile(std::move(quarantine_remote),
+                    quarantine::mojom::QuarantineFileResult::ANNOTATION_FAILED);
 #endif
   }
 }
 
 void NativeFileSystemFileWriterImpl::DidAnnotateFile(
-    CloseCallback callback,
     mojo::Remote<quarantine::mojom::Quarantine> quarantine_remote,
     quarantine::mojom::QuarantineFileResult result) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -677,13 +663,13 @@ void NativeFileSystemFileWriterImpl::DidAnnotateFile(
     // file will be deleted at this point by AttachmentServices on Windows.
     // There is nothing to do except to return the error message to the
     // application.
-    std::move(callback).Run(native_file_system_error::FromStatus(
+    CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::FromStatus(
         NativeFileSystemStatus::kOperationAborted,
         "Write operation aborted due to security policy."));
     return;
   }
 
-  std::move(callback).Run(native_file_system_error::Ok());
+  CallCloseCallbackAndMaybeDeleteThis(native_file_system_error::Ok());
 }
 
 void NativeFileSystemFileWriterImpl::ComputeHashForSwapFile(

content/browser/file_system_access/native_file_system_file_writer_impl.h

@@ -79,6 +79,10 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
 
   void OnDisconnect();
 
+  // Delete the FileWriter after Close if the mojo pipe is unbound.
+  void CallCloseCallbackAndMaybeDeleteThis(
+      blink::mojom::NativeFileSystemErrorPtr result);
+
   void WriteImpl(uint64_t offset,
                  mojo::PendingRemote<blink::mojom::Blob> data,
                  WriteCallback callback);
@@ -91,34 +95,18 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
                 bool complete);
   void TruncateImpl(uint64_t length, TruncateCallback callback);
   void CloseImpl(CloseCallback callback);
-  // The following two methods are static, because they need to be invoked to
-  // perform cleanup even if the writer was deleted before they were invoked.
-  static void DoAfterWriteCheck(
-      base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-      scoped_refptr<NativeFileSystemManagerImpl> manager,
-      const storage::FileSystemURL& swap_url,
-      NativeFileSystemFileWriterImpl::CloseCallback callback,
-      base::File::Error hash_result,
-      const std::string& hash,
-      int64_t size);
-  static void DidAfterWriteCheck(
-      base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-      scoped_refptr<NativeFileSystemManagerImpl> manager,
-      const storage::FileSystemURL& swap_url,
-      NativeFileSystemFileWriterImpl::CloseCallback callback,
+  void DoAfterWriteCheck(base::File::Error hash_result,
+                         const std::string& hash,
+                         int64_t size);
+  void DidAfterWriteCheck(
       NativeFileSystemPermissionContext::AfterWriteCheckResult result);
-  void DidPassAfterWriteCheck(CloseCallback callback);
-  void DidSwapFileSkipQuarantine(CloseCallback callback,
-                                 base::File::Error result);
-  static void DidSwapFileDoQuarantine(
-      base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
+  void DidSwapFileSkipQuarantine(base::File::Error result);
+  void DidSwapFileDoQuarantine(
       const storage::FileSystemURL& target_url,
       const GURL& referrer_url,
       mojo::Remote<quarantine::mojom::Quarantine> quarantine_remote,
-      CloseCallback callback,
       base::File::Error result);
   void DidAnnotateFile(
-      CloseCallback callback,
       mojo::Remote<quarantine::mojom::Quarantine> quarantine_remote,
       quarantine::mojom::QuarantineFileResult result);
 
@@ -156,7 +144,20 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
   // execute a move operation from the swap URL to the target URL at `url_`. In
   // most filesystems, this move operation is atomic.
   storage::FileSystemURL swap_url_;
+
+  // NativeFileSystemWriter lifetime management has the following cases:
+  // 1) The mojo pipe is severed before Close() is invoked.
+  //    - Abort the transaction from the OnDisconnect method.
+  // 2) The mojo pipe is severed before Close() finishes.
+  //    - The Close() call is allowed to finish.
+  //    - The Writer is destroyed immediately afterwards, via the
+  //      CallCloseCallbackAndMaybeDeleteThis method.
+  // 3) The mojo pipe exists when Close() finishes.
+  //    - The Writer will exist for as long as the mojo pipe is connected.
+  //    - The Writer is destroyed via the OnDisconnect method.
   State state_ = State::kOpen;
+  // This callback is non-null when the State is kClosePending or kCloseError.
+  CloseCallback close_callback_;
 
   download::QuarantineConnectionCallback quarantine_connection_callback_;
 

content/browser/file_system_access/native_file_system_file_writer_impl_unittest.cc

@@ -647,7 +647,8 @@ TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest, Block) {
       file_system_context_.get(), test_file_url_, 0));
 }
 
-TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest, HandleCloseDuringCheck) {
+TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest,
+       HandleCloseDuringCheckOK) {
   uint64_t bytes_written;
   NativeFileSystemStatus result = WriteSync(0, "abc", &bytes_written);
   EXPECT_EQ(result, NativeFileSystemStatus::kOk);
@@ -679,6 +680,48 @@ TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest, HandleCloseDuringCheck) {
   std::move(sb_callback)
       .Run(NativeFileSystemPermissionContext::AfterWriteCheckResult::kAllow);
 
+  // Swap file should now be deleted, target file should be written out.
+  task_environment_.RunUntilIdle();
+  EXPECT_FALSE(storage::AsyncFileTestHelper::FileExists(
+      file_system_context_.get(), test_swap_url_,
+      storage::AsyncFileTestHelper::kDontCheckSize));
+  EXPECT_TRUE(storage::AsyncFileTestHelper::FileExists(
+      file_system_context_.get(), test_file_url_, 3));
+}
+
+TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest,
+       HandleCloseDuringCheckNotOK) {
+  uint64_t bytes_written;
+  NativeFileSystemStatus result = WriteSync(0, "abc", &bytes_written);
+  EXPECT_EQ(result, NativeFileSystemStatus::kOk);
+  EXPECT_EQ(bytes_written, 3u);
+
+  using SBCallback = base::OnceCallback<void(
+      NativeFileSystemPermissionContext::AfterWriteCheckResult)>;
+  SBCallback sb_callback;
+  base::RunLoop loop;
+  EXPECT_CALL(permission_context_, PerformAfterWriteChecks_)
+      .WillOnce(testing::Invoke([&](NativeFileSystemWriteItem* item,
+                                    GlobalFrameRoutingId frame_id,
+                                    SBCallback& callback) {
+        sb_callback = std::move(callback);
+        loop.Quit();
+      }));
+
+  handle_->Close(base::DoNothing());
+  loop.Run();
+
+  remote_.reset();
+  // Destructor should not have deleted swap file with an active safe browsing
+  // check pending.
+  task_environment_.RunUntilIdle();
+  EXPECT_TRUE(storage::AsyncFileTestHelper::FileExists(
+      file_system_context_.get(), test_swap_url_,
+      storage::AsyncFileTestHelper::kDontCheckSize));
+
+  std::move(sb_callback)
+      .Run(NativeFileSystemPermissionContext::AfterWriteCheckResult::kBlock);
+
   // Swap file should now be deleted, target file should be unmodified.
   task_environment_.RunUntilIdle();
   EXPECT_FALSE(storage::AsyncFileTestHelper::FileExists(

content/browser/file_system_access/native_file_system_manager_impl_unittest.cc

@@ -517,7 +517,7 @@ TEST_F(NativeFileSystemManagerImplTest,
       storage::AsyncFileTestHelper::kDontCheckSize));
 }
 
-TEST_F(NativeFileSystemManagerImplTest, FileWriterCloseAbortsOnDestruct) {
+TEST_F(NativeFileSystemManagerImplTest, FileWriterCloseDoesNotAbortOnDestruct) {
   auto test_file_url = file_system_context_->CreateCrackedFileSystemURL(
       kTestOrigin, storage::kFileSystemTypeTest,
       base::FilePath::FromUTF8Unsafe("test"));
@@ -541,18 +541,54 @@ TEST_F(NativeFileSystemManagerImplTest, FileWriterCloseAbortsOnDestruct) {
       storage::AsyncFileTestHelper::kDontCheckSize));
   writer_remote->Close(base::DoNothing());
 
-  // Severs the mojo pipe, causing the writer to be destroyed.
+  EXPECT_CALL(permission_context_,
+              PerformAfterWriteChecks_(testing::_, kFrameId, testing::_))
+      .WillOnce(base::test::RunOnceCallback<2>(
+          NativeFileSystemPermissionContext::AfterWriteCheckResult::kAllow));
+
+  // Severs the mojo pipe, but the writer should not be destroyed.
   writer_remote.reset();
   base::RunLoop().RunUntilIdle();
 
-  // Since the writer was destroyed before close completed, the swap file should
-  // have been destroyed and the target file should have been left untouched.
+  // Since the close should complete, the swap file should have been destroyed
+  // and the write should be reflected in the target file.
   ASSERT_FALSE(storage::AsyncFileTestHelper::FileExists(
       file_system_context_.get(), test_swap_url,
       storage::AsyncFileTestHelper::kDontCheckSize));
+  ASSERT_TRUE(storage::AsyncFileTestHelper::FileExists(
+      file_system_context_.get(), test_file_url, 3));
+}
+
+TEST_F(NativeFileSystemManagerImplTest,
+       FileWriterNoWritesIfConnectionLostBeforeClose) {
+  auto test_file_url = file_system_context_->CreateCrackedFileSystemURL(
+      kTestOrigin, storage::kFileSystemTypeTest,
+      base::FilePath::FromUTF8Unsafe("test"));
+
+  auto test_swap_url = file_system_context_->CreateCrackedFileSystemURL(
+      kTestOrigin, storage::kFileSystemTypeTest,
+      base::FilePath::FromUTF8Unsafe("test.crswap"));
+
+  ASSERT_EQ(base::File::FILE_OK,
+            storage::AsyncFileTestHelper::CreateFileWithData(
+                file_system_context_.get(), test_swap_url, "foo", 3));
+
+  mojo::Remote<blink::mojom::NativeFileSystemFileWriter> writer_remote(
+      manager_->CreateFileWriter(kBindingContext, test_file_url, test_swap_url,
+                                 NativeFileSystemManagerImpl::SharedHandleState(
+                                     allow_grant_, allow_grant_, {})));
+
+  // Severs the mojo pipe. The writer should be destroyed.
+  writer_remote.reset();
+  base::RunLoop().RunUntilIdle();
+
+  // Neither the target file nor the swap file should exist.
   ASSERT_FALSE(storage::AsyncFileTestHelper::FileExists(
       file_system_context_.get(), test_file_url,
       storage::AsyncFileTestHelper::kDontCheckSize));
+  ASSERT_FALSE(storage::AsyncFileTestHelper::FileExists(
+      file_system_context_.get(), test_swap_url,
+      storage::AsyncFileTestHelper::kDontCheckSize));
 }
 
 TEST_F(NativeFileSystemManagerImplTest, SerializeHandle_SandboxedFile) {

third_party/blink/public/mojom/file_system_access/native_file_system_file_writer.mojom

@@ -32,9 +32,6 @@ interface NativeFileSystemFileWriter {
 
   // Closes the file writer. This will materialize the writes operations on the
   // intended file target in the case of atomic writes.
-  // Close() will close the mojo pipe after completion.
-  // If the mojo pipe closes before Close() is invoked, the write operation is deemed
-  // unsuccessful. Any temporary artifacts will be deleted.
   // Returns whether the operation succeeded.
   Close() => (NativeFileSystemError result);
 };