Discard compositor frames from unloaded web content

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 0f488d8d88c866c07413ade08acdc0fc1ff70551..3bfb7120ad6182d3572b9699e90ed117fae247a6 100644
--- a/content/browser/renderer_host/render_widget_host_impl.cc
+++ b/content/browser/renderer_host/render_widget_host_impl.cc
@@ -294,6 +294,7 @@ RenderWidgetHostImpl::RenderWidgetHostImpl(RenderWidgetHostDelegate* delegate,
       last_event_type_(blink::WebInputEvent::Undefined),
       new_content_rendering_delay_(
           base::TimeDelta::FromMilliseconds(kNewContentRenderingDelayMs)),
+      current_content_source_id_(0),
       weak_factory_(this) {
   CHECK(delegate_);
   CHECK_NE(MSG_ROUTING_NONE, routing_id_);
@@ -1010,7 +1011,12 @@ void RenderWidgetHostImpl::StopHangMonitorTimeout() {
   RendererIsResponsive();
 }
 
-void RenderWidgetHostImpl::StartNewContentRenderingTimeout() {
+void RenderWidgetHostImpl::StartNewContentRenderingTimeout(
+    uint32_t next_source_id) {
+  // current_content_source_id_ should be non-decreasing, but it is hard to

[enne (OOO), 2017/02/28 19:02:23]

Ignoring unsigned overflow here, how can raciness cause this to happen? It seems
like even with fast navigations, all of those did commit provisional loads will
happen in order.  Why not just always set current_source_id_ to next_source_id_
here?

[kenrb, 2017/02/28 22:18:44]

Done. You're right, I think I was being overly conservative, and it doesn't seem
like there is a way that these should get received out of order.

+  // guarantee that against raciness from a fast series of navigations.
+  if (current_content_source_id_ < next_source_id || next_source_id == 0)
+    current_content_source_id_ = next_source_id;
   // It is possible for a compositor frame to arrive before the browser is
   // notified about the page being committed, in which case no timer is
   // necessary.
@@ -1842,6 +1848,10 @@ bool RenderWidgetHostImpl::OnSwapCompositorFrame(
   std::vector<IPC::Message> messages_to_deliver_with_frame;
   messages_to_deliver_with_frame.swap(std::get<2>(param));
 
+  // Ignore this frame if its content has already been unloaded.

[enne (OOO), 2017/02/28 19:02:23]

I wonder if this early out needs to go after some of the things below.  In
particular, do the latency info, latency tracker, and reclaimed resources all
need to still happen?

Maybe this should be treated as if there's no view_ but otherwise similarly? 
I'm not 100% sure, but am just being skeptical about the placement of this
return.

[kenrb, 2017/02/28 22:18:44]

I've moved it after the latency tracking, but for the rest I feel like it is
better to early abort. The renderer is still alive and ostensibly trying to
produce compositor frames for a new page, and this frame shouldn't have any
relevance. In the specific example I am trying to prevent, this compositor frame
has been sent once before, but is being sent again because the tab was hidden
and then re-shown. In that case we are waiting for a new frame, but this isn't
the one we want.

Of course, I can change it if there is a specific reason to continue to further
down the method.

+  if (frame.metadata.content_source_id < current_content_source_id_)
+    return false;
+
   if (!ui::LatencyInfo::Verify(frame.metadata.latency_info,
                                "RenderWidgetHostImpl::OnSwapCompositorFrame")) {
     std::vector<ui::LatencyInfo>().swap(frame.metadata.latency_info);