Revert "IDB: direct reads for blobs"

This reverts commit 60632268ea.

Reason for revert: causing crashes (see crbug.com/386615753)

Original change's description:
> IDB: direct reads for blobs
>
> For the standard case of a page reading IDB data from the blob store,
> don't go through the blob registry on the i/o thread and instead
> connect the IDB bucket thread directly to the renderer.
>
> In some other (rarer) cases the blob registry is still used. This
> is accomplished by *additionally* registering a blob with
> BlobStorageContext using the same UUID, which is necessary for:
>
> * WriteBlobToFile(), which does lookup by UUID
> * loading data for a blob:// URL, i.e. mojom::Blob::Load, which is
>   thunked through to the registry blob because implementation is non-
>   trivial
>
> Bug: 373684390
> Change-Id: I9235f23303e4e6a05bf12a8acff32a5fb4e2a565
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6113789
> Commit-Queue: Evan Stade <estade@chromium.org>
> Reviewed-by: Steve Becker <stevebe@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#1400627}

Bug: 373684390,386615753
Change-Id: I8e7460b58c175d9cbd62e99845dd68c71ef6dd06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6132793
Auto-Submit: Evan Stade <estade@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1400977}

content/browser/indexed_db/blob_reader.cc

@@ -9,66 +9,13 @@
 #include <utility>
 #include <vector>
 
-#include "base/files/file_util.h"
 #include "base/functional/bind.h"
-#include "base/strings/utf_string_conversions.h"
-#include "base/uuid.h"
 #include "content/browser/indexed_db/file_stream_reader_to_data_pipe.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "net/base/net_errors.h"
 
 namespace content::indexed_db {
 
-void BlobReader::Clone(mojo::PendingReceiver<blink::mojom::Blob> receiver) {
-  receivers_.Add(this, std::move(receiver));
-}
-
-void BlobReader::AsDataPipeGetter(
-    mojo::PendingReceiver<network::mojom::DataPipeGetter> receiver) {
-  NOTREACHED();
-}
-
-void BlobReader::ReadRange(
-    uint64_t offset,
-    uint64_t length,
-    mojo::ScopedDataPipeProducerHandle handle,
-    mojo::PendingRemote<blink::mojom::BlobReaderClient> client) {
-  OpenFileAndReadIntoPipe(file_path_, blob_length_, offset, length,
-                          std::move(handle), std::move(client));
-}
-
-void BlobReader::ReadAll(
-    mojo::ScopedDataPipeProducerHandle handle,
-    mojo::PendingRemote<blink::mojom::BlobReaderClient> client) {
-  ReadRange(0, std::numeric_limits<uint64_t>::max(), std::move(handle),
-            std::move(client));
-}
-
-void BlobReader::Load(
-    mojo::PendingReceiver<network::mojom::URLLoader> loader,
-    const std::string& method,
-    const net::HttpRequestHeaders& headers,
-    mojo::PendingRemote<network::mojom::URLLoaderClient> client) {
-  // Bounce back to the registry so that we can avoid reimplementing
-  // `BlobUrlLoader`. This is used for Object URLs. It's not clear how often
-  // this is used or how important it is to make it super efficient.
-  registry_blob_->Load(std::move(loader), method, headers, std::move(client));
-}
-
-void BlobReader::ReadSideData(
-    blink::mojom::Blob::ReadSideDataCallback callback) {
-  std::move(callback).Run(std::nullopt);
-}
-
-void BlobReader::CaptureSnapshot(CaptureSnapshotCallback callback) {
-  // Only used for `File`.
-  NOTREACHED();
-}
-
-void BlobReader::GetInternalUUID(GetInternalUUIDCallback callback) {
-  std::move(callback).Run(uuid_);
-}
-
 void BlobReader::Read(
     uint64_t offset,
     uint64_t length,
@@ -84,41 +31,22 @@ void BlobReader::ReadSideData(
   std::move(callback).Run(net::ERR_NOT_IMPLEMENTED, mojo_base::BigBuffer());
 }
 
-BlobReader::BlobReader(const IndexedDBExternalObject& blob_info,
-                       storage::mojom::BlobStorageContext& blob_registry,
+BlobReader::BlobReader(const base::FilePath& file_path,
                        base::OnceClosure on_last_receiver_disconnected)
-    : uuid_(base::Uuid::GenerateRandomV4().AsLowercaseString()),
-      blob_length_(blob_info.size()),
-      file_path_(blob_info.indexed_db_file_path()),
+    : file_path_(file_path),
       on_last_receiver_disconnected_(std::move(on_last_receiver_disconnected)) {
-  receivers_.set_disconnect_handler(base::BindRepeating(
+  readers_.set_disconnect_handler(base::BindRepeating(
       &BlobReader::OnMojoDisconnect, base::Unretained(this)));
+}
 
-  auto element = storage::mojom::BlobDataItem::New();
-  element->size = blob_info.size();
-  element->side_data_size = 0;
-  element->content_type = base::UTF16ToUTF8(blob_info.type());
-  element->type = storage::mojom::BlobDataItemType::kIndexedDB;
-  reader_.Bind(element->reader.InitWithNewPipeAndPassReceiver());
-  reader_.set_disconnect_handler(base::BindRepeating(
-      &BlobReader::OnReaderDisconnect, base::Unretained(this)));
-  blob_registry.RegisterFromDataItem(
-      registry_blob_.BindNewPipeAndPassReceiver(), uuid_, std::move(element));
+void BlobReader::AddReader(mojo::PendingReceiver<BlobDataItemReader> receiver) {
+  readers_.Add(this, std::move(receiver));
 }
 
 BlobReader::~BlobReader() = default;
 
-void BlobReader::OnReaderDisconnect() {
-  reader_.reset();
-  OnMojoDisconnect();
-}
-
 void BlobReader::OnMojoDisconnect() {
-  if (receivers_.empty()) {
-    registry_blob_.reset();
-  }
-
-  if (receivers_.empty() && !reader_.is_bound()) {
+  if (readers_.empty()) {
     std::move(on_last_receiver_disconnected_).Run();
     // `this` is deleted.
   }

content/browser/indexed_db/blob_reader.h

@@ -8,53 +8,22 @@
 #include "base/files/file_path.h"
 #include "base/functional/callback_forward.h"
 #include "components/services/storage/public/mojom/blob_storage_context.mojom.h"
-#include "content/browser/indexed_db/indexed_db_external_object.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
-#include "third_party/blink/public/mojom/blob/blob.mojom.h"
+#include "services/network/public/mojom/data_pipe_getter.mojom.h"
 
 namespace content::indexed_db {
 
-// This class reads files from the IDB blob store, which are just plain files on
-// disk, and serves them through the mojom Blob interface. It does this in
-// multiple ways:
-//
-// * For the most common case, i.e. reading a blob with blink::mojom::ReadAll(),
-//   this class reads from disk and pipes bytes directly to the renderer.
-// * In cases where the blob registry needs to read bytes, such as for copying
-//   to a local file or to serve data for blob:// URLs, this class reads from
-//   disk and returns the bytes via `BlobDataItemReader`.
-class BlobReader : public blink::mojom::Blob,
-                   public storage::mojom::BlobDataItemReader {
+class BlobReader : public storage::mojom::BlobDataItemReader {
  public:
-  BlobReader(const IndexedDBExternalObject& blob_info,
-             storage::mojom::BlobStorageContext& blob_registry,
+  BlobReader(const base::FilePath& file_path,
              base::OnceClosure on_last_receiver_disconnected);
   ~BlobReader() override;
 
   BlobReader(const BlobReader&) = delete;
   BlobReader& operator=(const BlobReader&) = delete;
 
-  // blink::mojom::Blob:
-  void Clone(mojo::PendingReceiver<blink::mojom::Blob> receiver) override;
-  void AsDataPipeGetter(
-      mojo::PendingReceiver<network::mojom::DataPipeGetter> receiver) override;
-  void ReadRange(
-      uint64_t offset,
-      uint64_t length,
-      mojo::ScopedDataPipeProducerHandle handle,
-      mojo::PendingRemote<blink::mojom::BlobReaderClient> client) override;
-  void ReadAll(
-      mojo::ScopedDataPipeProducerHandle handle,
-      mojo::PendingRemote<blink::mojom::BlobReaderClient> client) override;
-  void Load(
-      mojo::PendingReceiver<network::mojom::URLLoader> loader,
-      const std::string& method,
-      const net::HttpRequestHeaders& headers,
-      mojo::PendingRemote<network::mojom::URLLoaderClient> client) override;
-  void ReadSideData(blink::mojom::Blob::ReadSideDataCallback callback) override;
-  void CaptureSnapshot(CaptureSnapshotCallback callback) override;
-  void GetInternalUUID(GetInternalUUIDCallback callback) override;
+  void AddReader(mojo::PendingReceiver<BlobDataItemReader> receiver);
 
   // storage::mojom::BlobDataItemReader:
   void Read(uint64_t offset,
@@ -65,27 +34,11 @@ class BlobReader : public blink::mojom::Blob,
                         callback) override;
 
  private:
-  void OnReaderDisconnect();
   void OnMojoDisconnect();
 
-  // This UUID is used for both the blob that's served via `blink::mojom::Blob`
-  // and the blob in the registry. This is crucial because operations such as
-  // copying the blob to a new file do so by identifying the blob to the blob
-  // registry using the UUID.
-  std::string uuid_;
-
-  // This is the expected length of the file, which comes from the LevelDB
-  // record. This acts on a cap on the number of bytes to be read from the file.
-  // It can even be zero, in which case even a missing file will be treated as
-  // normal (non-error).
-  uint64_t blob_length_;
-
   const base::FilePath file_path_;
 
-  mojo::ReceiverSet<blink::mojom::Blob> receivers_;
-
-  mojo::Receiver<storage::mojom::BlobDataItemReader> reader_{this};
-  mojo::Remote<blink::mojom::Blob> registry_blob_;
+  mojo::ReceiverSet<storage::mojom::BlobDataItemReader> readers_;
 
   base::OnceClosure on_last_receiver_disconnected_;
 };

content/browser/indexed_db/file_stream_reader_to_data_pipe.cc

@@ -6,11 +6,8 @@
 
 #include <optional>
 
-#include "base/files/file.h"
-#include "base/files/file_util.h"
 #include "base/functional/bind.h"
 #include "base/memory/scoped_refptr.h"
-#include "base/run_loop.h"
 #include "mojo/public/cpp/system/simple_watcher.h"
 #include "net/base/net_errors.h"
 #include "services/network/public/cpp/net_adapters.h"
@@ -19,22 +16,16 @@ namespace content::indexed_db {
 
 namespace {
 
+// This class owns itself and deletes itself after `completion_callback_` is
+// run.
 // TODO(estade): rename this class and this file.
 class FileStreamReaderToDataPipe {
  public:
-  FileStreamReaderToDataPipe(
-      const base::FilePath& file_path,
-      uint64_t expected_file_size,
-      uint64_t offset,
-      uint64_t read_length,
-      mojo::ScopedDataPipeProducerHandle dest,
-      mojo::PendingRemote<blink::mojom::BlobReaderClient> client);
   FileStreamReaderToDataPipe(const base::FilePath& file_path,
                              uint64_t offset,
                              uint64_t read_length,
                              mojo::ScopedDataPipeProducerHandle dest,
                              base::OnceCallback<void(int)> completion_callback);
-
   ~FileStreamReaderToDataPipe();
 
   void Start();
@@ -48,33 +39,16 @@ class FileStreamReaderToDataPipe {
 
   base::File file_;
   mojo::ScopedDataPipeProducerHandle dest_;
-
-  // Exactly one of these two members will be non-null.
-  mojo::Remote<blink::mojom::BlobReaderClient> client_;
   base::OnceCallback<void(int)> completion_callback_;
-
   uint64_t transferred_bytes_ = 0;
   uint64_t offset_;
-  uint64_t read_length_ = 0;
+  uint64_t read_length_;
 
   scoped_refptr<network::NetToMojoPendingBuffer> pending_write_;
   // Optional so that its construction can be deferred.
   std::optional<mojo::SimpleWatcher> writable_handle_watcher_;
 };
 
-FileStreamReaderToDataPipe::FileStreamReaderToDataPipe(
-    const base::FilePath& file_path,
-    uint64_t expected_file_size,
-    uint64_t offset,
-    uint64_t read_length,
-    mojo::ScopedDataPipeProducerHandle dest,
-    mojo::PendingRemote<blink::mojom::BlobReaderClient> client)
-    : dest_(std::move(dest)), client_(std::move(client)), offset_(offset) {
-  read_length_ = std::min(expected_file_size, read_length);
-  client_->OnCalculatedSize(expected_file_size, read_length_);
-  file_.Initialize(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
-}
-
 FileStreamReaderToDataPipe::FileStreamReaderToDataPipe(
     const base::FilePath& file_path,
     uint64_t offset,
@@ -85,22 +59,7 @@ FileStreamReaderToDataPipe::FileStreamReaderToDataPipe(
       completion_callback_(std::move(completion_callback)),
       offset_(offset),
       read_length_(read_length) {
-  file_.Initialize(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
-}
-
-FileStreamReaderToDataPipe::~FileStreamReaderToDataPipe() = default;
-
-void FileStreamReaderToDataPipe::Start() {
-  if (read_length_ == 0) {
-    OnComplete(net::OK);
-    return;
-  }
-
-  if (!file_.IsValid()) {
-    OnComplete(net::FileErrorToNetError(file_.error_details()));
-    return;
-  }
-
+  DCHECK(!writable_handle_watcher_.has_value());
   writable_handle_watcher_.emplace(FROM_HERE,
                                    mojo::SimpleWatcher::ArmingPolicy::MANUAL);
   writable_handle_watcher_->Watch(
@@ -108,7 +67,17 @@ void FileStreamReaderToDataPipe::Start() {
       base::BindRepeating(&FileStreamReaderToDataPipe::OnDataPipeWritable,
                           base::Unretained(this)));
 
-  ReadMore();
+  file_.Initialize(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+}
+
+FileStreamReaderToDataPipe::~FileStreamReaderToDataPipe() = default;
+
+void FileStreamReaderToDataPipe::Start() {
+  if (file_.IsValid()) {
+    ReadMore();
+  } else {
+    OnComplete(net::FileErrorToNetError(file_.error_details()));
+  }
 }
 
 void FileStreamReaderToDataPipe::ReadMore() {
@@ -182,35 +151,16 @@ void FileStreamReaderToDataPipe::OnDataPipeWritable(MojoResult result) {
 void FileStreamReaderToDataPipe::OnComplete(int result) {
   // Resets the watchers, pipes and the exchange handler, so that
   // we will never be called back.
-  if (writable_handle_watcher_) {
-    writable_handle_watcher_->Cancel();
-  }
+  writable_handle_watcher_->Cancel();
   pending_write_ = nullptr;
   dest_.reset();
 
-  if (client_) {
-    client_->OnComplete(result, transferred_bytes_);
-  } else {
-    std::move(completion_callback_).Run(result);
-  }
+  std::move(completion_callback_).Run(result);
   delete this;
 }
 
 }  // namespace
 
-void OpenFileAndReadIntoPipe(
-    const base::FilePath& file_path,
-    uint64_t expected_file_size,
-    uint64_t offset,
-    uint64_t read_length,
-    mojo::ScopedDataPipeProducerHandle dest,
-    mojo::PendingRemote<blink::mojom::BlobReaderClient> client) {
-  (new FileStreamReaderToDataPipe(file_path, expected_file_size, offset,
-                                  read_length, std::move(dest),
-                                  std::move(client)))
-      ->Start();
-}
-
 void OpenFileAndReadIntoPipe(
     const base::FilePath& file_path,
     uint64_t offset,

content/browser/indexed_db/file_stream_reader_to_data_pipe.h

@@ -8,20 +8,10 @@
 #include "base/files/file_path.h"
 #include "base/functional/callback_forward.h"
 #include "content/common/content_export.h"
-#include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/system/data_pipe.h"
-#include "third_party/blink/public/mojom/blob/blob.mojom.h"
 
 namespace content::indexed_db {
 
-CONTENT_EXPORT void OpenFileAndReadIntoPipe(
-    const base::FilePath& file_path,
-    uint64_t expected_file_size,
-    uint64_t offset,
-    uint64_t read_length,
-    mojo::ScopedDataPipeProducerHandle dest,
-    mojo::PendingRemote<blink::mojom::BlobReaderClient> client);
-
 CONTENT_EXPORT void OpenFileAndReadIntoPipe(
     const base::FilePath& file_path,
     uint64_t offset,

content/browser/indexed_db/instance/bucket_context.cc

@@ -66,6 +66,7 @@
 #include "components/services/storage/public/mojom/blob_storage_context.mojom-shared.h"
 #include "components/services/storage/public/mojom/blob_storage_context.mojom.h"
 #include "content/browser/indexed_db/file_path_util.h"
+#include "content/browser/indexed_db/file_stream_reader_to_data_pipe.h"
 #include "content/browser/indexed_db/indexed_db_data_format_version.h"
 #include "content/browser/indexed_db/indexed_db_data_loss_info.h"
 #include "content/browser/indexed_db/indexed_db_database_error.h"
@@ -489,7 +490,22 @@ void BucketContext::CreateAllExternalObjects(
           continue;
         }
 
-        BindBlobReader(blob_info, std::move(receiver));
+        auto element = storage::mojom::BlobDataItem::New();
+        // TODO(enne): do we have to handle unknown size here??
+        element->size = blob_info.size();
+        element->side_data_size = 0;
+        element->content_type = base::UTF16ToUTF8(blob_info.type());
+        element->type = storage::mojom::BlobDataItemType::kIndexedDB;
+
+        BindFileReader(blob_info,
+                       element->reader.InitWithNewPipeAndPassReceiver());
+
+        // Write results to output_info.
+        blob_storage_context_->RegisterFromDataItem(
+            std::move(receiver),
+            base::Uuid::GenerateRandomV4().AsLowercaseString(),
+            std::move(element));
+
         break;
       }
       case IndexedDBExternalObject::ObjectType::kFileSystemAccessHandle: {
@@ -914,10 +930,11 @@ void BucketContext::CloseNow() {
   QueueRunTasks();
 }
 
-void BucketContext::BindBlobReader(
+void BucketContext::BindFileReader(
     const IndexedDBExternalObject& blob_info,
-    mojo::PendingReceiver<blink::mojom::Blob> blob_receiver) {
+    mojo::PendingReceiver<storage::mojom::BlobDataItemReader> receiver) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(receiver.is_valid());
 
   const base::FilePath& path = blob_info.indexed_db_file_path();
 
@@ -925,9 +942,8 @@ void BucketContext::BindBlobReader(
   if (itr == file_reader_map_.end()) {
     // Unretained is safe because `this` owns the reader.
     auto reader = std::make_unique<BlobReader>(
-        blob_info, *blob_storage_context_,
-        base::BindOnce(&BucketContext::RemoveBoundReaders,
-                       base::Unretained(this), path));
+        path, base::BindOnce(&BucketContext::RemoveBoundReaders,
+                             base::Unretained(this), path));
     itr =
         file_reader_map_
             .insert({path, std::make_tuple(std::move(reader),
@@ -936,7 +952,7 @@ void BucketContext::BindBlobReader(
             .first;
   }
 
-  std::get<0>(itr->second)->Clone(std::move(blob_receiver));
+  std::get<0>(itr->second)->AddReader(std::move(receiver));
 }
 
 void BucketContext::RemoveBoundReaders(const base::FilePath& path) {

content/browser/indexed_db/instance/bucket_context.h

@@ -363,10 +363,9 @@ class CONTENT_EXPORT BucketContext
   // since `bucket_space_remaining_timestamp_`.
   int64_t GetBucketSpaceToAllot();
 
-  // Hooks up a `BlobReader` to `receiver` for the blob described by
-  // `blob_info`.
-  void BindBlobReader(const IndexedDBExternalObject& blob_info,
-                      mojo::PendingReceiver<blink::mojom::Blob> receiver);
+  void BindFileReader(
+      const IndexedDBExternalObject& blob_info,
+      mojo::PendingReceiver<storage::mojom::BlobDataItemReader> receiver);
   // Removes all readers for this file path.
   void RemoveBoundReaders(const base::FilePath& path);