RendererCompositorFrameSink should handle local surface id allocation

content/browser/renderer_host/render_widget_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 
 #include <math.h>
 
 #include <set>
 #include <tuple>
@@ skipping 1805 matching lines @@
 
 bool RenderWidgetHostImpl::OnSwapCompositorFrame(
     const IPC::Message& message) {
   // This trace event is used in
   // chrome/browser/extensions/api/cast_streaming/performance_test.cc
   TRACE_EVENT0("test_fps,benchmark", "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) {
     // Received an invalid ack, renderer misbehaved.
     bad_message::ReceivedBadMessage(
         GetProcess(),
         bad_message::RWH_INVALID_BEGIN_FRAME_ACK_COMPOSITOR_FRAME);
     return false;
   }
   // |has_damage| and |remaining_frames| are not transmitted.
@@ skipping 12 matching lines @@
   if (touch_emulator_)
     touch_emulator_->SetDoubleTapSupportForPageEnabled(!is_mobile_optimized);
 
   // Ignore this frame if its content has already been unloaded. Source ID
   // is always zero for an OOPIF because we are only concerned with displaying
   // stale graphics on top-level frames. We accept frames that have a source ID
   // greater than |current_content_source_id_| because in some cases the first
   // 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;
     cc::TransferableResource::ReturnResources(frame.resource_list, &resources);
     SendReclaimCompositorResources(routing_id_, compositor_frame_sink_id,
                                    process_->GetID(), true /* is_swap_ack */,
                                    resources);
   }
 
   RenderProcessHost* rph = GetProcess();
@@ skipping 718 matching lines @@
     // different from the receiver's.
     file_system_file.url =
         GURL(storage::GetIsolatedFileSystemRootURIString(
                  file_system_url.origin(), filesystem_id, std::string())
                  .append(register_name));
     file_system_file.filesystem_id = filesystem_id;
   }
 }
 
 }  // namespace content