storage: use mojo remotes instead of blob handles for idb writes

This is the last major patch to remove the use of direct blob interfaces
from IndexedDB code.

It removes BlobDataHandle from IndexedDBBlobInfo.  There's no longer
any need to load blobs on the IO thread as all the IndexedDBBlobInfo
can be created directly on the idb task runner.

The downside is that tests become a little bit more complicated because
asking for the uuid is asynchronous.

Bug: 1022214
Change-Id: I4bd07cf687b10a0c65efda2044e11bb88171d632
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1947388
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721284}

content/browser/indexed_db/indexed_db_backing_store.cc

@@ -58,6 +58,7 @@
 #include "storage/browser/file_system/local_file_stream_writer.h"
 #include "storage/common/database/database_identifier.h"
 #include "storage/common/file_system/file_system_mount_option.h"
+#include "third_party/blink/public/common/blob/blob_utils.h"
 #include "third_party/blink/public/common/indexeddb/indexeddb_key_range.h"
 #include "third_party/blink/public/common/indexeddb/web_idb_types.h"
 #include "third_party/leveldatabase/env_chromium.h"
@@ -1572,7 +1573,7 @@ class LocalWriteClosure : public base::RefCountedThreadSafe<LocalWriteClosure> {
       scoped_refptr<ChainedBlobWriter> chained_blob_writer,
       scoped_refptr<base::SequencedTaskRunner> idb_task_runner,
       const FilePath& file_path,
-      std::unique_ptr<storage::BlobDataHandle> blob,
+      mojo::SharedRemote<blink::mojom::Blob> blob,
       const base::Time& last_modified) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
     std::unique_ptr<storage::FileStreamWriter> writer =
@@ -1584,10 +1585,27 @@ class LocalWriteClosure : public base::RefCountedThreadSafe<LocalWriteClosure> {
             std::move(writer), chained_blob_writer->GetFlushPolicy()));
 
     DCHECK(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) {
+      chained_blob_writer->ReportWriteCompletion(false, 0u);
+      return;
+    }
+
+    blob->ReadAll(std::move(producer_handle), mojo::NullRemote());
     scoped_refptr<LocalWriteClosure> write_closure(new LocalWriteClosure(
         chained_blob_writer, idb_task_runner, file_path, last_modified));
     delegate->Start(
-        blob->CreateReader(),
+        std::move(consumer_handle),
         base::BindRepeating(&LocalWriteClosure::Run, std::move(write_closure)));
     chained_blob_writer->set_delegate(std::move(delegate));
   }
@@ -1656,11 +1674,9 @@ bool IndexedDBBackingStore::WriteBlobFile(
     DCHECK(descriptor.blob());
     base::PostTask(
         FROM_HERE, {content::BrowserThread::IO},
-        base::BindOnce(
-            &LocalWriteClosure::WriteBlobToFileOnIOThread,
-            base::WrapRefCounted(chained_blob_writer), task_runner_, path,
-            std::make_unique<storage::BlobDataHandle>(*descriptor.blob()),
-            descriptor.last_modified()));
+        base::BindOnce(&LocalWriteClosure::WriteBlobToFileOnIOThread,
+                       base::WrapRefCounted(chained_blob_writer), task_runner_,
+                       path, descriptor.blob(), descriptor.last_modified()));
   }
   return true;
 }
@@ -3016,9 +3032,8 @@ Status IndexedDBBackingStore::Transaction::HandleBlobPreTransaction(
             WriteDescriptor(entry.file_path(), next_blob_key, entry.size(),
                             entry.last_modified()));
       } else {
-        // TODO(enne): convert this to use mojo interfaces instead of handle.
         new_files_to_write->push_back(
-            WriteDescriptor(entry.blob_handle(), next_blob_key, entry.size(),
+            WriteDescriptor(entry.remote(), next_blob_key, entry.size(),
                             entry.last_modified()));
       }
       entry.set_key(next_blob_key);

content/browser/indexed_db/indexed_db_backing_store_unittest.cc

@@ -341,10 +341,7 @@ class IndexedDBBackingStoreTestWithBlobs : public IndexedDBBackingStoreTest {
     mojo::PendingRemote<blink::mojom::Blob> remote;
     auto receiver = remote.InitWithNewPipeAndPassReceiver();
     storage::BlobImpl::Create(std::move(handle), std::move(receiver));
-
-    handle = blob_context_->GetBlobDataFromUUID(uuid);
-    IndexedDBBlobInfo info(std::move(handle), std::move(remote), file_path,
-                           file_name, type);
+    IndexedDBBlobInfo info(std::move(remote), uuid, file_path, file_name, type);
     return info;
   }
 
@@ -356,9 +353,7 @@ class IndexedDBBackingStoreTestWithBlobs : public IndexedDBBackingStoreTest {
     mojo::PendingRemote<blink::mojom::Blob> remote;
     auto receiver = remote.InitWithNewPipeAndPassReceiver();
     storage::BlobImpl::Create(std::move(handle), std::move(receiver));
-
-    handle = blob_context_->GetBlobDataFromUUID(uuid);
-    IndexedDBBlobInfo info(std::move(handle), std::move(remote), type, size);
+    IndexedDBBlobInfo info(std::move(remote), uuid, type, size);
     return info;
   }
 
@@ -405,9 +400,47 @@ class IndexedDBBackingStoreTestWithBlobs : public IndexedDBBackingStoreTest {
     return true;
   }
 
-  bool CheckBlobWrites() const {
+  void GetBlobUUIDsForCheckBlobWrites() {
+    DCHECK(idb_context_->TaskRunner()->RunsTasksInCurrentSequence());
+
+    blob_remote_uuids_.clear();
+    blob_remote_uuids_.resize(backing_store_->writes().size(), "");
+
+    base::RunLoop loop;
+
+    size_t num_blobs = 0;
+    for (size_t i = 0; i < backing_store_->writes().size(); ++i) {
+      if (!backing_store_->writes()[i].is_file())
+        num_blobs++;
+    }
+    if (num_blobs == 0)
+      return;
+
+    for (size_t i = 0; i < backing_store_->writes().size(); ++i) {
+      const WriteDescriptor& desc = backing_store_->writes()[i];
+      if (desc.is_file())
+        continue;
+      desc.blob()->GetInternalUUID(
+          base::BindLambdaForTesting([&, i](const std::string& uuid) {
+            blob_remote_uuids_[i] = uuid;
+            num_blobs--;
+            if (num_blobs == 0)
+              loop.QuitClosure().Run();
+          }));
+    }
+    loop.Run();
+  }
+
+  bool CheckBlobWrites() {
     DCHECK(idb_context_->TaskRunner()->RunsTasksInCurrentSequence());
 
+    // Clear uuids so that GetBlobUUIDsForCheckBlobWrites must be re-called.
+    std::vector<std::string> uuids;
+    uuids.swap(blob_remote_uuids_);
+
+    DCHECK_EQ(uuids.size(), backing_store_->writes().size())
+        << "Run GetBlobUUIDsForCheckBlobWrites first";
+
     if (backing_store_->writes().size() != blob_info_.size())
       return false;
     for (size_t i = 0; i < backing_store_->writes().size(); ++i) {
@@ -420,7 +453,7 @@ class IndexedDBBackingStoreTestWithBlobs : public IndexedDBBackingStoreTest {
         if (desc.file_path() != info.file_path())
           return false;
       } else {
-        if (desc.blob()->uuid() != info.blob_handle()->uuid())
+        if (uuids[i] != info.uuid())
           return false;
       }
     }
@@ -451,6 +484,8 @@ class IndexedDBBackingStoreTestWithBlobs : public IndexedDBBackingStoreTest {
   // can be used to verify the state as a test progresses.
   std::vector<IndexedDBBlobInfo> blob_info_;
 
+  std::vector<std::string> blob_remote_uuids_;
+
   DISALLOW_COPY_AND_ASSIGN(IndexedDBBackingStoreTestWithBlobs);
 };
 
@@ -555,6 +590,8 @@ TEST_F(IndexedDBBackingStoreTestWithBlobs, PutGetConsistencyWithBlobs) {
       }));
   RunAllTasksUntilIdle();
 
+  GetBlobUUIDsForCheckBlobWrites();
+
   idb_context_->TaskRunner()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() {
         // Finish up transaction1, verifying blob writes.
@@ -850,6 +887,8 @@ TEST_F(IndexedDBBackingStoreTestWithBlobs, BlobJournalInterleavedTransactions) {
       }));
   RunAllTasksUntilIdle();
 
+  GetBlobUUIDsForCheckBlobWrites();
+
   idb_context_->TaskRunner()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() {
         // Verify transaction1 phase one completed.
@@ -874,6 +913,8 @@ TEST_F(IndexedDBBackingStoreTestWithBlobs, BlobJournalInterleavedTransactions) {
       }));
   RunAllTasksUntilIdle();
 
+  GetBlobUUIDsForCheckBlobWrites();
+
   idb_context_->TaskRunner()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() {
         // Verify transaction2 phase one completed.
@@ -920,6 +961,8 @@ TEST_F(IndexedDBBackingStoreTestWithBlobs, ActiveBlobJournal) {
       }));
   RunAllTasksUntilIdle();
 
+  GetBlobUUIDsForCheckBlobWrites();
+
   idb_context_->TaskRunner()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() {
         EXPECT_TRUE(callback_creator1.called);
@@ -1572,6 +1615,8 @@ TEST_F(IndexedDBBackingStoreTestWithBlobs, SchemaUpgradeWithBlobsCorrupt) {
       }));
   RunAllTasksUntilIdle();
 
+  GetBlobUUIDsForCheckBlobWrites();
+
   idb_context_->TaskRunner()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() {
         // Finish up transaction1, verifying blob writes.

content/browser/indexed_db/indexed_db_blob_info.cc

@@ -40,19 +40,16 @@ IndexedDBBlobInfo::IndexedDBBlobInfo()
 }
 
 IndexedDBBlobInfo::IndexedDBBlobInfo(
-    std::unique_ptr<storage::BlobDataHandle> blob_handle,
     mojo::PendingRemote<blink::mojom::Blob> blob_remote,
+    const std::string& uuid,
     const base::string16& type,
     int64_t size)
     : is_file_(false),
-      blob_handle_(*blob_handle),
       blob_remote_(std::move(blob_remote)),
+      uuid_(uuid),
       type_(type),
       size_(size),
-      key_(DatabaseMetaDataKey::kInvalidBlobKey) {
-  // blob_handle and blob_remote must either both or neither be true.
-  DCHECK(!blob_handle_.has_value() ^ blob_remote_.is_bound());
-}
+      key_(DatabaseMetaDataKey::kInvalidBlobKey) {}
 
 IndexedDBBlobInfo::IndexedDBBlobInfo(const base::string16& type,
                                      int64_t size,
@@ -60,22 +57,19 @@ IndexedDBBlobInfo::IndexedDBBlobInfo(const base::string16& type,
     : is_file_(false), type_(type), size_(size), key_(key) {}
 
 IndexedDBBlobInfo::IndexedDBBlobInfo(
-    std::unique_ptr<storage::BlobDataHandle> blob_handle,
     mojo::PendingRemote<blink::mojom::Blob> blob_remote,
+    const std::string& uuid,
     const base::FilePath& file_path,
     const base::string16& file_name,
     const base::string16& type)
     : is_file_(true),
-      blob_handle_(*blob_handle),
       blob_remote_(std::move(blob_remote)),
+      uuid_(uuid),
       type_(type),
       size_(-1),
       file_name_(file_name),
       file_path_(file_path),
-      key_(DatabaseMetaDataKey::kInvalidBlobKey) {
-  // blob_handle and blob_remote must either both or neither be true.
-  DCHECK(!blob_handle_.has_value() ^ blob_remote_.is_bound());
-}
+      key_(DatabaseMetaDataKey::kInvalidBlobKey) {}
 
 IndexedDBBlobInfo::IndexedDBBlobInfo(int64_t key,
                                      const base::string16& type,

content/browser/indexed_db/indexed_db_blob_info.h

@@ -32,16 +32,14 @@ class CONTENT_EXPORT IndexedDBBlobInfo {
 
   IndexedDBBlobInfo();
   // These two are used for Blobs.
-  // blob_handle and blob_remote must either both or neither be true.
-  IndexedDBBlobInfo(std::unique_ptr<storage::BlobDataHandle> blob_handle,
-                    mojo::PendingRemote<blink::mojom::Blob> blob_remote,
+  IndexedDBBlobInfo(mojo::PendingRemote<blink::mojom::Blob> blob_remote,
+                    const std::string& uuid,
                     const base::string16& type,
                     int64_t size);
   IndexedDBBlobInfo(const base::string16& type, int64_t size, int64_t key);
   // These two are used for Files.
-  // blob_handle and blob_remote must either both or neither be true.
-  IndexedDBBlobInfo(std::unique_ptr<storage::BlobDataHandle> blob_handle,
-                    mojo::PendingRemote<blink::mojom::Blob> blob_remote,
+  IndexedDBBlobInfo(mojo::PendingRemote<blink::mojom::Blob> blob_remote,
+                    const std::string& uuid,
                     const base::FilePath& file_path,
                     const base::string16& file_name,
                     const base::string16& type);
@@ -54,11 +52,13 @@ class CONTENT_EXPORT IndexedDBBlobInfo {
   IndexedDBBlobInfo& operator=(const IndexedDBBlobInfo& other);
 
   bool is_file() const { return is_file_; }
-  const storage::BlobDataHandle* blob_handle() const {
-    return blob_handle_.has_value() ? &blob_handle_.value() : nullptr;
-  }
   bool is_remote_valid() const { return blob_remote_.is_bound(); }
+  const std::string& uuid() const {
+    DCHECK(is_remote_valid());
+    return uuid_;
+  }
   void Clone(mojo::PendingReceiver<blink::mojom::Blob> receiver) const;
+  mojo::SharedRemote<blink::mojom::Blob> remote() const { return blob_remote_; }
   const base::string16& type() const { return type_; }
   int64_t size() const { return size_; }
   const base::string16& file_name() const { return file_name_; }
@@ -83,9 +83,9 @@ class CONTENT_EXPORT IndexedDBBlobInfo {
   bool is_file_;
 
   // Always for Blob; sometimes for File.
-  // TODO(enne): blob_handle is transitory, transition to blob_remote only.
-  base::Optional<storage::BlobDataHandle> blob_handle_;
   mojo::SharedRemote<blink::mojom::Blob> blob_remote_;
+  // If blob_remote_ is true, this is the blob's uuid.
+  std::string uuid_;
   // Mime type.
   base::string16 type_;
   // -1 if unknown for File.

content/browser/indexed_db/indexed_db_blob_storage.cc

@@ -27,12 +27,12 @@ std::unique_ptr<BlobChangeRecord> BlobChangeRecord::Clone() const {
   return record;
 }
 
-WriteDescriptor::WriteDescriptor(const storage::BlobDataHandle* blob,
+WriteDescriptor::WriteDescriptor(mojo::SharedRemote<blink::mojom::Blob> blob,
                                  int64_t key,
                                  int64_t size,
                                  base::Time last_modified)
     : is_file_(false),
-      blob_(*blob),
+      blob_(std::move(blob)),
       key_(key),
       size_(size),
       last_modified_(last_modified) {}

content/browser/indexed_db/indexed_db_blob_storage.h

@@ -79,7 +79,7 @@ typedef std::vector<std::pair<BlobEntryKey, std::string>>
 // This is a blob that is going to to be written to a file.
 class CONTENT_EXPORT WriteDescriptor {
  public:
-  WriteDescriptor(const storage::BlobDataHandle* blob,
+  WriteDescriptor(mojo::SharedRemote<blink::mojom::Blob> blob,
                   int64_t key,
                   int64_t size,
                   base::Time last_modified);
@@ -92,9 +92,9 @@ class CONTENT_EXPORT WriteDescriptor {
   WriteDescriptor& operator=(const WriteDescriptor& other);
 
   bool is_file() const { return is_file_; }
-  const storage::BlobDataHandle* blob() const {
+  mojo::SharedRemote<blink::mojom::Blob> blob() const {
     DCHECK(!is_file_);
-    return &blob_.value();
+    return blob_;
   }
   const base::FilePath& file_path() const {
     DCHECK(is_file_);
@@ -106,7 +106,7 @@ class CONTENT_EXPORT WriteDescriptor {
 
  private:
   bool is_file_;
-  base::Optional<storage::BlobDataHandle> blob_;
+  mojo::SharedRemote<blink::mojom::Blob> blob_;
   base::FilePath file_path_;
   int64_t key_;
   int64_t size_;

content/browser/indexed_db/indexed_db_dispatcher_host.cc

@@ -465,13 +465,10 @@ void IndexedDBDispatcherHost::CreateAllBlobs(
     auto& blob_info = blob_infos[i];
     auto& output_info = (*output_infos)[i];
 
+    auto receiver = output_info->blob.InitWithNewPipeAndPassReceiver();
     if (blob_info.is_remote_valid()) {
-      // TODO(enne): when blob handle gets removed entirely, there's no
-      // need for uuid to be stored here in IDBBlobInfoPtr.  For now,
-      // this needs to stay, unfortunately.
-      DCHECK(blob_info.blob_handle());
-      output_info->uuid = blob_info.blob_handle()->uuid();
-      blob_info.Clone(output_info->blob.InitWithNewPipeAndPassReceiver());
+      output_info->uuid = blob_info.uuid();
+      blob_info.Clone(std::move(receiver));
       continue;
     }
 
@@ -488,7 +485,6 @@ void IndexedDBDispatcherHost::CreateAllBlobs(
 
     // Write results to output_info.
     output_info->uuid = base::GenerateGUID();
-    auto receiver = output_info->blob.InitWithNewPipeAndPassReceiver();
 
     mojo_blob_storage_context()->RegisterFromDataItem(
         std::move(receiver), output_info->uuid, std::move(element));

content/browser/indexed_db/transaction_impl.h

@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/sequence_checker.h"
 #include "base/strings/string16.h"
+#include "content/browser/indexed_db/indexed_db_blob_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/blink/public/mojom/indexeddb/indexeddb.mojom.h"
 #include "third_party/blink/public/mojom/quota/quota_types.mojom.h"
@@ -53,9 +54,12 @@ class TransactionImpl : public blink::mojom::IDBTransaction {
                                    int64_t quota);
 
  private:
-  class IOHelper;
-
-  std::unique_ptr<IOHelper, BrowserThread::DeleteOnIOThread> io_helper_;
+  // Turns an IDBValue into a set of IndexedDBBlobInfo in |blob_infos|.
+  // Also returns whether there was any security failures reading the
+  // filenames passed in |value| via the |security_policy_failure| variable.
+  void CreateBlobInfos(blink::mojom::IDBValuePtr& value,
+                       std::vector<IndexedDBBlobInfo>* blob_infos,
+                       bool* security_policy_failure);
 
   base::WeakPtr<IndexedDBDispatcherHost> dispatcher_host_;
   scoped_refptr<IndexedDBContextImpl> indexed_db_context_;