Remove gpu side LatencyInfo merging

Remove gpu side LatencyInfo merging

This CL removes the LatecyInfo merging from the following path:

Compsitor to GPU:
CommandBufferProxyImpl::SetLatencyInfo()
-> (GpuCommandBufferMsg_SetLatencyInfo)
-> GpuCommandBufferStub::OnSetLatencyInfo()
-> ImageTransportHelper::SetLatencyInfo()
-> XXXImageTransportSurface:SetLatencyInfo()

After swap buffers:
XXXImageTransportSurface::SwapBuffers()
-> (GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params/
    GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params/
    GpuHostMsg_FrameDrawn)
-> RenderWidgetHostView

BUG=246034
TEST=unittests pass.
     Tested on Pixel. chrome://tracing shows correct InputLatency tracking for
     various inputs.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245260

[Yufeng Shen (Slow to review), 2014-01-02 23:28:54]

Hey John, 

Hopefully after this CL all the renderer side LatencyInfo merging are gone.

[Yufeng Shen (Slow to review), 2014-01-08 01:59:32]

On 2014/01/02 23:28:54, Yufeng Shen wrote:
> Hey John, 
> 
> Hopefully after this CL all the renderer side LatencyInfo merging are gone.

Ping.

[jbauman, 2014-01-09 01:28:47]

https://codereview.chromium.org/123563002/diff/20001/content/common/gpu/image...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/123563002/diff/20001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:276: params.latency_info =
std::vector<ui::LatencyInfo>();
Why aren't you passing latency_info_ through here?

https://codereview.chromium.org/123563002/diff/20001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:303: params.latency_info =
std::vector<ui::LatencyInfo>();
Likewise, you need to pass latency_info_ through here.

[Yufeng Shen (Slow to review), 2014-01-09 22:41:40]

https://codereview.chromium.org/123563002/diff/20001/content/common/gpu/image...
File content/common/gpu/image_transport_surface.cc (right):

https://codereview.chromium.org/123563002/diff/20001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:276: params.latency_info =
std::vector<ui::LatencyInfo>();
On 2014/01/09 01:28:47, jbauman wrote:
> Why aren't you passing latency_info_ through here?

Done.

https://codereview.chromium.org/123563002/diff/20001/content/common/gpu/image...
content/common/gpu/image_transport_surface.cc:303: params.latency_info =
std::vector<ui::LatencyInfo>();
On 2014/01/09 01:28:47, jbauman wrote:
> Likewise, you need to pass latency_info_ through here.

Done.

[jbauman, 2014-01-09 22:46:56]

lgtm

[Yufeng Shen (Slow to review), 2014-01-09 22:54:57]

Thanks John for checking the overall LatencyInfo unmerging flow.

+ kbr@ for OWNER of content/browser/gpu/*
                    content/browser/renderer_host/*
                    content/common/gpu/*

+ jln@ for OWNER of content/common/view_messages.h

[jln (very slow on Chromium), 2014-01-09 23:13:27]

https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1469: const
ViewHostMsg_CompositorSurfaceBuffersSwapped_Params& params) {
Could you add a check of latency_info.size() for something "reasonable"?

We're trying to remove / prevent cases where renderers can populate large
structures into the memory of the browser.

You also would need to add such a check in render_process_host_impl.cc.

[Yufeng Shen (Slow to review), 2014-01-10 01:14:00]

https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:1469: const
ViewHostMsg_CompositorSurfaceBuffersSwapped_Params& params) {
On 2014/01/09 23:13:27, jln wrote:
> Could you add a check of latency_info.size() for something "reasonable"?
> 
> We're trying to remove / prevent cases where renderers can populate large
> structures into the memory of the browser.
> 
> You also would need to add such a check in render_process_host_impl.cc.

I noticed that ViewHostMsg_CompositorSurfaceBuffersSwapped is generated in
GpuProcessHost::OnAcceleratedSurfaceBuffersSwapped().

So I put a guard there if the latency_info.size() is too big, then
end the LatencyInfo there (adding special component to the LatencyInfo so it
will be captured during telemetry test) and clear the latency_info list. So no
big msg will be sent to browser.

[jln (very slow on Chromium), 2014-01-10 01:20:39]

On 2014/01/10 01:14:00, Yufeng Shen wrote:
>
https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_impl.cc:1469: const
> ViewHostMsg_CompositorSurfaceBuffersSwapped_Params& params) {
> On 2014/01/09 23:13:27, jln wrote:
> > Could you add a check of latency_info.size() for something "reasonable"?
> > 
> > We're trying to remove / prevent cases where renderers can populate large
> > structures into the memory of the browser.
> > 
> > You also would need to add such a check in render_process_host_impl.cc.
> 
> I noticed that ViewHostMsg_CompositorSurfaceBuffersSwapped is generated in
> GpuProcessHost::OnAcceleratedSurfaceBuffersSwapped().
> 
> So I put a guard there if the latency_info.size() is too big, then
> end the LatencyInfo there (adding special component to the LatencyInfo so it
> will be captured during telemetry test) and clear the latency_info list. So no
> big msg will be sent to browser.

(I didn't read the code, I hope I didn't misunderstand).

We want to defend against a malicious renderer (compromised by an attacker)
sending messages to the browser. I don't think your fix would do that, correct?

Moreover, this needs to be easy to audit / review, i.e. someone looking at
_messages.h should quickly be able to be convinced that no oversize vector would
be accepted by the browser.

[Ken Russell (switch to Gerrit), 2014-01-10 07:51:33]

It sounds like there are issues to be resolved per jln's review. Please ping me
when this is ready for review again.

[Yufeng Shen (Slow to review), 2014-01-10 19:52:04]

On 2014/01/10 01:20:39, jln wrote:
> On 2014/01/10 01:14:00, Yufeng Shen wrote:
> >
>
https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/123563002/diff/90001/content/browser/renderer...
> > content/browser/renderer_host/render_widget_host_impl.cc:1469: const
> > ViewHostMsg_CompositorSurfaceBuffersSwapped_Params& params) {
> > On 2014/01/09 23:13:27, jln wrote:
> > > Could you add a check of latency_info.size() for something "reasonable"?
> > > 
> > > We're trying to remove / prevent cases where renderers can populate large
> > > structures into the memory of the browser.
> > > 
> > > You also would need to add such a check in render_process_host_impl.cc.
> > 
> > I noticed that ViewHostMsg_CompositorSurfaceBuffersSwapped is generated in
> > GpuProcessHost::OnAcceleratedSurfaceBuffersSwapped().
> > 
> > So I put a guard there if the latency_info.size() is too big, then
> > end the LatencyInfo there (adding special component to the LatencyInfo so it
> > will be captured during telemetry test) and clear the latency_info list. So
no
> > big msg will be sent to browser.
> 
> (I didn't read the code, I hope I didn't misunderstand).
> 
> We want to defend against a malicious renderer (compromised by an attacker)
> sending messages to the browser. I don't think your fix would do that,
correct?
> 
> Moreover, this needs to be easy to audit / review, i.e. someone looking at
> _messages.h should quickly be able to be convinced that no oversize vector
would
> be accepted by the browser.

ahh, I guess it was me that misunderstands your point.

I was thinking more like in some strange case, the latency_info list in
ViewHostMsg_CompositorSurfaceBuffersSwapped
gets too big, so I thought at the only place where
ViewHostMsg_CompositorSurfaceBuffersSwapped is
generated I check the size of the latency_info list, and if it is unusually big
I just clear it before sending it
to browser.

But what your point is that it is not reliable to assume that
ViewHostMsg_CompositorSurfaceBuffersSwapped
is always having the reasonable size latency_info, even with the guard mentioned
above, since it can be synthesized (?)
by some compromised renderer. Am I understanding it correctly ?

So your suggestion is that at the receiving side of
ViewHostMsg_CompositorSurfaceBuffersSwapped to check the 
size of the latency_info.size(), e.g. CHECK(latency_info.size() < 100 ) ?

[Yufeng Shen (Slow to review), 2014-01-14 00:28:18]

Offline talk to jln@ and got it clear that it is at the
receiving side of ViewHostMsg_CompositorSurfaceBuffersSwapped
where the size of latency_info list needs to be CHECKEed to
against maliciously synthesized msg.

CHECK added in gpu_process_host.cc & render_process_host_impl.cc.

Julien: PTAL, thanks.

[jln (very slow on Chromium), 2014-01-14 00:32:50]

Sorry, I should have been more clear, when I meant "checking" that I didn't mean
literally a CHECK().

https://chromiumcodereview.appspot.com/123563002/diff/320001/content/browser/...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/123563002/diff/320001/content/browser/...
content/browser/renderer_host/render_process_host_impl.cc:1994:
CHECK(params.latency_info.size() < kMaxLatencyInfoNumber);
The problem with this approach is that is gives a trivial denial of service from
a compromised renderer to the browser.

It's not a huge issue, but we would rather avoid that. Could we log an error and
ignore the message instead?

[Yufeng Shen (Slow to review), 2014-01-14 00:47:14]

https://codereview.chromium.org/123563002/diff/320001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/123563002/diff/320001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1994:
CHECK(params.latency_info.size() < kMaxLatencyInfoNumber);
On 2014/01/14 00:32:51, jln wrote:
> The problem with this approach is that is gives a trivial denial of service
from
> a compromised renderer to the browser.
> 
> It's not a huge issue, but we would rather avoid that. Could we log an error
and
> ignore the message instead?

Done.

[jln (very slow on Chromium), 2014-01-14 01:11:51]

So, content/common/gpu/gpu_messages.h is in scope for this review as well, which
unfortunately adds a few more required checks.

We (security team) need to devise a better way to address this issue. It should
be impossible to deserialize arbitrary size vectors so easily. It's too painful
to add so many checks.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:879: const
GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params& params) {
Check for vector size here as well.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:938: TRACE_EVENT0("gpu",
"GpuProcessHost::OnAcceleratedSurfaceBuffersSwapped");
Another size check required here.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:1010: 
And here as well (or someone will forget to check when they'll implement it).

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.cc:316: const
std::vector<ui::LatencyInfo>& latency_info) {
check for size() here as well.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.cc:322: TRACE_EVENT0("renderer",
check for size() here as well.

https://codereview.chromium.org/123563002/diff/460001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/123563002/diff/460001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:570: const
std::vector<ui::LatencyInfo>& latency_info) {
Check for .size() here as well.

[Yufeng Shen (Slow to review), 2014-01-14 20:02:41]

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:879: const
GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params& params) {
On 2014/01/14 01:11:52, jln wrote:
> Check for vector size here as well.

Done.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:938: TRACE_EVENT0("gpu",
"GpuProcessHost::OnAcceleratedSurfaceBuffersSwapped");
On 2014/01/14 01:11:52, jln wrote:
> Another size check required here.

Done.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:1010: 
On 2014/01/14 01:11:52, jln wrote:
> And here as well (or someone will forget to check when they'll implement it).

Done.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.cc:316: const
std::vector<ui::LatencyInfo>& latency_info) {
On 2014/01/14 01:11:52, jln wrote:
> check for size() here as well.

Done.

https://codereview.chromium.org/123563002/diff/460001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.cc:322: TRACE_EVENT0("renderer",
On 2014/01/14 01:11:52, jln wrote:
> check for size() here as well.

Done.

https://codereview.chromium.org/123563002/diff/460001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/123563002/diff/460001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:570: const
std::vector<ui::LatencyInfo>& latency_info) {
On 2014/01/14 01:11:52, jln wrote:
> Check for .size() here as well.

Done.

[jln (very slow on Chromium), 2014-01-14 21:52:31]

*_messages.h lgtm.

I have started a thread on security-dev to find a better way to handle this sort
of stuff.

[Yufeng Shen (Slow to review), 2014-01-14 22:33:50]

On 2014/01/14 21:52:31, jln wrote:
> *_messages.h lgtm.
> 
> I have started a thread on security-dev to find a better way to handle this
sort
> of stuff.

Thanks !

Ken, now it is ready for your OWNER review.

[Ken Russell (switch to Gerrit), 2014-01-15 06:41:29]

https://codereview.chromium.org/123563002/diff/580001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/123563002/diff/580001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:884: << " size " <<
params.latency_info.size() << " is too big.";
Please refactor this duplicated code and constant to be contained in one place.
I suggest adding a static method to ui::LatencyInfo like

  bool Verify(const std::vector<LatencyInfo>& info_vector, const char*
referring_message_name)

which you can call throughout this CL with two lines, like

  if (!ui::LatencyInfo::Verify(params.latency_info,
"GpuHostMsg_AcceleratedSurfaceBuffersSwapped"))
    return;

Please place the implementation in latency_info.cc so it doesn't get inlined
everywhere.

[Yufeng Shen (Slow to review), 2014-01-15 19:44:30]

https://codereview.chromium.org/123563002/diff/580001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/123563002/diff/580001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:884: << " size " <<
params.latency_info.size() << " is too big.";
On 2014/01/15 06:41:29, Ken Russell wrote:
> Please refactor this duplicated code and constant to be contained in one
place.
> I suggest adding a static method to ui::LatencyInfo like
> 
>   bool Verify(const std::vector<LatencyInfo>& info_vector, const char*
> referring_message_name)
> 
> which you can call throughout this CL with two lines, like
> 
>   if (!ui::LatencyInfo::Verify(params.latency_info,
> "GpuHostMsg_AcceleratedSurfaceBuffersSwapped"))
>     return;
> 
> Please place the implementation in latency_info.cc so it doesn't get inlined
> everywhere.

Done.

[Ken Russell (switch to Gerrit), 2014-01-16 02:05:13]

Thanks for the cleanup. LGTM

https://codereview.chromium.org/123563002/diff/910001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/123563002/diff/910001/ui/events/latency_info....
ui/events/latency_info.h:143: static const unsigned int kMaxLatencyInfoNumber =
100;
This could just go into latency_info.cc.

[Yufeng Shen (Slow to review), 2014-01-16 03:33:16]

https://codereview.chromium.org/123563002/diff/910001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/123563002/diff/910001/ui/events/latency_info....
ui/events/latency_info.h:143: static const unsigned int kMaxLatencyInfoNumber =
100;
On 2014/01/16 02:05:14, Ken Russell wrote:
> This could just go into latency_info.cc.

Done.

[commit-bot: I haz the power, 2014-01-16 03:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/123563002/960002

[commit-bot: I haz the power, 2014-01-16 03:55:33]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Yufeng Shen (Slow to review), 2014-01-16 04:53:58]

On 2014/01/16 03:55:33, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

ah, didn't realized that  content/common/gpu/gpu_messages.h  needs a separate
OWNER review.

Justin, can you help to take a review for content/common/gpu/gpu_messages.h ?

[jschuh, 2014-01-16 07:35:39]

On 2014/01/16 04:53:58, Yufeng Shen wrote:
> On 2014/01/16 03:55:33, I haz the power (commit-bot) wrote:
> > Retried try job too often on chromium_presubmit for step(s) presubmit
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
> 
> ah, didn't realized that  content/common/gpu/gpu_messages.h  needs a separate
> OWNER review.
> 
> Justin, can you help to take a review for content/common/gpu/gpu_messages.h ?

Rubberstamp IPC security lgtm since @jln already reviewed it. And it looks like
content/common/gpu/OWNERS is missing @jln and a few other IPC reviewers.

[commit-bot: I haz the power, 2014-01-16 07:39:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/123563002/960002

[commit-bot: I haz the power, 2014-01-16 07:39:35]

Failed to apply patch for
content/browser/aura/software_browser_compositor_output_surface.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: content/browser/aura/software_browser_compositor_output_surface.cc
  |diff --git
a/content/browser/aura/software_browser_compositor_output_surface.cc
b/content/browser/aura/software_browser_compositor_output_surface.cc
  |index
6516f93948a81fc173c19da523c1cf2921cc800f..05e2819761792e781e5198f7a0c575987b946e7f
100644
  |--- a/content/browser/aura/software_browser_compositor_output_surface.cc
  |+++ b/content/browser/aura/software_browser_compositor_output_surface.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       content/browser/aura/software_browser_compositor_output_surface.cc
Index: content/browser/aura/software_browser_compositor_output_surface.cc
diff --git a/content/browser/aura/software_browser_compositor_output_surface.cc
b/content/browser/aura/software_browser_compositor_output_surface.cc
index
6516f93948a81fc173c19da523c1cf2921cc800f..05e2819761792e781e5198f7a0c575987b946e7f
100644
--- a/content/browser/aura/software_browser_compositor_output_surface.cc
+++ b/content/browser/aura/software_browser_compositor_output_surface.cc
@@ -35,17 +35,15 @@ SoftwareBrowserCompositorOutputSurface::
 
 void SoftwareBrowserCompositorOutputSurface::SwapBuffers(
     cc::CompositorFrame* frame) {
-  // TODO(miletus) : Convert RenderWidgetHostImpl::CompositorFrameDrawn() to
-  // take std::vector<ui::LatencyInfo> directly so we only post the task once.
   for (size_t i = 0; i < frame->metadata.latency_info.size(); i++) {
     frame->metadata.latency_info[i].AddLatencyNumber(
         ui::INPUT_EVENT_LATENCY_TERMINATED_FRAME_SWAP_COMPONENT, 0, 0);
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &RenderWidgetHostImpl::CompositorFrameDrawn,
-            frame->metadata.latency_info[i]));
   }
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &RenderWidgetHostImpl::CompositorFrameDrawn,
+          frame->metadata.latency_info));
 
   gfx::VSyncProvider* vsync_provider = software_device()->GetVSyncProvider();
   if (vsync_provider) {

[commit-bot: I haz the power, 2014-01-16 16:23:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/123563002/960002

[commit-bot: I haz the power, 2014-01-16 18:25:47]

Change committed as 245260