Discard compositor frames from unloaded web content

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 276 matching lines @@
       owned_by_render_frame_host_(false),
       is_focused_(false),
       hung_renderer_delay_(
           base::TimeDelta::FromMilliseconds(kHungRendererDelayMs)),
       hang_monitor_reason_(
           RendererUnresponsiveType::RENDERER_UNRESPONSIVE_UNKNOWN),
       hang_monitor_event_type_(blink::WebInputEvent::Undefined),
       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_);
   latency_tracker_.SetDelegate(delegate_);
 
 #if defined(OS_WIN)
   // Update the display color profile cache so that it is likely to be up to
   // date when the renderer process requests the color profile.
   if (gfx::ICCProfile::CachedProfilesNeedUpdate()) {
     base::PostTaskWithTraits(
@@ skipping 696 matching lines @@
 
 void RenderWidgetHostImpl::StopHangMonitorTimeout() {
   if (hang_monitor_timeout_) {
     hang_monitor_timeout_->Stop();
     hang_monitor_reason_ =
         RendererUnresponsiveType::RENDERER_UNRESPONSIVE_UNKNOWN;
   }
   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.
   if (received_paint_after_load_) {
     received_paint_after_load_ = false;
     return;
   }
 
   new_content_rendering_timeout_->Start(new_content_rendering_delay_);
 }
@@ skipping 811 matching lines @@
   TRACE_EVENT0("test_fps,benchmark", "OnSwapCompositorFrame");
 
   ViewHostMsg_SwapCompositorFrame::Param param;
   if (!ViewHostMsg_SwapCompositorFrame::Read(&message, &param))
     return false;
   cc::CompositorFrame frame(std::move(std::get<1>(param)));
   uint32_t compositor_frame_sink_id = std::get<0>(param);
   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);
   }
 
   latency_tracker_.OnSwapCompositorFrame(&frame.metadata.latency_info);
 
   bool is_mobile_optimized = IsMobileOptimizedFrame(frame.metadata);
   input_router_->NotifySiteIsMobileOptimized(is_mobile_optimized);
   if (touch_emulator_)
@@ skipping 675 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