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

Created:
6 years, 9 months ago by mfomitchev

Modified:
6 years, 9 months ago

Reviewers:
Charlie Reis, sadrul, clamy, nasko

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Ensure RenderViewHost is not shut down until a screenshot of the page is taken. Continuation of the work started in issue 178843004. See https://chromiumcodereview.appspot.com/178843004/ =============================================== Pasted description from 178843004: [wip] Add some more state to RenderViewHost's lifetime. Keep track of in-flight copy-requests for a RenderViewHost. If a RenderViewHost has pending copy-requests, then: * Do not decrement active-view-count on the SiteInstance when the RenderViewHost becomes inactive. * Hold back sending was-swapped-out IPC to the renderer. * Do not run the shutdown-on-swap callback. When all the copy-requests are completed in the RenderViewHost: * Check if the current state of the RenderViewHost is not active anymore. If it's not active, then send the was-swapped-out message. This should work because new copy-requests are not made if the RenderViewHost isn't in an active state. * If the current state is not active, then also decrement the active-view count on the SiteInstance. * If the state is in a pending-shutdown state, then run the callback. BUG==340682

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+139 lines, -16 lines)			Patch
M	chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc	View	1 2	3 chunks	+29 lines, -1 line	0 comments	Download
M	content/browser/frame_host/navigation_entry_screenshot_manager.cc	View	1 2	3 chunks	+7 lines, -7 lines	0 comments	Download
M	content/browser/frame_host/render_frame_host_manager.cc	View	1 2	2 chunks	+8 lines, -2 lines	0 comments	Download
M	content/browser/renderer_host/render_view_host_impl.h	View	1 2	3 chunks	+23 lines, -2 lines	0 comments	Download
M	content/browser/renderer_host/render_view_host_impl.cc	View	1 2	5 chunks	+72 lines, -4 lines	2 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

mfomitchev

creis@, clamy@ - this is the continuation of the work on the fix for cross-site ...

6 years, 9 months ago (2014-03-26 18:59:30 UTC) #1

Charlie Reis

Nasko: Can you take a first look at this for me, since you've also been ...

6 years, 9 months ago (2014-03-26 19:26:01 UTC) #2

nasko

I think the CL needs a few changes, but overall it is a good start. ...

6 years, 9 months ago (2014-03-26 22:48:27 UTC) #3

clamy

Thanks! There are some corner cases that need to be addressed, and may require some ...

6 years, 9 months ago (2014-03-27 15:59:36 UTC) #4

Thanks! There are some corner cases that need to be addressed, and may require
some redesign. In particular, I am not sure the way you handle the modification
of active_view_count is right.

https://chromiumcodereview.appspot.com/213413005/diff/20001/content/browser/r...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/213413005/diff/20001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:663:
(instance_->active_view_count() - copy_requests_) == 0) {
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.

https://chromiumcodereview.appspot.com/213413005/diff/20001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:1731:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2014/03/26 18:59:30, mfomitchev wrote:
> 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?

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

I made a significant change in WasSwappedOut() logic: RVH now moves from STATE_WAITING_FOR_COMMIT to STATE_PENDING_SHUTDOWN ...

6 years, 9 months ago (2014-03-27 21:51:33 UTC) #5

mfomitchev

Looks like I picked up some changes from other CLs here when I rebased. Sorry ...

6 years, 9 months ago (2014-03-27 22:06:28 UTC) #6

clamy

Thanks! It does look better, but I am afraid that adding more booleans to RenderViewHostImpl ...

6 years, 9 months ago (2014-03-28 11:03:39 UTC) #7

Thanks! It does look better, but I am afraid that adding more booleans to
RenderViewHostImpl is not going to address all corner cases that could be
encountered. Instead, I would suggest you expend the number of states in
RenderViewHostImplState. It does sound ugly at first, but I believe it would be
more robust.

What I would propose is adding PENDING_SHUTDOWN_COPY_REQUEST and
PENDING_SWAP_OUT_COPY_REQUEST. In these state, the RVH has already received the
swap out ack, but is waiting for outstanding copy requests.

Then in RenderViewHostImpl:
- in OnSwappedOut:
switch (rvh_state_) {
  case STATE_WAITING_FOR_UNLOAD_ACK:
    SetState(STATE_WAITING_FOR_COMMIT);
    break;
  case STATE_PENDING_SWAP_OUT:
    SetState(copy_requests_ ?
        STATE_PENDING_SWAP_OUT_COPY_REQUESTS :
        STATE_SWAPPED_OUT);
    break;
  case STATE_PENDING_SHUTDOWN:
    DCHECK(!pending_shutdown_on_swap_out_.is_null());
    if (copy_requests_)
      SetState(STATE_PENDING_SHUTDOWN_COPY_REQUESTS);
    else
      pending_shutdown_on_swap_out_.Run();
    break;
  default:
    NOTREACHED();
}

- in WasSwappedOut:
if (rvh_state_ == STATE_WAITING_FOR_UNLOAD_ACK) {
  SetState(STATE_PENDING_SWAP_OUT);
  if (!instance_->active_view_count())
    SetPendingShutdown(pending_delete_on_swap_out);
} else if (rvh_state_ == STATE_WAITING_FOR_COMMIT) {
  if (copy_requests_) {
  SetState(STATE_PENDING_SWAP_OUT_COPY_REQUESTS);
  if (!instance_->active_view_count())
    // You would have to modify SetPendingShutdown so that
    // the state gets to
    // STATE_PENDING_SHUTDOWN_REQUESTS here.
    SetPendingShutdown(pending_delete_on_swap_out, true);
  } else {
    SetState(STATE_SWAPPED_OUT);
  }
}

- in CopyFromBackingStoreCallback:
if (copy_requests_ == 0) {
  ...
  if (rvh_state_ == STATE_PENDING_SHUTDOWN_COPY_REQUEST) {
    DCHECK(!pending_shutdown_on_swap_out_.is_null());
    pending_shutdown_on_swap_out_.Run();
  } else if (rvh_state_ == STATE_PENDING_SWAP_OUT_COPY_REQUEST) {
    SetState(STATE_SWAPPED_OUT);
  }
}

You would also need to modify SetPendingShutdown to take a boolean to know
whether you are only waiting for copy requests or not. IsRVHStateActive would
need to be modified as well so that the two new states are considered as not
active.

In RenderFrameHostManager, you would need to modify
ClearSwappedOutRFHsInSiteInstance so that when it encounters a RFH with a RVH in
STATE_PENDING_SWAP_OUT_COPY_REQUESTS in the map of swap out hosts, it will put
the RFH in the map of hosts pending delete, and set the state of the RVH to
STATE_PENDING_DELETE_COPY_REQUESTS.

WDYT?

https://chromiumcodereview.appspot.com/213413005/diff/40001/content/browser/r...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/213413005/diff/40001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:654:
SetState(STATE_SWAPPED_OUT);
When the RVH is set to SWAPPED_OUT here, it may be destroyed if the last active
RVH of its site instance goes away, event if it still has outstanding copy
requests.

https://chromiumcodereview.appspot.com/213413005/diff/40001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:690:
SetPendingShutdown(pending_delete_on_swap_out);
There is still a problem with your solution here. Let's suppose that we have
more than one active_view_count in the SiteInstance. If one of them is swapped
out and hits this path, it will be set to PENDING_SHUTDOWN. And there is no way
to get back from PENDING_SHUTDOWN to SWAPPED_OUT, which is the state the RVH
should end up in when copy_requests_ goes down to 0. Not to mention that the
corresponding RFH has been taken out of the map of swapped out hosts.

Charlie Reis

I haven't had the time to dive into this as deeply as I'd like due ...

6 years, 9 months ago (2014-03-28 17:12:29 UTC) #8

mfomitchev

6 years, 9 months ago (2014-03-28 17:50:12 UTC) #9

Thanks for the prompt feedback. 

1) I can certainly produce a design doc or edit the existing one
(https://docs.google.com/a/google.com/document/d/1CW3NZH8ZR6tCA7ick4d3O4exAjIw...).
That said, it sounds like we need to agree on whether the best approach is
adding new states or modifying the active view count. Can we have that
discussion without the design doc or we need the design doc first?

2) Assuming we want this to work with the new design, perhaps it would help to
get this straightened up before you guys make the jump? Otherwise aren't we
risking missing something important in the new design?

3) IIRC the issue is that the textures in the browser process will only contain
what's visible. If the browser window is partially off screen for example when
the screenshot is taken, the off screen portion won't be captured. If the
browser window is later shifted, this would present a problem. sadrul@ can
correct me if I am inaccurate on this one.

There's clearly no hope of getting this in for M35, so I will hold off with
further submissions in this review until we can figure some of these things out.

Expand Messages | Collapse Messages