[wip] Introduce SiteInstanceKeepAlive.

gesture-nav: Add mechanism to fix taking screenshot after cross-process navigation.

The screenshot capture code needs a SiteInstance (and its corresponding
RenderProcessHost) to stay alive for long enough for the capture to succeed. So
increment the active-view count on the SiteInstanceImpl and the pending view count
on its RenderProcessHost when taking the screenshot, and reduce the counts after
the taking the screenshot is complete.

BUG=340682

[sadrul, 2014-02-14 13:20:51]

Hi Charlie. What do you think of this approach to fix taking screenshot for
cross-process/cross-site navigations?

Alternate approaches I considered:
 * Use the same SiteInstanceKeepAlive as in this patch, but use it in
RenderViewHostImpl::CopyFromBackingStore() override, instead of in
NavigationEntryScreenshotManager.

 * Use separate keep-alive objects for the SiteInstanceImpl and
RenderProcessHostImpl.

 * Introduce a new RenderViewHostImplState (e.g.
STATE_PENDING_SHUTDOWN_AFTER_SCREENSHOT). But this looks like a more involved
solution that can affect a larger number of places.

What do you think? Do you have other proposals we could try out? If the approach
in this patch looks reasonable, I will add test coverage for it (and clean up
the implementation a bit more).

Thanks!

https://codereview.chromium.org/166533002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/166533002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1162:
ClearSwappedOutRFHsInSiteInstance(site_instance_id, frame_tree_node_);
I removed this line when making this a static function. It feels like the loop
below should take care of this, but I don't know this code well enough to be
sure.

[mfomitchev, 2014-02-14 16:21:35]

https://codereview.chromium.org/166533002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/166533002/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:140:
scoped_ptr<SiteInstanceKeepAlive> keep_alive(
Why not just keep_alive = site_instance->AcquireKeepAlive()? AcquireKeepAlive()
already returns a scoped_ptr..

https://codereview.chromium.org/166533002/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:158:
scoped_ptr<SiteInstanceKeepAlive> site_instance,
rename this to keep_alive?

https://codereview.chromium.org/166533002/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:161:
site_instance.reset();
The biggest question for me is whether this will guarantee that everything will
eventually get destroyed properly. I played around with this fix a bit, and RWH
and RFH don't seem to get destroyed after the navigation completes, at least not
right away.

https://codereview.chromium.org/166533002/diff/100001/content/browser/site_in...
File content/browser/site_instance_impl.h (right):

https://codereview.chromium.org/166533002/diff/100001/content/browser/site_in...
content/browser/site_instance_impl.h:132: explicit
SiteInstanceKeepAlive(scoped_refptr<SiteInstanceImpl> instance);
Perhaps we should make this constructor non-puiblic and force clients to use
AcquireKeepAlive() instead?

[Charlie Reis, 2014-02-15 01:57:46]

Sorry for the delayed reply-- I got hit with a stable blocker bug today.

I don't think this is a good idea.  RenderViewHosts can be deleted independently
of the rest of their SiteInstance, so keeping the SiteInstance alive won't
necessarily help.

It also doesn't solve the problem I discussed with mfomitchev@: during a
cross-process navigation, RenderViewHost::SwapOut sends a message to the old
RenderView telling it to navigate to a blank swappedout:// page.  That happens
before commit, meaning there's a race where you could end up taking a screenshot
of a blank page.

So far this seems like a fundamental problem to me until we replace the
swappedout:// logic with RenderFrameProxy.  (Several people on my team are
working on that this quarter, but it's a big project).

Barring that, it seems like we would have to defer the SwapOut message until the
screenshot is taken.  I'm nervous about what that would do to the cross-process
navigation timeline, though I suppose clamy@ has just made SwapOut run in the
background anyway.  Maybe there's a way forward there if we plan it out
carefully-- I would recommend discussing this first over an informal design doc
before diving into code.

[mfomitchev, 2014-02-19 23:24:04]

Hi Charlie,

We had some offline discussions about this. One important point is that I think
we are okay if the screenshot isn't captured from time to time as long is that
is rare and is handled gracefully (i.e. default "no screenshot" treatment). The
logic is:
- There's a limit on the number of screenshots we store, so users can already
occasionally run into this, and that's fine.
- Having it happen rarely is much better than having it happen every time with
cross-side nav as it does now.
- Ideally we'd like to avoid investing too heavily in the intermediate solution
- it sounds like the ideal solution would be different once the RenderFrameProxy
is in place.

Re swappedout:// page:
Sadrul believes this had been fixed and shouldn't be a problem:
https://codereview.chromium.org/11776010. The change in render_widget.cc stops a
swappedout:// page from painting anything. So in the worst case, we'd get no
screenshot (not a screenshot of a blank page), but in practice I am not sure if
even that is happening. I couldn't make it happen.

RenderViewHosts getting deleted independently of SiteInstance: Wouldn't the
pending view added on the RenderProcessHost by SiteInstanceKeepAlive keep the
process host and the view host alive? If we only incremented the count on the
SiteInstance, RVH would get deleted via
RenderProcessHostImpl::OnShutdownRequest(), but having a pending view prevents
that. Can RenderViewHosts be deleted via another code path?

Thanks!

[Charlie Reis, 2014-02-20 02:05:25]

On 2014/02/19 23:24:04, mfomitchev wrote:
> Hi Charlie,
> 
> We had some offline discussions about this. One important point is that I
think
> we are okay if the screenshot isn't captured from time to time as long is that
> is rare and is handled gracefully (i.e. default "no screenshot" treatment).
The
> logic is:
> - There's a limit on the number of screenshots we store, so users can already
> occasionally run into this, and that's fine.
> - Having it happen rarely is much better than having it happen every time with
> cross-side nav as it does now.
> - Ideally we'd like to avoid investing too heavily in the intermediate
solution
> - it sounds like the ideal solution would be different once the
RenderFrameProxy
> is in place.

This much makes sense to me.  We can find an interim solution that is better
than the status quo, until RenderFrameProxy is ready.

> Re swappedout:// page:
> Sadrul believes this had been fixed and shouldn't be a problem:
> https://codereview.chromium.org/11776010. The change in render_widget.cc stops
a
> swappedout:// page from painting anything. So in the worst case, we'd get no
> screenshot (not a screenshot of a blank page), but in practice I am not sure
if
> even that is happening. I couldn't make it happen.

Ah yes, I'd forgotten about that change.  Ok, that clears some things up.

> RenderViewHosts getting deleted independently of SiteInstance: Wouldn't the
> pending view added on the RenderProcessHost by SiteInstanceKeepAlive keep the
> process host and the view host alive? If we only incremented the count on the
> SiteInstance, RVH would get deleted via
> RenderProcessHostImpl::OnShutdownRequest(), but having a pending view prevents
> that. Can RenderViewHosts be deleted via another code path?
> 
> Thanks!

A RenderViewHost can be deleted if its tab is closed, which might not delete the
SiteInstance.  However, I see that doesn't matter too much for the screenshot
case (which should fail gracefully if the tab is closed), and I see that you're
trying to keep the swapped out RVH alive by pretending that the SiteInstance and
process are still being used.  I suppose that could work.

I still do not want to expose this as an abstraction on SiteInstance.  It
encourages other callers to try to keep SiteInstances around longer than they
should be, and it can have unexpected consequences if used out of context (e.g.,
the GetProcess() call creates a process if one isn't running, might
unintentionally leave zombie processes around, etc).  Is it possible to do the
increment_active_view_count() + AddPendingView() trick in the screenshot classes
(in a way that guarantees the decrement), without changing SiteInstance?  I'd
prefer to keep this behavior limited to that corner case until RenderFrameProxy
is ready to go.

Thanks,
Charlie

[sadrul, 2014-02-20 05:13:20]

On 2014/02/20 02:05:25, Charlie Reis wrote:
> On 2014/02/19 23:24:04, mfomitchev wrote:
> > Hi Charlie,
> > 
> > We had some offline discussions about this. One important point is that I
> think
> > we are okay if the screenshot isn't captured from time to time as long is
that
> > is rare and is handled gracefully (i.e. default "no screenshot" treatment).
> The
> > logic is:
> > - There's a limit on the number of screenshots we store, so users can
already
> > occasionally run into this, and that's fine.
> > - Having it happen rarely is much better than having it happen every time
with
> > cross-side nav as it does now.
> > - Ideally we'd like to avoid investing too heavily in the intermediate
> solution
> > - it sounds like the ideal solution would be different once the
> RenderFrameProxy
> > is in place.
> 
> This much makes sense to me.  We can find an interim solution that is better
> than the status quo, until RenderFrameProxy is ready.
> 
> > Re swappedout:// page:
> > Sadrul believes this had been fixed and shouldn't be a problem:
> > https://codereview.chromium.org/11776010. The change in render_widget.cc
stops
> a
> > swappedout:// page from painting anything. So in the worst case, we'd get no
> > screenshot (not a screenshot of a blank page), but in practice I am not sure
> if
> > even that is happening. I couldn't make it happen.
> 
> Ah yes, I'd forgotten about that change.  Ok, that clears some things up.
> 
> > RenderViewHosts getting deleted independently of SiteInstance: Wouldn't the
> > pending view added on the RenderProcessHost by SiteInstanceKeepAlive keep
the
> > process host and the view host alive? If we only incremented the count on
the
> > SiteInstance, RVH would get deleted via
> > RenderProcessHostImpl::OnShutdownRequest(), but having a pending view
prevents
> > that. Can RenderViewHosts be deleted via another code path?
> > 
> > Thanks!
> 
> A RenderViewHost can be deleted if its tab is closed, which might not delete
the
> SiteInstance.  However, I see that doesn't matter too much for the screenshot
> case (which should fail gracefully if the tab is closed), and I see that
you're
> trying to keep the swapped out RVH alive by pretending that the SiteInstance
and
> process are still being used.  I suppose that could work.
> 
> I still do not want to expose this as an abstraction on SiteInstance.  It
> encourages other callers to try to keep SiteInstances around longer than they
> should be, and it can have unexpected consequences if used out of context
(e.g.,
> the GetProcess() call creates a process if one isn't running, might
> unintentionally leave zombie processes around, etc).  Is it possible to do the
> increment_active_view_count() + AddPendingView() trick in the screenshot
classes
> (in a way that guarantees the decrement), without changing SiteInstance?  I'd
> prefer to keep this behavior limited to that corner case until
RenderFrameProxy
> is ready to go.

Thanks for the detailed reply!

I have moved over all of the code into NavigationEntryScreenshotManager, so it's
not exposed to outside world. PTAL.

[sadrul, 2014-02-20 05:13:30]

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:92:
instance_->decrement_active_view_count();
Do we need to do anything here to make sure the SiteInstance/RenderProcessHost
gets destroyed if a previous attempt to destroy them was avoided because of this
keep-alive object?

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.h (right):

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.h:59: void
OnScreenshotTakenInTest(int unique_id,
This is to avoid exposing SiteInstanceKeepAlive out of
NavigationEntryScreenshotManager

[Charlie Reis, 2014-02-20 19:01:52]

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:79: // A
temporary solution to keep a RenderViewHost and SiteInstanceImpl live
Please add a TODO saying when this can be removed (when RenderFrameProxy
replaces swappedout://).

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:92:
instance_->decrement_active_view_count();
On 2014/02/20 05:13:31, sadrul wrote:
> Do we need to do anything here to make sure the SiteInstance/RenderProcessHost
> gets destroyed if a previous attempt to destroy them was avoided because of
this
> keep-alive object?

Those methods don't do anything on their own, so yes.  I would recommend adding
a test for this, since this is very subtle behavior.

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:166:
base::Bind(&NavigationEntryScreenshotManager::OnScreenshotTaken,
What happens if we never make it to OnScreenshotTaken?  Is that possible?  Will
the process ever go away, or is it stuck as a zombie in that case?

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:169:
base::Passed(&keep_alive)));
Albert, can I get your eyes on this?

[mfomitchev, 2014-02-20 19:15:47]

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:200:
OnScreenshotSet(entry);
We should get rid of this and just have ClearScreenshot(entry) to fix the unit
test

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:253: if
(entry->screenshot().get())
I think this check is unnecessary if we make the fix in OnScreenshotTaken() - if
OnScreenshotSet() is called, the screenshot should be non-null

[awong, 2014-02-20 22:44:31]

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:169:
base::Passed(&keep_alive)));
On 2014/02/20 19:01:53, Charlie Reis wrote:
> Albert, can I get your eyes on this?

Seems legit, but using a raw pointer (as in the later patch set) also seems
legit.

[sadrul, 2014-02-21 03:51:16]

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:79: // A
temporary solution to keep a RenderViewHost and SiteInstanceImpl live
On 2014/02/20 19:01:53, Charlie Reis wrote:
> Please add a TODO saying when this can be removed (when RenderFrameProxy
> replaces swappedout://).

Done (is there a crbug I can reference from the TODO?)

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:92:
instance_->decrement_active_view_count();
On 2014/02/20 19:01:53, Charlie Reis wrote:
> On 2014/02/20 05:13:31, sadrul wrote:
> > Do we need to do anything here to make sure the
SiteInstance/RenderProcessHost
> > gets destroyed if a previous attempt to destroy them was avoided because of
> this
> > keep-alive object?
> 
> Those methods don't do anything on their own, so yes.  I would recommend
adding
> a test for this, since this is very subtle behavior.

I have exposed RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance()
as a public static function, and called it from here if
SiteInstanceImpl::active_view_count() becomes zero after dcrementing it here. I
believe this makes sure the SiteInstanceImpl is destroyed if it needs to be.

Now, I am also depending on this to clean up the RenderProcessHosts as needed. I
added some code to a browser-test
(WebContentsViewAuraTest.ScreenshotForSwappedOutRenderViews) that depends on
this, and it does seem to work OK. But is that always expected to cleanup
RenderProcessHosts like this? If not, should we do something more explicit (like
call into RenderProcessHostImpl::OnShutdownRequest(), or something less ugly)?

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:166:
base::Bind(&NavigationEntryScreenshotManager::OnScreenshotTaken,
On 2014/02/20 19:01:53, Charlie Reis wrote:
> What happens if we never make it to OnScreenshotTaken?  Is that possible? 
Will
> the process ever go away, or is it stuck as a zombie in that case?

CopyFromBackingStore callback is guaranteed to be called back. The possible
exception is during browser shutdown. So there's no risk of having zombies.

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:169:
base::Passed(&keep_alive)));
On 2014/02/20 19:01:53, Charlie Reis wrote:
> Albert, can I get your eyes on this?

I have changed the way the keep-alive object is sent to the callback.

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:200:
OnScreenshotSet(entry);
On 2014/02/20 19:15:48, mfomitchev wrote:
> We should get rid of this and just have ClearScreenshot(entry) to fix the unit
> test

Done.

https://codereview.chromium.org/166533002/diff/300001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:253: if
(entry->screenshot().get())
On 2014/02/20 19:15:48, mfomitchev wrote:
> I think this check is unnecessary if we make the fix in OnScreenshotTaken() -
if
> OnScreenshotSet() is called, the screenshot should be non-null

Done.

https://codereview.chromium.org/166533002/diff/540001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/166533002/diff/540001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1173:
ClearSwappedOutRFHsInSiteInstance(site_instance_id, frame_tree_node_);
This line is removed from the static version. From reading the code below, it
looks like this will be taken care of from inside the loop. But I don't know
this for sure. Can you confirm? (is there something I can look at to verify for
myself)?