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

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 175f1687e6f05778a8a79f281bf0831e8737ec5e..c54d7c072417f4a945a389ce7193cd52ca7c7a4b 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -553,6 +553,8 @@ bool RenderWidgetHostImpl::OnMessageReceived(const IPC::Message &msg) {
     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,
@@ -1838,6 +1840,20 @@ bool RenderWidgetHostImpl::OnSwapCompositorFrame(
   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
@@ -1869,6 +1885,24 @@ bool RenderWidgetHostImpl::OnSwapCompositorFrame(
   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");