[Offline Pages] Turn Offliner::Cancel into an async operation to resolve conflicting assumptions

[Offline Pages] Turn Offliner::Cancel into an async operation to resolve conflicting assumptions
between the Offliner and RequestCoordinator.

Upon Offliner::Cancel, RequestCoordinator assumes that the offliner is free, but
offliner may be waiting until save operation to complete. This discrepancy causes
RequestCoordinator to bombard the offliner with all the pending requests, potentially
blocking the main thread for the save operation callback to be called and causing
all pending requests to be labeled as EXPIRED

BUG=694864
Review-Url: https://codereview.chromium.org/2715433006
Cr-Commit-Position: refs/heads/master@{#454740}
Committed: https://chromium.googlesource.com/chromium/src/+/9f04f39d54f599a346c59670f91923f4af05e416

[chili, 2017-02-23 21:40:30]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 21:41:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2017-02-23 21:49:54]

chili@chromium.org changed reviewers:
+ fgorski@chromium.org, petewil@chromium.org

[chili, 2017-02-23 21:49:55]

Pete and I discussed 2 possible quickfixes:

1. Turn cancel into async, and request coordinator gets notified when cancel
finishes

2. Make Cancel on BackgroundLoader synchronous, by moving the web_contents into
another pointer and clearing state, thereby allowing the loader to start another
load.

#2 was not chosen because there would be possibility of 2 web_contents for 1
offliner running at the same time, which is a case we might not expect nor have
tested for

[commit-bot: I haz the power, 2017-02-23 22:03:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:03:46]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Pete Williamson, 2017-02-24 00:35:46]

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:259:
active_request_->request_id());
Does HandleCancelRecordResultCallback() get called twice?  Once here, and once
from the HandleCancelStopStatusCallback?

If we do make a function, would RecordCancelResult() be better, since this isn't
a callback per-se, but a function that the callback calls?

We might be able to move the UMA stuff into a function that gets called here,
and the is_busy_ and active_request_ handling into
HandleCancelStopStatusCallback.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:510:
nextCallback.Run(offline_id);
This callback chaining is kind of painful maintenance wise, as it causes the
logic for each ongoing operation to be split up even more.

Maybe HandleCancelStopStatus() could become a function that is called by
StartSchedulerCallback or TryNextRequestCallback, and we could pass those
callbacks directly to cancel.

[fgorski, 2017-02-27 18:07:26]

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:114: //
OfflinePageModel::SavePage() operation. We just ignore the callback.
Is this TODO valid after your change?

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:279:
Cancel(base::Bind(
Please document that it works, because Bind will make a copy of request and we
should not worry about reset being called before cancel callback is.

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:287: int64_t
offline_id) {
I don't understand the signature to the cancel callback. Definitely a separate
CL if we want to fix it, but maybe a todo would make sense here.

Also, what if offline_id does not match the request id?

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.cc:195: int64_t
request_id = 0L;
0LL?

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.cc:244:
Cancel(base::Bind(&PrerenderingOffliner::HandleApplicationStateChangeCancel,
add the same comment as before please.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/offliner_stub.cc (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/offliner_stub.cc:36: callback.Run(0L);
0LL?

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:247: void
RequestCoordinator::StopPrerendering(
Extra thing to consider: Now that this function is asynchronous and execution
will be split, is there a chance someone calls it again, before it finishes?
Would that cause problems? Can we guard from such situation?

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:248: const
Offliner::CancelCallback& next_callback,
nit: cancel_callback.
Or something indicating that is the final callback for this chain.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.h:258: void
HandleCancelStopStatusCallback(
Given Pete's concerns in the implementation, perhaps you could add some
documentation to this section, explaining how the calls are chained, which can
in turn help you naming the functions appropriately.

[chili, 2017-02-28 00:44:14]

Need to think about callback chaining and organization a little bit more...

Will follow-up with more updates

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:114: //
OfflinePageModel::SavePage() operation. We just ignore the callback.
On 2017/02/27 18:07:25, fgorski wrote:
> Is this TODO valid after your change?

TODO still somewhat valid, since this is simply getting around the problem.  We
don't need to cancel a pending OfflinePageModel::SavePage() operation, but if we
can, we should.

Updated comment.

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:279:
Cancel(base::Bind(
On 2017/02/27 18:07:25, fgorski wrote:
> Please document that it works, because Bind will make a copy of request and we
> should not worry about reset being called before cancel callback is.

Done.

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:287: int64_t
offline_id) {
On 2017/02/27 18:07:25, fgorski wrote:
> I don't understand the signature to the cancel callback. Definitely a separate
> CL if we want to fix it, but maybe a todo would make sense here.
> 
> Also, what if offline_id does not match the request id?

Added a check.

The cancel callback was mainly for possible case of multiple requests at the
same time, so callback can update proper request. On second thought though, this
might be unnecessary.  I'll update in a separate CL.

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.cc:195: int64_t
request_id = 0L;
On 2017/02/27 18:07:25, fgorski wrote:
> 0LL?

Done.

https://codereview.chromium.org/2715433006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.cc:244:
Cancel(base::Bind(&PrerenderingOffliner::HandleApplicationStateChangeCancel,
On 2017/02/27 18:07:25, fgorski wrote:
> add the same comment as before please.

Done.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/offliner_stub.cc (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/offliner_stub.cc:36: callback.Run(0L);
On 2017/02/27 18:07:25, fgorski wrote:
> 0LL?

Done.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:247: void
RequestCoordinator::StopPrerendering(
On 2017/02/27 18:07:25, fgorski wrote:
> Extra thing to consider: Now that this function is asynchronous and execution
> will be split, is there a chance someone calls it again, before it finishes?
> Would that cause problems? Can we guard from such situation?

I *don't* think calling it again before it finishes will cause any problems.
Additional cancels, as it currently stands, will replace the callback on the
offliner, and only the last callback will be called.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:248: const
Offliner::CancelCallback& next_callback,
On 2017/02/27 18:07:25, fgorski wrote:
> nit: cancel_callback.
> Or something indicating that is the final callback for this chain.

Done.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:259:
active_request_->request_id());
On 2017/02/24 00:35:46, Pete Williamson wrote:
> Does HandleCancelRecordResultCallback() get called twice?  Once here, and once
> from the HandleCancelStopStatusCallback?
> 
> If we do make a function, would RecordCancelResult() be better, since this
isn't
> a callback per-se, but a function that the callback calls?
> 
> We might be able to move the UMA stuff into a function that gets called here,
> and the is_busy_ and active_request_ handling into
> HandleCancelStopStatusCallback.

It shouldn't get called twice:

We return on line 255, so if there's an offliner_ && is_busy_, it will call
HandleCancelStopStatusCallback, which will call HandleCancelRecordResultCallback

If there isn't an offliner_ or it's not busy, we will call
HandleCancelRecordResultCallback here directly.

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.h (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.h:258: void
HandleCancelStopStatusCallback(
On 2017/02/27 18:07:25, fgorski wrote:
> Given Pete's concerns in the implementation, perhaps you could add some
> documentation to this section, explaining how the calls are chained, which can
> in turn help you naming the functions appropriately. 

Done.

[Pete Williamson, 2017-02-28 22:50:15]

Changes look good, one of my comments is still waiting for a reply.

[chili, 2017-03-01 02:37:33]

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2715433006/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:510:
nextCallback.Run(offline_id);
On 2017/02/24 00:35:46, Pete Williamson wrote:
> This callback chaining is kind of painful maintenance wise, as it causes the
> logic for each ongoing operation to be split up even more.
> 
> Maybe HandleCancelStopStatus() could become a function that is called by
> StartSchedulerCallback or TryNextRequestCallback, and we could pass those
> callbacks directly to cancel.

Per offline discussion, I updated the chain so one of them is simply a function
rather than a callback part of a chain.

[chili, 2017-03-01 02:37:43]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 02:38:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 04:09:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 04:09:59]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[fgorski, 2017-03-01 17:47:43]

Almost there.

https://codereview.chromium.org/2715433006/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2715433006/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:514: void
RequestCoordinator::RecordUMAForCancel(
This method name and state changes in implementation of it does not match.

RecordUMAForCancel should do just that. A good test for it would be to have it
as a function in .cc.

This method should definitely be called something else.

https://codereview.chromium.org/2715433006/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:532:
active_request_.reset(nullptr);
would .reset() work?

[chili, 2017-03-01 19:00:20]

https://codereview.chromium.org/2715433006/diff/40001/components/offline_page...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2715433006/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:514: void
RequestCoordinator::RecordUMAForCancel(
On 2017/03/01 17:47:43, fgorski wrote:
> This method name and state changes in implementation of it does not match.
> 
> RecordUMAForCancel should do just that. A good test for it would be to have it
> as a function in .cc.
> 
> This method should definitely be called something else.

Running a blank on good names: how about RecordUMAAndUpdateStatusForCancel?

https://codereview.chromium.org/2715433006/diff/40001/components/offline_page...
components/offline_pages/core/background/request_coordinator.cc:532:
active_request_.reset(nullptr);
On 2017/03/01 17:47:43, fgorski wrote:
> would .reset() work?

Done.

[chili, 2017-03-01 21:56:02]

Updated the RecordUMA method name to simply UpdateStatusForCancel, per offline
discussion with Filip.

PTAL

[fgorski, 2017-03-01 22:03:58]

lgtm

[Pete Williamson, 2017-03-01 22:17:09]

lgtm

[chili, 2017-03-03 21:31:33]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 21:31:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 23:00:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 23:00:31]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chili, 2017-03-04 00:35:09]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 00:35:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 01:13:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 01:13:40]

Dry run: This issue passed the CQ dry run.

[chili, 2017-03-04 01:16:53]

The CQ bit was checked by chili@chromium.org

[chili, 2017-03-04 01:16:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2715433006/#ps120001
(title: "forgot to init")

[commit-bot: I haz the power, 2017-03-04 01:17:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 01:24:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488590213587040,
"parent_rev": "3d708d4ee896db39e60f4de411851d5d85dea9b1", "commit_rev":
"9f04f39d54f599a346c59670f91923f4af05e416"}

[commit-bot: I haz the power, 2017-03-04 01:24:59]

Description was changed from

==========
[Offline Pages] Turn Offliner::Cancel into an async operation to resolve
conflicting assumptions
between the Offliner and RequestCoordinator.

Upon Offliner::Cancel, RequestCoordinator assumes that the offliner is free, but
offliner may be waiting until save operation to complete. This discrepancy
causes
RequestCoordinator to bombard the offliner with all the pending requests,
potentially
blocking the main thread for the save operation callback to be called and
causing
all pending requests to be labeled as EXPIRED

BUG=694864
==========

to

==========
[Offline Pages] Turn Offliner::Cancel into an async operation to resolve
conflicting assumptions
between the Offliner and RequestCoordinator.

Upon Offliner::Cancel, RequestCoordinator assumes that the offliner is free, but
offliner may be waiting until save operation to complete. This discrepancy
causes
RequestCoordinator to bombard the offliner with all the pending requests,
potentially
blocking the main thread for the save operation callback to be called and
causing
all pending requests to be labeled as EXPIRED

BUG=694864

Review-Url: https://codereview.chromium.org/2715433006
Cr-Commit-Position: refs/heads/master@{#454740}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f04f39d54f599a346c59670f919...
==========

[commit-bot: I haz the power, 2017-03-04 01:25:01]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9f04f39d54f599a346c59670f919...