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

storage/browser/blob/blob_async_builder_host.cc

Index: storage/browser/blob/blob_async_builder_host.cc
diff --git a/storage/browser/blob/blob_async_builder_host.cc b/storage/browser/blob/blob_async_builder_host.cc
index ec9abd2fd0125da9e66f5f6c4012b57c91ac5380..362d44b703d0a125ae279534fdcc4c456c452a64 100644
--- a/storage/browser/blob/blob_async_builder_host.cc
+++ b/storage/browser/blob/blob_async_builder_host.cc
@@ -16,6 +16,7 @@
 
 namespace storage {
 namespace {
+
 bool CalculateBlobMemorySize(const std::vector<DataElement>& elements,
                              size_t* shortcut_bytes,
                              uint64_t* total_bytes) {
@@ -40,6 +41,26 @@ bool CalculateBlobMemorySize(const std::vector<DataElement>& elements,
   *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 =
@@ -283,7 +304,9 @@ void BlobAsyncBuilderHost::CancelBuildingBlob(const std::string& uuid,
 
 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)) {
@@ -384,7 +407,8 @@ BlobTransportResult BlobAsyncBuilderHost::ContinueBlobMemoryRequests(
 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;
   }
@@ -394,7 +418,7 @@ void BlobAsyncBuilderHost::ReferencedBlobFinished(
   }
   if (!construction_success) {
     CancelBuildingBlob(owning_blob_uuid,
-                       IPCBlobCreationCancelCode::SOURCE_DIED_IN_TRANSIT,
+                       TransformReferencedBlobErrorToConstructingError(reason),
                        context.get());
     return;
   }