[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH.

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 535 matching lines @@
                         OnQueueSyntheticGesture)
     IPC_MESSAGE_HANDLER(InputHostMsg_ImeCancelComposition,
                         OnImeCancelComposition)
     IPC_MESSAGE_HANDLER(ViewHostMsg_Close, OnClose)
     IPC_MESSAGE_HANDLER(ViewHostMsg_UpdateScreenRects_ACK,
                         OnUpdateScreenRectsAck)
     IPC_MESSAGE_HANDLER(ViewHostMsg_RequestMove, OnRequestMove)
     IPC_MESSAGE_HANDLER(ViewHostMsg_SetTooltipText, OnSetTooltipText)
     IPC_MESSAGE_HANDLER_GENERIC(ViewHostMsg_SwapCompositorFrame,
                                 OnSwapCompositorFrame(msg))
+    IPC_MESSAGE_HANDLER(ViewHostMsg_BeginFrameDidNotSwap,
+                        OnBeginFrameDidNotSwap)
     IPC_MESSAGE_HANDLER(ViewHostMsg_UpdateRect, OnUpdateRect)
     IPC_MESSAGE_HANDLER(ViewHostMsg_SetCursor, OnSetCursor)
     IPC_MESSAGE_HANDLER(ViewHostMsg_TextInputStateChanged,
                         OnTextInputStateChanged)
     IPC_MESSAGE_HANDLER(ViewHostMsg_LockMouse, OnLockMouse)
     IPC_MESSAGE_HANDLER(ViewHostMsg_UnlockMouse, OnUnlockMouse)
     IPC_MESSAGE_HANDLER(ViewHostMsg_ShowDisambiguationPopup,
                         OnShowDisambiguationPopup)
     IPC_MESSAGE_HANDLER(ViewHostMsg_SelectionBoundsChanged,
                         OnSelectionBoundsChanged)
@@ skipping 1265 matching lines @@
     std::vector<ui::LatencyInfo>().swap(frame.metadata.latency_info);
   }
 
   latency_tracker_.OnSwapCompositorFrame(&frame.metadata.latency_info);
 
   bool is_mobile_optimized = IsMobileOptimizedFrame(frame.metadata);
   input_router_->NotifySiteIsMobileOptimized(is_mobile_optimized);
   if (touch_emulator_)
     touch_emulator_->SetDoubleTapSupportForPageEnabled(!is_mobile_optimized);
 
+  // |has_damage| is not transmitted.
+  frame.metadata.begin_frame_ack.has_damage = true;
+  // |remaining_frames| is not transmitted, but 0 by default.
+  DCHECK_EQ(0u, frame.metadata.begin_frame_ack.remaining_frames);
+
+  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);

[piman, 2017/03/16 18:32:14]

nit: ReceivedBadMessage is a bit of a big hammer. It's not necessarily bad per
se, in particular if we believe a misbehaving renderer could cause bad things to
happen in the browser. However it can make debugging hard, if some condition
causes this to happen, which could cause the renderer to be killed by the
browser as one tries to debug it.

In this case, I think just returning false is enough, maybe with a DLOG(ERROR).
If you feel otherwise, I'm ok if you want to leave it.

Either way, could you move this code to maybe l.1835, so that we do all sanity
checking before doing anything "active", to avoid doing parts of the work (e.g.
latency tracker, etc.) if we are going to refuse the message.

[dcheng, 2017/03/17 05:54:02]

I could see this going either way... with my security hat on, it does seem nice
to strongly enforce invariants are broken, but I also see your point. But if
it's just devs debugging, it's often not going to be an official build anyway,
right? At which point the devs can just patch it out... or patch out the call to
ReceivedBadMessage in the debugger entirely.

I don't feel strongly about this, I just happen to lean slightly the other way
=)

[Eric Seckler, 2017/03/17 11:06:45]

I was pointed to ReceivedBadMessage by mkwst@, who also mentioned that it'd be
fine to simply drop messages if the renderer doesn't expect a response to them.
But if the renderer sends a SwapCompositorFrame message, it also eventually
expects a ReclaimCompositorResources / CompositorFrameAck so would likely get
stuck somewhere down the line (?).

I'm happy either way, but terminating seems like a reasonable & explicit way to
surface that something broke. (Compared to a DLOG of a dropped frame that might
easily be missed?)

I agree that DCHECKs in the senders have better stack traces, as Antoine
mentioned, so I'm adding those, too.

If Antoine still leans towards removing the ReceivedBadMessage given this, I'll
do that. Otherwise let's leave them :)

+    return false;
+  }
+
   // 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_->DidReceiveRendererFrame();
   } else {
@@ skipping 11 matching lines @@
        ++i) {
     rph->OnMessageReceived(*i);
     if (i->dispatch_error())
       rph->OnBadMessageReceived(*i);
   }
   messages_to_deliver_with_frame.clear();
 
   return true;
 }
 
+void RenderWidgetHostImpl::OnBeginFrameDidNotSwap(
+    const cc::BeginFrameAck& ack) {
+  if (ack.sequence_number < cc::BeginFrameArgs::kStartingFrameNumber) {
+    // Received an invalid ack, renderer misbehaved.
+    bad_message::ReceivedBadMessage(
+        GetProcess(), bad_message::RWH_INVALID_BEGIN_FRAME_ACK_DID_NOT_SWAP);

[piman, 2017/03/16 18:32:14]

Ditto, ReceivedBadMessage may be overkill, and just DLOG + return may be enough.

[Eric Seckler, 2017/03/17 11:06:44]

Done.

+    return;
+  }
+
+  // |has_damage| is not transmitted but false by default.
+  DCHECK(!ack.has_damage);
+  // |remaining_frames| is not transmitted, but 0 by default.
+  DCHECK_EQ(0u, ack.remaining_frames);

[dcheng, 2017/03/17 05:54:03]

Will anything be negatively affected if these DCHECKs aren't true? It looks like
this eventually passes to DelegatedFrameHost, which makes a copy of this and
saves it (with potentially unexpected state in either of these two fields).

[Eric Seckler, 2017/03/17 11:06:44]

Yeah, they should actually have these values, otherwise things may not work as
expected. Sounds like we should just be forcing these to the correct values here
then, and not rely on default initialization taking care of this? Done, also in
OnSwapCompositorFrame.

+
+  if (view_)
+    view_->OnBeginFrameDidNotSwap(ack);
+}
+
 void RenderWidgetHostImpl::OnUpdateRect(
     const ViewHostMsg_UpdateRect_Params& params) {
   TRACE_EVENT0("renderer_host", "RenderWidgetHostImpl::OnUpdateRect");
   TimeTicks paint_start = TimeTicks::Now();
 
   // Update our knowledge of the RenderWidget's size.
   current_size_ = params.view_size;
 
   bool is_resize_ack =
       ViewHostMsg_UpdateRect_Flags::is_resize_ack(params.flags);
@@ skipping 677 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