Decouple lifetime of NFSFileWriter from mojo pipe

- This CL contains no behavioral changes
- NFSManager stores flat_set of NFSWriterImpls rather than a
  UniqueReceiverSet of blink::mojom::NFSWriters
- FileWriter destroys itself via disconnect handler
- Opens the door for autoClose flag as described here:
  https://github.com/WICG/file-system-access/issues/236

Bug: 1135687
Change-Id: Icbc7375174ee2d48dca481d6490d5eddc282ccff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523653
Auto-Submit: Austin Sullivan <asully@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828009}

content/browser/file_system_access/native_file_system_file_writer_impl.cc

@@ -136,18 +136,23 @@ struct NativeFileSystemFileWriterImpl::WriteState {
 
 NativeFileSystemFileWriterImpl::NativeFileSystemFileWriterImpl(
     NativeFileSystemManagerImpl* manager,
+    util::PassKey<NativeFileSystemManagerImpl> pass_key,
     const BindingContext& context,
     const storage::FileSystemURL& url,
     const storage::FileSystemURL& swap_url,
     const SharedHandleState& handle_state,
+    mojo::PendingReceiver<blink::mojom::NativeFileSystemFileWriter> receiver,
     bool has_transient_user_activation,
     download::QuarantineConnectionCallback quarantine_connection_callback)
     : NativeFileSystemHandleBase(manager, context, url, handle_state),
+      receiver_(this, std::move(receiver)),
       swap_url_(swap_url),
       quarantine_connection_callback_(
           std::move(quarantine_connection_callback)),
       has_transient_user_activation_(has_transient_user_activation) {
   DCHECK_EQ(swap_url.type(), url.type());
+  receiver_.set_disconnect_handler(base::BindOnce(
+      &NativeFileSystemFileWriterImpl::OnDisconnect, base::Unretained(this)));
 }
 
 NativeFileSystemFileWriterImpl::~NativeFileSystemFileWriterImpl() {
@@ -332,6 +337,12 @@ class BlobReaderClient : public base::SupportsWeakPtr<BlobReaderClient>,
 
 }  // namespace
 
+void NativeFileSystemFileWriterImpl::OnDisconnect() {
+  // TODO(https://crbug.com/1135687): Auto-close file on disconnect if flag is
+  // specified.
+  manager()->RemoveFileWriter(this);
+}
+
 void NativeFileSystemFileWriterImpl::WriteImpl(
     uint64_t offset,
     mojo::PendingRemote<blink::mojom::Blob> data,

content/browser/file_system_access/native_file_system_file_writer_impl.h

@@ -6,6 +6,7 @@
 #define CONTENT_BROWSER_FILE_SYSTEM_ACCESS_NATIVE_FILE_SYSTEM_FILE_WRITER_IMPL_H_
 
 #include "base/memory/weak_ptr.h"
+#include "base/util/type_safety/pass_key.h"
 #include "components/download/public/common/quarantine_connection.h"
 #include "components/download/quarantine/quarantine.h"
 #include "components/services/filesystem/public/mojom/types.mojom.h"
@@ -13,7 +14,9 @@
 #include "content/browser/file_system_access/native_file_system_handle_base.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/native_file_system_permission_context.h"
+#include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver.h"
 #include "storage/browser/file_system/file_system_url.h"
 #include "third_party/blink/public/mojom/file_system_access/native_file_system_file_writer.mojom.h"
 
@@ -36,12 +39,15 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
   // file, and is valid.
   // If no |quarantine_connection_callback| is passed in no quarantine is done,
   // other than setting source information directly if on windows.
+  // FileWriters should only be created via the NativeFileSystemManagerImpl.
   NativeFileSystemFileWriterImpl(
       NativeFileSystemManagerImpl* manager,
+      util::PassKey<NativeFileSystemManagerImpl> pass_key,
       const BindingContext& context,
       const storage::FileSystemURL& url,
       const storage::FileSystemURL& swap_url,
       const SharedHandleState& handle_state,
+      mojo::PendingReceiver<blink::mojom::NativeFileSystemFileWriter> receiver,
       bool has_transient_user_activation,
       download::QuarantineConnectionCallback quarantine_connection_callback);
   ~NativeFileSystemFileWriterImpl() override;
@@ -69,6 +75,10 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
   // progress until the write completes.
   struct WriteState;
 
+  mojo::Receiver<blink::mojom::NativeFileSystemFileWriter> receiver_;
+
+  void OnDisconnect();
+
   void WriteImpl(uint64_t offset,
                  mojo::PendingRemote<blink::mojom::Blob> data,
                  WriteCallback callback);

content/browser/file_system_access/native_file_system_file_writer_impl_unittest.cc

@@ -163,18 +163,17 @@ class NativeFileSystemFileWriterImplTest : public testing::Test {
           quarantine_receivers_.Add(&quarantine_, std::move(receiver));
         });
 
-    handle_ = std::make_unique<NativeFileSystemFileWriterImpl>(
-        manager_.get(),
+    handle_ = manager_->CreateFileWriter(
         NativeFileSystemManagerImpl::BindingContext(kTestOrigin, kTestURL,
                                                     kFrameId),
         test_file_url_, test_swap_url_,
         NativeFileSystemManagerImpl::SharedHandleState(
             permission_grant_, permission_grant_, std::move(fs)),
+        remote_.InitWithNewPipeAndPassReceiver(),
         /*has_transient_user_activation=*/false, quarantine_callback_);
   }
 
   void TearDown() override {
-    handle_.reset();
     manager_.reset();
 
     task_environment_.RunUntilIdle();
@@ -336,7 +335,9 @@ class NativeFileSystemFileWriterImplTest : public testing::Test {
       base::MakeRefCounted<FixedNativeFileSystemPermissionGrant>(
           FixedNativeFileSystemPermissionGrant::PermissionStatus::GRANTED,
           base::FilePath());
-  std::unique_ptr<NativeFileSystemFileWriterImpl> handle_;
+
+  mojo::PendingRemote<blink::mojom::NativeFileSystemFileWriter> remote_;
+  NativeFileSystemFileWriterImpl* handle_;
 };
 
 class NativeFileSystemFileWriterImplWriteTest
@@ -667,7 +668,7 @@ TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest, HandleCloseDuringCheck) {
   handle_->Close(base::DoNothing());
   loop.Run();
 
-  handle_.reset();
+  remote_.reset();
   // Destructor should not have deleted swap file with an active safe browsing
   // check pending.
   task_environment_.RunUntilIdle();
@@ -707,13 +708,14 @@ TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest,
             storage::AsyncFileTestHelper::CreateFile(file_system_context_.get(),
                                                      test_swap_url_));
 
-  handle_ = std::make_unique<NativeFileSystemFileWriterImpl>(
-      manager_.get(),
+  mojo::PendingRemote<blink::mojom::NativeFileSystemFileWriter> remote;
+  handle_ = manager_->CreateFileWriter(
       NativeFileSystemManagerImpl::BindingContext(kTestOrigin, kTestURL,
                                                   kFrameId),
       test_file_url_, test_swap_url_,
       NativeFileSystemManagerImpl::SharedHandleState(permission_grant_,
                                                      permission_grant_, {}),
+      remote.InitWithNewPipeAndPassReceiver(),
       /*has_transient_user_activation=*/false, quarantine_callback_);
 
   uint64_t bytes_written;
@@ -748,7 +750,7 @@ TEST_F(NativeFileSystemFileWriterAfterWriteChecksTest,
   // About to start the move operation. Now destroy the writer. The
   // move will still complete, but make sure that quarantine was also
   // applied to the resulting file.
-  handle_.reset();
+  remote_.reset();
   task_environment_.RunUntilIdle();
 
   // Swap file should have been deleted since writer was closed.

content/browser/file_system_access/native_file_system_manager_impl.cc

@@ -718,18 +718,37 @@ NativeFileSystemManagerImpl::CreateFileWriter(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   mojo::PendingRemote<blink::mojom::NativeFileSystemFileWriter> result;
-
   RenderFrameHost* rfh = RenderFrameHost::FromID(binding_context.frame_id);
   bool has_transient_user_activation = rfh && rfh->HasTransientUserActivation();
-  writer_receivers_.Add(
-      std::make_unique<NativeFileSystemFileWriterImpl>(
-          this, binding_context, url, swap_url, handle_state,
-          has_transient_user_activation,
-          GetContentClient()->browser()->GetQuarantineConnectionCallback()),
-      result.InitWithNewPipeAndPassReceiver());
+
+  CreateFileWriter(
+      binding_context, url, swap_url, handle_state,
+      result.InitWithNewPipeAndPassReceiver(), has_transient_user_activation,
+      GetContentClient()->browser()->GetQuarantineConnectionCallback());
   return result;
 }
 
+NativeFileSystemFileWriterImpl* NativeFileSystemManagerImpl::CreateFileWriter(
+    const BindingContext& binding_context,
+    const storage::FileSystemURL& url,
+    const storage::FileSystemURL& swap_url,
+    const SharedHandleState& handle_state,
+    mojo::PendingReceiver<blink::mojom::NativeFileSystemFileWriter> receiver,
+    bool has_transient_user_activation,
+    download::QuarantineConnectionCallback quarantine_connection_callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  auto writer = std::make_unique<NativeFileSystemFileWriterImpl>(
+      this, PassKey(), binding_context, url, swap_url, handle_state,
+      std::move(receiver), has_transient_user_activation,
+      quarantine_connection_callback);
+
+  NativeFileSystemFileWriterImpl* writer_ptr = writer.get();
+  writer_receivers_.insert(std::move(writer));
+
+  return writer_ptr;
+}
+
 void NativeFileSystemManagerImpl::CreateTransferToken(
     const NativeFileSystemFileHandleImpl& file,
     mojo::PendingReceiver<blink::mojom::NativeFileSystemTransferToken>
@@ -1045,6 +1064,14 @@ void NativeFileSystemManagerImpl::CreateTransferTokenImpl(
   transfer_tokens_.emplace(token, std::move(token_impl));
 }
 
+void NativeFileSystemManagerImpl::RemoveFileWriter(
+    NativeFileSystemFileWriterImpl* writer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  size_t count_removed = writer_receivers_.erase(writer);
+  DCHECK_EQ(1u, count_removed);
+}
+
 void NativeFileSystemManagerImpl::RemoveToken(
     const base::UnguessableToken& token) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

content/browser/file_system_access/native_file_system_manager_impl.h

@@ -5,9 +5,13 @@
 #ifndef CONTENT_BROWSER_FILE_SYSTEM_ACCESS_NATIVE_FILE_SYSTEM_MANAGER_IMPL_H_
 #define CONTENT_BROWSER_FILE_SYSTEM_ACCESS_NATIVE_FILE_SYSTEM_MANAGER_IMPL_H_
 
+#include "base/containers/flat_set.h"
+#include "base/containers/unique_ptr_adapters.h"
 #include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/sequence_bound.h"
+#include "base/util/type_safety/pass_key.h"
+#include "components/download/public/common/quarantine_connection.h"
 #include "components/services/storage/public/mojom/native_file_system_context.mojom.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/file_system_access/file_system_chooser.h"
@@ -37,6 +41,7 @@ class NativeFileSystemFileHandleImpl;
 class NativeFileSystemDirectoryHandleImpl;
 class NativeFileSystemTransferTokenImpl;
 class NativeFileSystemDragDropTokenImpl;
+class NativeFileSystemFileWriterImpl;
 class StoragePartitionImpl;
 
 // This is the browser side implementation of the
@@ -56,6 +61,7 @@ class CONTENT_EXPORT NativeFileSystemManagerImpl
       public storage::mojom::NativeFileSystemContext {
  public:
   using BindingContext = NativeFileSystemEntryFactory::BindingContext;
+  using PassKey = util::PassKey<NativeFileSystemManagerImpl>;
 
   // State that is shared between handles that are derived from each other.
   // Handles that are created through ChooseEntries or GetSandboxedFileSystem
@@ -155,6 +161,16 @@ class CONTENT_EXPORT NativeFileSystemManagerImpl
                    const storage::FileSystemURL& url,
                    const storage::FileSystemURL& swap_url,
                    const SharedHandleState& handle_state);
+  // Returns a raw pointer to a newly created NativeFileSystemFileWriterImpl.
+  // Useful for tests
+  NativeFileSystemFileWriterImpl* CreateFileWriter(
+      const BindingContext& binding_context,
+      const storage::FileSystemURL& url,
+      const storage::FileSystemURL& swap_url,
+      const SharedHandleState& handle_state,
+      mojo::PendingReceiver<blink::mojom::NativeFileSystemFileWriter> receiver,
+      bool has_transient_user_activation,
+      download::QuarantineConnectionCallback quarantine_connection_callback);
 
   // Create a transfer token for a specific file or directory.
   void CreateTransferToken(
@@ -210,6 +226,10 @@ class CONTENT_EXPORT NativeFileSystemManagerImpl
     auto_file_picker_result_for_test_ = result_entry;
   }
 
+  // Remove |writer| from |writer_receivers|. It is an error to try to remove
+  // a writer that doesn't exist.
+  void RemoveFileWriter(NativeFileSystemFileWriterImpl* writer);
+
   // Remove |token| from |transfer_tokens_|. It is an error to try to remove
   // a token that doesn't exist.
   void RemoveToken(const base::UnguessableToken& token);
@@ -340,7 +360,8 @@ class CONTENT_EXPORT NativeFileSystemManagerImpl
       file_receivers_;
   mojo::UniqueReceiverSet<blink::mojom::NativeFileSystemDirectoryHandle>
       directory_receivers_;
-  mojo::UniqueReceiverSet<blink::mojom::NativeFileSystemFileWriter>
+  base::flat_set<std::unique_ptr<NativeFileSystemFileWriterImpl>,
+                 base::UniquePtrComparator>
       writer_receivers_;
 
   bool off_the_record_;