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

content/browser/renderer_host/render_view_host_impl.cc

Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index b9be7b032dcabeef778a274ec66878a06b339e7e..27a678463ec0fd64746a09107394d0eea55a4216 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -213,6 +213,7 @@ RenderViewHostImpl::RenderViewHostImpl(
       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
@@ -644,7 +645,8 @@ void RenderViewHostImpl::OnSwappedOut(bool timed_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();
@@ -653,11 +655,14 @@ void RenderViewHostImpl::OnSwappedOut(bool timed_out) {
 
 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) {
@@ -1709,6 +1714,44 @@ 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));
 }
@@ -1902,7 +1945,8 @@ void RenderViewHostImpl::SetState(RenderViewHostImplState rvh_state) {
   // 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