Use shared memory for moving data over BlobChannel Mojo interface.

blimp/engine/renderer/blob_channel_sender_proxy.cc

 // Copyright 2016 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 "blimp/engine/renderer/blob_channel_sender_proxy.h"
 
+#include "blimp/common/blob_cache/id_util.h"
+#include "content/public/common/service_registry.h"
 #include "content/public/renderer/render_thread.h"
 #include "services/shell/public/cpp/interface_provider.h"
 
 namespace blimp {
 namespace engine {
 namespace {
 
 mojom::BlobChannelPtr GetConnectedBlobChannel() {
   mojom::BlobChannelPtr blob_channel_ptr;
   content::RenderThread::Get()->GetRemoteInterfaces()->GetInterface(
       &blob_channel_ptr);
   CHECK(blob_channel_ptr) << "Could not connect to BlobChannel Mojo interface.";
   return blob_channel_ptr;
 }
 
+// Manages the creation and lifetime of Mojo shared memory buffers for blobs.
+// Cleans up the shared memory state when deleted.
+// The SharedMemoryBlob must not be deleted until the data consumer has
+// acknowledged that it is finished using the buffer.

[Wez, 2016/07/01 01:06:56]

Why not? Isn't it the case that the buffer will persist until the last process
with a handle or mapping to it has stopped referencing it?

[Kevin M, 2016/07/18 16:35:16]

Yes. I should've removed this comment; I updated the code to reflect those
semantics but the comments were untouched.

+class SharedMemoryBlob {
+ public:
+  explicit SharedMemoryBlob(BlobDataPtr data);
+  ~SharedMemoryBlob();
+
+  mojo::ScopedSharedBufferHandle GetRemoteHandle();

[Wez, 2016/07/01 01:06:57]

nit: Add comments to explain why we e.g. have a local_handle_ member, but a
GetRemoteHandle() getter.  Should the latter be CreateRemoteHandle(), for
example?

[Kevin M, 2016/07/18 16:35:16]

Create is better, done.

+
+ private:
+  mojo::ScopedSharedBufferHandle local_handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedMemoryBlob);
+};
+
+SharedMemoryBlob::SharedMemoryBlob(BlobDataPtr data) {
+  DCHECK_LT(data->data.size(), kMaxBlobSizeBytes);

[Wez, 2016/07/01 01:06:57]

nit: We tend to try to keep these (expectation, actual) ordered, if possible.

[Kevin M, 2016/07/18 16:35:16]

Done.

+
+  local_handle_ = mojo::SharedBufferHandle::Create(data->data.size());
+  DCHECK(local_handle_.is_valid()) << "Mojo error when creating shared buffer.";

[Wez, 2016/07/01 01:06:56]

nit: Here and below, does the message text help? Isn't it implicit from the
DCHECK failure?

[Kevin M, 2016/07/18 16:35:16]

Done.

+
+  auto mapped = local_handle_->Map(data->data.size());
+  DCHECK(mapped) << "Mojo error when memory mapping shared buffer.";
+  memcpy(mapped.get(), data->data.data(), data->data.size());
+}
+
+SharedMemoryBlob::~SharedMemoryBlob() {}
+
+mojo::ScopedSharedBufferHandle SharedMemoryBlob::GetRemoteHandle() {
+  auto remote_handle =

[Wez, 2016/07/01 01:06:57]

As noted elsewhere, it's best not to use auto unless the type is obvious from
the context (e.g. when iterating over a vector or something) - in this case I
need to look up Clone() to see what the type is.

[Kevin M, 2016/07/18 16:35:16]

Done.

+      local_handle_->Clone(mojo::SharedBufferHandle::AccessMode::READ_ONLY);
+  CHECK(remote_handle.is_valid())
+      << "Mojo error when creating read-only buffer handle.";
+  return remote_handle;
+}
+
 }  // namespace
 
 BlobChannelSenderProxy::BlobChannelSenderProxy()
-    : blob_channel_(GetConnectedBlobChannel()) {}
+    : BlobChannelSenderProxy(GetConnectedBlobChannel()) {}
 
 BlobChannelSenderProxy::~BlobChannelSenderProxy() {}
 
+BlobChannelSenderProxy::BlobChannelSenderProxy(
+    mojom::BlobChannelPtr blob_channel)
+    : blob_channel_(std::move(blob_channel)) {}
+
+// static
+std::unique_ptr<BlobChannelSenderProxy> BlobChannelSenderProxy::CreateForTest(
+    mojom::BlobChannelPtr blob_channel) {
+  return base::WrapUnique(new BlobChannelSenderProxy(std::move(blob_channel)));
+}
+
 bool BlobChannelSenderProxy::IsInEngineCache(const std::string& id) const {
   return replication_state_.find(id) != replication_state_.end();
 }
 
 bool BlobChannelSenderProxy::IsInClientCache(const std::string& id) const {
-  return replication_state_.find(id)->second;
+  auto found = replication_state_.find(id);
+  return found != replication_state_.end() && found->second;
 }
 
 void BlobChannelSenderProxy::PutBlob(const BlobId& id, BlobDataPtr data) {
   DCHECK(!IsInEngineCache(id));
 
+  size_t size = data->data.size();
+  if (size == 0) {

[Wez, 2016/07/01 01:06:57]

nit: It would be more readable to if(data->data.empty() here, IMO.

[Kevin M, 2016/07/18 16:35:16]

Made this a CHECK and used empty()

+    DLOG(FATAL) << "Zero length blob sent: " << BlobIdToString(id);
+    return;
+  }
+
   replication_state_[id] = false;
-  blob_channel_->PutBlob(id, data->data);
+  std::unique_ptr<SharedMemoryBlob> shared_mem_blob(
+      new SharedMemoryBlob(std::move(data)));
+  blob_channel_->PutBlob(id, shared_mem_blob->GetRemoteHandle(), size);
 }
 
 void BlobChannelSenderProxy::DeliverBlob(const std::string& id) {
-  DCHECK(!IsInClientCache(id));
+  if (!IsInEngineCache(id)) {
+    DLOG(FATAL) << "Attempted to deliver an invalid blob: "
+                << BlobIdToString(id);

[Wez, 2016/07/01 01:06:57]

Does this indicate a renderer misbehaviour?

[Kevin M, 2016/07/18 16:35:16]

Yes, though since it's probably a deterministic control flow check, I'll make it
a DCHECK.

+    return;
+  }
+  if (IsInClientCache(id)) {
+    DVLOG(1) << "Blob is already in the remote cache:" << BlobIdToString(id);

[Wez, 2016/07/01 01:06:57]

Similarly, why would we be asked to re-send?

[dcheng, 2016/07/01 02:04:28]

I think these should probably be DCHECKs as well, since we changed PutBlob()
back to use a DCHECK (within a same process, we should be able to assume that
our callers follow the various preconditions expected in the code)

[Kevin M, 2016/07/18 16:35:16]

Done.

[Kevin M, 2016/07/18 16:35:16]

Done.

+    return;
+  }
 
   // We assume that the client will have the blob if we push it.
   // TODO(kmarshall): Revisit this assumption when asynchronous blob transport
   // is supported.
   replication_state_[id] = true;
 
   blob_channel_->DeliverBlob(id);
 }
 
 }  // namespace engine
 }  // namespace blimp