[BlobAsync] Adding better error reporting and some new tests.

storage/browser/blob/blob_async_builder_host.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "storage/browser/blob/blob_async_builder_host.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/memory/shared_memory.h"
 #include "storage/browser/blob/blob_data_handle.h"
 #include "storage/browser/blob/blob_storage_context.h"
 
 namespace storage {
 namespace {
+
 bool CalculateBlobMemorySize(const std::vector<DataElement>& elements,
                              size_t* shortcut_bytes,
                              uint64_t* total_bytes) {
   DCHECK(shortcut_bytes);
   DCHECK(total_bytes);
   base::CheckedNumeric<uint64_t> total_size_checked = 0;
   base::CheckedNumeric<size_t> shortcut_size_checked = 0;
   for (const auto& e : elements) {
     if (e.type() == DataElement::TYPE_BYTES) {
       total_size_checked += e.length();
       shortcut_size_checked += e.length();
     } else if (e.type() == DataElement::TYPE_BYTES_DESCRIPTION) {
       total_size_checked += e.length();
     } else {
       continue;
     }
     if (!total_size_checked.IsValid() || !shortcut_size_checked.IsValid()) {
       return false;
     }
   }
   *shortcut_bytes = shortcut_size_checked.ValueOrDie();
   *total_bytes = total_size_checked.ValueOrDie();
   return true;
 }
+
+IPCBlobCreationCancelCode TransformReferencedBlobErrorToConstructingError(
+    IPCBlobCreationCancelCode referenced_blob_error) {
+  switch (referenced_blob_error) {
+    // For most cases we propogate the error.

[jsbell, 2016/04/04 20:41:19]

Typo: propagate

[dmurph, 2016/04/04 22:17:42]

Done.

+    case IPCBlobCreationCancelCode::FILE_WRITE_FAILED:
+    case IPCBlobCreationCancelCode::SOURCE_DIED_IN_TRANSIT:
+    case IPCBlobCreationCancelCode::REFERENCED_BLOB_BROKEN:
+    case IPCBlobCreationCancelCode::OUT_OF_MEMORY:
+      return referenced_blob_error;
+    // Others we report that the referenced blob is broken, as we don't know
+    // why (the BLOB_DEREFERENCED_WHILE_BUILDING should never happen, as we hold

[jsbell, 2016/04/04 20:41:19]

Do we want to DCHECK so we'd notice this during development?

[dmurph, 2016/04/04 22:17:42]

Done.

+    // onto the reference of the blobs we're using).
+    case IPCBlobCreationCancelCode::UNKNOWN:
+    case IPCBlobCreationCancelCode::BLOB_DEREFERENCED_WHILE_BUILDING:
+      return IPCBlobCreationCancelCode::REFERENCED_BLOB_BROKEN;
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 using MemoryItemRequest =
     BlobAsyncTransportRequestBuilder::RendererMemoryItemRequest;
 
 BlobAsyncBuilderHost::BlobBuildingState::BlobBuildingState(
     const std::string& uuid,
     std::set<std::string> referenced_blob_uuids,
     std::vector<scoped_ptr<BlobDataHandle>>* referenced_blob_handles)
     : data_builder(uuid),
@@ skipping 223 matching lines @@
   // 'built'. In this case, it's destructed in the context, but we still have
   // it in our map. Hence we make sure the context has the entry before
   // calling cancel.
   if (context->registry().HasEntry(uuid))
     context->CancelPendingBlob(uuid, code);
   async_blob_map_.erase(state_it);
 }
 
 void BlobAsyncBuilderHost::CancelAll(BlobStorageContext* context) {
   DCHECK(context);
-  // Some of our pending blobs may still be referenced elsewhere.
+  // If the blob still exists in the context (and is being built), then we know
+  // that someone else is expecting our blob, and we need to cancel it to let
+  // the dependency know it's gone.
   std::vector<scoped_ptr<BlobDataHandle>> referenced_pending_blobs;
   for (const auto& uuid_state_pair : async_blob_map_) {
     if (context->IsBeingBuilt(uuid_state_pair.first)) {
       referenced_pending_blobs.emplace_back(
           context->GetBlobDataFromUUID(uuid_state_pair.first));
     }
   }
   // We clear the map before canceling them to prevent any strange reentry into
   // our class (see ReferencedBlobFinished) if any blobs were waiting for others
   // to construct.
@@ skipping 80 matching lines @@
 
   state->request_memory_callback.Run(
       std::move(byte_requests), std::move(shared_memory),
       make_scoped_ptr(new std::vector<base::File>()));
   return BlobTransportResult::PENDING_RESPONSES;
 }
 
 void BlobAsyncBuilderHost::ReferencedBlobFinished(
     const std::string& owning_blob_uuid,
     base::WeakPtr<BlobStorageContext> context,
-    bool construction_success) {
+    bool construction_success,
+    IPCBlobCreationCancelCode reason) {

[jsbell, 2016/04/04 20:41:19]

ISTM we should combine these, e.g. ResultCode w/ OK/SUCCESS as an option. Is
there any reason not to?

[dmurph, 2016/04/04 22:17:42]

YES this is definitely better. However, this would be a much larger change, so
I'd like to do that in a followup CL. Bug here:
crbug.com/600528

   if (!context) {
     return;
   }
   auto state_it = async_blob_map_.find(owning_blob_uuid);
   if (state_it == async_blob_map_.end()) {
     return;
   }
   if (!construction_success) {
     CancelBuildingBlob(owning_blob_uuid,
-                       IPCBlobCreationCancelCode::SOURCE_DIED_IN_TRANSIT,
+                       TransformReferencedBlobErrorToConstructingError(reason),
                        context.get());
     return;
   }
   BlobBuildingState* state = state_it->second.get();
   DCHECK_GT(state->num_referenced_blobs_building, 0u);
   if (--state->num_referenced_blobs_building == 0) {
     context->CompletePendingBlob(state->data_builder);
     async_blob_map_.erase(state->data_builder.uuid());
   }
 }
@@ skipping 20 matching lines @@
     if (state->num_referenced_blobs_building > 0) {
       // We wait until referenced blobs are done.
       return;
     }
   }
   context->CompletePendingBlob(state->data_builder);
   async_blob_map_.erase(state->data_builder.uuid());
 }
 
 }  // namespace storage