[Offline pages] Removing active_request_ from request coordinator

[Offline pages] Removing active_request_ from request coordinator

This patch removes active_request_ from RequestCoordinator.
Referring to it when updating state of the database was a source
of error in the past. This is safe to do with a minor tweak to
Offliner interface (mainly cancel callback), because it already
keeps a copy of a pending request.

It turns out that simply keeping an ID of the currently processed
request is enough. This opens us to offlining multiple things in
parallel in future, as we can keep a map of id to offliner.

Relevant offliner implementations and tests are adjusted for this
change.

#fixit This bug is part of Paquete fixit effort.

BUG=714262
R=chili@chromium.org,petewil@chromium.org

Review-Url: https://codereview.chromium.org/2836863002
Cr-Commit-Position: refs/heads/master@{#467013}
Committed: https://chromium.googlesource.com/chromium/src/+/3d9408b382d593aa59f01dc5cb9ae8f0bc564e1d

[fgorski, 2017-04-24 05:59:44]

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

[commit-bot: I haz the power, 2017-04-24 05:59:58]

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-04-24 06:21:25]

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

[commit-bot: I haz the power, 2017-04-24 06:21:26]

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...)

[fgorski, 2017-04-24 17:01:16]

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

[commit-bot: I haz the power, 2017-04-24 17:02:30]

Dry run: CQ is trying da patch.

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

[fgorski, 2017-04-24 17:05:38]

PTAL

[fgorski, 2017-04-24 17:09:00]

Description was changed from

==========
[Offline pages] Removing active_request_ from request coordinator

BUG=714262
R=chili@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Removing active_request_ from request coordinator

This patch removes active_request_ from RequestCoordinator.
Referring to it when updating state of the database was a source
of error in the past. This is safe to do with a minor tweak to
Offliner interface (mainly cancel callback), because it already
keeps a copy of a pending request.

It turns out that simply keeping an ID of the currently processed
request is enough. This opens us to offlining multiple things in
parallel in future, as we can keep a map of id to offliner.

Relevant offliner implementations and tests are adjusted for this
change.

#fixit This bug is part of Paquete fixit effort.

BUG=714262
R=chili@chromium.org,petewil@chromium.org
==========

[Pete Williamson, 2017-04-24 17:41:38]

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:196: if
(!pending_request_)
I know it shouldn't happen, but if it does somehow, shouldn't we still call the
callback?

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:216: if
(is_low_bar_met_ && (pending_request_->started_attempt_count() + 1 >=
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.

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:467:
completion_callback_.Run(canceled_request,
Was Wez using the background loader offliner?  (I think that means was he
running dev or canary or beta when he saw the bug)?

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:259: if
(!pending_request_)
Shouldn't we be running the callback even if there is no pending request?

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:322:
weak_ptr_factory_.GetWeakPtr()));
If we don't pass the pending request here, how is the input parameter for
HandleApplicationStateChangeCancel getting set?  I missed it somewhere.

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

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:465: //
TODO(chili, petewil): So where is the reset state called, and might that be
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

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:322:
weak_ptr_factory_.GetWeakPtr()));
On 2017/04/24 17:41:38, Pete Williamson wrote:
> If we don't pass the pending request here, how is the input parameter for
> HandleApplicationStateChangeCancel getting set?  I missed it somewhere.

I think it's simply passing in the pending_request_

[commit-bot: I haz the power, 2017-04-24 18:33:56]

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

[commit-bot: I haz the power, 2017-04-24 18:33:56]

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-04-24 21:39:09]

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

[commit-bot: I haz the power, 2017-04-24 21:39:45]

Dry run: CQ is trying da patch.

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

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

PTAL.

Because I may have changed callback for cancel call, there might be some tests
to fix, but I couldn't find anything obvious locally.

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:196: if
(!pending_request_)
On 2017/04/24 17:41:37, Pete Williamson wrote:
> I know it shouldn't happen, but if it does somehow, shouldn't we still call
the
> callback?

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.

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:216: if
(is_low_bar_met_ && (pending_request_->started_attempt_count() + 1 >=
On 2017/04/24 17:41:37, Pete Williamson wrote:
> 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.

It was through git cl format.

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:465: //
TODO(chili, petewil): So where is the reset state called, and might that be
On 2017/04/24 17:44:53, chili wrote:
> 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

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

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:467:
completion_callback_.Run(canceled_request,
On 2017/04/24 17:41:37, Pete Williamson wrote:
> Was Wez using the background loader offliner?  (I think that means was he
> running dev or canary or beta when he saw the bug)?

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.

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:259: if
(!pending_request_)
On 2017/04/24 17:41:38, Pete Williamson wrote:
> Shouldn't we be running the callback even if there is no pending request?

Handled by signature change as explained earlier.

https://codereview.chromium.org/2836863002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:322:
weak_ptr_factory_.GetWeakPtr()));
On 2017/04/24 17:44:53, chili wrote:
> On 2017/04/24 17:41:38, Pete Williamson wrote:
> > If we don't pass the pending request here, how is the input parameter for
> > HandleApplicationStateChangeCancel getting set?  I missed it somewhere.
> 
> I think it's simply passing in the pending_request_

Cathy is correct here, the value comes from Cancel and is captured prior
to resetting it.
I am doing the same fix as I did in BLO.

[commit-bot: I haz the power, 2017-04-24 22:36:03]

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

[commit-bot: I haz the power, 2017-04-24 22:36:04]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-04-24 23:40:45]

lgtm

[fgorski, 2017-04-25 16:31:34]

Cathy is sick, but she expressed no concerns offline. I'll try to land this.

[fgorski, 2017-04-25 16:31:40]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2017-04-25 16:32:48]

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-04-25 16:36:33]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493137900606900,
"parent_rev": "32e2eec87fcd9c791791f4e67c76a85eab7f9ed8", "commit_rev":
"3d9408b382d593aa59f01dc5cb9ae8f0bc564e1d"}

[commit-bot: I haz the power, 2017-04-25 16:36:47]

Description was changed from

==========
[Offline pages] Removing active_request_ from request coordinator

This patch removes active_request_ from RequestCoordinator.
Referring to it when updating state of the database was a source
of error in the past. This is safe to do with a minor tweak to
Offliner interface (mainly cancel callback), because it already
keeps a copy of a pending request.

It turns out that simply keeping an ID of the currently processed
request is enough. This opens us to offlining multiple things in
parallel in future, as we can keep a map of id to offliner.

Relevant offliner implementations and tests are adjusted for this
change.

#fixit This bug is part of Paquete fixit effort.

BUG=714262
R=chili@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Removing active_request_ from request coordinator

This patch removes active_request_ from RequestCoordinator.
Referring to it when updating state of the database was a source
of error in the past. This is safe to do with a minor tweak to
Offliner interface (mainly cancel callback), because it already
keeps a copy of a pending request.

It turns out that simply keeping an ID of the currently processed
request is enough. This opens us to offlining multiple things in
parallel in future, as we can keep a map of id to offliner.

Relevant offliner implementations and tests are adjusted for this
change.

#fixit This bug is part of Paquete fixit effort.

BUG=714262
R=chili@chromium.org,petewil@chromium.org

Review-Url: https://codereview.chromium.org/2836863002
Cr-Commit-Position: refs/heads/master@{#467013}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d9408b382d593aa59f01dc5cb9a...
==========

[commit-bot: I haz the power, 2017-04-25 16:36:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3d9408b382d593aa59f01dc5cb9a...