Read from Remote<blob> directly instead of converting it to a BlobDataHandle

This CL addresses some comments left in:
https://chromium-review.googlesource.com/c/chromium/src/+/1832723.
It now reads the remote blob interface directly rather than
converting it to a BlobDataHandle first.

BUG=947395

Change-Id: Ie9ecdaab3fa392f0c99f51eb73d5b0f8590cfbef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869751
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710153}

content/browser/download/data_url_blob_reader.cc

@@ -4,112 +4,73 @@
 
 #include "content/browser/download/data_url_blob_reader.h"
 
-#include "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
-#include "content/public/browser/browser_thread.h"
-#include "net/base/io_buffer.h"
-#include "net/base/net_errors.h"
-#include "storage/browser/blob/blob_data_handle.h"
-#include "storage/browser/blob/blob_reader.h"
 
-namespace {
-
-// Class for reading data from a BlobReader. Once blob reading completes,
-// it constructs a data URL from the data and calls the callback
-// given by the caller. All the operations are on the IO thread.
-class BlobReadJob : public base::RefCounted<BlobReadJob> {
- public:
-  BlobReadJob(content::DataURLBlobReader::ReadCompletionCallback
-                  read_completion_callback,
-              std::unique_ptr<storage::BlobReader> blob_reader);
-
-  // Called to start the job.
-  void Start();
-
- private:
-  friend class base::RefCounted<BlobReadJob>;
-  ~BlobReadJob();
-
-  // Called when blob size is calculated.
-  void OnSizeCalculated(int result_code);
+namespace content {
 
-  // Called when all the data is read.
-  void OnReadComplete(int size);
+// static
+void DataURLBlobReader::ReadDataURLFromBlob(
+    mojo::PendingRemote<blink::mojom::Blob> data_url_blob,
+    DataURLBlobReader::ReadCompletionCallback read_completion_callback) {
+  DataURLBlobReader* reader = new DataURLBlobReader(std::move(data_url_blob));
+  auto data_url_blob_reader = base::WrapUnique(reader);
+
+  // Move the reader to be owned by the callback.
+  base::OnceClosure closure = base::BindOnce(
+      [](ReadCompletionCallback callback,
+         std::unique_ptr<DataURLBlobReader> blob_reader) {
+        GURL url = base::StartsWith(blob_reader->url_data_,
+                                    "data:", base::CompareCase::SENSITIVE)
+                       ? GURL(blob_reader->url_data_)
+                       : GURL();
+        std::move(callback).Run(std::move(url));
+      },
+      std::move(read_completion_callback), std::move(data_url_blob_reader));
+  reader->Start(std::move(closure));
+}
 
-  content::DataURLBlobReader::ReadCompletionCallback read_completion_callback_;
-  std::unique_ptr<storage::BlobReader> blob_reader_;
-  scoped_refptr<net::IOBufferWithSize> io_buffer_;
+DataURLBlobReader::DataURLBlobReader(
+    mojo::PendingRemote<blink::mojom::Blob> data_url_blob)
+    : data_url_blob_(std::move(data_url_blob)) {
+  data_url_blob_.set_disconnect_handler(
+      base::BindOnce(&DataURLBlobReader::OnFailed, base::Unretained(this)));
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
-  DISALLOW_COPY_AND_ASSIGN(BlobReadJob);
-};
+DataURLBlobReader::~DataURLBlobReader() = default;
 
-BlobReadJob::BlobReadJob(
-    content::DataURLBlobReader::ReadCompletionCallback read_completion_callback,
-    std::unique_ptr<storage::BlobReader> blob_reader)
-    : read_completion_callback_(std::move(read_completion_callback)),
-      blob_reader_(std::move(blob_reader)) {}
+void DataURLBlobReader::Start(base::OnceClosure callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  mojo::ScopedDataPipeProducerHandle producer_handle;
+  mojo::ScopedDataPipeConsumerHandle consumer_handle;
+  MojoResult result =
+      CreateDataPipe(nullptr, &producer_handle, &consumer_handle);
+  if (result != MOJO_RESULT_OK) {
+    std::move(callback).Run();
+    return;
+  }
 
-BlobReadJob::~BlobReadJob() = default;
+  callback_ = std::move(callback);
 
-void BlobReadJob::Start() {
-  const storage::BlobReader::Status size_status = blob_reader_->CalculateSize(
-      base::BindOnce(&BlobReadJob::OnSizeCalculated, this));
-  switch (size_status) {
-    case storage::BlobReader::Status::NET_ERROR:
-      std::move(read_completion_callback_).Run(GURL());
-      return;
-    case storage::BlobReader::Status::IO_PENDING:
-      return;
-    case storage::BlobReader::Status::DONE:
-      OnSizeCalculated(net::OK);
-      return;
-  }
+  data_url_blob_->ReadAll(std::move(producer_handle), mojo::NullRemote());
+  data_pipe_drainer_ =
+      std::make_unique<mojo::DataPipeDrainer>(this, std::move(consumer_handle));
 }
 
-void BlobReadJob::OnSizeCalculated(int result_code) {
-  if (result_code != net::OK) {
-    std::move(read_completion_callback_).Run(GURL());
-    return;
-  }
-  io_buffer_ = base::MakeRefCounted<net::IOBufferWithSize>(
-      static_cast<size_t>(blob_reader_->total_size()));
-  int bytes_read;
-  storage::BlobReader::Status read_status = blob_reader_->Read(
-      io_buffer_.get(), blob_reader_->total_size(), &bytes_read,
-      base::BindOnce(&BlobReadJob::OnReadComplete, this));
-  switch (read_status) {
-    case storage::BlobReader::Status::NET_ERROR:
-      std::move(read_completion_callback_).Run(GURL());
-      return;
-    case storage::BlobReader::Status::IO_PENDING:
-      return;
-    case storage::BlobReader::Status::DONE:
-      OnReadComplete(bytes_read);
-      return;
-  }
+void DataURLBlobReader::OnDataAvailable(const void* data, size_t num_bytes) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  url_data_.append(static_cast<const char*>(data), num_bytes);
 }
 
-void BlobReadJob::OnReadComplete(int size) {
-  base::StringPiece url_data = base::StringPiece(io_buffer_->data(), size);
-  GURL url = base::StartsWith(url_data, "data:", base::CompareCase::SENSITIVE)
-                 ? GURL(url_data)
-                 : GURL();
-  std::move(read_completion_callback_).Run(std::move(url));
+void DataURLBlobReader::OnDataComplete() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  std::move(callback_).Run();
 }
 
-}  // namespace
-
-namespace content {
-
-// static
-void DataURLBlobReader::ReadDataURLFromBlob(
-    std::unique_ptr<storage::BlobDataHandle> blob_data_handle,
-    DataURLBlobReader::ReadCompletionCallback read_completion_callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  scoped_refptr<BlobReadJob> blob_read_job = base::MakeRefCounted<BlobReadJob>(
-      std::move(read_completion_callback), blob_data_handle->CreateReader());
-  blob_read_job->Start();
+void DataURLBlobReader::OnFailed() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  url_data_.clear();
+  std::move(callback_).Run();
 }
 
 }  // namespace content

content/browser/download/data_url_blob_reader.h

@@ -5,10 +5,16 @@
 #ifndef CONTENT_BROWSER_DOWNLOAD_DATA_URL_BLOB_READER_H_
 #define CONTENT_BROWSER_DOWNLOAD_DATA_URL_BLOB_READER_H_
 
-#include <memory>
+#include <string>
 
 #include "base/callback_forward.h"
+#include "base/sequence_checker.h"
 #include "content/common/content_export.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/system/data_pipe_drainer.h"
+#include "third_party/blink/public/mojom/blob/blob.mojom.h"
 #include "url/gurl.h"
 
 namespace storage {
@@ -17,18 +23,48 @@ class BlobDataHandle;
 
 namespace content {
 
-// Helper class to read a data url from a BlobDataHandle on IO thread.
-class CONTENT_EXPORT DataURLBlobReader {
+// Helper class to read a data url from a BlobDataHandle.
+class CONTENT_EXPORT DataURLBlobReader : public mojo::DataPipeDrainer::Client {
  public:
   using ReadCompletionCallback = base::OnceCallback<void(GURL)>;
 
   // Read the data URL from |blob_data_handle|, and call
   // |read_completion_callback| once it completes. If the data URL cannot be
   // retrieved, |read_completion_callback| will be called with an empty URL.
-  // This method must be called on the IO thread.
+  // This method must be called on the UI thread.
   static void ReadDataURLFromBlob(
-      std::unique_ptr<storage::BlobDataHandle> blob_data_handle,
+      mojo::PendingRemote<blink::mojom::Blob> data_url_blob,
       ReadCompletionCallback read_completion_callback);
+
+  ~DataURLBlobReader() override;
+
+ private:
+  DataURLBlobReader(mojo::PendingRemote<blink::mojom::Blob> data_url_blob);
+
+  // Starts reading from the |data_url_blob_| and calls |callback| once
+  // completes.
+  void Start(base::OnceClosure callback);
+
+  // mojo::DataPipeDrainer:
+  void OnDataAvailable(const void* data, size_t num_bytes) override;
+  void OnDataComplete() override;
+
+  // Called when failed to read from blob.
+  void OnFailed();
+
+  std::unique_ptr<mojo::DataPipeDrainer> data_pipe_drainer_;
+
+  mojo::Remote<blink::mojom::Blob> data_url_blob_;
+
+  // Data URL retrieved from the blob.
+  std::string url_data_;
+
+  // Callback to run once blob data is read.
+  base::OnceClosure callback_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(DataURLBlobReader);
 };
 
 }  // namespace content

content/browser/frame_host/render_frame_host_impl.cc

@@ -529,7 +529,7 @@ void GetRestrictedCookieManager(
 }
 
 // Helper method to download a URL on UI thread.
-void DownloadUrlOnUIThread(
+void StartDownload(
     std::unique_ptr<download::DownloadUrlParameters> parameters,
     mojo::PendingRemote<blink::mojom::BlobURLToken> blob_url_token) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -555,37 +555,16 @@ void DownloadUrlOnUIThread(
                                 std::move(blob_url_loader_factory));
 }
 
-// Called on the IO thread when the data URL in the BlobDataHandle
+// Called on the UI thread when the data URL in the BlobDataHandle
 // is read.
 void OnDataURLRetrieved(
     std::unique_ptr<download::DownloadUrlParameters> parameters,
     GURL data_url) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!data_url.is_valid())
+    return;
   parameters->set_url(std::move(data_url));
-  base::PostTask(FROM_HERE, {BrowserThread::UI},
-                 base::BindOnce(&DownloadUrlOnUIThread, std::move(parameters),
-                                mojo::NullRemote()));
-}
-// Called on the IO thread when the BlobDataHandle for the data URL
-// is retrieved.
-void OnDataURLBlobRetrieved(
-    std::unique_ptr<download::DownloadUrlParameters> parameters,
-    std::unique_ptr<storage::BlobDataHandle> blob_data_handle) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DataURLBlobReader::ReadDataURLFromBlob(
-      std::move(blob_data_handle),
-      base::BindOnce(&OnDataURLRetrieved, std::move(parameters)));
-}
-
-// Called to retrieve the data URL on the IO thread.
-void RetrieveDataURLOnIOThread(
-    std::unique_ptr<download::DownloadUrlParameters> parameters,
-    mojo::PendingRemote<blink::mojom::Blob> data_url_blob,
-    scoped_refptr<ChromeBlobStorageContext> blob_context) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  blob_context->context()->GetBlobDataFromBlobRemote(
-      std::move(data_url_blob),
-      base::BindOnce(&OnDataURLBlobRetrieved, std::move(parameters)));
+  StartDownload(std::move(parameters), mojo::NullRemote());
 }
 
 // TODO(crbug.com/977040): Remove when no longer needed.
@@ -4163,18 +4142,14 @@ void RenderFrameHostImpl::DownloadUrl(
   parameters->set_initiator(initiator);
   parameters->set_download_source(download::DownloadSource::FROM_RENDERER);
 
-  BrowserContext* browser_context = GetSiteInstance()->GetBrowserContext();
   if (data_url_blob) {
-    scoped_refptr<ChromeBlobStorageContext> blob_context =
-        ChromeBlobStorageContext::GetFor(browser_context);
-    base::PostTask(
-        FROM_HERE, {BrowserThread::IO},
-        base::BindOnce(&RetrieveDataURLOnIOThread, std::move(parameters),
-                       std::move(data_url_blob), blob_context));
+    DataURLBlobReader::ReadDataURLFromBlob(
+        std::move(data_url_blob),
+        base::BindOnce(&OnDataURLRetrieved, std::move(parameters)));
     return;
   }
 
-  DownloadUrlOnUIThread(std::move(parameters), std::move(blob_url_token));
+  StartDownload(std::move(parameters), std::move(blob_url_token));
 }
 
 #if BUILDFLAG(USE_EXTERNAL_POPUP_MENU)

third_party/blink/public/platform/url_conversion.h

@@ -16,8 +16,8 @@ class WebString;
 
 BLINK_PLATFORM_EXPORT GURL WebStringToGURL(const WebString&);
 
-// Convert a data url to to message pipe handle so that it can be passed across
-// processes.
+// Convert a data url to a message pipe handle that corresponds to a remote
+// blob, so that it can be passed across processes.
 BLINK_PLATFORM_EXPORT mojo::ScopedMessagePipeHandle DataURLToMessagePipeHandle(
     const WebString&);