Ensure RenderViewHost is not shut down until a screenshot of the page is taken.

content/browser/renderer_host/render_view_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_view_host_impl.h"
 
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 195 matching lines @@
       has_accessed_initial_document_(false),
       main_frame_routing_id_(main_frame_routing_id),
       run_modal_reply_msg_(NULL),
       run_modal_opener_id_(MSG_ROUTING_NONE),
       is_waiting_for_beforeunload_ack_(false),
       unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
       sudden_termination_allowed_(false),
       render_view_termination_status_(base::TERMINATION_STATUS_STILL_RUNNING),
       virtual_keyboard_requested_(false),
+      copy_requests_(0),
       weak_factory_(this) {
   DCHECK(instance_.get());
   CHECK(delegate_);  // http://crbug.com/82827
 
   GetProcess()->EnableSendQueue();
 
   if (swapped_out) {
     rvh_state_ = STATE_SWAPPED_OUT;
   } else {
     rvh_state_ = STATE_DEFAULT;
@@ skipping 23 matching lines @@
         base::Bind(&ResourceDispatcherHostImpl::OnRenderViewHostDeleted,
                    base::Unretained(ResourceDispatcherHostImpl::Get()),
                    GetProcess()->GetID(), GetRoutingID()));
   }
 
   delegate_->RenderViewDeleted(this);
 
   // Be sure to clean up any leftover state from cross-site requests.
   CrossSiteRequestManager::GetInstance()->SetHasPendingCrossSiteRequest(
       GetProcess()->GetID(), GetRoutingID(), false);
 

[nasko, 2014/03/26 22:48:27]

Since we don't want to delete this object before all the screenshot copies are
done, it will be useful to have a DCHECK here for the copy_requests_ counter.

[mfomitchev, 2014/03/27 21:51:34]

Done.

   // If this was swapped out, it already decremented the active view
   // count of the SiteInstance it belongs to.
   if (IsRVHStateActive(rvh_state_))
     instance_->decrement_active_view_count();
 }
 
 RenderViewHostDelegate* RenderViewHostImpl::GetDelegate() const {
   return delegate_;
 }
 
@@ skipping 367 matching lines @@
 
   switch (rvh_state_) {
     case STATE_WAITING_FOR_UNLOAD_ACK:
       SetState(STATE_WAITING_FOR_COMMIT);
       break;
     case STATE_PENDING_SWAP_OUT:
       SetState(STATE_SWAPPED_OUT);
       break;
     case STATE_PENDING_SHUTDOWN:
       DCHECK(!pending_shutdown_on_swap_out_.is_null());
-      pending_shutdown_on_swap_out_.Run();
+      if (copy_requests_ == 0)
+        pending_shutdown_on_swap_out_.Run();
       break;
     default:
       NOTREACHED();
   }
 }
 
 void RenderViewHostImpl::WasSwappedOut(
     const base::Closure& pending_delete_on_swap_out) {
-  Send(new ViewMsg_WasSwappedOut(GetRoutingID()));
+  if (copy_requests_ == 0)

[nasko, 2014/03/26 22:48:27]

nit: A comment explaining why we aren't sending the IPC message is going to be
very useful.

[mfomitchev, 2014/03/27 21:51:34]

Done.

+    Send(new ViewMsg_WasSwappedOut(GetRoutingID()));
   if (rvh_state_ == STATE_WAITING_FOR_UNLOAD_ACK) {
     SetState(STATE_PENDING_SWAP_OUT);
-    if (!instance_->active_view_count())
+    if (!instance_->active_view_count() ||
+        (instance_->active_view_count() - copy_requests_) == 0) {

[clamy, 2014/03/27 15:59:36]

I don't think this is correct. If you have 2 copy requests and 1 active view for
example, you would get an integer overflow for the check instead of a value of
0.

Additionally, my comment on the decrementation of active_view count still
stands. Let's suppose you have 2 RVH that have the same site instance. The first
one is swapped out while having copy_requests_ != 0 -> active_view_count = 2,
and its corresponding RFH is placed in the map of swapped out hosts. Now the
second one is swapped out before CopyFromBackingStoreCallback is called. ->
active_view_count = 1, its corresponding RFH is placed in the map of swapped out
RFH as well. CopyFromBackingStoreCallback is called -> active_view_count = 0.
But nothing else happens and both RFH (and RVH) remain swapped out when they
should be deleted.

Overall, I would advise against modifying the way the active_view_count is
increased/decreased, as there are many corner cases. In particular, I am not
sure why just putting the check not to execute the deletion callback in
OnSwappedOut (and executing it in CopyFromBackingStoreCallback) not enough.

[mfomitchev, 2014/03/27 21:51:34]

Ok, so the reason we were doing this was that RVH could get SwapOut ACK before
WasSwappedOut was called. Then it would got from STATE_WAITING_FOR_UNLOAD_ACK to
STATE_WAITING_FOR_COMMIT to STATE_SWAPPED_OUT and then it would get deleted in
RFH::ClearSwappedOutRFHsInSiteInstance since the active view count would be 0.
To fix this I changed the flow in WasSwappedOut() to move the host into
STATE_PENDING_SHUTDOWN instead. It seems to be working, can you please take a
look?

       SetPendingShutdown(pending_delete_on_swap_out);
+    }
   } else if (rvh_state_ == STATE_WAITING_FOR_COMMIT) {
     SetState(STATE_SWAPPED_OUT);
   } else if (rvh_state_ == STATE_DEFAULT) {
     // When the RenderView is not live, the RenderFrameHostManager will call
     // CommitPending directly, without calling SwapOut on the old RVH. This will
     // cause WasSwappedOut to be called directly on the live old RVH.
     DCHECK(!IsRenderViewLive());
     SetState(STATE_SWAPPED_OUT);
   } else {
     NOTREACHED();
@@ skipping 1031 matching lines @@
     const GetAudioOutputControllersCallback& callback) const {
   AudioRendererHost* audio_host =
       static_cast<RenderProcessHostImpl*>(GetProcess())->audio_renderer_host();
   audio_host->GetOutputControllers(GetRoutingID(), callback);
 }
 
 void RenderViewHostImpl::ClearFocusedElement() {
   Send(new ViewMsg_ClearFocusedElement(GetRoutingID()));
 }
 
+void RenderViewHostImpl::CopyFromBackingStoreCallback(
+    const base::Callback<void(bool, const SkBitmap&)>& callback,
+    bool success,
+    const SkBitmap& bitmap) {
+  --copy_requests_;
+  callback.Run(success, bitmap);
+  if (copy_requests_ == 0) {
+    if (!IsRVHStateActive(rvh_state_)) {

[nasko, 2014/03/26 22:48:27]

Since we are only trying to delay the shutdown, this check is overly broad. 
Is there a reason why this is separate from the subsequent check for
STATE_PENDING_SHUTDOWN?

[mfomitchev, 2014/03/27 21:51:34]

This looks fine to me, maybe I am missing something?

We know that when the copy request was initiated, RVH was in the active state
(there is a check in CopyFromBackingStore). So it went into the inactive state
when copy_requests_ was greater than 0. This means that WasSwappedOut()  was
called with copy_requests_ > 0. So ViewMsg_WasSwappedOut wasn't sent, so we have
to send it here.

PS: Note I removed decrement_active_count() now.

+      Send(new ViewMsg_WasSwappedOut(GetRoutingID()));
+      instance_->decrement_active_view_count();

[nasko, 2014/03/26 22:48:27]

A comment explaining what we are doing here is definitely needed. It isn't
immediately clear why specifically those two operations are needed.

[mfomitchev, 2014/03/27 21:51:34]

Think it should be more clear now with pending_shutdown_on_copy_requests_done_
check and comments around pending_shutdown_on_copy_requests_done_.

+    }
+
+    if (rvh_state_ == STATE_PENDING_SHUTDOWN) {
+      DCHECK(!pending_shutdown_on_swap_out_.is_null());
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,

[mfomitchev, 2014/03/26 18:59:30]

I am wondering if we can have a situation where this gets executed before
RenderViewHostImpl::OnSwappedOut()? If that happens,
pending_shutdown_on_swap_out_ will be executed twice..

Do we need to protect against this?

[clamy, 2014/03/27 15:59:36]

What pending_shutdown_on_swap_out_ does is remove the corresponding
RenderFrameHost from the map of RenderFrameHost pending deletion. Even if it
gets called twice, it should not be a problem (you just try to remove a value
from a map that is no longer here). Additionally, deleting the RenderFrameHost
should lead to the deletion of the RenderViewHostImpl as well. The only corner
case is when you have two RenderFrameHost pending delete that use the same
RenderViewHost.

The real problem though, IMHO, is that you could delete the RenderViewHostImpl
too soon. This whole mechanism was put in place to ensure the RenderViewHostImpl
would stay in place long enough to execute the unload event. If
CopyFromBackingStoreCallback is called before OnSwappedOut, the RVH gets deleted
immediately, and the unload event does not get to execute correctly.

[mfomitchev, 2014/03/27 21:51:34]

Added pending_shutdown_on_copy_requests_done_ to address this.

+                              pending_shutdown_on_swap_out_);

[nasko, 2014/03/26 22:48:27]

I don't see why we need to delay the shutdown here. It will make lifetime of RVH
quite a bit longer and there isn't clear reason to do so.

[mfomitchev, 2014/03/27 21:51:34]

Done. Sadrul mentioned that he saw something on the stack destroyed when
pending_shutdown_on_swap_out_ was executed which caused a crash, but it seems to
be working fine now.

+    }
+  }
+}
+
+void RenderViewHostImpl::CopyFromBackingStore(
+    const gfx::Rect& src_rect,
+    const gfx::Size& accelerated_dst_size,
+    const base::Callback<void(bool, const SkBitmap&)>& callback,
+    const SkBitmap::Config& bitmap_config) {
+  DCHECK(IsRVHStateActive(rvh_state_));
+  if (!IsRVHStateActive(rvh_state_)) {
+    callback.Run(false, SkBitmap());
+    return;
+  }
+  ++copy_requests_;
+  RenderWidgetHostImpl::CopyFromBackingStore(src_rect, accelerated_dst_size,
+      base::Bind(&RenderViewHostImpl::CopyFromBackingStoreCallback,
+                 weak_factory_.GetWeakPtr(),
+                 callback),
+                 bitmap_config);
+}
+
 void RenderViewHostImpl::Zoom(PageZoom zoom) {
   Send(new ViewMsg_Zoom(GetRoutingID(), zoom));
 }
 
 void RenderViewHostImpl::ReloadFrame() {
   Send(new ViewMsg_ReloadFrame(GetRoutingID()));
 }
 
 void RenderViewHostImpl::DisableScrollbarsForThreshold(const gfx::Size& size) {
   Send(new ViewMsg_DisableScrollbarsForSmallWindows(GetRoutingID(), size));
@@ skipping 173 matching lines @@
                         params.allow_multiple_selection);
   }
 }
 #endif
 
 void RenderViewHostImpl::SetState(RenderViewHostImplState rvh_state) {
   // We update the number of RenderViews in a SiteInstance when the
   // swapped out status of this RenderView gets flipped to/from live.
   if (!IsRVHStateActive(rvh_state_) && IsRVHStateActive(rvh_state))
     instance_->increment_active_view_count();
-  else if (IsRVHStateActive(rvh_state_) && !IsRVHStateActive(rvh_state))
+  else if (IsRVHStateActive(rvh_state_) && !IsRVHStateActive(rvh_state) &&
+           copy_requests_ == 0)
     instance_->decrement_active_view_count();
 
   // Whenever we change the RVH state to and from live or swapped out state, we
   // should not be waiting for beforeunload or unload acks.  We clear them here
   // to be safe, since they can cause navigations to be ignored in OnNavigate.
   if (rvh_state == STATE_DEFAULT ||
       rvh_state == STATE_SWAPPED_OUT ||
       rvh_state_ == STATE_DEFAULT ||
       rvh_state_ == STATE_SWAPPED_OUT) {
     is_waiting_for_beforeunload_ack_ = false;
@@ skipping 16 matching lines @@
   return true;
 }
 
 void RenderViewHostImpl::AttachToFrameTree() {
   FrameTree* frame_tree = delegate_->GetFrameTree();
 
   frame_tree->ResetForMainFrameSwap();
 }
 
 }  // namespace content