RendererCompositorFrameSink should handle local surface id allocation

content/browser/renderer_host/render_widget_host_impl.cc

Index: content/browser/renderer_host/render_widget_host_impl.cc
diff --git a/content/browser/renderer_host/render_widget_host_impl.cc b/content/browser/renderer_host/render_widget_host_impl.cc
index f6281364fc7bb3cc94f32013a2882549e03f8728..efa38423c7a5e8027b0a118e533d04ac95182e7b 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -1823,10 +1823,32 @@ bool RenderWidgetHostImpl::OnSwapCompositorFrame(
   ViewHostMsg_SwapCompositorFrame::Param param;
   if (!ViewHostMsg_SwapCompositorFrame::Read(&message, &param))
     return false;
-  cc::CompositorFrame frame(std::move(std::get<1>(param)));
   uint32_t compositor_frame_sink_id = std::get<0>(param);
+  cc::LocalSurfaceId local_surface_id = std::get<1>(param);
+  cc::CompositorFrame frame(std::move(std::get<2>(param)));
   std::vector<IPC::Message> messages_to_deliver_with_frame;
-  messages_to_deliver_with_frame.swap(std::get<2>(param));
+  messages_to_deliver_with_frame.swap(std::get<3>(param));
+
+  // The renderer should not send empty frames.
+  if (frame.render_pass_list.empty()) {
+    NOTREACHED();

[piman, 2017/03/20 22:51:45]

nit: not NOTREACHED because it can be reached from a malicious renderer.
DLOG(ERROR), and/or bad_message::ReceivedBadMessage is appropriate.

[Saman Sami, 2017/03/21 16:33:38]

Done.

+    return false;
+  }
+
+  // The renderer must allocate a new LocalSurfaceId if frame size or device
+  // scale factor changes.
+  float device_scale_factor = frame.metadata.device_scale_factor;
+  const gfx::Size& frame_size =
+      frame.render_pass_list.back()->output_rect.size();
+  if (local_surface_id == last_local_surface_id_ &&
+      (frame_size != last_frame_size_ ||
+       device_scale_factor != last_device_scale_factor_)) {
+    NOTREACHED();

[piman, 2017/03/20 22:51:45]

nit: ditto

[Saman Sami, 2017/03/21 16:33:38]

Done.

+    return false;
+  }
+  last_local_surface_id_ = local_surface_id;

[piman, 2017/03/20 22:51:45]

To confirm: is checking the last local_surface_id enough? I.e. if a renderer
tries to quickly ping-pong between 2 local_surface_id with arbitrary sizes/DSF,
we won't try to reference an old version of the surface, right?

[Saman Sami, 2017/03/21 02:10:25]

Good question. If the renderer alternates between two local surface ids then
it's possible that we submit frames of different size to the same surface, but
should we care? I'm not sure how much we should care if a client is trying to
sabotage its own surfaces.

[Saman Sami, 2017/03/21 16:33:38]

I guess my point is that we cared about two consecutive frames because if the
renderer doesn't update its LocalSurfaceId properly, DelegatedFrameHost can be
left in an inconsistent state, but in your ping-pong case this will not happen.

+  last_frame_size_ = frame_size;
+  last_device_scale_factor_ = device_scale_factor;
 
   if (frame.metadata.begin_frame_ack.sequence_number <
       cc::BeginFrameArgs::kStartingFrameNumber) {
@@ -1859,7 +1881,8 @@ bool RenderWidgetHostImpl::OnSwapCompositorFrame(
   // compositor frame can arrive before the navigation commit message that
   // updates that value.
   if (view_ && frame.metadata.content_source_id >= current_content_source_id_) {
-    view_->OnSwapCompositorFrame(compositor_frame_sink_id, std::move(frame));
+    view_->OnSwapCompositorFrame(compositor_frame_sink_id, local_surface_id,
+                                 std::move(frame));
     view_->DidReceiveRendererFrame();
   } else {
     cc::ReturnedResourceArray resources;