navigation: Retain a screenshot of the page before it unloads

navigation: Retain a screenshot of the page before it unloads

This screenshot is used during the overscroll navigation to display
a preview of the page.

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

[sadrul, 2012-12-21 17:17:41]

Hi! The follow-up CL that uses the screenshot in the UI is here:
https://codereview.chromium.org/11659011/

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

https://codereview.chromium.org/11635059/diff/5001/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:1492:
TakeScreenshot();
Calling this here seems to work only for navigations that happen from the
browser (e.g. going back/forward in history, pressing enter in the omnibox
etc.), but not when doing a navigation from the page itself (e.g. clicking on a
link). Where should we take the screenshot in such cases?

[Charlie Reis, 2012-12-21 19:59:50]

Hmm.  Before I get into the review, have there been any discussions about the
memory impact of this?  A single tab can have up to 50 NavigationEntries, and
some users have many tabs.  Especially with large displays, that's a lot of
screenshot data to keep in the browser process's memory.

[Charlie Reis, 2012-12-21 20:01:08]

https://codereview.chromium.org/11635059/diff/9001/content/browser/web_conten...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/11635059/diff/9001/content/browser/web_conten...
content/browser/web_contents/navigation_entry_impl.h:212: // restore).
What would happen then if you restored a session and tried to swipe back?

[sadrul, 2012-12-21 20:12:39]

On 2012/12/21 19:59:50, creis wrote:
> Hmm.  Before I get into the review, have there been any discussions about the
> memory impact of this?  A single tab can have up to 50 NavigationEntries, and
> some users have many tabs.  Especially with large displays, that's a lot of
> screenshot data to keep in the browser process's memory.

I don't believe this has been discussed at length yet. The possible options for
this would be:
 * Keep screenshot for only a fixed number of entries.
 * Release the screenshots as the memory usage increases over time.
 * A combination of both.

Right now, this should be turned off by default, and turned on only when the
overscroll-navigation feature is turned on in chrome://flags (I realize I don't
have this guard in the current patchset, will fix). Once the
overscroll-navigation is more complete, we can have an experiment to turn it on,
and do a better measurement of how much this affects the memory consumption, and
try to find a good solution for that. Does that sound like a reasonable plan?

+davemoore@ for discussion.

[sadrul, 2012-12-21 20:13:24]

https://codereview.chromium.org/11635059/diff/9001/content/browser/web_conten...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/11635059/diff/9001/content/browser/web_conten...
content/browser/web_contents/navigation_entry_impl.h:212: // restore).
On 2012/12/21 20:01:08, creis wrote:
> What would happen then if you restored a session and tried to swipe back?

In the absence of a screenshot (after a restore, or if taking the screenshot
failed for some reason), a placeholder image (currently all-gray) is displayed
instead.

[sadrul, 2012-12-21 20:58:20]

I made a couple of changes:

 * Take screenshot of the last-committed entry instead of the 'active entry',
since the 'pending entry' can also be the 'active entry', but we don't want to
get the screenshot of the pending entry.

 * Do nothing from ::TakeScreenshot() if overscroll nav. is not turned on.

[Charlie Reis, 2012-12-21 23:22:07]

Ok, evaluating it behind a flag sounds more reasonable.  The best option as you
move forward might be capping the total amount of memory we allocate for
screenshots, so that it doesn't depend on the size of the window or the number
of entries.

Perhaps you can add a test (e.g., navigation_controller_impl_unittest.cc) to
ensure this gets set after navigation?  (That would also help document your
expectations of when it should be valid.)

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:485:
CHECK(web_contents_);
This CHECK isn't really necessary.  A NavigationController always has one, and
if we didn't, it would crash a few lines below anyway.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:487:
NavigationEntryImpl* active_entry =
Calling this active_entry is confusing.  Perhaps just entry?

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:499:
view->GetViewBounds().size(),
What happens if the view size changes when you want to use it?

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:526: if (!success) {
Should we just move this to the top and skip searching for the entry in this
case?  Not sure having the URL is that useful, and you could at least show the
ID.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:758:
TakeScreenshot();
Are we certain this is going to show the previous page, and can't show anything
from page that has just committed?  The screenshot function looks asynchronous.

Also, please add a comment explaining why this exists here.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1510:
TakeScreenshot();
Why is this needed here?  This isn't called for all navigations (e.g.,
renderer-initiated ones), and it's redundant with the call in
RendererDidNavigate.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.h (right):

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.h:320: void
TakingScreenshotComplete(int unique_id,
Maybe "OnScreenshotTaken"?  Or is that too close to the IPC handler naming
scheme?

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_entry_impl.h:170: void
SetScreenshotPNGData(const std::vector<unsigned char>& png_data);
We might be prematurely optimizing with PNG (especially putting it in the
function name).  Your suggestion of capping the amount of space we use might be
sufficient to avoid the encoding/decoding costs, or we might find that WebP is a
better option.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_entry_impl.h:211: // This is also a
transient memeber (i.e. is not persisted with session
typo: memeber

Also, please explain when this screenshot is taken, why it is needed, and why
it's ok for it to be missing.

[sadrul, 2012-12-22 00:07:10]

> Ok, evaluating it behind a flag sounds more reasonable.
> The best option as you move forward might be capping
> the total amount of memory we allocate for screenshots,

Do you mean a size-cap on individual screenshots (i.e. for each
NavigationEntryImpl), or for all screenshots in a single
NavigationControllerImpl?

> so that it doesn't depend on the size of the window or
> the number of entries.

> Perhaps you can add a test

I am working on a test. Thanks!

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:485:
CHECK(web_contents_);
On 2012/12/21 23:22:07, creis wrote:
> This CHECK isn't really necessary.  A NavigationController always has one, and
> if we didn't, it would crash a few lines below anyway.

Removed.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:487:
NavigationEntryImpl* active_entry =
On 2012/12/21 23:22:07, creis wrote:
> Calling this active_entry is confusing.  Perhaps just entry?

Done.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:499:
view->GetViewBounds().size(),
On 2012/12/21 23:22:07, creis wrote:
> What happens if the view size changes when you want to use it?

Currently, we paint the screenshot in the original size, i.e. we do not try to
fit it into the new size (so there will be blanks if the size increases after
the navigation). An alternate approach would be to discard the screenshot in
such cases, or display the placeholder image instead.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:526: if (!success) {
On 2012/12/21 23:22:07, creis wrote:
> Should we just move this to the top and skip searching for the entry in this
> case?  Not sure having the URL is that useful, and you could at least show the
> ID.

Sounds good. Done.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:758:
TakeScreenshot();
On 2012/12/21 23:22:07, creis wrote:
> Are we certain this is going to show the previous page, and can't show
anything
> from page that has just committed?  The screenshot function looks
asynchronous.

Hm, good point. For the software compositing case, the data is copied from the
backing-store synchronously. So this should work properly.

For the compositing case, from what I understand, the new page should have a
different texture; so the worst that can happen is that the texture of the
current page will be destroyed, and taking the screenshot will fail. But the
screenshot should not include contents of the newly committed page in either
case.

> Also, please add a comment explaining why this exists here.

Done.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1510:
TakeScreenshot();
On 2012/12/21 23:22:07, creis wrote:
> Why is this needed here?  This isn't called for all navigations (e.g.,
> renderer-initiated ones), and it's redundant with the call in
> RendererDidNavigate.

Removed.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.h (right):

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.h:320: void
TakingScreenshotComplete(int unique_id,
On 2012/12/21 23:22:07, creis wrote:
> Maybe "OnScreenshotTaken"?  Or is that too close to the IPC handler naming
> scheme?

I think OnXXYY is quite common in non-IPC code too, so made this change.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_entry_impl.h:170: void
SetScreenshotPNGData(const std::vector<unsigned char>& png_data);
On 2012/12/21 23:22:07, creis wrote:
> We might be prematurely optimizing with PNG (especially putting it in the
> function name).  Your suggestion of capping the amount of space we use might
be
> sufficient to avoid the encoding/decoding costs, or we might find that WebP is
a
> better option.

I didn't have optimization in mind when I used the PNG data here :)

We get an SkBitmap from the screenshot callback, and we need to store the data
in a way that can be copied (since NavigationEntryImpl is copy-able) without
much difficulty, and also displayed by the UI code. PNG seemed like a good
choice for this.

https://codereview.chromium.org/11635059/diff/12004/content/browser/web_conte...
content/browser/web_contents/navigation_entry_impl.h:211: // This is also a
transient memeber (i.e. is not persisted with session
On 2012/12/21 23:22:07, creis wrote:
> typo: memeber
> 
> Also, please explain when this screenshot is taken, why it is needed, and why
> it's ok for it to be missing.

Done.

[Charlie Reis, 2012-12-22 00:23:59]

Thanks.  This LGTM, provided you add a test.  (Please make sure the test code
gets reviewed as well, but I'll be out for the holidays after today, so feel
free to get another reviewer for the test itself if I'm gone.)

On 2012/12/22 00:07:10, sadrul wrote:
> > Ok, evaluating it behind a flag sounds more reasonable.
> > The best option as you move forward might be capping
> > the total amount of memory we allocate for screenshots,
> 
> Do you mean a size-cap on individual screenshots (i.e. for each
> NavigationEntryImpl), or for all screenshots in a single
> NavigationControllerImpl?

I meant a total for the whole browser process, since there can be tons of tabs. 
But per-NavigationController may be reasonable (i.e., considering it a per-tab
overhead), since at least the number of tabs isn't something an attacker can
control, while creating new navigation entries is.  Still, I'd recommend a
security review before turning it on by default, since it does notably increase
the amount of browser-side memory an attacker can control.

[sadrul, 2012-12-22 00:49:16]

On 2012/12/22 00:23:59, creis wrote:
> Thanks.  This LGTM, provided you add a test.  (Please make sure the test code
> gets reviewed as well, but I'll be out for the holidays after today, so feel
> free to get another reviewer for the test itself if I'm gone.)

Cool. Thanks. Looks like a unit-test won't be sufficient for testing this (since
a backing store doesn't seem to be created there). I will look into adding a
browser-test instead.

> 
> On 2012/12/22 00:07:10, sadrul wrote:
> > > Ok, evaluating it behind a flag sounds more reasonable.
> > > The best option as you move forward might be capping
> > > the total amount of memory we allocate for screenshots,
> > 
> > Do you mean a size-cap on individual screenshots (i.e. for each
> > NavigationEntryImpl), or for all screenshots in a single
> > NavigationControllerImpl?
> 
> I meant a total for the whole browser process, since there can be tons of
tabs. 
> But per-NavigationController may be reasonable (i.e., considering it a per-tab
> overhead), since at least the number of tabs isn't something an attacker can
> control, while creating new navigation entries is.  Still, I'd recommend a
> security review before turning it on by default, since it does notably
increase
> the amount of browser-side memory an attacker can control.

Filed crbug.com/167397 to keep track of this. Thanks.

[sadrul, 2012-12-22 01:13:24]

On 2012/12/22 00:49:16, sadrul wrote:
> On 2012/12/22 00:23:59, creis wrote:
> > Thanks.  This LGTM, provided you add a test.  (Please make sure the test
code
> > gets reviewed as well, but I'll be out for the holidays after today, so feel
> > free to get another reviewer for the test itself if I'm gone.)
> 
> Cool. Thanks. Looks like a unit-test won't be sufficient for testing this
(since
> a backing store doesn't seem to be created there). I will look into adding a
> browser-test instead.

Added a browser-test to test that the page has has a screenshot when navigation
happens:
 - from within the page (from a JS function)
 - interactively, when user does an overscroll gesture
 - interactively, when user navigates in history without the overscroll gesture.

Since overscroll currently works only for aura (on chromeos), I put the
browser-test in the same place. Please have a look if you get a chance.
Otherwise, we can continue with the review next year :)

[Charlie Reis, 2012-12-22 01:55:49]

On 2012/12/22 01:13:24, sadrul wrote:
> On 2012/12/22 00:49:16, sadrul wrote:
> > On 2012/12/22 00:23:59, creis wrote:
> > > Thanks.  This LGTM, provided you add a test.  (Please make sure the test
> code
> > > gets reviewed as well, but I'll be out for the holidays after today, so
feel
> > > free to get another reviewer for the test itself if I'm gone.)
> > 
> > Cool. Thanks. Looks like a unit-test won't be sufficient for testing this
> (since
> > a backing store doesn't seem to be created there). I will look into adding a
> > browser-test instead.
> 
> Added a browser-test to test that the page has has a screenshot when
navigation
> happens:
>  - from within the page (from a JS function)
>  - interactively, when user does an overscroll gesture
>  - interactively, when user navigates in history without the overscroll
gesture.

Can you add this bit to the comment above the browser test, to help others
follow the test?  Otherwise, LGTM.

> 
> Since overscroll currently works only for aura (on chromeos), I put the
> browser-test in the same place. Please have a look if you get a chance.
> Otherwise, we can continue with the review next year :)

[sadrul, 2012-12-22 02:53:55]

On 2012/12/22 01:55:49, creis wrote:
> On 2012/12/22 01:13:24, sadrul wrote:
> > On 2012/12/22 00:49:16, sadrul wrote:
> > > On 2012/12/22 00:23:59, creis wrote:
> > > > Thanks.  This LGTM, provided you add a test.  (Please make sure the test
> > code
> > > > gets reviewed as well, but I'll be out for the holidays after today, so
> feel
> > > > free to get another reviewer for the test itself if I'm gone.)
> > > 
> > > Cool. Thanks. Looks like a unit-test won't be sufficient for testing this
> > (since
> > > a backing store doesn't seem to be created there). I will look into adding
a
> > > browser-test instead.
> > 
> > Added a browser-test to test that the page has has a screenshot when
> navigation
> > happens:
> >  - from within the page (from a JS function)
> >  - interactively, when user does an overscroll gesture
> >  - interactively, when user navigates in history without the overscroll
> gesture.
> 
> Can you add this bit to the comment above the browser test, to help others
> follow the test?  Otherwise, LGTM.

Done. Thanks a lot for the quick reviews!