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

content/browser/renderer_host/render_widget_host_view_aura.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_view_aura.h"
 
 #include <set>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 919 matching lines @@
 
     start_edge_top.Scale(viewportToDIPScale);
     start_edge_bottom.Scale(viewportToDIPScale);
     end_edge_top.Scale(viewportToDIPScale);
     end_edge_bottom.Scale(viewportToDIPScale);
 
     selection.start.SetEdge(start_edge_top, start_edge_bottom);
     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() {
   if (delegated_frame_host_)
     delegated_frame_host_->ClearDelegatedFrame();
 }
 
 void RenderWidgetHostViewAura::DidStopFlinging() {
   selection_controller_client_->OnScrollCompleted();
 }
@@ skipping 1412 matching lines @@
 
 void RenderWidgetHostViewAura::SetPopupChild(
     RenderWidgetHostViewAura* popup_child_host_view) {
   popup_child_host_view_ = popup_child_host_view;
   event_handler_->SetPopupChild(
       popup_child_host_view,
       popup_child_host_view ? popup_child_host_view->event_handler() : nullptr);
 }
 
 }  // namespace content