Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2033013003:
Use shared memory for moving data over BlobChannel Mojo interface. (Closed)
|
Created:
4 years, 6 months ago by Kevin M Modified:
4 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, anandc+watch-blimp_chromium.org, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), dtrainor+watch-blimp_chromium.org, jam, jessicag+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, qsr+mojo_chromium.org, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@blobchannel-master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse shared memory for moving data over BlobChannel Mojo interface.
* Change BlobChannel PutBlob() Mojo IDL to use shared memory handles
instead of a byte array.
* Create "SharedMemoryBlob" class for encapsulating Mojo shared
memory initialization and lifetime management semantics.
* Add BlobChannelSenderProxy class, which manages interactions with the renderer side of the BlobChannel IPC interface.
* Add Blimp-specific run_all_unittests.cc, which bootstraps the Mojo environment before executing unittests.
R=wez@chromium.org
CC=maniscalco@chromium.org
BUG=600719, 614564
Committed: https://crrev.com/ca6057dcb4c724085356bda1706b48cf54e08fba
Cr-Commit-Position: refs/heads/master@{#406045}
Patch Set 1 #
Total comments: 18
Patch Set 2 : maniscalco feedback #Patch Set 3 : . #
Total comments: 2
Patch Set 4 : added tests #
Total comments: 2
Patch Set 5 : jam feedback #
Total comments: 2
Patch Set 6 : Added new test runner with Mojo initialization #
Total comments: 46
Patch Set 7 : . #Patch Set 8 : Use Mojo's unmapping capabilities #Patch Set 9 : make da comment more better #Patch Set 10 : resolve indirect GN dependencies #Patch Set 11 : Switch to new Mojo shared buffer API #
Total comments: 14
Patch Set 12 : dcheng feedback #
Total comments: 4
Patch Set 13 : dcheng feedback #
Total comments: 24
Patch Set 14 : IPC owners file added. #
Messages
Total messages: 54 (17 generated)
Will add unit tests for the proxy in a followup patch.
maniscalco@chromium.org changed reviewers: + maniscalco@chromium.org
Neat, that's pretty simple change. I've got some drive-by comments/questions. Also, in the commit description I see two bugs referenced, but one doesn't appear to be related (614654). https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel.mojom:10: PutBlob(string id, handle<shared_buffer> data, uint32 size) => (); I want to understand the motivation for using shared memory. You mentioned it's more efficient. Is the idea that we'll use less CPU when transferring blobs? Do you have a feel for where the break even point is? In other words, I'm assuming if we've got a 100MB blob, using shared memory will be more efficient, but if we have a 10B blob, it'll be less efficient (e.g. overhead of open/mmap/close, etc.). https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel_service.cc:28: mojo::MapBuffer(data.get(), 0, size, reinterpret_cast<void**>(&blob_buf), #include "mojo/public/cpp/system/buffer.h" to get mojo::MapBuffer? https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel_service.cc:36: mojo::UnmapBuffer(blob_buf); Check the result of UnmapBuffer? https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:26: class SharedMemoryBlob { I saw your note about adding a test for the proxy in another CL. How about also adding a simple unit test for this class that constructs/destroys an instance. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:52: ~SharedMemoryBlob() { mojo::UnmapBuffer(mapped_data_); } Check result of UnmapBuffer? https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:90: size_t size = data->data.size(); I'm guessing it doesn't really make sense to construct a ShareMemoryBlob of size 0, right? Should we check data->data.size() or do we rely on mojo to do that? https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:96: replication_state_[id] = false; Question about ordering. I noticed that the order of setting replication_state_ and PutBlob has changed. Does that matter? I ask because if in the future PutComplete (or something it eventually calls) cares about replication_state_, it could see the value true if PutBlob invokes its callback synchronously (I'm not sure if it does or not).
Description was changed from ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. R=wez@chromium.org CC=maniscalco@chromium.org BUG=614654,600719 ========== to ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719 ==========
https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel.mojom:10: PutBlob(string id, handle<shared_buffer> data, uint32 size) => (); On 2016/06/03 16:43:53, maniscalco wrote: > I want to understand the motivation for using shared memory. You mentioned it's > more efficient. Is the idea that we'll use less CPU when transferring blobs? > > Do you have a feel for where the break even point is? In other words, I'm > assuming if we've got a 100MB blob, using shared memory will be more efficient, > but if we have a 10B blob, it'll be less efficient (e.g. overhead of > open/mmap/close, etc.). The primary reason is that Mojo shares the same IPC pipe with all other features that need to communicate across the browser/renderer boundary. Pushing large amounts of data creates contention which can degrade the performance of Chrome. You can see similar usage for other features which transfer media over Mojo, e.g. the video decryption or audio output. A secondary reason is that is that reading and writing 10kb-100kb over a socket will probably be more overhead than creating a shared buffer, in terms of time and system resources. This is based on my intuition, though; I haven't written any tests to prove this empirically. If the overhead of allocation proves to be too high, I can create a pool for reusable buffers. However, since the transfers are going to be bursty and will *not* be steady state, I thought that allocating and immediately freeing is appropriate, to reduce the space-time application footprint. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel_service.cc:28: mojo::MapBuffer(data.get(), 0, size, reinterpret_cast<void**>(&blob_buf), On 2016/06/03 16:43:53, maniscalco wrote: > #include "mojo/public/cpp/system/buffer.h" to get mojo::MapBuffer? Done. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel_service.cc:36: mojo::UnmapBuffer(blob_buf); On 2016/06/03 16:43:53, maniscalco wrote: > Check the result of UnmapBuffer? OK. I'm using DCHECK w/return so that we don't blow up the browser process if the bug is essentially recoverable. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:26: class SharedMemoryBlob { On 2016/06/03 16:43:53, maniscalco wrote: > I saw your note about adding a test for the proxy in another CL. How about also > adding a simple unit test for this class that constructs/destroys an instance. Good idea. I'll add it in this CL, I just wanted to get the impl out there and vetted before committing to writing tests. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:52: ~SharedMemoryBlob() { mojo::UnmapBuffer(mapped_data_); } On 2016/06/03 16:43:53, maniscalco wrote: > Check result of UnmapBuffer? Done. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:90: size_t size = data->data.size(); On 2016/06/03 16:43:53, maniscalco wrote: > I'm guessing it doesn't really make sense to construct a ShareMemoryBlob of size > 0, right? Should we check data->data.size() or do we rely on mojo to do that? Should we prevent that? IIRC zero-length arrays are valid, even if they are wasteful. I think that we might want to enforce that constraint on the data producing layer, since I would prefer to keep the layering somewhat orthogonal. In our case, should a failure occur at any point during WebP transcoding, blob creation will be aborted and this method will never be called. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:96: replication_state_[id] = false; On 2016/06/03 16:43:53, maniscalco wrote: > Question about ordering. I noticed that the order of setting replication_state_ > and PutBlob has changed. Does that matter? I ask because if in the future > PutComplete (or something it eventually calls) cares about replication_state_, > it could see the value true if PutBlob invokes its callback synchronously (I'm > not sure if it does or not). Done. I put it there to emphasize the postcondition, but I don't feel strongly about it. I don't think PutBlob() is capable of returning synchronously *unless* I declare the method as synchronous in the .mojom - that ability exists!
https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel.mojom:10: PutBlob(string id, handle<shared_buffer> data, uint32 size) => (); On 2016/06/03 17:14:57, Kevin M wrote: > On 2016/06/03 16:43:53, maniscalco wrote: > > I want to understand the motivation for using shared memory. You mentioned > it's > > more efficient. Is the idea that we'll use less CPU when transferring blobs? > > > > Do you have a feel for where the break even point is? In other words, I'm > > assuming if we've got a 100MB blob, using shared memory will be more > efficient, > > but if we have a 10B blob, it'll be less efficient (e.g. overhead of > > open/mmap/close, etc.). > > The primary reason is that Mojo shares the same IPC pipe with all other features > that need to communicate across the browser/renderer boundary. Pushing large > amounts of data creates contention which can degrade the performance of Chrome. > You can see similar usage for other features which transfer media over Mojo, > e.g. the video decryption or audio output. > > A secondary reason is that is that reading and writing 10kb-100kb over a socket > will probably be more overhead than creating a shared buffer, in terms of time > and system resources. This is based on my intuition, though; I haven't written > any tests to prove this empirically. > > If the overhead of allocation proves to be too high, I can create a pool for > reusable buffers. However, since the transfers are going to be bursty and will > *not* be steady state, I thought that allocating and immediately freeing is > appropriate, to reduce the space-time application footprint. Thanks for the details. I agree about not implementing a pool mechanism (increases memory use, requires tuning, increases complexity w/o clear benefit). Consider adding a comment in this .mojom as to why we're using shared memory. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:90: size_t size = data->data.size(); On 2016/06/03 17:14:57, Kevin M wrote: > On 2016/06/03 16:43:53, maniscalco wrote: > > I'm guessing it doesn't really make sense to construct a ShareMemoryBlob of > size > > 0, right? Should we check data->data.size() or do we rely on mojo to do that? > > Should we prevent that? IIRC zero-length arrays are valid, even if they are > wasteful. I think that we might want to enforce that constraint on the data > producing layer, since I would prefer to keep the layering somewhat orthogonal. > > In our case, should a failure occur at any point during WebP transcoding, blob > creation will be aborted and this method will never be called. I'm not real familiar with the applications that will use BlobChannel so I don't have a strong opinion on whether we should allow or prevent zero-length blobs, but I do think it's something that should be documented (probably where we define BlobData?) and verified with tests. Looking at the mojo implementation I got the impression that mojo doesn't allow for zero-length maps: https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_shared_buffer... (probably worth checking to make sure I didn't miss something when reading the code) https://codereview.chromium.org/2033013003/diff/40001/blimp/engine/mojo/blob_... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/40001/blimp/engine/mojo/blob_... blimp/engine/mojo/blob_channel_service.cc:43: LOG(ERROR) << "Couldn't unmap buffer for blob ID: " << id; Assuming mojo differentiates between different types of failure (e.g. permission denied, too many open files), consider logging the error result. That way, when we see map/unmap failures we can more easily figure out the cause.
kmarshall@chromium.org changed reviewers: + jam@chromium.org
+jam for new content/app/mojo (mojo_init.h) DEPS. Sorry for the delay in getting these tests out. Life is always more interesting when Mojo's a part of your testing. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/mojo/blob_chan... blimp/engine/mojo/blob_channel.mojom:10: PutBlob(string id, handle<shared_buffer> data, uint32 size) => (); On 2016/06/06 15:43:56, maniscalco wrote: > On 2016/06/03 17:14:57, Kevin M wrote: > > On 2016/06/03 16:43:53, maniscalco wrote: > > > I want to understand the motivation for using shared memory. You mentioned > > it's > > > more efficient. Is the idea that we'll use less CPU when transferring > blobs? > > > > > > Do you have a feel for where the break even point is? In other words, I'm > > > assuming if we've got a 100MB blob, using shared memory will be more > > efficient, > > > but if we have a 10B blob, it'll be less efficient (e.g. overhead of > > > open/mmap/close, etc.). > > > > The primary reason is that Mojo shares the same IPC pipe with all other > features > > that need to communicate across the browser/renderer boundary. Pushing large > > amounts of data creates contention which can degrade the performance of > Chrome. > > You can see similar usage for other features which transfer media over Mojo, > > e.g. the video decryption or audio output. > > > > A secondary reason is that is that reading and writing 10kb-100kb over a > socket > > will probably be more overhead than creating a shared buffer, in terms of time > > and system resources. This is based on my intuition, though; I haven't written > > any tests to prove this empirically. > > > > If the overhead of allocation proves to be too high, I can create a pool for > > reusable buffers. However, since the transfers are going to be bursty and will > > *not* be steady state, I thought that allocating and immediately freeing is > > appropriate, to reduce the space-time application footprint. > > Thanks for the details. > > I agree about not implementing a pool mechanism (increases memory use, requires > tuning, increases complexity w/o clear benefit). > > Consider adding a comment in this .mojom as to why we're using shared memory. Done. https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/1/blimp/engine/renderer/blob_... blimp/engine/renderer/blob_channel_sender_proxy.cc:90: size_t size = data->data.size(); On 2016/06/06 15:43:56, maniscalco wrote: > On 2016/06/03 17:14:57, Kevin M wrote: > > On 2016/06/03 16:43:53, maniscalco wrote: > > > I'm guessing it doesn't really make sense to construct a ShareMemoryBlob of > > size > > > 0, right? Should we check data->data.size() or do we rely on mojo to do > that? > > > > Should we prevent that? IIRC zero-length arrays are valid, even if they are > > wasteful. I think that we might want to enforce that constraint on the data > > producing layer, since I would prefer to keep the layering somewhat > orthogonal. > > > > In our case, should a failure occur at any point during WebP transcoding, blob > > creation will be aborted and this method will never be called. > > I'm not real familiar with the applications that will use BlobChannel so I don't > have a strong opinion on whether we should allow or prevent zero-length blobs, > but I do think it's something that should be documented (probably where we > define BlobData?) and verified with tests. > > Looking at the mojo implementation I got the impression that mojo doesn't allow > for zero-length maps: > > https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_shared_buffer... > > (probably worth checking to make sure I didn't miss something when reading the > code) You're right, thank you very much for looking into that and correcting my assumption. Done. https://codereview.chromium.org/2033013003/diff/40001/blimp/engine/mojo/blob_... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/40001/blimp/engine/mojo/blob_... blimp/engine/mojo/blob_channel_service.cc:43: LOG(ERROR) << "Couldn't unmap buffer for blob ID: " << id; On 2016/06/06 15:43:56, maniscalco wrote: > Assuming mojo differentiates between different types of failure (e.g. permission > denied, too many open files), consider logging the error result. That way, when > we see map/unmap failures we can more easily figure out the cause. Done.
https://codereview.chromium.org/2033013003/diff/60001/blimp/engine/renderer/b... File blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc (right): https://codereview.chromium.org/2033013003/diff/60001/blimp/engine/renderer/b... blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc:15: #include "content/app/mojo/mojo_init.h" code outside of content/ can only include content/public just call mojo::edk::Init() in your test suite harness like a lot of our test suites already do: https://cs.chromium.org/search/?q=%22mojo::edk::Init()%22&sq=package:chromium...
kmarshall@chromium.org changed reviewers: + rockot@chromium.org - jam@chromium.org
Thanks sky@! No need for content/ DEPS approval anymore. (hits eject button) rockot@, can you look at this for mojo/edk DEPS? https://codereview.chromium.org/2033013003/diff/60001/blimp/engine/renderer/b... File blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc (right): https://codereview.chromium.org/2033013003/diff/60001/blimp/engine/renderer/b... blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc:15: #include "content/app/mojo/mojo_init.h" On 2016/06/09 15:55:04, jam wrote: > code outside of content/ can only include content/public > > just call mojo::edk::Init() in your test suite harness like a lot of our test > suites already do: > https://cs.chromium.org/search/?q=%22mojo::edk::Init()%22&sq=package:chromium... Done.
DEPS is fine but I'm pretty sure this is broken (see inline comment) https://codereview.chromium.org/2033013003/diff/80001/blimp/engine/renderer/b... File blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc (right): https://codereview.chromium.org/2033013003/diff/80001/blimp/engine/renderer/b... blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc:35: mojo::edk::Init(); This should almost definitely hit a DCHECK - you can only call it once per process. Generally we just update the test runner to call it in main(), but if you really want to initialize mojo only in this test suite, you'll want to use a singleton (LazyInstance or something) to do it for you.
https://codereview.chromium.org/2033013003/diff/80001/blimp/engine/renderer/b... File blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc (right): https://codereview.chromium.org/2033013003/diff/80001/blimp/engine/renderer/b... blimp/engine/renderer/blob_channel_sender_proxy_unittest.cc:35: mojo::edk::Init(); On 2016/06/09 17:55:40, Ken Rockot wrote: > This should almost definitely hit a DCHECK - you can only call it once per > process. Generally we just update the test runner to call it in main(), but if > you really want to initialize mojo only in this test suite, you'll want to use a > singleton (LazyInstance or something) to do it for you. Thanks for that pointer. I added a new run_all_unittests.cc file w/Mojo initialization under blimp/test, PTAL.
Haven't reviewed the unit-tests yet; will follow-up w/ that. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:9: // large data payloads out of the Mojo IPC channel. nit: Isn't the use of shared-memory obvious from the function signature? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:11: PutBlob(string id, handle<shared_buffer> data, uint32 size) => (); nit: Do we need |size| to be explicit? Does Mojo not convey the size of the shared-memory area as part of |data|? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:27: char* blob_buf = nullptr; nit: Should this be const char*? We don't really want the receiver modifying the Blob content, right? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:32: LOG(ERROR) << "Couldn't map buffer for blob ID: " << id; It's best not to log unbounded ERRORs - if we fail to map a blob then we'll continue on, rather than exiting, and we're likely to fail to map the next one, so we'll get cascades of errors. One way to provide the signal without such detailed logging would be to add UMA stats in this code, for example. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:36: DCHECK(blob_buf); nit: You're about to copy data from |blob_buf|, so you'll get a nice clean crash stack anyway if it's null here. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:40: blob_channel_sender_->PutBlob(id, std::move(new_blob)); nit: Use a blank line here to separate the put-blob block from the unmap-memory block? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:46: return; This feels like a use-case for CHECK_EQ() - we really shouldn't fail to un-map memory, surely? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:25: // Caller is responsible for ensuring that |this| is not deleted until the From the caller's PoV, |this| is itself - I think you are saying don't delete the SharedMemoryBlob until the Mojo service has ACK'd it..? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:29: explicit SharedMemoryBlob(BlobDataPtr data) { As per style-guide, don't inline methods unless they're trivial setter/getters, plz! https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:32: mojo::CreateSharedBuffer(NULL, data->data.size(), &local_handle); nit: nullptr https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:34: << "Mojo error when creating shared buffer: " << result; CHECK_EQ will already log |result| (and the expected value) in Debug builds, so do you really need the additional log text? If you do then could it simply be: CHECK_EQ(...) << "mojo::CreateSharedBuffer failed: " << result; and similarly, below? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:42: // Create read-only handle for browser-side consumption. Why not do this on-the-fly in take_remote_handle()? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:55: CHECK_EQ(MOJO_RESULT_OK, result); Looks like you can move the un-mapping into the ctor, immediately after the memcpy()? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:64: void* mapped_data_; Why do we store this? It doesn't look like anything can/does access it? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:98: DLOG(ERROR) << "Redundant blob put attempted: " << BlobIdToString(id); nit: If this is a coding error, consider DLOG(FATAL), otherwise make it a DVLOG(1)? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:104: DLOG(FATAL) << "Zero length blob requested: " << BlobIdToString(id); nit: This is PutBlob - surely the zero-length blob was being sent, not requested? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:119: << BlobIdToString(id); This seems like a serious coding error - caller has violated the PutBlob->DeliverBlob contract? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:123: DLOG(ERROR) << "Blob is already in the remote cache:" << BlobIdToString(id); nit: As noted above, is it worth having this be an error, or can it be DVLOG(1)? https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.h (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.h:26: explicit BlobChannelSenderProxy(mojom::BlobChannelPtr blob_channel); I'd recommend having: static scoped_ptr<BlobChannelSender> CreateForTest(...) instead - that way you can get PRESUBMIT checks to fire if anyone tries to use it from production code.
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:32: LOG(ERROR) << "Couldn't map buffer for blob ID: " << id; On 2016/06/11 00:16:37, Wez wrote: > It's best not to log unbounded ERRORs - if we fail to map a blob then we'll > continue on, rather than exiting, and we're likely to fail to map the next one, > so we'll get cascades of errors. > > One way to provide the signal without such detailed logging would be to add UMA > stats in this code, for example. I asked for logging here because I'm operating under the assumption failure to map/unmap a blob is a serious issue we cannot recover from and that logging the result (we should log the result here like we do below) would help us find out what happened. The DLOG(FATAL) on the next line will terminate the program if we're built with debug checks on, right?
lgtm https://codereview.chromium.org/2033013003/diff/100001/blimp/test/run_all_uni... File blimp/test/run_all_unittests.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/test/run_all_uni... blimp/test/run_all_unittests.cc:16: nitty nit: redundant vspace
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:32: LOG(ERROR) << "Couldn't map buffer for blob ID: " << id; On 2016/06/13 15:23:08, maniscalco wrote: > On 2016/06/11 00:16:37, Wez wrote: > > It's best not to log unbounded ERRORs - if we fail to map a blob then we'll > > continue on, rather than exiting, and we're likely to fail to map the next > one, > > so we'll get cascades of errors. > > > > One way to provide the signal without such detailed logging would be to add > UMA > > stats in this code, for example. > > I asked for logging here because I'm operating under the assumption failure to > map/unmap a blob is a serious issue we cannot recover from and that logging the > result (we should log the result here like we do below) would help us find out > what happened. TBH I'd expect that both map & unmap should only fail if we pass them invalid args (e.g invalid memory handle, or already-unmapped address), so crashing is probably the cleanest thing to do. I think map may also fail if the address-space is too fragmented but that seems incredibly unlikely (and would probably indicate some other coding error anyway...) > > The DLOG(FATAL) on the next line will terminate the program if we're built with > debug checks on, right? Correct. I'd prefer that if we want to log something here then we just use LOG(FATAL), so that we'll log _and_ crash in all builds.
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:32: LOG(ERROR) << "Couldn't map buffer for blob ID: " << id; On 2016/06/13 23:37:50, Wez wrote: > On 2016/06/13 15:23:08, maniscalco wrote: > > On 2016/06/11 00:16:37, Wez wrote: > > > It's best not to log unbounded ERRORs - if we fail to map a blob then we'll > > > continue on, rather than exiting, and we're likely to fail to map the next > > one, > > > so we'll get cascades of errors. > > > > > > One way to provide the signal without such detailed logging would be to add > > UMA > > > stats in this code, for example. > > > > I asked for logging here because I'm operating under the assumption failure to > > map/unmap a blob is a serious issue we cannot recover from and that logging > the > > result (we should log the result here like we do below) would help us find out > > what happened. > > TBH I'd expect that both map & unmap should only fail if we pass them invalid > args (e.g invalid memory handle, or already-unmapped address), so crashing is > probably the cleanest thing to do. I think map may also fail if the > address-space is too fragmented but that seems incredibly unlikely (and would > probably indicate some other coding error anyway...) > > > > > The DLOG(FATAL) on the next line will terminate the program if we're built > with > > debug checks on, right? > > Correct. I'd prefer that if we want to log something here then we just use > LOG(FATAL), so that we'll log _and_ crash in all builds. LOG(FATAL) SGTM
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:9: // large data payloads out of the Mojo IPC channel. On 2016/06/11 00:16:37, Wez wrote: > nit: Isn't the use of shared-memory obvious from the function signature? It wasn't to maniscalco@, who requested that I add this comment. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:11: PutBlob(string id, handle<shared_buffer> data, uint32 size) => (); On 2016/06/11 00:16:37, Wez wrote: > nit: Do we need |size| to be explicit? Does Mojo not convey the size of the > shared-memory area as part of |data|? It doesn't. Handles are just opaque ints, and the memory-mapping functions that take a handle as a parameter require the size to be provided explicitly. :( https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:27: char* blob_buf = nullptr; On 2016/06/11 00:16:37, Wez wrote: > nit: Should this be const char*? We don't really want the receiver modifying the > Blob content, right? MapBuffer() takes a non-const void*. I'd have to do some nasty const_cast'ing to get this to work. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:32: LOG(ERROR) << "Couldn't map buffer for blob ID: " << id; On 2016/06/16 15:31:34, maniscalco wrote: > On 2016/06/13 23:37:50, Wez wrote: > > On 2016/06/13 15:23:08, maniscalco wrote: > > > On 2016/06/11 00:16:37, Wez wrote: > > > > It's best not to log unbounded ERRORs - if we fail to map a blob then > we'll > > > > continue on, rather than exiting, and we're likely to fail to map the next > > > one, > > > > so we'll get cascades of errors. > > > > > > > > One way to provide the signal without such detailed logging would be to > add > > > UMA > > > > stats in this code, for example. > > > > > > I asked for logging here because I'm operating under the assumption failure > to > > > map/unmap a blob is a serious issue we cannot recover from and that logging > > the > > > result (we should log the result here like we do below) would help us find > out > > > what happened. > > > > TBH I'd expect that both map & unmap should only fail if we pass them invalid > > args (e.g invalid memory handle, or already-unmapped address), so crashing is > > probably the cleanest thing to do. I think map may also fail if the > > address-space is too fragmented but that seems incredibly unlikely (and would > > probably indicate some other coding error anyway...) > > > > > > > > The DLOG(FATAL) on the next line will terminate the program if we're built > > with > > > debug checks on, right? > > > > Correct. I'd prefer that if we want to log something here then we just use > > LOG(FATAL), so that we'll log _and_ crash in all builds. > > LOG(FATAL) SGTM Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:36: DCHECK(blob_buf); On 2016/06/11 00:16:37, Wez wrote: > nit: You're about to copy data from |blob_buf|, so you'll get a nice clean crash > stack anyway if it's null here. Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:40: blob_channel_sender_->PutBlob(id, std::move(new_blob)); On 2016/06/11 00:16:37, Wez wrote: > nit: Use a blank line here to separate the put-blob block from the unmap-memory > block? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:46: return; On 2016/06/11 00:16:37, Wez wrote: > This feels like a use-case for CHECK_EQ() - we really shouldn't fail to un-map > memory, surely? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:25: // Caller is responsible for ensuring that |this| is not deleted until the On 2016/06/11 00:16:38, Wez wrote: > From the caller's PoV, |this| is itself - I think you are saying don't delete > the SharedMemoryBlob until the Mojo service has ACK'd it..? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:29: explicit SharedMemoryBlob(BlobDataPtr data) { On 2016/06/11 00:16:38, Wez wrote: > As per style-guide, don't inline methods unless they're trivial setter/getters, > plz! Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:32: mojo::CreateSharedBuffer(NULL, data->data.size(), &local_handle); On 2016/06/11 00:16:38, Wez wrote: > nit: nullptr Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:34: << "Mojo error when creating shared buffer: " << result; On 2016/06/11 00:16:38, Wez wrote: > CHECK_EQ will already log |result| (and the expected value) in Debug builds, so > do you really need the additional log text? If you do then could it simply be: > > CHECK_EQ(...) << "mojo::CreateSharedBuffer failed: " << result; > > and similarly, below? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:42: // Create read-only handle for browser-side consumption. On 2016/06/11 00:16:37, Wez wrote: > Why not do this on-the-fly in take_remote_handle()? Uh, why not, indeed! Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:55: CHECK_EQ(MOJO_RESULT_OK, result); On 2016/06/11 00:16:37, Wez wrote: > Looks like you can move the un-mapping into the ctor, immediately after the > memcpy()? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:64: void* mapped_data_; On 2016/06/11 00:16:37, Wez wrote: > Why do we store this? It doesn't look like anything can/does access it? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:98: DLOG(ERROR) << "Redundant blob put attempted: " << BlobIdToString(id); On 2016/06/11 00:16:38, Wez wrote: > nit: If this is a coding error, consider DLOG(FATAL), otherwise make it a > DVLOG(1)? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:104: DLOG(FATAL) << "Zero length blob requested: " << BlobIdToString(id); On 2016/06/11 00:16:38, Wez wrote: > nit: This is PutBlob - surely the zero-length blob was being sent, not > requested? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:119: << BlobIdToString(id); On 2016/06/11 00:16:38, Wez wrote: > This seems like a serious coding error - caller has violated the > PutBlob->DeliverBlob contract? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:123: DLOG(ERROR) << "Blob is already in the remote cache:" << BlobIdToString(id); On 2016/06/11 00:16:37, Wez wrote: > nit: As noted above, is it worth having this be an error, or can it be DVLOG(1)? Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.h (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.h:26: explicit BlobChannelSenderProxy(mojom::BlobChannelPtr blob_channel); On 2016/06/11 00:16:38, Wez wrote: > I'd recommend having: > > static scoped_ptr<BlobChannelSender> CreateForTest(...) > > instead - that way you can get PRESUBMIT checks to fire if anyone tries to use > it from production code. Done. https://codereview.chromium.org/2033013003/diff/100001/blimp/test/run_all_uni... File blimp/test/run_all_unittests.cc (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/test/run_all_uni... blimp/test/run_all_unittests.cc:16: On 2016/06/13 17:36:43, Ken Rockot wrote: > nitty nit: redundant vspace Done.
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:9: // large data payloads out of the Mojo IPC channel. On 2016/06/16 21:00:38, Kevin M wrote: > On 2016/06/11 00:16:37, Wez wrote: > > nit: Isn't the use of shared-memory obvious from the function signature? > > It wasn't to maniscalco@, who requested that I add this comment. Sorry if I was unclear. The "why" is the part I'm interested in capturing in the comment. Assuming the following is correct, how about something like: "The Mojo IPC channel is a shared resource and blobs can be quite large. We don't want to impact other features/components that use the IPC channel so we send blobs using shared memory to reduce the message latency experienced by other components/features."
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:9: // large data payloads out of the Mojo IPC channel. On 2016/06/16 21:07:14, maniscalco wrote: > On 2016/06/16 21:00:38, Kevin M wrote: > > On 2016/06/11 00:16:37, Wez wrote: > > > nit: Isn't the use of shared-memory obvious from the function signature? > > > > It wasn't to maniscalco@, who requested that I add this comment. > > Sorry if I was unclear. The "why" is the part I'm interested in capturing in > the comment. Assuming the following is correct, how about something like: > > "The Mojo IPC channel is a shared resource and blobs can be quite large. We > don't want to impact other features/components that use the IPC channel so we > send blobs using shared memory to reduce the message latency experienced by > other components/features." How's this?
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033013003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033013003/180001
https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/100001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:9: // large data payloads out of the Mojo IPC channel. On 2016/06/21 17:19:46, Kevin M wrote: > On 2016/06/16 21:07:14, maniscalco wrote: > > On 2016/06/16 21:00:38, Kevin M wrote: > > > On 2016/06/11 00:16:37, Wez wrote: > > > > nit: Isn't the use of shared-memory obvious from the function signature? > > > > > > It wasn't to maniscalco@, who requested that I add this comment. > > > > Sorry if I was unclear. The "why" is the part I'm interested in capturing in > > the comment. Assuming the following is correct, how about something like: > > > > "The Mojo IPC channel is a shared resource and blobs can be quite large. We > > don't want to impact other features/components that use the IPC channel so we > > send blobs using shared memory to reduce the message latency experienced by > > other components/features." > > How's this? That captures it nicely.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2033013003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kmarshall@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for IPC security review
https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:26: DCHECK(data.is_valid()); The comments imply this is coming from the renderer. I'm a bit fuzzy on the Blimp process model, but I guess the renderer is still untrusted? So it seems like this needs to be an if (), not a DCHECK(). https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (left): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:38: DCHECK(!IsInEngineCache(id)); How come these (and other checks) changed from DCHECKs()? https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:41: local_handle_ = mojo::SharedBufferHandle::Create(data->data.size()); Any idea how likely it is to run into shared memory limits with this? https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:42: CHECK(local_handle_.is_valid()) << "Mojo error when creating shared buffer."; IMO, DCHECK() if this is something that should "never happen" or if () if it can happen. CHECK() should be quite rare. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:51: mojo::ScopedSharedBufferHandle SharedMemoryBlob::take_remote_handle() { Take implies that this is destructive, but I don't think it is? Style guide also dictates NamingLikeThis for this sort of method. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:73: return base::WrapUnique(new BlobChannelSenderProxy(std::move(blob_channel))); Nit: base::MakeUnique
https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:26: DCHECK(data.is_valid()); On 2016/06/25 01:06:31, dcheng wrote: > The comments imply this is coming from the renderer. I'm a bit fuzzy on the > Blimp process model, but I guess the renderer is still untrusted? So it seems > like this needs to be an if (), not a DCHECK(). Done. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (left): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:38: DCHECK(!IsInEngineCache(id)); On 2016/06/25 01:06:31, dcheng wrote: > How come these (and other checks) changed from DCHECKs()? So that these conditions are correctly handled by release builds, and so that we can develop unit tests for it. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:41: local_handle_ = mojo::SharedBufferHandle::Create(data->data.size()); On 2016/06/25 01:06:31, dcheng wrote: > Any idea how likely it is to run into shared memory limits with this? Added size check. Done. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:42: CHECK(local_handle_.is_valid()) << "Mojo error when creating shared buffer."; On 2016/06/25 01:06:31, dcheng wrote: > IMO, DCHECK() if this is something that should "never happen" or if () if it can > happen. CHECK() should be quite rare. Done. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:51: mojo::ScopedSharedBufferHandle SharedMemoryBlob::take_remote_handle() { On 2016/06/25 01:06:31, dcheng wrote: > Take implies that this is destructive, but I don't think it is? > > Style guide also dictates NamingLikeThis for this sort of method. Done. https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:73: return base::WrapUnique(new BlobChannelSenderProxy(std::move(blob_channel))); On 2016/06/25 01:06:31, dcheng wrote: > Nit: base::MakeUnique This is a cool method, but N/A in this case. The ctor that this method is calling is private, and therefore is inaccessible to MakeUnique().
https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (left): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:38: DCHECK(!IsInEngineCache(id)); On 2016/06/27 17:31:12, Kevin M wrote: > On 2016/06/25 01:06:31, dcheng wrote: > > How come these (and other checks) changed from DCHECKs()? > > So that these conditions are correctly handled by release builds, and so that we > can develop unit tests for it. Right, but violating these conditions means that the consumer of this class is violating the expected preconditions, right? Can we fix the consumer, since it's all in the same process? https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:7: interface BlobChannel { If possible, a comment describing this interface would be useful (e.g. where the service lives, what it's used for, what's in the blobs, expected size/limitations, etc) https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:46: CHECK_LT(data->data.size(), kMaxBlobSizeBytes); I think this should be a DCHECK(), with a browser-side check.
https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (left): https://codereview.chromium.org/2033013003/diff/200001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:38: DCHECK(!IsInEngineCache(id)); On 2016/06/28 06:26:18, dcheng wrote: > On 2016/06/27 17:31:12, Kevin M wrote: > > On 2016/06/25 01:06:31, dcheng wrote: > > > How come these (and other checks) changed from DCHECKs()? > > > > So that these conditions are correctly handled by release builds, and so that > we > > can develop unit tests for it. > > Right, but violating these conditions means that the consumer of this class is > violating the expected preconditions, right? Can we fix the consumer, since it's > all in the same process? Good point... done. https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel.mojom (right): https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel.mojom:7: interface BlobChannel { On 2016/06/28 06:26:18, dcheng wrote: > If possible, a comment describing this interface would be useful (e.g. where the > service lives, what it's used for, what's in the blobs, expected > size/limitations, etc) Done, though I don't want to go into too much detail here - the canonical descriptions should be kept in blimp/net/blob_channel/*. https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/220001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:46: CHECK_LT(data->data.size(), kMaxBlobSizeBytes); On 2016/06/28 06:26:18, dcheng wrote: > I think this should be a DCHECK(), with a browser-side check. Done and done.
Description was changed from ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719 ========== to ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 ==========
LGTM but please update the description to cover the other changes, e.g. to the way we manage run_all_unittests. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:27: LOG(ERROR) << "Invalid data handle received from renderer process."; Not for this CL: We should work out how in general to turn these sorts of failures into renderer teardowns, since they indicate the renderer violating the protocol. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:37: auto mapping = data->Map(size); nit: Don't use auto here, since it's not obvious to the reader what the resulting type will be without looking up the Map() API, and the explicit type name won't make the code too verbose. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:27: // acknowledged that it is finished using the buffer. 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? https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:33: mojo::ScopedSharedBufferHandle GetRemoteHandle(); 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? https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:42: DCHECK_LT(data->data.size(), kMaxBlobSizeBytes); nit: We tend to try to keep these (expectation, actual) ordered, if possible. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:45: DCHECK(local_handle_.is_valid()) << "Mojo error when creating shared buffer."; nit: Here and below, does the message text help? Isn't it implicit from the DCHECK failure? https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:55: auto remote_handle = 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. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:92: if (size == 0) { nit: It would be more readable to if(data->data.empty() here, IMO. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:106: << BlobIdToString(id); Does this indicate a renderer misbehaviour? https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:110: DVLOG(1) << "Blob is already in the remote cache:" << BlobIdToString(id); Similarly, why would we be asked to re-send?
mojo LGTM with comments addressed. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:37: auto mapping = data->Map(size); I missed this, but we need to check that the Map() call succeeds (it will return nullptr on failure) https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:110: DVLOG(1) << "Blob is already in the remote cache:" << BlobIdToString(id); On 2016/07/01 01:06:57, Wez wrote: > Similarly, why would we be asked to re-send? 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)
Description was changed from ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 ========== to ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. * Add BlobChannelSenderProxy class, which manages interactions with the renderer side of the BlobChannel IPC interface. * Add Blimp-specific run_all_unittests.cc, which bootstraps the Mojo environment before executing unittests. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 ==========
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, wez@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2033013003/#ps260001 (title: "IPC owners file added.")
https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... File blimp/engine/mojo/blob_channel_service.cc (right): https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:27: LOG(ERROR) << "Invalid data handle received from renderer process."; On 2016/07/01 01:06:56, Wez wrote: > Not for this CL: We should work out how in general to turn these sorts of > failures into renderer teardowns, since they indicate the renderer violating the > protocol. Filed crbug.com/629096 https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:37: auto mapping = data->Map(size); On 2016/07/01 02:04:28, dcheng wrote: > I missed this, but we need to check that the Map() call succeeds (it will return > nullptr on failure) Done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/mojo/blob... blimp/engine/mojo/blob_channel_service.cc:37: auto mapping = data->Map(size); On 2016/07/01 01:06:56, Wez wrote: > nit: Don't use auto here, since it's not obvious to the reader what the > resulting type will be without looking up the Map() API, and the explicit type > name won't make the code too verbose. Done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... File blimp/engine/renderer/blob_channel_sender_proxy.cc (right): https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:27: // acknowledged that it is finished using the buffer. On 2016/07/01 01:06:56, Wez wrote: > 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? Yes. I should've removed this comment; I updated the code to reflect those semantics but the comments were untouched. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:33: mojo::ScopedSharedBufferHandle GetRemoteHandle(); On 2016/07/01 01:06:57, Wez wrote: > 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? Create is better, done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:42: DCHECK_LT(data->data.size(), kMaxBlobSizeBytes); On 2016/07/01 01:06:57, Wez wrote: > nit: We tend to try to keep these (expectation, actual) ordered, if possible. Done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:45: DCHECK(local_handle_.is_valid()) << "Mojo error when creating shared buffer."; On 2016/07/01 01:06:56, Wez wrote: > nit: Here and below, does the message text help? Isn't it implicit from the > DCHECK failure? Done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:55: auto remote_handle = On 2016/07/01 01:06:57, Wez wrote: > 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. Done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:92: if (size == 0) { On 2016/07/01 01:06:57, Wez wrote: > nit: It would be more readable to if(data->data.empty() here, IMO. Made this a CHECK and used empty() https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:106: << BlobIdToString(id); On 2016/07/01 01:06:57, Wez wrote: > Does this indicate a renderer misbehaviour? Yes, though since it's probably a deterministic control flow check, I'll make it a DCHECK. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:110: DVLOG(1) << "Blob is already in the remote cache:" << BlobIdToString(id); On 2016/07/01 02:04:28, dcheng wrote: > On 2016/07/01 01:06:57, Wez wrote: > > Similarly, why would we be asked to re-send? > > 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) Done. https://codereview.chromium.org/2033013003/diff/240001/blimp/engine/renderer/... blimp/engine/renderer/blob_channel_sender_proxy.cc:110: DVLOG(1) << "Blob is already in the remote cache:" << BlobIdToString(id); On 2016/07/01 01:06:57, Wez wrote: > Similarly, why would we be asked to re-send? Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. * Add BlobChannelSenderProxy class, which manages interactions with the renderer side of the BlobChannel IPC interface. * Add Blimp-specific run_all_unittests.cc, which bootstraps the Mojo environment before executing unittests. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 ========== to ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. * Add BlobChannelSenderProxy class, which manages interactions with the renderer side of the BlobChannel IPC interface. * Add Blimp-specific run_all_unittests.cc, which bootstraps the Mojo environment before executing unittests. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. * Add BlobChannelSenderProxy class, which manages interactions with the renderer side of the BlobChannel IPC interface. * Add Blimp-specific run_all_unittests.cc, which bootstraps the Mojo environment before executing unittests. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 ========== to ========== Use shared memory for moving data over BlobChannel Mojo interface. * Change BlobChannel PutBlob() Mojo IDL to use shared memory handles instead of a byte array. * Create "SharedMemoryBlob" class for encapsulating Mojo shared memory initialization and lifetime management semantics. * Add BlobChannelSenderProxy class, which manages interactions with the renderer side of the BlobChannel IPC interface. * Add Blimp-specific run_all_unittests.cc, which bootstraps the Mojo environment before executing unittests. R=wez@chromium.org CC=maniscalco@chromium.org BUG=600719,614564 Committed: https://crrev.com/ca6057dcb4c724085356bda1706b48cf54e08fba Cr-Commit-Position: refs/heads/master@{#406045} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/ca6057dcb4c724085356bda1706b48cf54e08fba Cr-Commit-Position: refs/heads/master@{#406045} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
