Add IPC messages to transfer compositor protos

Add IPC messages to transfer compositor protos

- Be able to send and receive compositor protos to/from the render
  process and the browser process.  Messages are piped through the
  browser process to/from the network and renderer.  A separate client
  (go/blimp) will render the compositor protos.  This is part of the
  compositor refactor around splitting ThreadProxy into two components.
- Hook the proto sending into the BlimpEngineSession class.
- Set up a RemoteProtoChannel interface on the CC side to be used by the
  ChannelMain class once we start building that from the LayerTreeHost.

BUG=552055
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5ef644e199b927c4fadb80e87ac68fd397686f63
Cr-Commit-Position: refs/heads/master@{#360452}

[David Trainor- moved to gerrit, 2015-11-14 00:01:12]

dtrainor@chromium.org changed reviewers:
+ enne@chromium.org, haibinlu@chromium.org, sievers@chromium.org

[David Trainor- moved to gerrit, 2015-11-14 00:01:13]

ptal thanks.  This is a much slimmer version of
https://chromiumcodereview.appspot.com/1416443004/...

enne@: cc/
sievers@: content/
haibinlu@: blimp/

This adds the glue from BlimpEngineSession to the LayerTreeHost.  Following CLs
will handle tying this glue into those components.

[no sievers, 2015-11-14 00:13:43]

https://codereview.chromium.org/1442853005/diff/40001/cc/trees/remote_proto_c...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1442853005/diff/40001/cc/trees/remote_proto_c...
cc/trees/remote_proto_channel.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
nit: drop (c)

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:166: // Called when
the widget has sent a compositor proto.
nit: can you mention it's used in blimp mode?

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:476:
IPC_MESSAGE_HANDLER(ViewHostMsg_SendCompositorProto,
nit: it's slightly contradictory between Send/OnHandle. would the notion of
'ViewHostMsg_ForwardCompositorProto/OnForwardCompositorProto' make sense? or
'relay' or something?

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:1264: void
RenderWidgetHostImpl::HandleCompositorProto(
oops, now i'm even more confused, 'Handle' overload :)
this goes into the other direction... so hhmm.. how about some notion of
'incoming' vs. 'outgoing' to differentiate?

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/1442853005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:282: void
OnHandleCompositorProto(const std::vector<char>& proto);
nit: can it be private?

[David Trainor- moved to gerrit, 2015-11-16 04:33:58]

changed method names.  thanks!

https://chromiumcodereview.appspot.com/1442853005/diff/40001/cc/trees/remote_...
File cc/trees/remote_proto_channel.h (right):

https://chromiumcodereview.appspot.com/1442853005/diff/40001/cc/trees/remote_...
cc/trees/remote_proto_channel.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
On 2015/11/14 00:13:43, sievers wrote:
> nit: drop (c)

Done.

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
content/browser/renderer_host/render_widget_host_delegate.h:166: // Called when
the widget has sent a compositor proto.
On 2015/11/14 00:13:43, sievers wrote:
> nit: can you mention it's used in blimp mode?

Done.

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
content/browser/renderer_host/render_widget_host_impl.cc:476:
IPC_MESSAGE_HANDLER(ViewHostMsg_SendCompositorProto,
On 2015/11/14 00:13:43, sievers wrote:
> nit: it's slightly contradictory between Send/OnHandle. would the notion of
> 'ViewHostMsg_ForwardCompositorProto/OnForwardCompositorProto' make sense? or
> 'relay' or something?

Done.

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
content/browser/renderer_host/render_widget_host_impl.cc:1264: void
RenderWidgetHostImpl::HandleCompositorProto(
On 2015/11/14 00:13:43, sievers wrote:
> oops, now i'm even more confused, 'Handle' overload :)
> this goes into the other direction... so hhmm.. how about some notion of
> 'incoming' vs. 'outgoing' to differentiate?

Yeah this is weird.  I made it HandleCompositorProto for incoming and
ForwardCompositorProto for outgoing (after reading the previous comment).  I can
make this explicitly incoming/outgoing if you want though :).

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://chromiumcodereview.appspot.com/1442853005/diff/40001/content/browser/...
content/browser/renderer_host/render_widget_host_impl.h:282: void
OnHandleCompositorProto(const std::vector<char>& proto);
On 2015/11/14 00:13:43, sievers wrote:
> nit: can it be private?

Done.

[David Trainor- moved to gerrit, 2015-11-16 04:34:19]

dtrainor@chromium.org changed reviewers:
+ palmer@chromium.org

[David Trainor- moved to gerrit, 2015-11-16 04:34:20]

palmer@: ptal at view_messages.h.  Thanks! :)

[haibinlu, 2015-11-16 18:36:56]

lgtm

lgtm for blimp/

[enne (OOO), 2015-11-16 19:00:29]

https://codereview.chromium.org/1442853005/diff/80001/cc/trees/remote_proto_c...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1442853005/diff/80001/cc/trees/remote_proto_c...
cc/trees/remote_proto_channel.h:23: class Receiver {
Can you call this something more specific than just "Reciever" if it's going to
be implemented by some other class? Like, ProtoReciever? Or
CompositorProtoReceiver?

https://codereview.chromium.org/1442853005/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1442853005/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1702: if (render_widget_host
!= GetRenderViewHost()->GetWidget())
When would this happen?

https://codereview.chromium.org/1442853005/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1442853005/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1024: if
(!remote_proto_channel_receiver_)
Should this dcheck or something? It seems bad to drop messages on the ground
because there's no receiver.  I'd assume that the receiver will generally be set
up right away?

[David Trainor- moved to gerrit, 2015-11-16 19:48:07]

https://codereview.chromium.org/1442853005/diff/80001/cc/trees/remote_proto_c...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1442853005/diff/80001/cc/trees/remote_proto_c...
cc/trees/remote_proto_channel.h:23: class Receiver {
On 2015/11/16 19:00:28, enne wrote:
> Can you call this something more specific than just "Reciever" if it's going
to
> be implemented by some other class? Like, ProtoReciever? Or
> CompositorProtoReceiver?

Done.

https://codereview.chromium.org/1442853005/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1442853005/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1702: if (render_widget_host
!= GetRenderViewHost()->GetWidget())
On 2015/11/16 19:00:28, enne wrote:
> When would this happen?

It looks like a few methods do this before processing the method call.  I think
we just don't want to ever send messages for a swapped out RVH at this point.

https://codereview.chromium.org/1442853005/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1442853005/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1024: if
(!remote_proto_channel_receiver_)
On 2015/11/16 19:00:28, enne wrote:
> Should this dcheck or something? It seems bad to drop messages on the ground
> because there's no receiver.  I'd assume that the receiver will generally be
set
> up right away?

Good point.  Done.

[enne (OOO), 2015-11-16 22:05:30]

Thanks.  lgtm

[no sievers, 2015-11-16 22:24:56]

lgtm

[palmer, 2015-11-16 23:00:29]

The CL description and the bug don't say much about why we're doing this. Can
you please elaborate? In particular, what is the privileged browser process
going to do with these compositor protos that have been created by untrustworthy
renderers?

https://codereview.chromium.org/1442853005/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1442853005/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:980: std::vector<char>
serialized(proto.ByteSize());
char isn't really/isn't necessarily a byte; use uint8_t for certainty.

https://codereview.chromium.org/1442853005/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:981:
proto.SerializeToArray(serialized.data(), serialized.size());
Sadly, SerializeToArray is defined as taking an int as the size, a tpy ewhich
differs in signedness and possibly in width from the size_t that vector::size
returns. For correctness as written (as opposed to correctness by circumstance),
use the safe conversion functions in base/numeric, or manually check that the
size fits into an int.

[David Trainor- moved to gerrit, 2015-11-17 00:55:27]

Updated the description.  Let me know if that makes it more clear.  Thanks! :)

https://codereview.chromium.org/1442853005/diff/100001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1442853005/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:980: std::vector<char>
serialized(proto.ByteSize());
On 2015/11/16 23:00:29, palmer wrote:
> char isn't really/isn't necessarily a byte; use uint8_t for certainty.

Done.

https://codereview.chromium.org/1442853005/diff/100001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:981:
proto.SerializeToArray(serialized.data(), serialized.size());
On 2015/11/16 23:00:29, palmer wrote:
> Sadly, SerializeToArray is defined as taking an int as the size, a tpy ewhich
> differs in signedness and possibly in width from the size_t that vector::size
> returns. For correctness as written (as opposed to correctness by
circumstance),
> use the safe conversion functions in base/numeric, or manually check that the
> size fits into an int.

Done.

[David Trainor- moved to gerrit, 2015-11-17 00:58:34]

https://codereview.chromium.org/1442853005/diff/120001/cc/trees/remote_proto_...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1442853005/diff/120001/cc/trees/remote_proto_...
cc/trees/remote_proto_channel.h:26: scoped_ptr<proto::CompositorMessage> proto)
= 0;
Had to make this a scoped_ptr because it's unclear if the receiver will actually
process the proto on this thread (and if it's a const& allocated on the stack
it'd have to be copied otherwise IIUC).

https://codereview.chromium.org/1442853005/diff/120001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1442853005/diff/120001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:983:
proto.SerializeToArray(serialized.data(), serialized.size());
Hmm I should probably make this use proto.ByteSize() as the size of the array. 
I'm assuming the checked_cast will kill this if anything bad could happen
though?

[palmer, 2015-11-17 01:25:32]

So, just to be clear: the browser will never interpret or use the compositors,
just forward them to the Blimp process?

https://codereview.chromium.org/1442853005/diff/120001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1442853005/diff/120001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:983:
proto.SerializeToArray(serialized.data(), serialized.size());
On 2015/11/17 00:58:34, David Trainor wrote:
> Hmm I should probably make this use proto.ByteSize() as the size of the array.

> I'm assuming the checked_cast will kill this if anything bad could happen
> though?

Yep, checked_cast will CHECK if there is a problem. The way you have written it
here works just fine. If you wanted you could fold line 981 into line 982, but
it doesn't matter.

[David Trainor- moved to gerrit, 2015-11-17 04:52:26]

Yeah.  The browser process just passes the protobuf through as a block of data
to the Blimp client application (It will add it as a byte array to another
protobuf that frames the messages), which has no chrome/ or content/ code and is
just a dumb rendering front end APK.

https://chromiumcodereview.appspot.com/1442853005/diff/120001/content/rendere...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://chromiumcodereview.appspot.com/1442853005/diff/120001/content/rendere...
content/renderer/gpu/render_widget_compositor.cc:983:
proto.SerializeToArray(serialized.data(), serialized.size());
On 2015/11/17 01:25:32, palmer wrote:
> On 2015/11/17 00:58:34, David Trainor wrote:
> > Hmm I should probably make this use proto.ByteSize() as the size of the
array.
> 
> > I'm assuming the checked_cast will kill this if anything bad could happen
> > though?
> 
> Yep, checked_cast will CHECK if there is a problem. The way you have written
it
> here works just fine. If you wanted you could fold line 981 into line 982, but
> it doesn't matter.

Ah okay thanks!  I tweaked this a bit to just make sure I'm passing the right
thing to the right method, but I guess I didn't have to.

[David Trainor- moved to gerrit, 2015-11-17 21:10:32]

palmer@: Does this look okay? :)

[palmer, 2015-11-17 21:23:25]

This LGTM.

But, who is working on securing the Blimp app itself?

[David Trainor- moved to gerrit, 2015-11-17 22:11:21]

On 2015/11/17 21:23:25, palmer wrote:
> This LGTM.
> 
> But, who is working on securing the Blimp app itself?

As the TL of the client, probably me :).  Should we meet sometime this week?  Do
you have any pointers to what documentation/what things I need to start putting
together for a security review?

Currently we don't execute any JS on the client and we just deserialize the
bytes back into a proto.  If that fails, the proto gets dropped.  If it
succeeds, we apply and render the data structures from the proto.  I could into
this more but a meeting/email thread is probably a better venue for it.

[David Trainor- moved to gerrit, 2015-11-17 22:29:22]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-11-17 22:29:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, sievers@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1442853005/#ps140001
(title: "More signed stuff")

[commit-bot: I haz the power, 2015-11-17 22:30:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442853005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442853005/140001

[commit-bot: I haz the power, 2015-11-17 22:40:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 22:40:06]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[David Trainor- moved to gerrit, 2015-11-18 22:12:05]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-11-18 22:12:06]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
haibinlu@chromium.org, sievers@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1442853005/#ps260001
(title: "Trying to remove CC_EXPORT...")

[commit-bot: I haz the power, 2015-11-18 22:13:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442853005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442853005/260001

[commit-bot: I haz the power, 2015-11-19 00:12:55]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-19 00:14:22]

Patchset 14 (id:??) landed as
https://crrev.com/5ef644e199b927c4fadb80e87ac68fd397686f63
Cr-Commit-Position: refs/heads/master@{#360452}