[NativeFS] Decouple remaining IO thread bound work from the rest of the code.

This refactors the blob interactions to either be behind a PostTask to the IO
thread, or in the case of writes eliminates the direct interaction with the blob
system entirely by reading the blob through its mojo interface instead.

Also migrates one remaining FileSystemContext call to be behind a PostTask to
the IO thread.

This, combined with a previous CL, eliminates all code that required running on
the IO thread, paving the way for moving most of the native file system API
implementation to the UI thread.

Bug: 1011534
Change-Id: I8ef3ce97a77efb42bd12b020e98d7c1430ca3292
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845914Reviewed-by: Olivier Yiptong <oyiptong@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705698}

content/browser/native_file_system/native_file_system_file_handle_impl.cc

@@ -7,7 +7,9 @@
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
+#include "base/task/post_task.h"
 #include "content/browser/native_file_system/native_file_system_error.h"
+#include "content/public/browser/browser_task_traits.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "net/base/mime_util.h"
 #include "storage/browser/blob/blob_data_builder.h"
@@ -97,6 +99,42 @@ void NativeFileSystemFileHandleImpl::Transfer(
   manager()->CreateTransferToken(*this, std::move(token));
 }
 
+namespace {
+
+void CreateBlobOnIOThread(
+    scoped_refptr<storage::FileSystemContext> file_system_context,
+    const scoped_refptr<ChromeBlobStorageContext>& blob_context,
+    mojo::PendingReceiver<blink::mojom::Blob> blob_receiver,
+    const storage::FileSystemURL& url,
+    const std::string& blob_uuid,
+    const std::string& content_type,
+    const base::File::Info& info) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  auto blob_builder = std::make_unique<storage::BlobDataBuilder>(blob_uuid);
+  // Only append if the file has data.
+  if (info.size > 0) {
+    // Use AppendFileSystemFile here, since we're streaming the file directly
+    // from the file system backend, and the file thus might not actually be
+    // backed by a file on disk.
+    blob_builder->AppendFileSystemFile(url.ToGURL(), 0, info.size,
+                                       info.last_modified,
+                                       std::move(file_system_context));
+  }
+  blob_builder->set_content_type(content_type);
+
+  std::unique_ptr<BlobDataHandle> blob_handle =
+      blob_context->context()->AddFinishedBlob(std::move(blob_builder));
+
+  // Since the blob we're creating doesn't depend on other blobs, and doesn't
+  // require blob memory/disk quota, creating the blob can't fail.
+  DCHECK(!blob_handle->IsBroken());
+
+  BlobImpl::Create(std::move(blob_handle), std::move(blob_receiver));
+}
+
+}  // namespace
+
 void NativeFileSystemFileHandleImpl::DidGetMetaDataForBlob(
     AsBlobCallback callback,
     base::File::Error result,
@@ -110,17 +148,7 @@ void NativeFileSystemFileHandleImpl::DidGetMetaDataForBlob(
   }
 
   std::string uuid = base::GenerateGUID();
-  auto blob_builder = std::make_unique<storage::BlobDataBuilder>(uuid);
-
-  // Only append if the file has data.
-  if (info.size > 0) {
-    // Use AppendFileSystemFile here, since we're streaming the file directly
-    // from the file system backend, and the file thus might not actually be
-    // backed by a file on disk.
-    blob_builder->AppendFileSystemFile(url().ToGURL(), 0, info.size,
-                                       info.last_modified,
-                                       file_system_context());
-  }
+  std::string content_type;
 
   base::FilePath::StringType extension = url().path().Extension();
   if (!extension.empty()) {
@@ -130,28 +158,27 @@ void NativeFileSystemFileHandleImpl::DidGetMetaDataForBlob(
     // however that method can potentially block and thus can't be called from
     // the IO thread.
     if (net::GetWellKnownMimeTypeFromExtension(extension.substr(1), &mime_type))
-      blob_builder->set_content_type(mime_type);
-  }
-
-  std::unique_ptr<BlobDataHandle> blob_handle =
-      blob_context()->AddFinishedBlob(std::move(blob_builder));
-  if (blob_handle->IsBroken()) {
-    std::move(callback).Run(
-        native_file_system_error::FromStatus(
-            NativeFileSystemStatus::kOperationFailed, "Failed to create blob."),
-        nullptr);
-    return;
+      content_type = std::move(mime_type);
   }
+  // TODO(https://crbug.com/962306): Consider some kind of fallback type when
+  // the above mime type detection fails.
 
-  std::string content_type = blob_handle->content_type();
   mojo::PendingRemote<blink::mojom::Blob> blob_remote;
-  BlobImpl::Create(std::move(blob_handle),
-                   blob_remote.InitWithNewPipeAndPassReceiver());
+  mojo::PendingReceiver<blink::mojom::Blob> blob_receiver =
+      blob_remote.InitWithNewPipeAndPassReceiver();
 
   std::move(callback).Run(
       native_file_system_error::Ok(),
       blink::mojom::SerializedBlob::New(uuid, content_type, info.size,
                                         std::move(blob_remote)));
+
+  base::PostTask(
+      FROM_HERE, {BrowserThread::IO},
+      base::BindOnce(&CreateBlobOnIOThread,
+                     base::WrapRefCounted(file_system_context()),
+                     base::WrapRefCounted(manager()->blob_context()),
+                     std::move(blob_receiver), url(), std::move(uuid),
+                     std::move(content_type), info));
 }
 
 void NativeFileSystemFileHandleImpl::CreateFileWriterImpl(

content/browser/native_file_system/native_file_system_file_writer_impl.cc

@@ -14,6 +14,7 @@
 #include "crypto/secure_hash.h"
 #include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/fileapi/file_system_operation_runner.h"
+#include "third_party/blink/public/common/blob/blob_utils.h"
 #include "third_party/blink/public/mojom/blob/blob.mojom.h"
 #include "third_party/blink/public/mojom/native_file_system/native_file_system_error.mojom.h"
 
@@ -193,32 +194,31 @@ void NativeFileSystemFileWriterImpl::WriteImpl(
     return;
   }
 
-  blob_context()->GetBlobDataFromBlobRemote(
-      std::move(data),
-      base::BindOnce(&NativeFileSystemFileWriterImpl::DoWriteBlob,
-                     weak_factory_.GetWeakPtr(), std::move(callback), offset));
-}
-
-void NativeFileSystemFileWriterImpl::DoWriteBlob(
-    WriteCallback callback,
-    uint64_t position,
-    std::unique_ptr<BlobDataHandle> blob) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  if (!blob) {
+  MojoCreateDataPipeOptions options;
+  options.struct_size = sizeof(MojoCreateDataPipeOptions);
+  options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
+  options.element_num_bytes = 1;
+  options.capacity_num_bytes =
+      blink::BlobUtils::GetDataPipeCapacity(blink::BlobUtils::kUnknownSize);
+
+  mojo::ScopedDataPipeProducerHandle producer_handle;
+  mojo::ScopedDataPipeConsumerHandle consumer_handle;
+  MojoResult rv =
+      mojo::CreateDataPipe(&options, &producer_handle, &consumer_handle);
+  if (rv != MOJO_RESULT_OK) {
     std::move(callback).Run(
         native_file_system_error::FromStatus(
-            NativeFileSystemStatus::kInvalidArgument, "Blob does not exist"),
+            NativeFileSystemStatus::kOperationFailed,
+            "Internal read error: failed to create mojo data pipe."),
         /*bytes_written=*/0);
     return;
   }
 
-  DoFileSystemOperation(
-      FROM_HERE, &FileSystemOperationRunner::Write,
-      base::BindRepeating(&NativeFileSystemFileWriterImpl::DidWrite,
-                          weak_factory_.GetWeakPtr(),
-                          base::Owned(new WriteState{std::move(callback)})),
-      swap_url(), std::move(blob), position);
+  // TODO(mek): We can do this transformation from Blob to DataPipe in the
+  // renderer, and simplify the mojom exposed interface.
+  mojo::Remote<blink::mojom::Blob> blob(std::move(data));
+  blob->ReadAll(std::move(producer_handle), mojo::NullRemote());
+  WriteStreamImpl(offset, std::move(consumer_handle), std::move(callback));
 }
 
 void NativeFileSystemFileWriterImpl::WriteStreamImpl(

content/browser/native_file_system/native_file_system_file_writer_impl.h

@@ -76,9 +76,6 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
   void WriteImpl(uint64_t offset,
                  mojo::PendingRemote<blink::mojom::Blob> data,
                  WriteCallback callback);
-  void DoWriteBlob(WriteCallback callback,
-                   uint64_t position,
-                   std::unique_ptr<storage::BlobDataHandle> blob);
   void WriteStreamImpl(uint64_t offset,
                        mojo::ScopedDataPipeConsumerHandle stream,
                        WriteStreamCallback callback);

content/browser/native_file_system/native_file_system_file_writer_impl_unittest.cc

@@ -280,13 +280,17 @@ INSTANTIATE_TEST_SUITE_P(NativeFileSystemFileWriterImplTest,
                          ::testing::Bool());
 
 TEST_F(NativeFileSystemFileWriterImplTest, WriteInvalidBlob) {
+  // This test primarily verifies behavior of the browser process in the
+  // presence of a compromised renderer process. The situation this tests for
+  // normally can't occur. As such it doesn't really matter what status the
+  // write operation returns, the important part is that nothing crashes.
+
   mojo::PendingRemote<blink::mojom::Blob> blob;
   ignore_result(blob.InitWithNewPipeAndPassReceiver());
 
   uint64_t bytes_written;
   NativeFileSystemStatus result =
       WriteBlobSync(0, std::move(blob), &bytes_written);
-  EXPECT_EQ(result, NativeFileSystemStatus::kInvalidArgument);
   EXPECT_EQ(bytes_written, 0u);
 
   result = CloseSync();

content/browser/native_file_system/native_file_system_handle_base.h

@@ -19,7 +19,6 @@
 namespace storage {
 class FileSystemContext;
 class FileSystemOperationRunner;
-class BlobStorageContext;
 }  // namespace storage
 
 namespace content {
@@ -83,9 +82,6 @@ class CONTENT_EXPORT NativeFileSystemHandleBase
   storage::FileSystemContext* file_system_context() {
     return manager()->context();
   }
-  storage::BlobStorageContext* blob_context() {
-    return manager()->blob_context();
-  }
 
   virtual base::WeakPtr<NativeFileSystemHandleBase> AsWeakPtr() = 0;
 

content/browser/native_file_system/native_file_system_manager_impl.cc

@@ -37,6 +37,7 @@ using blink::mojom::NativeFileSystemStatus;
 using PermissionStatus = NativeFileSystemPermissionGrant::PermissionStatus;
 using SensitiveDirectoryResult =
     NativeFileSystemPermissionContext::SensitiveDirectoryResult;
+using storage::FileSystemContext;
 
 namespace {
 
@@ -182,14 +183,29 @@ void NativeFileSystemManagerImpl::BindReceiverFromUIThread(
 void NativeFileSystemManagerImpl::GetSandboxedFileSystem(
     GetSandboxedFileSystemCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  url::Origin origin = receivers_.current_context().origin;
-
-  context()->OpenFileSystem(
-      origin.GetURL(), storage::kFileSystemTypeTemporary,
-      storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
-      base::BindOnce(&NativeFileSystemManagerImpl::DidOpenSandboxedFileSystem,
-                     weak_factory_.GetWeakPtr(), receivers_.current_context(),
-                     std::move(callback)));
+
+  auto response_callback = base::BindOnce(
+      [](base::WeakPtr<NativeFileSystemManagerImpl> manager,
+         const BindingContext& binding_context,
+         GetSandboxedFileSystemCallback callback,
+         scoped_refptr<base::SequencedTaskRunner> task_runner, const GURL& root,
+         const std::string& fs_name, base::File::Error result) {
+        task_runner->PostTask(
+            FROM_HERE,
+            base::BindOnce(
+                &NativeFileSystemManagerImpl::DidOpenSandboxedFileSystem,
+                std::move(manager), binding_context, std::move(callback), root,
+                fs_name, result));
+      },
+      weak_factory_.GetWeakPtr(), receivers_.current_context(),
+      std::move(callback), base::SequencedTaskRunnerHandle::Get());
+
+  GURL origin = receivers_.current_context().origin.GetURL();
+  base::PostTask(FROM_HERE, {BrowserThread::IO},
+                 base::BindOnce(&FileSystemContext::OpenFileSystem, context(),
+                                origin, storage::kFileSystemTypeTemporary,
+                                storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
+                                std::move(response_callback)));
 }
 
 void NativeFileSystemManagerImpl::ChooseEntries(

content/browser/native_file_system/native_file_system_manager_impl.h

@@ -158,9 +158,7 @@ class CONTENT_EXPORT NativeFileSystemManagerImpl
     DCHECK_CURRENTLY_ON(BrowserThread::IO);
     return context_.get();
   }
-  storage::BlobStorageContext* blob_context() {
-    return blob_context_->context();
-  }
+  ChromeBlobStorageContext* blob_context() { return blob_context_.get(); }
   const base::SequenceBound<storage::FileSystemOperationRunner>&
   operation_runner();