Last_n: Delete previously saved snapshot when navigating.

chrome/browser/android/offline_pages/recent_tab_helper.cc

 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/recent_tab_helper.h"
 
 #include <queue>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 194 matching lines @@
            << web_contents()->GetLastCommittedURL().spec();
 
   // If there is an ongoing downloads request, lets make Background Offliner
   // take over downloading that page.
   if (downloads_ongoing_snapshot_info_) {
     DVLOG(1) << " - Passing ongoing downloads request to Background Offliner";
     ReportDownloadStatusToRequestCoordinator(
         downloads_ongoing_snapshot_info_.get(), false);
   }
 
+  // If the previous page was saved, delete it now.
+  if (last_n_latest_saved_snapshot_info_) {

[dewittj, 2017/04/17 21:17:42]

This is an interesting strategy, I wonder if there are any race conditions (with
Chrome exiting for example) that might prevent us from deleting the page.

Also, why have a whole new snapshot info rather than just annotating the
existing snapshot info as complete?

[carlosk, 2017/04/18 17:56:19]

There is a very small window of time between the page committing and this
observer method being called when if Chrome should be terminated within it then
it would cause the snapshot to be left alive and potentially accessible.

But upon future restarts the snapshot will eventually be deleted either when a
new snapshot should be saved for that tab or when the tab should be closed.
 
This mimics the way we already keep the last successful snapshot info for
downloads so it makes it consistent with how things already work inside this
file.

I am not creating a new instance but transferring from one smart pointer to
another. I can't keep the "ongoing" pointer after snapshot completion because
its existence is used as a flag for ongoing process of snapshot creation.

+    std::vector<int64_t> id{last_n_latest_saved_snapshot_info_->request_id};
+    page_model_->DeletePagesByOfflineId(id, DeletePageCallback());
+  }
+
   // Cancel any and all in flight snapshot tasks from the previous page.
-  DVLOG_IF(1, last_n_ongoing_snapshot_info_)
-      << " - Canceling ongoing last_n snapshot";
   CancelInFlightSnapshots();
   downloads_snapshot_on_hold_ = false;
 
   // Always reset so that posted tasks get canceled.
   snapshot_controller_->Reset();
 
   // Check for conditions that would cause us not to snapshot.
   bool can_save =
       !navigation_handle->IsErrorPage() &&
       OfflinePageModel::CanSaveURL(web_contents()->GetLastCommittedURL()) &&
@@ skipping 184 matching lines @@
       OfflinePageUtils::GetOriginalURLFromWebContents(web_contents());
   page_model_->SavePage(
       save_page_params, delegate_->CreatePageArchiver(web_contents()),
       base::Bind(&RecentTabHelper::SavePageCallback,
                  weak_ptr_factory_.GetWeakPtr(), snapshot_info));
 }
 
 void RecentTabHelper::SavePageCallback(SnapshotProgressInfo* snapshot_info,
                                        OfflinePageModel::SavePageResult result,
                                        int64_t offline_id) {
-  DCHECK(snapshot_info->IsForLastN() ||
+  DCHECK((snapshot_info->IsForLastN() &&
+          snapshot_info->request_id == OfflinePageModel::kInvalidOfflineId) ||
          snapshot_info->request_id == offline_id);
+  // Store the assigned offline_id (for downloads case it will already contain
+  // the same value).
+  snapshot_info->request_id = offline_id;
   ReportSnapshotCompleted(snapshot_info, result == SavePageResult::SUCCESS);
 }
 
 // Note: this is the final step in the chain of callbacks and it's where the
 // behavior is different depending on this being a last_n or downloads snapshot.
 void RecentTabHelper::ReportSnapshotCompleted(
     SnapshotProgressInfo* snapshot_info,
     bool success) {
   DVLOG(1) << (snapshot_info->IsForLastN() ? "Last_n" : "Downloads")
            << " snapshot " << (success ? "succeeded" : "failed")
            << " for: " << web_contents()->GetLastCommittedURL().spec();
   if (snapshot_info->IsForLastN()) {
     DCHECK_EQ(snapshot_info, last_n_ongoing_snapshot_info_.get());
-    last_n_ongoing_snapshot_info_.reset();
+    if (success) {
+      last_n_latest_saved_snapshot_info_ =
+          std::move(last_n_ongoing_snapshot_info_);
+    } else {
+      last_n_ongoing_snapshot_info_.reset();
+    }
     return;
   }
 
   DCHECK_EQ(snapshot_info, downloads_ongoing_snapshot_info_.get());
   snapshot_controller_->PendingSnapshotCompleted();
   // Tell RequestCoordinator how the request should be processed further.
   ReportDownloadStatusToRequestCoordinator(snapshot_info, success);
   if (success) {
     downloads_latest_saved_snapshot_info_ =
         std::move(downloads_ongoing_snapshot_info_);
@@ skipping 26 matching lines @@
     request_coordinator->EnableForOffliner(snapshot_info->request_id,
                                            snapshot_info->client_id);
   }
 }
 
 ClientId RecentTabHelper::GetRecentPagesClientId() const {
   return ClientId(kLastNNamespace, tab_id_);
 }
 
 void RecentTabHelper::CancelInFlightSnapshots() {
+  DVLOG_IF(1, last_n_ongoing_snapshot_info_)
+      << " - Canceling ongoing last_n snapshot";
+  DVLOG_IF(1, downloads_ongoing_snapshot_info_)
+      << " - Canceling ongoing downloads snapshot";
   weak_ptr_factory_.InvalidateWeakPtrs();
   downloads_ongoing_snapshot_info_.reset();
   downloads_latest_saved_snapshot_info_.reset();
   last_n_ongoing_snapshot_info_.reset();
+  last_n_latest_saved_snapshot_info_.reset();
 }
 
 }  // namespace offline_pages