[BlobAsync] Asynchronous Blob Construction Final Patch

content/browser/fileapi/blob_storage_host.cc

Index: content/browser/fileapi/blob_storage_host.cc
diff --git a/content/browser/fileapi/blob_storage_host.cc b/content/browser/fileapi/blob_storage_host.cc
index faf81010dd3f201dd48476a2a988ebeed51ca9b4..d8988932c1b4b9ae8b24d75d7225a796a8d909cb 100644
--- a/content/browser/fileapi/blob_storage_host.cc
+++ b/content/browser/fileapi/blob_storage_host.cc
@@ -4,25 +4,48 @@
 
 #include "content/browser/fileapi/blob_storage_host.h"
 
+#include <vector>
+
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
+#include "storage/browser/blob/blob_async_builder_host.h"
+#include "storage/browser/blob/blob_data_builder.h"
+#include "storage/browser/blob/blob_data_handle.h"
 #include "storage/browser/blob/blob_storage_context.h"
+#include "storage/common/blob_storage/blob_item_bytes_request.h"
+#include "storage/common/blob_storage/blob_item_bytes_response.h"
 #include "url/gurl.h"
 
 using storage::BlobStorageContext;
 
 namespace content {
+namespace {
+// We don't support sending files over yet, so this is a temporary adapter to
+// to bridge the output of the BlobTransportStrategy and the message format.

[kinuko, 2015/12/13 08:41:45]

nit: double 'to'

[dmurph, 2015/12/18 03:22:49]

Done.

+void NoopFileHandleAdapter(

[michaeln, 2015/12/15 02:40:48]

The name makes it sounds like a noop, which is definitely not true,
RequestMemoryCallbackAdapter?

[dmurph, 2015/12/18 03:22:49]

Done.

+    const base::Callback<void(const std::vector<storage::BlobItemBytesRequest>&,
+                              const std::vector<base::SharedMemoryHandle>&,
+                              const std::vector<IPC::PlatformFileForTransit>&)>&
+        request_memory_callback,
+    const std::vector<storage::BlobItemBytesRequest>& requests,
+    const std::vector<base::SharedMemoryHandle>& shared_memory,
+    const std::vector<uint64_t>& file_handles) {
+  DCHECK(file_handles.empty());
+  request_memory_callback.Run(requests, shared_memory,
+                              std::vector<IPC::PlatformFileForTransit>());
+}
+}  // namespace
 
 BlobStorageHost::BlobStorageHost(BlobStorageContext* context)
-    : context_(context->AsWeakPtr()) {
-}
+    : context_(context->AsWeakPtr()) {}
 
 BlobStorageHost::~BlobStorageHost() {
   if (!context_.get())
     return;
-  for (std::set<GURL>::iterator iter = public_blob_urls_.begin();
-       iter != public_blob_urls_.end(); ++iter) {
-    context_->RevokePublicBlobURL(*iter);
+  for (const GURL& url : public_blob_urls_) {
+    context_->RevokePublicBlobURL(url);
   }
   for (BlobReferenceMap::iterator iter = blobs_inuse_map_.begin();
        iter != blobs_inuse_map_.end(); ++iter) {

[kinuko, 2015/12/13 08:41:44]

nit: can use range for

[dmurph, 2015/12/18 03:22:50]

Done.

@@ -31,42 +54,64 @@ BlobStorageHost::~BlobStorageHost() {
   }

[michaeln, 2015/12/15 20:55:57]

What if BlobAsyncBuilderHost.ascync_blob_map_ is not empty?

[dmurph, 2015/12/18 03:22:49]

Since we're storing a reference in our in other map, we call Decrement in the
context, and that will correctly destruct the blob.

 }
 
-bool BlobStorageHost::StartBuildingBlob(const std::string& uuid) {
-  if (!context_.get() || uuid.empty() || context_->IsInUse(uuid))
+bool BlobStorageHost::RegisterBlobUUID(const std::string& uuid) {
+  if (!context_.get() || uuid.empty() || context_->registry().GetEntry(uuid)) {

[michaeln, 2015/12/15 02:40:48]

context_->registry().GetEntry(uuid)

That looks like a fairly common test, I'd vote for retaining the
context->IsInUse() helper.

[dmurph, 2015/12/18 03:22:49]

Done :)

[michaeln, 2016/01/13 02:42:43]

what happened to this change?

     return false;
-  context_->StartBuildingBlob(uuid);
+  }
   blobs_inuse_map_[uuid] = 1;
+  context_->RegisterBlobUUID(uuid);
   return true;
 }
 
-bool BlobStorageHost::AppendBlobDataItem(
+void BlobStorageHost::StartBuildingBlob(
     const std::string& uuid,
-    const storage::DataElement& data_item) {
-  if (!context_.get() || !IsBeingBuiltInHost(uuid))
-    return false;
-  context_->AppendBlobDataItem(uuid, data_item);
-  return true;
+    const std::string& type,
+    const std::vector<storage::DataElement>& descriptions,
+    const base::Callback<void(const std::vector<storage::BlobItemBytesRequest>&,
+                              const std::vector<base::SharedMemoryHandle>&,
+                              const std::vector<IPC::PlatformFileForTransit>&)>&
+        request_memory,
+    const base::Closure& done,
+    const base::Callback<void(storage::IPCBlobCreationCancelCode)>& cancel) {
+  if (!context_.get() || uuid.empty() || !context_->registry().GetEntry(uuid)) {
+    cancel.Run(storage::IPCBlobCreationCancelCode::UNKNOWN);
+    return;

[kinuko, 2015/12/13 08:41:44]

For uuid.empty(), !GetEntry(uuid) and StartBuildingBlob returning false cases--
should we return false from this method too and handle it as BadMessageReceived?
 (I remember we talked something like that when we worked on async_builder's
StartBuildingBlob, didn't we?)

[dmurph, 2015/12/18 03:22:49]

Hooked up the bad message handling. I don't think we ever called the bad message
handler for blobs, so yay we're more annoying for malicious IPCers.

+  }
+  if (!context_->MaybeStartAsyncBlobTransfer(uuid)) {
+    cancel.Run(storage::IPCBlobCreationCancelCode::UNKNOWN);
+    // Just in case we have it, let's cancel everything.
+    context_->CleanupCanceledAsyncBlobTransfer(uuid);
+    async_builder_.StopBuildingBlob(uuid);
+    return;
+  }
+  async_builder_.StartBuildingBlob(
+      uuid, type, descriptions, context_->memory_available(),
+      base::Bind(&NoopFileHandleAdapter, request_memory),
+      base::Bind(&BlobStorageContext::FinishAsyncBlobTransfer,
+                 context_->AsWeakPtr(), done, cancel),
+      base::Bind(&BlobStorageContext::CancelAsyncBlobTransfer,
+                 context_->AsWeakPtr(), uuid, cancel));

[kinuko, 2015/12/13 08:41:45]

It's a bit confusing that we have many slightly different methods to cancel
building blob with several indirect callbacks. I've tried to decrypt these...

cancel callback passed to this method is: DH::CancelBlob()
DH::CancelBlob() does:
- call Host::CancelBuildingBlob
- Send(CancelBuildingBlobMsg)
Host::CancelBuildingBlob does:
- call Context::CleanupCanceledAsync
- delete from map
Context::CancelAsync does:
- call Context::CleanupCanceledAsync
- call given callback (which is DH::CancelBlob)

And in the line 81-84 case we do:
- call given callback (which is DH::CancelBlob)
- call Context::CleanupCanceledAsync
- call AsyncBuilder::StopBuildingBlob to unregister from the builder

Are all these really organized?  For example CleanupCanceledAsync seems to be
attempted multiple times in some call cases, and calling sequence is not really
fixed either?

Also I don't think adapter methods like
Context::{Finish,Cancel}AsyncBlobTransfer should be placed on Context, which
doesn't give clearer definition about what these methods are supposed to do
without callback context.

Could we at least unify the canceling sequence used within this method?

[michaeln, 2015/12/15 20:55:57]

Agreed that having these callbacks bound to Context methods is a tough read.

I was looking thru the asyncbuilder class and noticed that the callbacks are
always run while processing an incoming ipc msg. They're never run truely
asyncly with respect to the API directly called by the BlobStorageHost class.
The callbacks always occur from within either StartBuildingBlob() or
OnMemoryResponses() and I think at most one callback is invoked per
AsyncBuilderHost method call. Is that right and is that fair to say will remain
true always?

If so, here's an idea to help readability. Instead of using callbacks for those
return paths, return values...

result_code = builder->Start(id, type, item_infos, &memory_requests_out);
switch (result_code) {
 case DONE:
    // woohoo, we're done
    break;
 case NOTDONEYET:
    // send some memory requests
    break;
 case ERROR:
    // doh, something ain't right
    break;
 case BADMESSAGE:
    // yikes, something really ain't right
    break;
}

result_code = builder->AddMemoryResponse(id, memory_responses,
&memory_requests_out);
// similar maybe the same, maybe use a helper method for that part?

wdyt?

[dmurph, 2015/12/18 03:22:49]

Did this!

 }
 
-bool BlobStorageHost::CancelBuildingBlob(const std::string& uuid) {
-  if (!context_.get() || !IsBeingBuiltInHost(uuid))
-    return false;
-  blobs_inuse_map_.erase(uuid);
-  context_->CancelBuildingBlob(uuid);
-  return true;
+void BlobStorageHost::OnBlobMemoryResponses(
+    const std::string& uuid,
+    const std::vector<storage::BlobItemBytesResponse>& responses) {
+  if (!context_.get() || uuid.empty() || !IsBeingBuiltInHost(uuid))
+    return;
+  async_builder_.OnMemoryResponses(uuid, responses);

[kinuko, 2015/12/13 08:41:45]

ditto

[dmurph, 2015/12/18 03:22:50]

Done.

 }
 
-bool BlobStorageHost::FinishBuildingBlob(
-    const std::string& uuid, const std::string& content_type) {
-  if (!context_.get() || !IsBeingBuiltInHost(uuid))
+bool BlobStorageHost::CancelBuildingBlob(const std::string& uuid) {
+  if (!context_.get() || uuid.empty() || !IsInUseInHost(uuid))
     return false;

[michaeln, 2015/12/15 02:40:48]

Should this involve decrementing the refcount now that references to blobs can
be passed around prior to them being fully built?

[dmurph, 2015/12/18 03:22:49]

Sure, we can do it that way.

-  context_->FinishBuildingBlob(uuid, content_type);
-  return true;
+  bool deleted = context_->CleanupCanceledAsyncBlobTransfer(uuid);
+  if (deleted)
+    blobs_inuse_map_.erase(uuid);
+  return deleted;
 }
 
 bool BlobStorageHost::IncrementBlobRefCount(const std::string& uuid) {
-  if (!context_.get() || !context_->IsInUse(uuid) ||
-      context_->IsBeingBuilt(uuid))
+  if (!context_.get() || !context_->registry().GetEntry(uuid))
     return false;
   context_->IncrementBlobRefCount(uuid);
   blobs_inuse_map_[uuid] += 1;
@@ -83,10 +128,10 @@ bool BlobStorageHost::DecrementBlobRefCount(const std::string& uuid) {
   return true;
 }
 
-bool BlobStorageHost::RegisterPublicBlobURL(
-    const GURL& blob_url, const std::string& uuid) {
+bool BlobStorageHost::RegisterPublicBlobURL(const GURL& blob_url,
+                                            const std::string& uuid) {
   if (!context_.get() || !IsInUseInHost(uuid) ||
-      context_->IsUrlRegistered(blob_url))
+      context_->registry().IsURLMapped(blob_url))
     return false;
   context_->RegisterPublicBlobURL(blob_url, uuid);
   public_blob_urls_.insert(blob_url);