RecentTabHelper won't accept overlapping requests from Downloads anymore.

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 38 matching lines @@
   bool GetTabId(content::WebContents* web_contents, int* tab_id) override {
     return offline_pages::OfflinePageUtils::GetTabId(web_contents, tab_id);
   }
 };
 }  // namespace
 
 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),
       delegate_(new DefaultDelegate()),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
 RecentTabHelper::~RecentTabHelper() {
 }
 
 void RecentTabHelper::SetDelegate(
     std::unique_ptr<RecentTabHelper::Delegate> delegate) {
   DCHECK(delegate);
   delegate_ = std::move(delegate);
 }
 
 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
 // TabHelper - so can't initialize in constructor because of uncertain order
 // of creation of TabHelpers.
 bool RecentTabHelper::EnsureInitialized() {
   if (snapshot_controller_)  // Initialized already.
     return snapshots_enabled_;
 
   snapshot_controller_.reset(
@@ skipping 22 matching lines @@
 void RecentTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() ||
       !navigation_handle->HasCommitted()) {
     return;
   }
 
   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.
   snapshot_url_ = web_contents()->GetLastCommittedURL();
 
   // 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() &&
@@ skipping 13 matching lines @@
   EnsureInitialized();
   snapshot_controller_->DocumentAvailableInMainFrame();
 }
 
 void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
   EnsureInitialized();
   snapshot_controller_->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();
 }
 
 // TODO(carlosk): this method is also called when the tab is being closed, when
 // saving a snapshot is probably useless (low probability of the user undoing
 // the close). We should detect that and avoid the saving.
 void RecentTabHelper::WasHidden() {
   if (!IsOffliningRecentPagesEnabled())
     return;
 
   // Return immediately if last_n is not listening to tab hidden events or if a
   // last_n snapshot is currently being saved.
   if (!last_n_listen_to_tab_hidden_ || last_n_ongoing_snapshot_info_)
     return;
 
   // Do not save if page quality is too low or if we already have a snapshot
   // with the current quality level.
   // Note: we assume page quality for a page can only increase.
   PageQuality current_quality = snapshot_controller_->current_page_quality();
   if (current_quality == PageQuality::POOR ||
       current_quality == last_n_latest_saved_quality_) {
     return;
   }
 
   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(
       GetRecentPagesClientId(),
       base::Bind(&RecentTabHelper::ContinueSnapshotWithIdsToPurge,
                  weak_ptr_factory_.GetWeakPtr(),
                  last_n_ongoing_snapshot_info_.get()));
 }
 
 // TODO(carlosk): rename this to RequestSnapshot and make it return a bool
 // representing the acceptance of the snapshot request.
 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
 // callbacks, mostly shared between last_n and downloads:
 // 1) Compute the set of old 'last_n' pages that have to be purged.
 // 2) Delete the pages found in the previous step.
 // 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) {
   DCHECK(snapshot_info);
 
   page_model_->DeletePagesByOfflineId(
       page_ids, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
                            weak_ptr_factory_.GetWeakPtr(), snapshot_info));
 }
 
 void RecentTabHelper::ContinueSnapshotAfterPurge(
     SnapshotProgressInfo* snapshot_info,
     OfflinePageModel::DeletePageResult result) {
   DCHECK_EQ(snapshot_url_, web_contents()->GetLastCommittedURL());
   if (result != OfflinePageModel::DeletePageResult::SUCCESS) {
-    ReportSnapshotCompleted(snapshot_info);
+    ReportSnapshotCompleted(snapshot_info, false);
     return;
   }
 
   snapshot_info->expected_page_quality =
       snapshot_controller_->current_page_quality();
   OfflinePageModel::SavePageParams save_page_params;
   save_page_params.url = snapshot_url_;
   save_page_params.client_id = snapshot_info->client_id;
   save_page_params.proposed_offline_id = snapshot_info->request_id;
   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() ||
          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());
 
   RequestCoordinator* request_coordinator =
       RequestCoordinatorFactory::GetForBrowserContext(
           web_contents()->GetBrowserContext());
   if (!request_coordinator)
     return;
 
   // 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,
                                            snapshot_info->client_id);
 }
 
 ClientId RecentTabHelper::GetRecentPagesClientId() const {
   return ClientId(kLastNNamespace, tab_id_);
 }
 
 }  // namespace offline_pages