[Blobs] Inform BlobRegistrImpl if a blob is destroyed while being build.

Which means that we abort transporting bytes for blobs we don't need.
Also fixes a memory leak if blob is dereferenced while allocating quota.

Bug: 740596
Change-Id: I03a32b920f4e213adf59b54745c2d5fe071b3132
Reviewed-on: https://chromium-review.googlesource.com/965243
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543773}

storage/browser/blob/blob_entry.cc

@@ -40,13 +40,13 @@ BlobEntry::BuildingState::~BuildingState() {
   DCHECK(!transport_quota_request);
 }
 
-void BlobEntry::BuildingState::CancelRequests() {
-  if (copy_quota_request) {
+void BlobEntry::BuildingState::CancelRequestsAndAbort() {
+  if (copy_quota_request)
     copy_quota_request->Cancel();
-  }
-  if (transport_quota_request) {
+  if (transport_quota_request)
     transport_quota_request->Cancel();
-  }
+  if (build_aborted_callback)
+    std::move(build_aborted_callback).Run();
 }
 
 BlobEntry::BlobEntry(const std::string& content_type,

storage/browser/blob/blob_entry.h

@@ -28,6 +28,7 @@ class STORAGE_EXPORT BlobEntry {
  public:
   using TransportAllowedCallback = base::OnceCallback<
       void(BlobStatus, std::vector<BlobMemoryController::FileCreationInfo>)>;
+  using BuildAbortedCallback = base::OnceClosure;
 
   // Records a copy from a referenced blob. Copies happen after referenced blobs
   // are complete & quota for the copies is granted.
@@ -60,8 +61,9 @@ class STORAGE_EXPORT BlobEntry {
                   size_t num_building_dependent_blobs);
     ~BuildingState();
 
-    // Cancels pending memory or file requests.
-    void CancelRequests();
+    // Cancels pending memory or file requests, and calls aborted callback if it
+    // is set.
+    void CancelRequestsAndAbort();
 
     const bool transport_items_present;
     // We can have trasnport data that's either populated or unpopulated. If we
@@ -69,6 +71,8 @@ class STORAGE_EXPORT BlobEntry {
     TransportAllowedCallback transport_allowed_callback;
     std::vector<ShareableBlobDataItem*> transport_items;
 
+    BuildAbortedCallback build_aborted_callback;
+
     // Stores all blobs that we're depending on for building. This keeps the
     // blobs alive while we build our blob.
     std::vector<std::unique_ptr<BlobDataHandle>> dependent_blobs;

storage/browser/blob/blob_registry_impl.cc

@@ -89,6 +89,9 @@ class BlobRegistryImpl::BlobUnderConstruction {
                     const std::string& bad_message_reason = "") {
     DCHECK(BlobStatusIsError(reason));
     DCHECK_EQ(bad_message_reason.empty(), !BlobStatusIsBadIPC(reason));
+    // Cancelling would also try to delete |this| by removing it from
+    // blobs_under_construction_, so preemptively own |this|.
+    auto self = std::move(blob_registry_->blobs_under_construction_[uuid()]);
     // The blob might no longer have any references, in which case it may no
     // longer exist. If that happens just skip calling cancel.
     if (context() && context()->registry().HasEntry(uuid()))
@@ -526,8 +529,10 @@ void BlobRegistryImpl::Register(
       this, uuid, content_type, content_disposition, std::move(elements),
       bindings_.GetBadMessageCallback());
 
-  std::unique_ptr<BlobDataHandle> handle =
-      context_->AddFutureBlob(uuid, content_type, content_disposition);
+  std::unique_ptr<BlobDataHandle> handle = context_->AddFutureBlob(
+      uuid, content_type, content_disposition,
+      base::BindOnce(&BlobRegistryImpl::BlobBuildAborted,
+                     weak_ptr_factory_.GetWeakPtr(), uuid));
   BlobImpl::Create(std::move(handle), std::move(blob));
 
   blobs_under_construction_[uuid]->StartTransportation();
@@ -596,6 +601,10 @@ void BlobRegistryImpl::SetURLStoreCreationHookForTesting(
   g_url_store_creation_hook = hook;
 }
 
+void BlobRegistryImpl::BlobBuildAborted(const std::string& uuid) {
+  blobs_under_construction_.erase(uuid);
+}
+
 void BlobRegistryImpl::StreamingBlobDone(
     RegisterFromStreamCallback callback,
     BlobBuilderFromStream* builder,

storage/browser/blob/blob_registry_impl.h

@@ -70,6 +70,8 @@ class STORAGE_EXPORT BlobRegistryImpl : public blink::mojom::BlobRegistry {
  private:
   class BlobUnderConstruction;
 
+  void BlobBuildAborted(const std::string& uuid);
+
   void StreamingBlobDone(RegisterFromStreamCallback callback,
                          BlobBuilderFromStream* builder,
                          std::unique_ptr<BlobDataHandle> result);

storage/browser/blob/blob_registry_impl_unittest.cc

@@ -917,7 +917,8 @@ TEST_F(BlobRegistryImplTest,
   const std::string kDepId = "dep-id";
 
   // Create future blob.
-  auto blob_handle = context_->AddFutureBlob(kDepId, "", "");
+  auto blob_handle = context_->AddFutureBlob(
+      kDepId, "", "", BlobStorageContext::BuildAbortedCallback());
   blink::mojom::BlobPtrInfo referenced_blob_info;
   mojo::MakeStrongBinding(std::make_unique<MockBlob>(kDepId),
                           MakeRequest(&referenced_blob_info));
@@ -988,6 +989,42 @@ TEST_F(BlobRegistryImplTest,
   EXPECT_EQ(0u, BlobsUnderConstruction());
 }
 
+TEST_F(BlobRegistryImplTest,
+       Register_DefereferencedWhileBuildingBeforeTransportingByFile) {
+  const std::string kId = "id";
+  const std::string kData =
+      base::RandBytesAsString(kTestBlobStorageMaxBlobMemorySize + 42);
+
+  blink::mojom::BytesProviderPtrInfo bytes_provider_info;
+  auto request = MakeRequest(&bytes_provider_info);
+
+  std::vector<blink::mojom::DataElementPtr> elements;
+  elements.push_back(
+      blink::mojom::DataElement::NewBytes(blink::mojom::DataElementBytes::New(
+          kData.size(), base::nullopt, std::move(bytes_provider_info))));
+
+  blink::mojom::BlobPtr blob;
+  EXPECT_TRUE(registry_->Register(MakeRequest(&blob), kId, "", "",
+                                  std::move(elements)));
+  EXPECT_TRUE(bad_messages_.empty());
+
+  EXPECT_TRUE(context_->registry().HasEntry(kId));
+  EXPECT_TRUE(context_->GetBlobDataFromUUID(kId)->IsBeingBuilt());
+  EXPECT_EQ(1u, BlobsUnderConstruction());
+
+  // Now drop all references to the blob.
+  blob.reset();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(context_->registry().HasEntry(kId));
+
+  // Now cause construction to complete, if it would still be going on.
+  CreateBytesProvider(kData, std::move(request));
+  scoped_task_environment_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0u, BlobsUnderConstruction());
+}
+
 TEST_F(BlobRegistryImplTest, RegisterFromStream) {
   const std::string kData = "hello world, this is a blob";
   const std::string kContentType = "content/type";

storage/browser/blob/blob_storage_context.cc

@@ -157,7 +157,8 @@ void BlobStorageContext::RevokePublicBlobURL(const GURL& blob_url) {
 std::unique_ptr<BlobDataHandle> BlobStorageContext::AddFutureBlob(
     const std::string& uuid,
     const std::string& content_type,
-    const std::string& content_disposition) {
+    const std::string& content_disposition,
+    BuildAbortedCallback build_aborted_callback) {
   DCHECK(!registry_.HasEntry(uuid));
 
   BlobEntry* entry =
@@ -166,6 +167,8 @@ std::unique_ptr<BlobDataHandle> BlobStorageContext::AddFutureBlob(
   entry->set_status(BlobStatus::PENDING_CONSTRUCTION);
   entry->set_building_state(std::make_unique<BlobEntry::BuildingState>(
       false, TransportAllowedCallback(), 0));
+  entry->building_state_->build_aborted_callback =
+      std::move(build_aborted_callback);
   return CreateHandle(uuid, entry);
 }
 
@@ -315,6 +318,8 @@ std::unique_ptr<BlobDataHandle> BlobStorageContext::BuildBlobInternal(
     DCHECK(previous_building_state->copies.empty());
     std::swap(building_state->build_completion_callbacks,
               previous_building_state->build_completion_callbacks);
+    building_state->build_aborted_callback =
+        std::move(previous_building_state->build_aborted_callback);
     auto runner = base::ThreadTaskRunnerHandle::Get();
     for (auto& callback : previous_building_state->build_started_callbacks)
       runner->PostTask(FROM_HERE,
@@ -652,7 +657,7 @@ void BlobStorageContext::OnDependentBlobFinished(
 
 void BlobStorageContext::ClearAndFreeMemory(BlobEntry* entry) {
   if (entry->building_state_)
-    entry->building_state_->CancelRequests();
+    entry->building_state_->CancelRequestsAndAbort();
   entry->ClearItems();
   entry->ClearOffsets();
   entry->set_size(0);

storage/browser/blob/blob_storage_context.h

@@ -48,6 +48,7 @@ class STORAGE_EXPORT BlobStorageContext
     : public base::trace_event::MemoryDumpProvider {
  public:
   using TransportAllowedCallback = BlobEntry::TransportAllowedCallback;
+  using BuildAbortedCallback = BlobEntry::BuildAbortedCallback;
 
   // Initializes the context without disk support.
   BlobStorageContext();
@@ -115,7 +116,8 @@ class STORAGE_EXPORT BlobStorageContext
   std::unique_ptr<BlobDataHandle> AddFutureBlob(
       const std::string& uuid,
       const std::string& content_type,
-      const std::string& content_disposition);
+      const std::string& content_disposition,
+      BuildAbortedCallback build_aborted_callback);
 
   // Same as BuildBlob, but for a blob that was previously registered by calling
   // AddFutureBlob.