Make extensions BlobReader not self-deleting

This definitely looks extremely sketchy when reading code.  Make the
BlobReader owned by a callback itself instead of needing to document
everywhere that a raw new is ok because the class is self-deleting.
Ultimately, this ends up being the same, but is a readability
improvement.

Change-Id: I56f19679a97e3bf220a09fc7f84d3772972b52c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1804821
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Auto-Submit: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697716}

chrome/browser/apps/platform_apps/api/media_galleries/blob_data_source_factory.cc

@@ -38,13 +38,11 @@ class BlobMediaDataSource : public chrome::mojom::MediaDataSource {
   void StartBlobRequest(chrome::mojom::MediaDataSource::ReadCallback callback,
                         int64_t position,
                         int64_t length) {
-    BlobReader* reader = new BlobReader(  // BlobReader is self-deleting.
-        browser_context_, blob_uuid_,
+    BlobReader::Read(browser_context_, blob_uuid_,
                      base::BindRepeating(&BlobMediaDataSource::OnBlobReaderDone,
                                          weak_factory_.GetWeakPtr(),
-                            base::Passed(&callback)));
-    reader->SetByteRange(position, length);
-    reader->Start();
+                                         base::Passed(&callback)),
+                     position, length);
   }
 
   void OnBlobReaderDone(chrome::mojom::MediaDataSource::ReadCallback callback,

chrome/browser/apps/platform_apps/api/media_galleries/media_galleries_api.cc

@@ -628,13 +628,10 @@ void MediaGalleriesGetMetadataFunction::OnPreferencesInit(
     const std::string& blob_uuid) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // BlobReader is self-deleting.
-  BlobReader* reader =
-      new BlobReader(GetProfile(), blob_uuid,
+  BlobReader::Read(GetProfile(), blob_uuid,
                    base::Bind(&MediaGalleriesGetMetadataFunction::GetMetadata,
-                                this, metadata_type, blob_uuid));
-  reader->SetByteRange(0, net::kMaxBytesToSniff);
-  reader->Start();
+                              this, metadata_type, blob_uuid),
+                   0, net::kMaxBytesToSniff);
 }
 
 void MediaGalleriesGetMetadataFunction::GetMetadata(

extensions/browser/api/feedback_private/feedback_service.cc

@@ -40,21 +40,15 @@ void FeedbackService::SendFeedback(scoped_refptr<FeedbackData> feedback_data,
   feedback_data->set_user_agent(ExtensionsBrowserClient::Get()->GetUserAgent());
 
   if (!feedback_data->attached_file_uuid().empty()) {
-    // Self-deleting object.
-    BlobReader* attached_file_reader =
-        new BlobReader(browser_context_, feedback_data->attached_file_uuid(),
+    BlobReader::Read(browser_context_, feedback_data->attached_file_uuid(),
                      base::Bind(&FeedbackService::AttachedFileCallback,
                                 AsWeakPtr(), feedback_data, callback));
-    attached_file_reader->Start();
   }
 
   if (!feedback_data->screenshot_uuid().empty()) {
-    // Self-deleting object.
-    BlobReader* screenshot_reader =
-        new BlobReader(browser_context_, feedback_data->screenshot_uuid(),
+    BlobReader::Read(browser_context_, feedback_data->screenshot_uuid(),
                      base::Bind(&FeedbackService::ScreenshotCallback,
                                 AsWeakPtr(), feedback_data, callback));
-    screenshot_reader->Start();
   }
 
   CompleteSendFeedback(feedback_data, callback);

extensions/browser/blob_reader.cc

@@ -11,34 +11,64 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 
-BlobReader::BlobReader(content::BrowserContext* browser_context,
+// static
+void BlobReader::Read(content::BrowserContext* browser_context,
                       const std::string& blob_uuid,
-                       BlobReadCallback callback)
-    : BlobReader(
-          content::BrowserContext::GetBlobRemote(browser_context, blob_uuid),
-          std::move(callback)) {}
+                      BlobReader::BlobReadCallback callback,
+                      int64_t offset,
+                      int64_t length) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  CHECK_GE(offset, 0);
+  CHECK_GT(length, 0);
+  CHECK_LE(offset, std::numeric_limits<int64_t>::max() - length);
 
-BlobReader::BlobReader(mojo::PendingRemote<blink::mojom::Blob> blob,
-                       BlobReadCallback callback)
-    : callback_(std::move(callback)), blob_(std::move(blob)) {
+  base::Optional<Range> range = Range{offset, length};
+  Read(browser_context, blob_uuid, std::move(callback), std::move(range));
+}
+
+// static
+void BlobReader::Read(content::BrowserContext* browser_context,
+                      const std::string& blob_uuid,
+                      BlobReader::BlobReadCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  blob_.set_disconnect_handler(
-      base::BindOnce(&BlobReader::Failed, base::Unretained(this)));
+  Read(browser_context, blob_uuid, std::move(callback), base::nullopt);
 }
 
 BlobReader::~BlobReader() { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); }
 
-void BlobReader::SetByteRange(int64_t offset, int64_t length) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  CHECK_GE(offset, 0);
-  CHECK_GT(length, 0);
-  CHECK_LE(offset, std::numeric_limits<int64_t>::max() - length);
+// static
+void BlobReader::Read(content::BrowserContext* browser_context,
+                      const std::string& blob_uuid,
+                      BlobReader::BlobReadCallback callback,
+                      base::Optional<BlobReader::Range> range) {
+  std::unique_ptr<BlobReader> reader(new BlobReader(
+      content::BrowserContext::GetBlobRemote(browser_context, blob_uuid),
+      std::move(range)));
+
+  // Move the reader to be owned by the callback, so hold onto a temporary
+  // pointer to it so we can still call Start on it.
+  BlobReader* raw_reader = reader.get();
+  base::OnceClosure wrapped = base::BindOnce(
+      [](BlobReadCallback callback, std::unique_ptr<BlobReader> reader) {
+        std::move(callback).Run(std::move(reader->blob_data_),
+                                *reader->blob_length_);
+      },
+      std::move(callback), std::move(reader));
+  raw_reader->Start(std::move(wrapped));
+}
 
-  read_range_ = Range{offset, length};
+BlobReader::BlobReader(mojo::PendingRemote<blink::mojom::Blob> blob,
+                       base::Optional<Range> range)
+    : blob_(std::move(blob)), read_range_(std::move(range)) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  blob_.set_disconnect_handler(
+      base::BindOnce(&BlobReader::Failed, base::Unretained(this)));
 }
 
-void BlobReader::Start() {
+void BlobReader::Start(base::OnceClosure callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  callback_ = std::move(callback);
+
   mojo::ScopedDataPipeProducerHandle producer_handle;
   mojo::ScopedDataPipeConsumerHandle consumer_handle;
   MojoResult result =
@@ -81,11 +111,11 @@ void BlobReader::OnDataComplete() {
 }
 
 void BlobReader::Failed() {
-  std::move(callback_).Run(std::make_unique<std::string>(), 0);
-  delete this;
+  blob_length_ = 0;
+  blob_data_ = std::make_unique<std::string>();
+  std::move(callback_).Run();
 }
 
 void BlobReader::Succeeded() {
-  std::move(callback_).Run(std::move(blob_data_), *blob_length_);
-  delete this;
+  std::move(callback_).Run();
 }

extensions/browser/blob_reader.h

@@ -23,8 +23,7 @@ namespace content {
 class BrowserContext;
 }
 
-// This class may only be used from the UI thread. It self-deletes when finished
-// reading.
+// This class may only be used from the UI thread.
 class BlobReader : public blink::mojom::BlobReaderClient,
                    public mojo::DataPipeDrainer::Client {
  public:
@@ -35,18 +34,32 @@ class BlobReader : public blink::mojom::BlobReaderClient,
                                   int64_t blob_total_size)>
       BlobReadCallback;
 
-  BlobReader(content::BrowserContext* browser_context,
+  static void Read(content::BrowserContext* browser_context,
+                   const std::string& blob_uuid,
+                   BlobReadCallback callback,
+                   int64_t offset,
+                   int64_t length);
+  static void Read(content::BrowserContext* browser_context,
                    const std::string& blob_uuid,
                    BlobReadCallback callback);
-  BlobReader(mojo::PendingRemote<blink::mojom::Blob> blob,
-             BlobReadCallback callback);
+
   ~BlobReader() override;
 
-  void SetByteRange(int64_t offset, int64_t length);
+ private:
+  struct Range {
+    uint64_t offset;
+    uint64_t length;
+  };
 
-  void Start();
+  static void Read(content::BrowserContext* browser_context,
+                   const std::string& blob_uuid,
+                   BlobReadCallback callback,
+                   base::Optional<BlobReader::Range> range);
+
+  BlobReader(mojo::PendingRemote<blink::mojom::Blob> blob,
+             base::Optional<Range> range);
+  void Start(base::OnceClosure callback);
 
- private:
   // blink::mojom::BlobReaderClient:
   void OnCalculatedSize(uint64_t total_size,
                         uint64_t expected_content_size) override;
@@ -59,12 +72,8 @@ class BlobReader : public blink::mojom::BlobReaderClient,
   void Failed();
   void Succeeded();
 
-  BlobReadCallback callback_;
+  base::OnceClosure callback_;
   mojo::Remote<blink::mojom::Blob> blob_;
-  struct Range {
-    uint64_t offset;
-    uint64_t length;
-  };
   base::Optional<Range> read_range_;
 
   mojo::Receiver<blink::mojom::BlobReaderClient> receiver_{this};