[BlobAsync] Patch 4: Browser Classes & Logic.

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 "base/memory/shared_memory.h"
+#include "base/stl_util.h"
+
+namespace storage {
+
+using MemoryItemRequest = BlobAsyncTransportStrategy::MemoryItemRequest;
+
+BlobAsyncBuilderHost::BlobAsyncBuilderHost()
+    : max_ipc_memory_size_(kBlobStorageIPCThresholdBytes),
+      max_shared_memory_size_(kBlobStorageMaxSharedMemoryBytes),
+      max_file_size_(kBlobStorageMaxFileSizeBytes),
+      max_blob_in_memory_size_(kBlobStorageMaxBlobMemorySize) {}
+
+BlobAsyncBuilderHost::~BlobAsyncBuilderHost() {
+  STLDeleteContainerPairSecondPointers(async_blob_map_.begin(),
+                                       async_blob_map_.end());
+}
+
+void BlobAsyncBuilderHost::BuildBlobAsync(
+    const std::string& uuid,
+    const std::string& type,
+    const std::vector<DataElement>& descriptions,
+    size_t memory_available,
+    base::Callback<void(const std::vector<storage::BlobItemBytesRequest>&,
+                        const std::vector<base::SharedMemoryHandle>&,
+                        const std::vector<IPC::PlatformFileForTransit>&)>
+        request_memory,
+    base::Callback<void(BlobDataBuilder*)> done,
+    base::Callback<void(IPCBlobCreationCancelCode)> cancel) {
+  if (BlobAsyncTransportStrategy::ShouldBeShortcut(descriptions,
+                                                   memory_available)) {
+    // We have enough memory, and all the data is here, so we use the shortcut
+    // method and populate the old way.
+    BlobDataBuilder builder(uuid);
+    builder.set_content_type(type);
+    for (const DataElement& element : descriptions) {
+      builder.AppendDataElement(element);
+    }
+    done.Run(&builder);

[kinuko, 2015/11/17 14:51:41]

Calling this synchronously while the method name tells 'Async' makes me feel a
bit nervous, could this be fired asyncly or I feel at least this should be
clearly documented.

[dmurph, 2015/11/17 20:40:19]

Renamed, and clarified behavior in comment.

+    return;
+  }
+
+  DCHECK(async_blob_map_.find(uuid) == async_blob_map_.end());
+  BlobAsyncBuilderHost::BlobBuildingState* state =
+      new BlobAsyncBuilderHost::BlobBuildingState();
+  async_blob_map_[uuid] = state;
+  state->type = type;
+  state->request_memory = request_memory;
+  state->done = done;
+  state->cancel = cancel;

[michaeln, 2015/11/17 21:55:28]

naming nit: consider appending _callback to request_memory, done, and cancel
(they look bool'ish at a glance)

[dmurph, 2015/11/19 02:06:17]

Done.

+
+  state->transport_strategy.Initialize(
+      BlobAsyncTransportStrategy::MODE_NO_DISK, max_ipc_memory_size_,
+      max_shared_memory_size_, max_file_size_, max_blob_in_memory_size_, 0,
+      memory_available, uuid, descriptions);

[kinuko, 2015/11/17 14:51:41]

Can you add a comment for '0'? (disk_space_left?)

[dmurph, 2015/11/17 20:40:19]

Done.

+  if (state->transport_strategy.error() !=
+      BlobAsyncTransportStrategy::ERROR_NONE) {
+    LOG(ERROR) << "Error initializing transport strategy: "

[kinuko, 2015/11/17 14:51:41]

DVLOG ?

[dmurph, 2015/11/17 20:40:19]

Done.

[michaeln, 2015/11/17 21:55:28]

is the logging needed in a release build, here and below?

[dmurph, 2015/11/19 02:06:17]

Can I log to debug? There's no way of telling what the error for the developer,
and I can check fail because we need to tell the renderer to cancel & release
the blob info.

+               << state->transport_strategy.error();
+    CancelAndCleanup(uuid, state->transport_strategy.error() ==
+                                   BlobAsyncTransportStrategy::ERROR_TOO_LARGE
+                               ? IPCBlobCreationCancelCode::OUT_OF_MEMORY
+                               : IPCBlobCreationCancelCode::UNKNOWN);
+    return;
+  }
+
+  ContinueBlobMemoryRequests(uuid);
+}
+
+void BlobAsyncBuilderHost::OnMemoryResponses(
+    const std::string& uuid,
+    const std::vector<BlobItemBytesResponse> responses) {
+  AsyncBlobMap::iterator state_it = async_blob_map_.find(uuid);
+  if (state_it == async_blob_map_.end()) {

[michaeln, 2015/11/17 21:55:27]

is this indicative of a bad ipc message or could this legitimately happen?

[dmurph, 2015/11/19 02:06:17]

Bad IPC message. I could have the check happen earlier, would that be better? So
there's a

IsBlobBeingBuilt(const std::string& uuid)

method, and the the BlobStorageHost calls that method to see if we're currently
building.  Then we DCHECK everywhere else. Look good?

+    return;
+  }
+  BlobAsyncBuilderHost::BlobBuildingState* state = state_it->second;
+  BlobAsyncTransportStrategy& strategy = state->transport_strategy;
+  bool populate_success = false;
+  const auto& requests = strategy.requests();
+  for (const BlobItemBytesResponse& response : responses) {
+    if (response.request_number >= requests.size()) {
+      LOG(ERROR) << "Invalid request number " << response.request_number;

[michaeln, 2015/11/17 21:55:28]

we might want to treat this as a badipc?

[dmurph, 2015/11/19 02:06:17]

See above resolution.

+      CancelAndCleanup(uuid, IPCBlobCreationCancelCode::UNKNOWN);
+      return;
+    }
+    const MemoryItemRequest& request = requests[response.request_number];
+    if (request.received) {
+      LOG(ERROR) << "Already received response for that request.";

[michaeln, 2015/11/17 21:55:28]

and this one too, when would this be expected?

[dmurph, 2015/11/19 02:06:17]

Done.

+      CancelAndCleanup(uuid, IPCBlobCreationCancelCode::UNKNOWN);
+      return;
+    }
+    switch (request.message.transport_strategy) {
+      case IPCBlobItemRequestStrategy::IPC:
+        if (response.inline_data.size() < request.message.size) {

[michaeln, 2015/11/17 21:55:28]

ditto

[dmurph, 2015/11/19 02:06:17]

Done.

+          LOG(ERROR) << "Invalid data size " << response.inline_data.size()
+                     << " vs requested size of " << request.message.size;
+          CancelAndCleanup(uuid, IPCBlobCreationCancelCode::UNKNOWN);
+          return;
+        }
+        populate_success = strategy.blob_builder()->PopulateFutureData(
+            request.browser_item_index, &response.inline_data[0],
+            request.browser_item_offset, request.message.size);
+        break;
+      case IPCBlobItemRequestStrategy::SHARED_MEMORY:
+        if (state->num_shared_memory_requests == 0) {

[michaeln, 2015/11/17 21:55:27]

ditto

+          LOG(ERROR) << "Received too many responses for shared memory.";
+          CancelAndCleanup(uuid, IPCBlobCreationCancelCode::UNKNOWN);
+          return;
+        }
+        state->num_shared_memory_requests--;
+        if (!state->shared_memory_block->memory()) {
+          // We just map the whole block, as we'll probably be accessing the
+          // whole thing in this group of responses. Another option is to use
+          // MapAt, remove the mapped boolean, and then exclude the
+          // handle_offset below.
+          size_t handle_size = strategy.shared_memory_handles().at(
+              state->current_shared_memory_handle_index);
+          if (!state->shared_memory_block->Map(handle_size)) {
+            LOG(ERROR) << "Unable to map memory to size " << handle_size;
+            populate_success = false;
+            break;
+          }
+        }
+
+        populate_success = strategy.blob_builder()->PopulateFutureData(
+            request.browser_item_index,
+            static_cast<const char*>(state->shared_memory_block->memory()) +
+                request.message.handle_offset,
+            request.browser_item_offset, request.message.size);
+        break;
+      case IPCBlobItemRequestStrategy::FILE:
+      case IPCBlobItemRequestStrategy::UNKNOWN:
+        NOTREACHED() << "Not implemented yet.";
+        break;
+    }
+    if (!populate_success) {
+      LOG(ERROR) << "Unable to populate blob data.";
+      CancelAndCleanup(uuid, IPCBlobCreationCancelCode::UNKNOWN);
+      return;
+    }
+    state->num_fulfilled_requests++;
+  }
+  ContinueBlobMemoryRequests(uuid);
+}
+
+void BlobAsyncBuilderHost::StopBuildingBlob(const std::string& uuid) {
+  AsyncBlobMap::iterator state_it = async_blob_map_.find(uuid);
+  if (state_it == async_blob_map_.end()) {
+    return;
+  }
+  delete state_it->second;
+  async_blob_map_.erase(state_it);
+}
+
+BlobAsyncBuilderHost::BlobBuildingState::BlobBuildingState()

[kinuko, 2015/11/17 14:51:41]

nit: could these inner-class definitions be placed at the top of this file so
that class definitions don't interleave?

[dmurph, 2015/11/17 20:40:19]

Done.

+    : next_request(0),
+      num_fulfilled_requests(0),
+      num_shared_memory_requests(0),
+      current_shared_memory_handle_index(0) {}
+
+BlobAsyncBuilderHost::BlobBuildingState::~BlobBuildingState() {}
+
+void BlobAsyncBuilderHost::ContinueBlobMemoryRequests(const std::string& uuid) {
+  AsyncBlobMap::iterator state_it = async_blob_map_.find(uuid);
+  DCHECK(state_it != async_blob_map_.end());
+  BlobAsyncBuilderHost::BlobBuildingState* state = state_it->second;
+
+  const auto& requests = state->transport_strategy.requests();

[kinuko, 2015/11/17 14:51:41]

nit: prefer using non-auto class here

[dmurph, 2015/11/17 20:40:19]

Done.

+  BlobAsyncTransportStrategy& strategy = state->transport_strategy;
+  size_t num_requests = requests.size();
+  if (state->num_fulfilled_requests == num_requests) {
+    DoneAndCleanup(uuid);
+    return;
+  }
+  DCHECK_LT(state->num_fulfilled_requests, num_requests);
+
+  std::vector<BlobItemBytesRequest> byte_requests;
+  std::vector<base::SharedMemoryHandle> shared_memory;
+  std::vector<IPC::PlatformFileForTransit> files;
+
+  if (state->next_request == num_requests) {

[michaeln, 2015/11/17 21:55:28]

move this test/early exit up above the local vectors

[dmurph, 2015/11/19 02:06:17]

Done.

+    // We are still waiting on other requests to come back.
+    return;
+  }
+
+  for (; state->next_request < num_requests; ++state->next_request) {
+    const MemoryItemRequest& request = requests[state->next_request];
+
+    bool stop_accumulating = false;
+    bool using_shared_memory_handle = state->num_shared_memory_requests > 0;
+    switch (request.message.transport_strategy) {
+      case IPCBlobItemRequestStrategy::IPC:
+        byte_requests.push_back(request.message);
+        break;
+      case IPCBlobItemRequestStrategy::SHARED_MEMORY:
+        if (using_shared_memory_handle &&
+            state->current_shared_memory_handle_index !=
+                request.message.handle_index) {
+          // We only want one shared memory per requesting blob.
+          stop_accumulating = true;
+          break;
+        }
+        using_shared_memory_handle = true;
+        state->current_shared_memory_handle_index =
+            request.message.handle_index;
+        state->num_shared_memory_requests++;
+
+        if (!state->shared_memory_block) {
+          state->shared_memory_block.reset(new base::SharedMemory());
+          size_t size =
+              strategy.shared_memory_handles()[request.message.handle_index];
+          if (!state->shared_memory_block->CreateAnonymous(size)) {
+            LOG(ERROR) << "Unable to allocate shared memory for blob transfer.";
+            CancelAndCleanup(uuid, IPCBlobCreationCancelCode::OUT_OF_MEMORY);
+            return;
+          }
+        }
+        shared_memory.push_back(state->shared_memory_block->handle());
+        byte_requests.push_back(request.message);
+        // Since we are only using one handle at a time, transform our handle
+        // index correctly back to 0.
+        byte_requests.back().handle_index = 0;
+        break;
+      case IPCBlobItemRequestStrategy::FILE:
+      case IPCBlobItemRequestStrategy::UNKNOWN:
+        NOTREACHED() << "Not implemented yet.";
+        break;
+    }
+    if (stop_accumulating) {
+      break;
+    }
+  }
+
+  DCHECK(!requests.empty());
+
+  state->request_memory.Run(byte_requests, shared_memory, files);
+}
+
+void BlobAsyncBuilderHost::CancelAndCleanup(const std::string& uuid,
+                                            IPCBlobCreationCancelCode code) {
+  AsyncBlobMap::iterator state_it = async_blob_map_.find(uuid);
+  if (state_it == async_blob_map_.end()) {

[kinuko, 2015/11/17 14:51:41]

I feel this and the similar line in DoneAndCleanup should be simply DCHECK,
could this happen?

[dmurph, 2015/11/17 20:40:19]

Done.

[michaeln, 2015/11/17 21:55:28]

looks like this could be dcheck'able instead, i think all callers of this method
have an early exit for this condition prior to calling this method

+    return;
+  }
+  BlobAsyncBuilderHost::BlobBuildingState* state = state_it->second;
+
+  state->cancel.Run(code);
+  delete state;

[kinuko, 2015/11/17 14:51:41]

could use ScopedPtrMap or assign state_it->second to scoped_ptr on line 252,
let's avoid manual deletion

[dmurph, 2015/11/17 20:40:19]

Done.

+  async_blob_map_.erase(state_it);
+}
+
+void BlobAsyncBuilderHost::DoneAndCleanup(const std::string& uuid) {
+  AsyncBlobMap::iterator state_it = async_blob_map_.find(uuid);
+  if (state_it == async_blob_map_.end()) {

[michaeln, 2015/11/17 21:55:27]

ditto

[dmurph, 2015/11/19 02:06:17]

Changed to scoped ptr map, and cleaned this up a bit.

+    return;
+  }
+  BlobAsyncBuilderHost::BlobBuildingState* state = state_it->second;
+  BlobDataBuilder* builder = state->transport_strategy.blob_builder();
+  builder->set_content_type(state->type);
+  DCHECK(builder);
+  state->done.Run(builder);
+  delete state;
+  async_blob_map_.erase(state_it);
+}
+
+}  // namespace storage