[BlobAsync] Patch 3: Renderer Classes & Logic

content/child/blob_storage/blob_transport_controller.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 "content/child/blob_storage/blob_transport_controller.h"
+
+#include <vector>
+
+#include "base/containers/scoped_ptr_hash_map.h"
+#include "base/lazy_instance.h"
+#include "base/memory/shared_memory.h"
+#include "base/stl_util.h"
+#include "content/child/blob_storage/blob_consolidation.h"
+#include "content/child/thread_safe_sender.h"
+#include "ipc/ipc_sender.h"
+#include "storage/common/blob_storage/blob_item_bytes_request.h"
+#include "storage/common/blob_storage/blob_item_bytes_response.h"
+#include "storage/common/data_element.h"
+
+using base::SharedMemory;
+using base::SharedMemoryHandle;
+using storage::BlobItemBytesRequest;
+using storage::BlobItemBytesResponse;
+using storage::IPCBlobItemRequestStrategy;
+using storage::DataElement;
+
+namespace content {
+
+using storage::IPCBlobCreationCancelCode;
+
+using ConsolidatedItem = BlobConsolidation::ConsolidatedItem;
+using ReadStatus = BlobConsolidation::ReadStatus;
+
+namespace {
+const size_t kLargeThresholdBytes = 250 * 1024;
+static base::LazyInstance<BlobTransportController> controller_ =

[kinuko, 2015/10/26 06:44:22]

nit: prefer prefixing with g_ without trailing _, or whatever that can be easily
distinguished from member variables

[dmurph, 2015/10/28 01:00:51]

Done.

+    LAZY_INSTANCE_INITIALIZER;
+}
+
+BlobTransportController* BlobTransportController::GetInstance() {
+  return controller_.Pointer();
+}
+
+BlobTransportController::~BlobTransportController() {
+  STLDeleteValues(&blob_storage_);
+}
+
+void BlobTransportController::InitiateBlobTransfer(
+    const std::string& uuid,
+    const std::string& type,
+    scoped_ptr<BlobConsolidation> consolidation,
+    IPC::Sender* sender) {
+  bool success = HoldBlobConsolidation(uuid, consolidation.Pass());
+  CHECK(success);
+
+  std::vector<storage::DataElement> descriptions;
+  GetDescriptions(uuid, kLargeThresholdBytes, &descriptions);
+  // TODO(dmurph): Uncomment when IPC messages are added.
+  // sender->Send(new BlobStorageMsg_StartBuildingBlob(uuid, type,
+  // descriptions));
+}
+
+void BlobTransportController::OnMemoryRequest(
+    const std::string& uuid,
+    const std::vector<storage::BlobItemBytesRequest>& requests,
+    std::vector<base::SharedMemoryHandle>* memory_handles,
+    const std::vector<IPC::PlatformFileForTransit>& file_handles,
+    IPC::Sender* sender) {
+  std::vector<storage::BlobItemBytesResponse> responses;
+  ResponsesStatus status =
+      GetResponses(uuid, requests, memory_handles, file_handles, &responses);
+
+  bool success = false;
+  switch (status) {

[kinuko, 2015/10/26 06:44:22]

For some of those errors I'm not fully sure how these could happen, assuming
that browser should be returning trustful responses.

[dmurph, 2015/10/28 01:00:52]

They *shouldn't*, but someone can screw up the blob system. Should I do
something different?

[michaeln, 2015/10/29 01:24:53]

Are BLOB_NOT_FOUND and SHARED_MEMORY_MAP_FAILED the only ones that should be
expected to happen in a release build? If so, simply don't write code to handle
the other conditions besides DCHECK'ing deeper down the stack where the error is
apparent. If the condition could be expliotable, use CHECK instead of DCHECK.

[dmurph, 2015/10/29 20:42:27]

Done.

+    case ResponsesStatus::BLOB_NOT_FOUND:
+      DVLOG(1) << "Unable to find blob " << uuid << ".";
+      return;

[michaeln, 2015/10/29 01:24:53]

should this be a break so we send the BlobStorageMsg_CancelBuildingBlob msg in
response?

[dmurph, 2015/10/29 20:42:27]

Done now.

+    case ResponsesStatus::INVALID_ITEM_INDEX:
+      DVLOG(1) << "Index out of bounds for blob item in blob " << uuid << ".";
+      break;
+    case ResponsesStatus::INVALID_DATA_RANGE:
+      DVLOG(1) << "Data range out of bounds for blob item in blob " << uuid
+               << ".";
+      break;
+    case ResponsesStatus::INVALID_ITEM:
+      DVLOG(1) << "Requesting data from non-memory item in blob " << uuid
+               << ".";
+      break;
+    case ResponsesStatus::INVALID_HANDLE_INDEX:
+      DVLOG(1) << "Invalid handle index for transferring in blob " << uuid
+               << ".";
+      break;
+    case ResponsesStatus::SHARED_MEMORY_MAP_FAILED:
+      // This would happen if the renderer process doesn't have enough memory
+      // to map the shared memory, which is possible if we don't have much
+      // memory. If this scenario happens often, we could delay the response
+      // until we have enough memory.  For now we just fail.
+      DVLOG(1) << "Unable to map shared memory to send blob " << uuid << ".";
+      break;
+    case ResponsesStatus::SUCCESS:
+      success = true;
+      break;
+  }
+
+  if (success) {
+    // TODO(dmurph): Uncomment when IPC messages are added.
+    // sender->Send(new BlobStorageMsg_MemoryItemResponse(uuid, responses));
+  } else {
+    DVLOG(1) << "Canceling blob transfer for blob " << uuid;
+    CancelBlobTransfer(uuid, IPCBlobCreationCancelCode::UNKNOWN, sender);
+  }
+}
+
+void BlobTransportController::OnCancel(
+    const std::string& uuid,
+    storage::IPCBlobCreationCancelCode code) {
+  DVLOG(1) << "Received blob cancel for blob " << uuid << " with reason:";
+  switch (code) {
+    case IPCBlobCreationCancelCode::UNKNOWN:
+      DVLOG(1) << "Unknown.";
+      break;
+    case IPCBlobCreationCancelCode::OUT_OF_MEMORY:
+      DVLOG(1) << "Out of Memory.";
+      break;
+    case IPCBlobCreationCancelCode::FILE_WRITE_FAILED:
+      DVLOG(1) << "File Write Failed (Invalid cancel reason!).";
+      break;
+  }
+  ReleaseBlobConsolidation(uuid);
+}
+
+void BlobTransportController::OnDone(const std::string& uuid) {
+  ReleaseBlobConsolidation(uuid);
+}
+
+void BlobTransportController::Clear() {
+  STLDeleteValues(&blob_storage_);
+  blob_storage_.clear();
+}
+
+BlobTransportController::BlobTransportController() {}
+
+void BlobTransportController::CancelBlobTransfer(
+    const std::string& uuid,
+    storage::IPCBlobCreationCancelCode code,
+    IPC::Sender* sender) {
+  // TODO(dmurph): Uncomment when IPC messages are added.
+  // sender->Send(new BlobStorageMsg_CancelBuildingBlob(uuid, code));
+  ReleaseBlobConsolidation(uuid);
+}
+
+bool BlobTransportController::HoldBlobConsolidation(
+    const std::string& uuid,
+    scoped_ptr<BlobConsolidation> consolidation) {
+  if (blob_storage_.find(uuid) != blob_storage_.end()) {

[michaeln, 2015/10/23 23:52:12]

nit: there's code here that doesn't need to be. since all of the tests are
friends of this class, can't they just as easily stuff values directly into the
map?

  controller.blob_storage_[uuid] = p;

[dmurph, 2015/10/28 01:00:51]

Done.

+    return false;
+  }
+  blob_storage_.insert(std::make_pair(uuid, consolidation.release()));
+  return true;
+}
+
+void BlobTransportController::GetDescriptions(
+    const std::string& uuid,
+    size_t max_data_population,
+    std::vector<storage::DataElement>* out) {
+  DCHECK(out->empty());
+  BlobConsolidation* consolidation = GetConsolidation(uuid);
+  if (!consolidation) {
+    return;
+  }

[kinuko, 2015/10/26 06:44:22]

nit: should this be just DCHECK(consolidation) ?

[dmurph, 2015/10/28 01:00:51]

Done.

+  const auto& consolidated_items = consolidation->consolidated_items();
+
+  size_t current_memory_population = 0;
+  size_t current_item = 0;
+  out->reserve(consolidated_items.size());
+  for (const ConsolidatedItem& item : consolidated_items) {
+    out->push_back(DataElement());
+    auto& element = out->back();
+
+    switch (item.type) {
+      case DataElement::TYPE_BYTES: {
+        size_t bytes_length = static_cast<size_t>(item.length);
+        if (current_memory_population + bytes_length <= max_data_population) {
+          element.SetToAllocatedBytes(bytes_length);
+          consolidation->ReadMemory(current_item, 0, bytes_length,
+                                    element.mutable_bytes());
+          current_memory_population += bytes_length;
+        } else {
+          element.SetToBytesDescription(bytes_length);
+        }
+        break;
+      }
+      case DataElement::TYPE_FILE: {
+        element.SetToFilePathRange(
+            item.path, item.offset, item.length,
+            base::Time::FromDoubleT(item.expected_modification_time));
+        break;
+      }
+      case DataElement::TYPE_BLOB: {
+        element.SetToBlobRange(item.blob_uuid, item.offset, item.length);
+        break;
+      }
+      case DataElement::TYPE_FILE_FILESYSTEM: {
+        element.SetToFileSystemUrlRange(
+            item.filesystem_url, item.offset, item.length,
+            base::Time::FromDoubleT(item.expected_modification_time));
+        break;
+      }
+      case DataElement::TYPE_DISK_CACHE_ENTRY:
+      case DataElement::TYPE_BYTES_DESCRIPTION:
+      case DataElement::TYPE_UNKNOWN:
+        NOTREACHED();
+    }
+    ++current_item;
+  }
+}
+
+BlobTransportController::ResponsesStatus BlobTransportController::GetResponses(
+    const std::string& uuid,
+    const std::vector<BlobItemBytesRequest>& requests,
+    std::vector<SharedMemoryHandle>* memory_handles,
+    const std::vector<IPC::PlatformFileForTransit>& file_handles,
+    std::vector<BlobItemBytesResponse>* out) {
+  DCHECK(out->empty());
+  BlobConsolidation* consolidation = GetConsolidation(uuid);
+  if (!consolidation)
+    return ResponsesStatus::BLOB_NOT_FOUND;
+  const auto& consolidated_items = consolidation->consolidated_items();
+
+  base::ScopedPtrHashMap<size_t, scoped_ptr<SharedMemory>> opened_memory;

[kinuko, 2015/10/26 06:44:22]

Sorry, I'm probably missing something obvious... so we create SharedMemory from
the given handle, memcpy the data into it, and hold it as ScopedPtrHashMap. Then
is it ok to just release all these at the end of this method?

[dmurph, 2015/10/28 01:00:51]

So we need to handle the case where multiple items are getting saved to the same
shared memory piece. That's why we have the index.  And yes, releasing the
SharedMemory object at the end of the method is what we want, as we no longer
need access to the memory in the renderer.

[kinuko (google), 2015/10/28 03:53:36]

Ah I see thanks, that makes it clearer why we have this ScopedPtrHashMap. Could
we have a brief comment about that (to save poor readers like me)?

[dmurph, 2015/10/29 20:42:27]

Done.  I changed it into a scoped vector as that's way more efficient.

+  for (const BlobItemBytesRequest& request : requests) {
+    if (request.renderer_item_index >= consolidated_items.size())
+      return ResponsesStatus::INVALID_ITEM_INDEX;
+
+    const ConsolidatedItem& item =
+        consolidated_items[request.renderer_item_index];
+    if (request.renderer_item_offset + request.size > item.length)
+      return ResponsesStatus::INVALID_DATA_RANGE;
+    if (item.type != DataElement::TYPE_BYTES)
+      return ResponsesStatus::INVALID_ITEM;
+
+    out->push_back(BlobItemBytesResponse(request.request_number));
+    switch (request.transport_strategy) {
+      case IPCBlobItemRequestStrategy::IPC: {
+        BlobItemBytesResponse& response = out->back();
+        ReadStatus status = consolidation->ReadMemory(
+            request.renderer_item_index, request.renderer_item_offset,
+            request.size, response.allocate_mutable_data(request.size));
+        CHECK(status == ReadStatus::OK)
+            << "Error reading from consolidated blob: "
+            << static_cast<int>(status);

[kinuko (google), 2015/10/28 03:53:36]

Does this need to be CHECK?

[dmurph, 2015/10/29 20:42:27]

no, changed.

+        break;
+      }
+      case IPCBlobItemRequestStrategy::SHARED_MEMORY: {
+        if (request.handle_index >= memory_handles->size()) {
+          DCHECK_LT(request.handle_index, memory_handles->size());

[kinuko, 2015/10/26 06:44:22]

Is this handling DCHECK failure pattern?

[dmurph, 2015/10/28 01:00:51]

Question:

How do I test for a failure case, but have a DCHECK there? How does this sound:
This method will return the error enums, and then we CHECK in the calling
method.

[kinuko (google), 2015/10/28 03:53:36]

Could this error happen without any coding error? If not let's just use DCHECK,
or CHECK if it has security implication. If the error could happen and we want
to handle the error case return an error code. If this shouldn't happen but we
still want to see if we have any crashes in release builds we could temporarily
use CHECK, but would be nice to have a comment about it too... is our common
guide, which probably you already know.

Actually this method has a lot of CHECK and DCHECKs and still returning some
error code that is handled by LOG(FATAL), which also terminates process. It'd be
nice if these are a bit more consistent.

[michaeln, 2015/10/29 01:24:53]

When it comes to CHECKs and DCHECKs and LOG'ing, our style guide generally has
us tread as lightly as possible in release builds to keep the size of the binary
down. Keeping extraneous strings, conditions, and branches out of an individual
class doesn't add up to much, but doing so across the large number of classes in
the project definitely adds up.

Here's a suggestion, whenever you see console output in a release build,
consider whether your mom would find that output useful :)

Am I right in thinking that many of the error returns in this function could
only be hit if the browser process sent us a very bad message or if the renderer
is in a seriously wrong state? For those kinds of possibilities, where we never
expect to see it happen, DCHECK() is a good choice.

The early returns on 238 and 240, is it true that browser should never ask to
read data from the wrong kind of item or a nonsense offset/range within the
right kind of item? DCHECK. Same question for the return here on line 257?

[dmurph, 2015/10/29 20:42:27]

Done. I cleaned this up a lot and only have a couple error cases now.

+          return ResponsesStatus::INVALID_HANDLE_INDEX;
+        }
+        SharedMemory* memory = nullptr;
+        auto memory_it = opened_memory.find(request.handle_index);
+        if (memory_it == opened_memory.end()) {
+          SharedMemoryHandle& handle = (*memory_handles)[request.handle_index];
+          DCHECK(SharedMemory::IsHandleValid(handle));
+          scoped_ptr<SharedMemory> shared_memory(
+              new SharedMemory(handle, false));
+          if (!shared_memory->Map(request.size))
+            return ResponsesStatus::SHARED_MEMORY_MAP_FAILED;
+          memory = shared_memory.get();
+          opened_memory.add(request.handle_index, shared_memory.Pass());
+        } else {
+          memory = memory_it->second;
+        }
+        CHECK(memory->memory()) << "Couldn't map memory for blob transfer.";
+        ReadStatus status = consolidation->ReadMemory(
+            request.renderer_item_index, request.renderer_item_offset,
+            request.size,
+            static_cast<char*>(memory->memory()) + request.handle_offset);
+        CHECK(status == ReadStatus::OK)
+            << "Error reading from consolidated blob: "
+            << static_cast<int>(status);
+        break;
+      }
+      case IPCBlobItemRequestStrategy::FILE:
+        NOTREACHED() << "TODO(dmurph): Not implemented.";
+        break;
+      case IPCBlobItemRequestStrategy::UNKNOWN:
+        NOTREACHED();
+        break;
+    }
+  }
+  return ResponsesStatus::SUCCESS;
+}
+
+void BlobTransportController::ReleaseBlobConsolidation(
+    const std::string& uuid) {
+  auto iter = blob_storage_.find(uuid);
+  if (iter != blob_storage_.end()) {
+    BlobConsolidation* consolidation = iter->second;
+    delete consolidation;
+    blob_storage_.erase(iter);
+  }
+}
+
+BlobConsolidation* BlobTransportController::GetConsolidation(
+    const std::string& uuid) {
+  auto iter = blob_storage_.find(uuid);
+  if (iter == blob_storage_.end())
+    return nullptr;
+  return iter->second;
+}
+
+}  // namespace content