[Offline pages] Removing active_request_ from request coordinator

chrome/browser/android/offline_pages/background_loader_offliner.cc

 // Copyright 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/background_loader_offliner.h"
 
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "base/time/time.h"
@@ skipping 172 matching lines @@
   loader_.get()->LoadPage(request.url());
 
   snapshot_controller_.reset(
       new SnapshotController(base::ThreadTaskRunnerHandle::Get(), this,
                              kOfflineDomContentLoadedMs, page_delay_ms_));
 
   return true;
 }
 
 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) {
         did_snapshot_on_last_retry_ = true;
         StartSnapshot();
       }
       return true;
     }
   }
   return false;
@@ skipping 166 matching lines @@
                                            int64_t offline_id) {
   if (!pending_request_)
     return;
 
   SavePageRequest request(*pending_request_.get());
   bool did_snapshot_on_last_retry = did_snapshot_on_last_retry_;
   ResetState();
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
-    cancel_callback_.Run(request.request_id());
+    cancel_callback_.Run(request);
     return;
   }
 
   save_state_ = NONE;
 
   Offliner::RequestStatus save_status;
   if (save_result == SavePageResult::SUCCESS) {
     if (did_snapshot_on_last_retry)
       save_status = RequestStatus::SAVED_ON_LAST_RETRY;
     else
@@ skipping 23 matching lines @@
   content::WebContentsObserver::Observe(contents);
   OfflinerData::AddToWebContents(contents, this);
 }
 
 void BackgroundLoaderOffliner::OnApplicationStateChange(
     base::android::ApplicationState application_state) {
   if (pending_request_ && is_low_end_device_ &&
       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) {
   base::TimeTicks current_time = base::TimeTicks::Now();
   base::TimeDelta delay_so_far = current_time - load_start_time_;
   // We would prefer to use int64_t here, but JSON does not support that type.
   // Given the choice between int and double, we choose to implicitly convert to
   // a double since it maintains more precision (we can get a longer time in
   // milliseconds than we can with a 2 bit int, 53 bits vs 32).
   double delay = delay_so_far.InMilliseconds();
   signal_data_.SetDouble(signal_name, delay);
 }
 
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);