overscroll: Take a screenshot at more appropriate times.

overscroll: Make sure screenshot is taken correctly for swapped out RenderViews.

When a RenderView is swapped out, it navigates to a swapped-out url
(swappedout://), and thus corrupts the paint content of the actual view. To fix 
this issue, prevent a swapped-out RenderView from painting. Also, make sure the 
screenshot is taken in the browser side before the RenderViewHost is swapped 
out.

BUG=160668
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175995

[sadrul, 2013-01-07 16:11:54]

(To refresh the memory after the break: the original CL that introduced this
was: https://codereview.chromium.org/11635059)

[Charlie Reis, 2013-01-07 19:50:23]

https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:140: // Returns
whether a screenshot should be taken because of the page transition.
What's the intention here?  You're returning no for some known cases and yes for
the rest, so it's not clear what cases you're looking for.  It's also fragile,
if additional transition types are added, so a whitelist would be better if
possible.

https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:1576: else if
(reload_type == NO_RELOAD)
We should put a return after DiscardNonCommittedEntries and skip the "else". 
The existing logic below won't run in that case anyway, since pending_entry_
will be null.

Aside from that, I don't think this is the right place to take the screenshot. 
The navigation might never commit (making it unnecessary), and a large amount of
time could pass before it does (making it possibly stale).

It seems like you want to take the screenshot just before the previous page
tries to unload.  That would be in RenderViewHost::SwapOut, which gets called by
RVHM::OnCrossSiteResponse.

Either way, can you explain why ShouldTakeScreenshot matters in the other case
but not this one?

[sadrul, 2013-01-08 00:16:29]

https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:1576: else if
(reload_type == NO_RELOAD)
On 2013/01/07 19:50:23, creis wrote:
> We should put a return after DiscardNonCommittedEntries and skip the "else". 
> The existing logic below won't run in that case anyway, since pending_entry_
> will be null.
> 
> Aside from that, I don't think this is the right place to take the screenshot.

> The navigation might never commit (making it unnecessary), and a large amount
of
> time could pass before it does (making it possibly stale).
> 
> It seems like you want to take the screenshot just before the previous page
> tries to unload.  That would be in RenderViewHost::SwapOut, which gets called
by
> RVHM::OnCrossSiteResponse.
> 
> Either way, can you explain why ShouldTakeScreenshot matters in the other case
> but not this one?

The issue here is that, in some cases (especially when the navigation happens
because of a user action in the UI, e.g. entering a URL in the omnibox, or
clicking a bookmark button), we want to take the screenshot before the
RenderView is swapped out. So I do that here. But as you point it, this may be
the wrong place for this.

The check in ShouldTakeScreenshot is [an attempt] to essentially enumerate the
cases where we take a screenshot before the navigation, so that we do not try to
take the screenshot again after the navigation. But I suppose this will not be
necessary anymore if we can tell in RendererDidNavigate that
web_contents_->GetRenderViewHost() did not cause the old RVH to be swapped out.
Is that possible?

[sadrul, 2013-01-08 03:00:25]

On 2013/01/08 00:16:29, sadrul wrote:
>
https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
> File content/browser/web_contents/navigation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/11776010/diff/9001/content/browser/web_conten...
> content/browser/web_contents/navigation_controller_impl.cc:1576: else if
> (reload_type == NO_RELOAD)
> On 2013/01/07 19:50:23, creis wrote:
> > We should put a return after DiscardNonCommittedEntries and skip the "else".

> > The existing logic below won't run in that case anyway, since pending_entry_
> > will be null.
> > 
> > Aside from that, I don't think this is the right place to take the
screenshot.
> 
> > The navigation might never commit (making it unnecessary), and a large
amount
> of
> > time could pass before it does (making it possibly stale).
> > 
> > It seems like you want to take the screenshot just before the previous page
> > tries to unload.  That would be in RenderViewHost::SwapOut, which gets
called
> by
> > RVHM::OnCrossSiteResponse.
> > 
> > Either way, can you explain why ShouldTakeScreenshot matters in the other
case
> > but not this one?
> 
> The issue here is that, in some cases (especially when the navigation happens
> because of a user action in the UI, e.g. entering a URL in the omnibox, or
> clicking a bookmark button), we want to take the screenshot before the
> RenderView is swapped out. So I do that here. But as you point it, this may be
> the wrong place for this.
> 
> The check in ShouldTakeScreenshot is [an attempt] to essentially enumerate the
> cases where we take a screenshot before the navigation, so that we do not try
to
> take the screenshot again after the navigation.

I have changed this function in Patchset3 to better indicate what it is meant to
do.

[Charlie Reis, 2013-01-08 21:50:55]

Let's take a step back here, since I think I'm starting to understand the
underlying problem.

First, it looks like you're trying to detect if a navigation is
browser-initiated rather than renderer-initiated, because you're worried a
browser-initiated navigation might swap out the RVH and return a blank image in
the screenshot.  You can already use NavigationEntryImpl::is_renderer_initiated
to distinguish between browser and renderer initiated navigations, so the enum
wouldn't be necessary.

However, that's not the right signal in this case.  Renderer-initiated
navigations can also cause a process swap (e.g., extension page -> normal page),
and many browser-initiated navigations don't cause process swaps.  You might be
able to peek at the state of RenderViewHostManager to see if a cross-process
navigation is in progress.

I think we can come up with a better approach, though.  Wouldn't it be better if
the blank swapped out page was never visible in the first place, and you could
always take the screenshot in RendererDidNavigate?  It seems better from my
perspective, since we don't want to show that blank swapped out page to users. 
Maybe we can just leave the previous state in the backing store when swapping a
renderer out.

Would that fix your bug?  If so, I can take a look at what additional IPC
filtering we might need to do to accomplish that.

[sadrul, 2013-01-08 23:43:01]

On 2013/01/08 21:50:55, creis wrote:
> Let's take a step back here, since I think I'm starting to understand the
> underlying problem.
> 
> First, it looks like you're trying to detect if a navigation is
> browser-initiated rather than renderer-initiated, because you're worried a
> browser-initiated navigation might swap out the RVH and return a blank image
in
> the screenshot.  You can already use
NavigationEntryImpl::is_renderer_initiated
> to distinguish between browser and renderer initiated navigations, so the enum
> wouldn't be necessary.
> 
> However, that's not the right signal in this case.  Renderer-initiated
> navigations can also cause a process swap (e.g., extension page -> normal
page),
> and many browser-initiated navigations don't cause process swaps.  You might
be
> able to peek at the state of RenderViewHostManager to see if a cross-process
> navigation is in progress.
> 
> I think we can come up with a better approach, though.  Wouldn't it be better
if
> the blank swapped out page was never visible in the first place, and you could
> always take the screenshot in RendererDidNavigate?  It seems better from my
> perspective, since we don't want to show that blank swapped out page to users.

> Maybe we can just leave the previous state in the backing store when swapping
a
> renderer out.
> 
> Would that fix your bug?  If so, I can take a look at what additional IPC
> filtering we might need to do to accomplish that.

Whoa! It totally does! Thanks! :D

I have uploaded two different patches based on this idea:

The approach in patchset 4 is:
 * Mark a RenderViewImpl as hidden right before it is marked as swapped-out (but
after it has already run the unload handler).
 * Take the screenshot in WebContents::DidNavigate, instead of in
RendererDidNavigate (since RenderViewHostManager::DidNavigateMainFrame is called
earlier, and it changes the RenderViewHost for the WebContents)

The approach in patchset 5 is pretty similar to the one in patchset 4, except
that instead of marking the RenderViewImpl as hidden before swapping it out, we
just don't send any paint-updates from a swapped-out RenderWidget from the
renderer to the browser.

Both approaches seems to fix the issue for me.

[Charlie Reis, 2013-01-09 00:49:23]

Nice!  I'm ok with either approach, and I'd probably lean towards patch set 4
(to avoid having is_swapped_out_ leak into the paint logic).

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2689:
controller_.TakeScreenshot();
nit: Please put a blank line below this.

Also, you may want to clarify what "the screenshot" is, since there's no other
context here and it sounds ominous.  :)

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:374:
base::StringPrintf("files/overscroll_navigation.html#%d", index)));
Just curious-- these tests don't appear to be doing a cross-process navigation,
so I'm not sure if they're triggering the bug.  Do they fail without your fix? 
(If so, I wonder what causes it.)

https://codereview.chromium.org/11776010/diff/3003/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11776010/diff/3003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5452: SetHidden(true);
Interesting.  This looks similar to the setVisibilityState call below.  I wonder
if we should move that call up alongside this one, and whether both are needed
or if just moving that up here is sufficient.

https://codereview.chromium.org/11776010/diff/1003/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11776010/diff/1003/content/renderer/render_wi...
content/renderer/render_widget.cc:973: if (is_swapped_out_)
Or just move this up into the is_hidden_ check above.  But then we're back to
patch 4.

[sadrul, 2013-01-09 17:44:19]

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
content/browser/web_contents/web_contents_impl.cc:2689:
controller_.TakeScreenshot();
On 2013/01/09 00:49:23, creis wrote:
> nit: Please put a blank line below this.
> 
> Also, you may want to clarify what "the screenshot" is, since there's no other
> context here and it sounds ominous.  :)

Done.

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/11776010/diff/3003/content/browser/web_conten...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:374:
base::StringPrintf("files/overscroll_navigation.html#%d", index)));
On 2013/01/09 00:49:23, creis wrote:
> Just curious-- these tests don't appear to be doing a cross-process
navigation,
> so I'm not sure if they're triggering the bug.  Do they fail without your fix?

> (If so, I wonder what causes it.)

Great catch! You are right. These do not fail without the rest of the changes in
this CL.

I have changed the test to make sure they do fail without the change now. I have
added checks to make sure that screenshot is taken for the correct RV

https://codereview.chromium.org/11776010/diff/3003/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11776010/diff/3003/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5452: SetHidden(true);
On 2013/01/09 00:49:23, creis wrote:
> Interesting.  This looks similar to the setVisibilityState call below.  I
wonder
> if we should move that call up alongside this one, and whether both are needed
> or if just moving that up here is sufficient.

I moved the call to setVisibilityState up here.

SetHidden() and setVisibilityState() do different things, and both are called in
response to a WasHidden/WasShown message from the browser. I tried removing the
call to SetHidden, but that didn't work.

Do you think it might be necessary to call SetHidden(false) when the renderer
gets swapped back in, or does it always get a WasShown message when necessary?

[sadrul, 2013-01-09 19:27:58]

[snip]
> Do you think it might be necessary to call SetHidden(false) when the renderer
> gets swapped back in, or does it always get a WasShown message when necessary?

Looks like this would be necessary; since otherwise the
RenderViewHostManagerTest.SwappedOutViewHasCorrectVisibilityState test was
failing. But I suppose blindly calling SetHidden(false) would also be wrong. So
I went back to patchset-5l; i.e. do not do paint-updates for a swapped out
renderer.

I also split the test into two, and changed some parts so that the test doesn't
timeout.

[Charlie Reis, 2013-01-09 23:33:31]

Sounds reasonable.  LGTM with nits.

https://codereview.chromium.org/11776010/diff/18010/content/browser/web_conte...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/11776010/diff/18010/content/browser/web_conte...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:29: void
Dummy(RenderViewHost* host) {
nit: DummyCallback

https://codereview.chromium.org/11776010/diff/18010/content/browser/web_conte...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:403: // Tests
that screenshot is taken correct when navigation causes a
nit: correctly

[sadrul, 2013-01-09 23:37:58]

Awesome! Thanks for the great review!

https://codereview.chromium.org/11776010/diff/18010/content/browser/web_conte...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/11776010/diff/18010/content/browser/web_conte...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:29: void
Dummy(RenderViewHost* host) {
On 2013/01/09 23:33:31, creis wrote:
> nit: DummyCallback

Done.

https://codereview.chromium.org/11776010/diff/18010/content/browser/web_conte...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:403: // Tests
that screenshot is taken correct when navigation causes a
On 2013/01/09 23:33:31, creis wrote:
> nit: correctly

Done.