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

content/browser/renderer_host/render_widget_host_view_aura.cc

Index: content/browser/renderer_host/render_widget_host_view_aura.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index 732394d07ba0b1771d60a1c392333d15f63d27a2..97c92a2220a00c95ba8258e48cb53031081dd976 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -937,12 +937,34 @@ void RenderWidgetHostViewAura::OnSwapCompositorFrame(
     selection.end.SetEdge(end_edge_top, end_edge_bottom);
   }
 
+  // |has_damage| is not transmitted.
+  frame.metadata.begin_frame_ack.has_damage = true;

[piman, 2017/03/15 21:17:10]

nit: you could do this in RenderWidgetHostImpl, so that it applies to all
platforms. Arguably same with the checks below.

[Eric Seckler, 2017/03/16 10:24:14]

Moved all to RWHI.

+  cc::BeginFrameAck ack(frame.metadata.begin_frame_ack);
+  DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);

[piman, 2017/03/15 21:17:10]

What makes this DCHECK true, if ack comes (after a copy) from an untrusted
renderer?
We can have that DCHECK on the sender side, but maybe we need to refuse/discard
the message if it doesn't satisfy this condition?

[Eric Seckler, 2017/03/16 10:24:15]

Replaced it with a ReceivedBadMessage, assuming this is more sensible?

The reason I was simply DCHECKing is that I was under the impression that it's
fine to DCHECK, as opposed to CHECK, for validity (so that bad input is flagged
on the bots) provided that you also safely handle / throw out this input in
production. Since the compositor code ignores bad BeginFrameAcks, that would
have been the case here.

I'm still not entirely clear whether such DCHECKing would be fine, since [1]
only mentions that "DCHECK is acceptable", but doesn't explain this further...

Anyway, I'm also happy with ReceivedBadMessage :)

[1]
https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

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

See comments in RWHI.
That page explicitly says "Avoid browser side DCHECKs and WebKit ASSERTs that
fail with invalid input from the renderer. Such conditions should be explicitly
checked instead. Otherwise the macros lead to false sense of security since they
aren't compiled into release builds."

From my POV, the DCHECK is actively harmful, because as a reader of the code, I
take the DCHECK as a guarantee that this can't happen. That is, it makes me
think that some other code (e.g. the struct traits) made sure to fail
deserialization if this condition could happen. In turn I may start writing code
that depends on this being true, causing security issues. Separately, the DCHECK
also prevents attempts at testing that the browser does behave correctly if
given bad input, since such a test would assert in debug.

OTOH, when possible, a DCHECK on the sender side is a good thing to have, it
won't prevent security issues, but it can help debugging, because it is closer
to the code that made the mistake, and can even give you a useful stack trace
(this one wouldn't).

+  // |remaining_frames| is not transmitted, but 0 by default.
+  DCHECK_EQ(0u, ack.remaining_frames);
+
   if (delegated_frame_host_) {
     delegated_frame_host_->SwapDelegatedFrame(compositor_frame_sink_id,
                                               std::move(frame));
   }
   selection_controller_->OnSelectionBoundsChanged(selection.start,
                                                   selection.end);
+
+  if (begin_frame_source_)
+    begin_frame_source_->DidFinishFrame(this, ack);
+}
+
+void RenderWidgetHostViewAura::OnBeginFrameDidNotSwap(
+    const cc::BeginFrameAck& ack) {
+  DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);

[piman, 2017/03/15 21:17:10]

Ditto here.

[Eric Seckler, 2017/03/16 10:24:15]

Done, moved as well.

+  // |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);
+
+  if (begin_frame_source_)
+    begin_frame_source_->DidFinishFrame(this, ack);
 }
 
 void RenderWidgetHostViewAura::ClearCompositorFrame() {