Issue 655697  has been merged into this issue.

Linking related issues.

Even though this is an existing issue I would like to have this solved before M-58, at least for the last_n case.

I'm starting to work on the last_n part of this issue.

Note that this change will make the logic a bit less robust IRT leaving extra last_n snapshots behind. The reason being that we will first create the snapshot and only then delete any previous existing one only if it was a successful save (currently the order is reversed, hence this issue). So if a event out of our control happens -- i.e. a crash -- between these steps, two snapshots will exist for the same tab.

However this should not be a problem because we do have cleanup procedures to handle leftover snapshots:
- There already is a periodical cleanup task that runs upon Chrome start to detect lingering last_n snapshots.
- When either CL [1] or [2] lands another cleanup step will also run upon tab closure.

Please dewittj@ and romax@, correct me if I'm wrong here.

[1] https://codereview.chromium.org/2684973014/

[2] https://codereview.chromium.org/2706343007/

If you're talking about consistency check yes there's a cleanup procedure to handle leftover snapshots. However it only takes care of:
1. you have a file on disk, but no entry in the database, delete the file
2. you have an entry in database, but no file on disk, delete the database entry.

it's here: https://cs.chromium.org/chromium/src/components/offline_pages/core/offline_page_model_impl.cc?rcl=1aa3f1c709ebdc77c75fa90263b983d181af91e1&l=984

I talked with romax@ offline and figured out why I was a bit confused. There are in fact two different cleanup tasks. The one that runs when Chrome is started and loads the Offline Model is a "consistency check". That's what he  described in #7.

The one I was interested at runs continuously but throttled by Offline storage usage and a timeout. This one does cleanup expired offline pages, what would take care of any last_n pages left behind after 2 days (current expiration time for last_n pages).

This is a good candidate to fix soon.

Si disattivato e non posso più attivarlo

This does not block NTP anymore.

This has indeed evolved to be two separate issues:

*Last_n*
Presents the simpler fix as (hopefully) the main thing we need to do is to move the deletion of the previous snapshot to be the last step in the callbacks chain started in `WebContentsWasHidden`.

If anything happens and we end up with two pages, our interceptor should favor opening the last one, which is the desired behavior.

Another adequate change would be to update the "previous pages deletion step" in RecentTabHelper::DidFinishNavigation to delete any and all previous last_n pages bound to the assigned tab in case of a successful navigation (not to an error page for instance). This should lessen any privacy concern matters IRT leaving behind pages for previously navigated URLs in that tab.


*Downloads*
In this case we probably want to create a new Offline Page Model task that allows replacing an offline page with a newly generated archive. This is to avoid the current problem of losing the previously saved page in case the new save fails but also to avoid the user ending up with two manually downloaded pages instead.

This logic might already have been implemented during the development of offline pages sharing as the "publishing of a private page" has to deal with a similar problem.