RecentTabHelper won't accept overlapping requests from Downloads anymore.

chrome/browser/android/offline_pages/recent_tab_helper.cc

Index: chrome/browser/android/offline_pages/recent_tab_helper.cc
diff --git a/chrome/browser/android/offline_pages/recent_tab_helper.cc b/chrome/browser/android/offline_pages/recent_tab_helper.cc
index 2231629bb90da2db9c6fc81bf27606d3dfd60bc5..6ac409d44996341abbabba695e838c74f668b1f5 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.cc
@@ -56,10 +56,33 @@ namespace offline_pages {
 
 using PageQuality = SnapshotController::PageQuality;
 
-bool RecentTabHelper::SnapshotProgressInfo::IsForLastN() {
-  // A last_n snapshot always has an invalid request id.
-  return request_id == OfflinePageModel::kInvalidOfflineId;
-}
+// Keeps client_id/request_id that will be used for the offline snapshot.
+struct RecentTabHelper::SnapshotProgressInfo {
+ public:
+  // For a downloads snapshot request, where the |request_id| is defined.
+  SnapshotProgressInfo(const ClientId& client_id, int64_t request_id)
+      : client_id(client_id), request_id(request_id) {}
+
+  // For a last_n snapshot request.
+  explicit SnapshotProgressInfo(const ClientId& client_id)
+      : client_id(client_id) {}
+
+  bool IsForLastN() { return client_id.name_space == kLastNNamespace; }
+
+  // The ClientID to go with the offline page.
+  ClientId client_id;
+
+  // Id of the suspended request in Background Offliner. Used to un-suspend
+  // the request if the capture of the current page was not possible (e.g.
+  // the user navigated to another page before current one was loaded).
+  // 0 if this is a "last_n" info.
+  int64_t request_id = OfflinePageModel::kInvalidOfflineId;
+
+  // Expected snapshot quality should the saving succeed. This value is only
+  // valid if |page_snapshot_completed| is true.

[Dmitry Titov, 2017/01/27 04:18:22]

Not clear which "|page_snapshot_completed|" this comment refers to.

[carlosk, 2017/01/27 18:47:14]

Done.

+  SnapshotController::PageQuality expected_page_quality =
+      SnapshotController::PageQuality::POOR;
+};
 
 RecentTabHelper::RecentTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
@@ -79,30 +102,46 @@ void RecentTabHelper::SetDelegate(
 
 void RecentTabHelper::ObserveAndDownloadCurrentPage(
     const ClientId& client_id, int64_t request_id) {
+  // Note: as this implementation only supports one client namespace, enforce
+  // that the call is from Downloads.

[dewittj, 2017/01/27 18:48:57]

Suggestion to remove the DCHECK: change signature to only accept GUID

[carlosk, 2017/01/27 23:13:30]

As OfflinePageDownloadBridge also uses the ClientId value provided here it makes
more sense to keep that as is.

+  DCHECK_EQ(kDownloadNamespace, client_id.name_space);
   auto new_downloads_snapshot_info =
       base::MakeUnique<SnapshotProgressInfo>(client_id, request_id);
 
   // If this tab helper is not enabled, immediately give the job back to
   // RequestCoordinator.
   if (!EnsureInitialized()) {
-    ReportDownloadStatusToRequestCoordinator(new_downloads_snapshot_info.get());
+    ReportDownloadStatusToRequestCoordinator(new_downloads_snapshot_info.get(),
+                                             false);
     return;
   }
 
-  // TODO(carlosk): This is a good moment check if a snapshot is currently being
-  // generated. This would allow the early cancellation of this request (without
-  // incurring in scheduling a background download).
+  // If there is an ongoing snapshot request, completely ignore this one and
+  // cancel the Background Offliner request.
+  // TODO(carlosk): it might be better to make the decision to schedule or not

[dewittj, 2017/01/27 18:48:57]

This TODO is not really actionable.  Is there a bug or doc or another way to
phrase it so that another person could take it on?

[carlosk, 2017/01/27 23:13:30]

Done.

+  // the background request here.
+  // TODO(carlosk): an edge case happens when the ongoing request was

[dewittj, 2017/01/27 18:48:57]

This should be more explicit about the race condition, and/or should refer to a
bug with more details.  Otherwise it's very mysterious to someone who is not
familiar with the known issue.

[carlosk, 2017/01/27 23:13:30]

Added a crbug with further details.

+  // automatically and transparently scheduled by a navigation event and this
+  // call happens due to the user pressing the download button. The user's
+  // request to download the page will be immediately dismissed.
+  if (downloads_ongoing_snapshot_info_) {
+    ReportDownloadStatusToRequestCoordinator(new_downloads_snapshot_info.get(),
+                                             true);
+    return;
+  }
 
   // Stores the new snapshot info.
-  downloads_latest_snapshot_info_ = std::move(new_downloads_snapshot_info);
+  downloads_ongoing_snapshot_info_ = std::move(new_downloads_snapshot_info);
 
   // If the page is not yet ready for a snapshot return now as it will be
   // started later, once page loading advances.
-  if (PageQuality::POOR == snapshot_controller_->current_page_quality())
+  if (PageQuality::POOR == snapshot_controller_->current_page_quality()) {
+    downloads_snapshot_on_hold_ = true;
     return;
+  }
 
   // Otherwise start saving the snapshot now.
-  SaveSnapshotForDownloads();
+  SaveSnapshotForDownloads(false);
 }
 
 // Initialize lazily. It needs TabAndroid for initialization, which is also a
@@ -145,15 +184,18 @@ void RecentTabHelper::DidFinishNavigation(
   if (!EnsureInitialized())
     return;
 
-  // We navigated to a different page, lets report progress to Background
-  // Offliner.
-  if (downloads_latest_snapshot_info_)
+  // If there is an ongoing downloads request, lets allow Background Offliner to
+  // continue downloading this page.
+  if (downloads_ongoing_snapshot_info_) {
     ReportDownloadStatusToRequestCoordinator(
-        downloads_latest_snapshot_info_.get());
+        downloads_ongoing_snapshot_info_.get(), false);
+  }
 
   // Cancel any and all in flight snapshot tasks from the previous page.
   weak_ptr_factory_.InvalidateWeakPtrs();

[dewittj, 2017/01/27 18:48:57]

perhaps the reset() + invalidateweakptrs should be in a "CancelInFlightTasks"
helper method.

[carlosk, 2017/01/27 23:13:30]

Done.

-  downloads_latest_snapshot_info_.reset();
+  downloads_ongoing_snapshot_info_.reset();
+  downloads_latest_saved_snapshot_info_.reset();
+  downloads_snapshot_on_hold_ = false;
   last_n_ongoing_snapshot_info_.reset();
 
   // New navigation, new snapshot session.
@@ -187,10 +229,11 @@ void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
 }
 
 void RecentTabHelper::WebContentsDestroyed() {
-  // WebContents (and maybe Tab) is destroyed, report status to Offliner.
-  if (downloads_latest_snapshot_info_)
+  // If there is an ongoing downloads request, lets allow Background Offliner to
+  // continue downloading this page.
+  if (downloads_ongoing_snapshot_info_)
     ReportDownloadStatusToRequestCoordinator(
-        downloads_latest_snapshot_info_.get());
+        downloads_ongoing_snapshot_info_.get(), false);
   // And cancel any ongoing snapshots.
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
@@ -218,6 +261,7 @@ void RecentTabHelper::WasHidden() {
 
   last_n_ongoing_snapshot_info_ =
       base::MakeUnique<SnapshotProgressInfo>(GetRecentPagesClientId());
+  DCHECK(last_n_ongoing_snapshot_info_->IsForLastN());
   DCHECK(snapshots_enabled_);
   // Remove previously captured pages for this tab.
   page_model_->GetOfflineIdsForClientId(
@@ -232,28 +276,49 @@ void RecentTabHelper::WasHidden() {
 void RecentTabHelper::StartSnapshot() {
   DCHECK_NE(PageQuality::POOR, snapshot_controller_->current_page_quality());
 
-  // This is a navigation based snapshot request so check that snapshots are
-  // both enabled and there is a downloads request for one.
-  // TODO(carlosk): This is a good moment to add the check for an ongoing
-  // snapshot that could trigger the early cancellation of this request.
-  if (snapshots_enabled_ && downloads_latest_snapshot_info_)
-    SaveSnapshotForDownloads();
-  else
-    snapshot_controller_->PendingSnapshotCompleted();
+  // As long as snapshots are enabled for this tab, there are two situations
+  // that allow for a navigation event to start a snapshot:
+  // 1) There is a request on hold waiting for the page to be minimally loaded.
+  if (snapshots_enabled_ && downloads_snapshot_on_hold_) {
+    downloads_snapshot_on_hold_ = false;
+    SaveSnapshotForDownloads(false);
+    return;
+  }
+
+  // 2) There's no ongoing snapshot and a previous one was saved with lower
+  // expected quality than what would be possible now.
+  if (snapshots_enabled_ &&
+      (!downloads_ongoing_snapshot_info_ &&
+       downloads_latest_saved_snapshot_info_ &&
+       downloads_latest_saved_snapshot_info_->expected_page_quality <
+           snapshot_controller_->current_page_quality())) {
+    SaveSnapshotForDownloads(true);
+    return;
+  }
+
+  // Notify the controller that a snapshot was not started.
+  snapshot_controller_->PendingSnapshotCompleted();
 }
 
-// TODO(carlosk): There is still the possibility of overlapping snapshots with
-// some combinations of calls to ObserveAndDownloadCurrentPage and
-// StartSnapshot. It won't cause side effects beyond wasted resources and will
-// be dealt with later.
-void RecentTabHelper::SaveSnapshotForDownloads() {
+void RecentTabHelper::SaveSnapshotForDownloads(bool replace_latest) {
   DCHECK_NE(PageQuality::POOR, snapshot_controller_->current_page_quality());
-  DCHECK(downloads_latest_snapshot_info_);
 
-  // Requests the deletion of a potentially existing previous snapshot of this
-  // page.
-  std::vector<int64_t> ids{downloads_latest_snapshot_info_->request_id};
-  ContinueSnapshotWithIdsToPurge(downloads_latest_snapshot_info_.get(), ids);
+  if (replace_latest) {
+    // Start by requesting the deletion of the existing previous snapshot of
+    // this page.
+    DCHECK(downloads_latest_saved_snapshot_info_);
+    DCHECK(!downloads_ongoing_snapshot_info_);
+    downloads_ongoing_snapshot_info_ = base::MakeUnique<SnapshotProgressInfo>(
+        downloads_latest_saved_snapshot_info_->client_id,
+        downloads_latest_saved_snapshot_info_->request_id);
+    std::vector<int64_t> ids{downloads_latest_saved_snapshot_info_->request_id};
+    ContinueSnapshotWithIdsToPurge(downloads_ongoing_snapshot_info_.get(), ids);
+  } else {
+    // Otherwise go straight to saving the page.
+    DCHECK(downloads_ongoing_snapshot_info_);
+    ContinueSnapshotAfterPurge(downloads_ongoing_snapshot_info_.get(),
+                               OfflinePageModel::DeletePageResult::SUCCESS);
+  }
 }
 
 // This is the 1st step of a sequence of async operations chained through
@@ -263,9 +328,9 @@ void RecentTabHelper::SaveSnapshotForDownloads() {
 // 3) Snapshot the current web contents.
 // 4) Notify requesters about the final result of the operation.
 //
-// For last_n requests the sequence is started in 1); for downloads it starts in
-// 2). Step 4) might be called anytime during the chain for early termination in
-// case of errors.
+// For last_n requests the sequence is always started in 1). For downloads it
+// starts in either 2) or 3). Step 4) might be called anytime during the chain
+// for early termination in case of errors.
 void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
     SnapshotProgressInfo* snapshot_info,
     const std::vector<int64_t>& page_ids) {
@@ -281,7 +346,7 @@ void RecentTabHelper::ContinueSnapshotAfterPurge(
     OfflinePageModel::DeletePageResult result) {
   DCHECK_EQ(snapshot_url_, web_contents()->GetLastCommittedURL());
   if (result != OfflinePageModel::DeletePageResult::SUCCESS) {
-    ReportSnapshotCompleted(snapshot_info);
+    ReportSnapshotCompleted(snapshot_info, false);
     return;
   }
 
@@ -302,32 +367,40 @@ void RecentTabHelper::SavePageCallback(SnapshotProgressInfo* snapshot_info,
                                        int64_t offline_id) {
   DCHECK(snapshot_info->IsForLastN() ||
          snapshot_info->request_id == offline_id);
-  snapshot_info->page_snapshot_completed = (result == SavePageResult::SUCCESS);
-  ReportSnapshotCompleted(snapshot_info);
+  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) {
+    SnapshotProgressInfo* snapshot_info,
+    bool success) {
   if (snapshot_info->IsForLastN()) {
     DCHECK_EQ(snapshot_info, last_n_ongoing_snapshot_info_.get());
-    if (snapshot_info->page_snapshot_completed)
+    if (success)
       last_n_latest_saved_quality_ = snapshot_info->expected_page_quality;
     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);
+  ReportDownloadStatusToRequestCoordinator(snapshot_info, success);
+  if (success) {
+    downloads_latest_saved_snapshot_info_ =
+        std::move(downloads_ongoing_snapshot_info_);
+  } else {
+    downloads_ongoing_snapshot_info_.reset();
+  }
 }
 
 // Note: we cannot assume that snapshot_info == downloads_latest_snapshot_info_
 // because further calls made to ObserveAndDownloadCurrentPage will replace
 // downloads_latest_snapshot_info_ with a new instance.
 void RecentTabHelper::ReportDownloadStatusToRequestCoordinator(
-    SnapshotProgressInfo* snapshot_info) {
+    SnapshotProgressInfo* snapshot_info,
+    bool cancel_background_request) {
   DCHECK(snapshot_info);
   DCHECK(!snapshot_info->IsForLastN());
 
@@ -340,7 +413,7 @@ void RecentTabHelper::ReportDownloadStatusToRequestCoordinator(
   // It is OK to call these methods more then once, depending on
   // number of snapshots attempted in this tab helper. If the request_id is not
   // in the list of RequestCoordinator, these calls have no effect.
-  if (snapshot_info->page_snapshot_completed)
+  if (cancel_background_request)
     request_coordinator->MarkRequestCompleted(snapshot_info->request_id);
   else
     request_coordinator->EnableForOffliner(snapshot_info->request_id,