[Offline pages] Removing active_request_ from request coordinator

chrome/browser/android/offline_pages/background_loader_offliner.cc

Index: chrome/browser/android/offline_pages/background_loader_offliner.cc
diff --git a/chrome/browser/android/offline_pages/background_loader_offliner.cc b/chrome/browser/android/offline_pages/background_loader_offliner.cc
index 74d0f08af3773e979efb8e6c79bc350401de8e94..93c05e457efb4be89980dcea1b6e3d09994bef40 100644
--- a/chrome/browser/android/offline_pages/background_loader_offliner.cc
+++ b/chrome/browser/android/offline_pages/background_loader_offliner.cc
@@ -190,32 +190,33 @@ bool BackgroundLoaderOffliner::LoadAndSave(
 }
 
 void BackgroundLoaderOffliner::Cancel(const CancelCallback& callback) {
+  DCHECK(pending_request_);
+  // We ignore the case where pending_request_ is not set, but given the checks
+  // in RequestCoordinator this should not happen.
+  if (!pending_request_)

[Pete Williamson, 2017/04/24 17:41:37]

I know it shouldn't happen, but if it does somehow, shouldn't we still call the
callback?

[fgorski, 2017/04/24 21:40:58]

Done.
I addressed that by changing the interface of Cancel to return a bool,
just like the other 2 methods. If there is a follow up callback, it will
happen when we return true. False means there will not be a follow up,
which means we should trigger final_callback logic immediately in call site.

+    return;
+
   // TODO(chili): We are not able to cancel a pending
   // OfflinePageModel::SaveSnapshot() operation. We will notify caller that
   // cancel completed once the SavePage operation returns.
-  if (!pending_request_) {
-    callback.Run(0LL);
-    return;
-  }
-
   if (save_state_ != NONE) {
     save_state_ = DELETE_AFTER_SAVE;
     cancel_callback_ = callback;
     return;
   }
 
-  int64_t request_id = pending_request_->request_id();
+  SavePageRequest canceled_request(*pending_request_.get());
   ResetState();
-  callback.Run(request_id);
+  callback.Run(canceled_request);
 }
 
-bool BackgroundLoaderOffliner::HandleTimeout(const SavePageRequest& request) {
+bool BackgroundLoaderOffliner::HandleTimeout(int64_t request_id) {
   if (pending_request_) {
-    DCHECK(request.request_id() == pending_request_->request_id());
-    if (is_low_bar_met_ &&
-        (request.started_attempt_count() + 1 >= policy_->GetMaxStartedTries() ||
-         request.completed_attempt_count() + 1 >=
-             policy_->GetMaxCompletedTries())) {
+    DCHECK(request_id == pending_request_->request_id());
+    if (is_low_bar_met_ && (pending_request_->started_attempt_count() + 1 >=

[Pete Williamson, 2017/04/24 17:41:37]

nit - the new line wraps make the code a bit more difficult to read than the old
line wraps.  I'm assuming you got here via git cl format, in which case, there
isn't much point in changing it back, since git cl format will just keep
changing it to this way.  If by chance it was not by using git cl format,
consider the old indentation scheme.

[fgorski, 2017/04/24 21:40:58]

It was through git cl format.

+                                policy_->GetMaxStartedTries() ||
+                            pending_request_->completed_attempt_count() + 1 >=
+                                policy_->GetMaxCompletedTries())) {
       // If we are already in the middle of a save operation, let it finish
       // but do not return SAVED_ON_LAST_RETRY
       if (save_state_ == NONE) {
@@ -402,7 +403,7 @@ void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
-    cancel_callback_.Run(request.request_id());
+    cancel_callback_.Run(request);
     return;
   }
 
@@ -446,23 +447,25 @@ void BackgroundLoaderOffliner::OnApplicationStateChange(
       application_state ==
           base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
     DVLOG(1) << "App became active, canceling current offlining request";
-    SavePageRequest* request = pending_request_.get();
-    // This works because Bind will make a copy of request, and we
-    // should not have to worry about reset being called before cancel callback.
     Cancel(base::Bind(
         &BackgroundLoaderOffliner::HandleApplicationStateChangeCancel,
-        weak_ptr_factory_.GetWeakPtr(), *request));
+        weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void BackgroundLoaderOffliner::HandleApplicationStateChangeCancel(
-    const SavePageRequest& request,
-    int64_t offline_id) {
+    const SavePageRequest& canceled_request) {
+  DCHECK_EQ(pending_request_->request_id(), canceled_request.request_id());
   // If for some reason the request was reset during while waiting for callback
   // ignore the completion callback.
-  if (pending_request_ && pending_request_->request_id() != offline_id)
+  if (pending_request_ &&
+      pending_request_->request_id() != canceled_request.request_id()) {
     return;
-  completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
+  }
+  // TODO(chili, petewil): So where is the reset state called, and might that be

[chili, 2017/04/24 17:44:53]

The reset state is handled in the cancel:

ApplicationStateChange triggers OnApplicationStateChange in line 444
which calls Cancel if there is a pending request
::
which calls ResetState if we're not in a save
which calls HandleApplicationStateChangeCancel on callback

OR

which sets DELETE_AFTER_SAVE if we're in a save
which calls ResetState after the save completes
which calls HandleApplicationStateChangeCancel

[fgorski, 2017/04/24 21:40:58]

The problem is that pending_request_ is already reset when we get to the
callback.
I am fixing that.

+  // the reason for wez's problems?
+  completion_callback_.Run(canceled_request,

[Pete Williamson, 2017/04/24 17:41:37]

Was Wez using the background loader offliner?  (I think that means was he
running dev or canary or beta when he saw the bug)?

[fgorski, 2017/04/24 21:40:58]

Per our discussion it wasn't the bug filed by wez, but we still have a problem
here because Cancel wipes the pending_request_ and completion_callback_
never gets called.

It would not matter where the code was, as this is duplicated between the
offliners.

+                           RequestStatus::FOREGROUND_CANCELED);
 }
 
 void BackgroundLoaderOffliner::AddLoadingSignal(const char* signal_name) {