If MHTML saving is cancelled, delete the page afterwards.

If MHTML saving is cancelled, delete the page afterwards.

BUG=705097
Review-Url: https://codereview.chromium.org/2850943002
Cr-Commit-Position: refs/heads/master@{#468515}
Committed: https://chromium.googlesource.com/chromium/src/+/2915191f0a64f34fcdf8c4e4a0a5d89b2271d816

[Pete Williamson, 2017-04-28 21:32:31]

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

[Pete Williamson, 2017-04-28 22:31:20]

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

[commit-bot: I haz the power, 2017-04-28 22:32:10]

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-28 23:49:38]

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

[commit-bot: I haz the power, 2017-04-28 23:49:39]

Dry run: This issue passed the CQ dry run.

[chili, 2017-04-29 00:11:33]

general lgtm

question to consider in comments

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

https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
offline_ids, base::Bind(&LocalDeletePageCallback));
should we only run the cancel_callback_.Run once the delete completes?

Do we have to worry about a situation where the subsequent pending_request
starts to save before delete completes?

[Pete Williamson, 2017-04-29 00:14:44]

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

https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
offline_ids, base::Bind(&LocalDeletePageCallback));
On 2017/04/29 00:11:33, chili wrote:
> should we only run the cancel_callback_.Run once the delete completes?
> 
> Do we have to worry about a situation where the subsequent pending_request
> starts to save before delete completes?

My thinking here is that it is fine to return control.  We only need to clean up
the page.  If we get another request for the same page, it will have a different
request ID, so the page being cleaned up should not interefere with the
operation.  If another request starts to save while deleting the page, they
should also be independent.

[chili, 2017-04-29 00:34:56]

On 2017/04/29 00:14:44, Pete Williamson wrote:
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> File chrome/browser/android/offline_pages/background_loader_offliner.cc
(right):
> 
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
> offline_ids, base::Bind(&LocalDeletePageCallback));
> On 2017/04/29 00:11:33, chili wrote:
> > should we only run the cancel_callback_.Run once the delete completes?
> > 
> > Do we have to worry about a situation where the subsequent pending_request
> > starts to save before delete completes?
> 
> My thinking here is that it is fine to return control.  We only need to clean
up
> the page.  If we get another request for the same page, it will have a
different
> request ID, so the page being cleaned up should not interefere with the
> operation.  If another request starts to save while deleting the page, they
> should also be independent.

What if we get the same request again? (there's only 1 in the queue)?

[Pete Williamson, 2017-04-29 00:52:48]

On 2017/04/29 00:34:56, chili wrote:
> On 2017/04/29 00:14:44, Pete Williamson wrote:
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > File chrome/browser/android/offline_pages/background_loader_offliner.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
> > offline_ids, base::Bind(&LocalDeletePageCallback));
> > On 2017/04/29 00:11:33, chili wrote:
> > > should we only run the cancel_callback_.Run once the delete completes?
> > > 
> > > Do we have to worry about a situation where the subsequent pending_request
> > > starts to save before delete completes?
> > 
> > My thinking here is that it is fine to return control.  We only need to
clean
> up
> > the page.  If we get another request for the same page, it will have a
> different
> > request ID, so the page being cleaned up should not interefere with the
> > operation.  If another request starts to save while deleting the page, they
> > should also be independent.
> 
> What if we get the same request again? (there's only 1 in the queue)?

We should not get the same request again, since it is being cancelled.  It is
getting removed from the request queue (which is independent of removing the
file) before any new requests are pulled from the queue.  If there is another
request in the queue for the same URL, it will have a different request_id, so
there will not be any file name collision.

[chili, 2017-04-29 04:36:16]

On 2017/04/29 00:52:48, Pete Williamson wrote:
> On 2017/04/29 00:34:56, chili wrote:
> > On 2017/04/29 00:14:44, Pete Williamson wrote:
> > >
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > > File chrome/browser/android/offline_pages/background_loader_offliner.cc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > > chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
> > > offline_ids, base::Bind(&LocalDeletePageCallback));
> > > On 2017/04/29 00:11:33, chili wrote:
> > > > should we only run the cancel_callback_.Run once the delete completes?
> > > > 
> > > > Do we have to worry about a situation where the subsequent
pending_request
> > > > starts to save before delete completes?
> > > 
> > > My thinking here is that it is fine to return control.  We only need to
> clean
> > up
> > > the page.  If we get another request for the same page, it will have a
> > different
> > > request ID, so the page being cleaned up should not interefere with the
> > > operation.  If another request starts to save while deleting the page,
they
> > > should also be independent.
> > 
> > What if we get the same request again? (there's only 1 in the queue)?
> 
> We should not get the same request again, since it is being cancelled.  It is
> getting removed from the request queue (which is independent of removing the
> file) before any new requests are pulled from the queue.  If there is another
> request in the queue for the same URL, it will have a different request_id, so
> there will not be any file name collision.

Cancelled in the offliner could mean user-cancelled or request coordinator
cancelled from timeout.  The former will be removed from the queue, but the
latter might result in the same request getting tried again?

[Pete Williamson, 2017-05-01 16:37:41]

On 2017/04/29 04:36:16, chili wrote:
> On 2017/04/29 00:52:48, Pete Williamson wrote:
> > On 2017/04/29 00:34:56, chili wrote:
> > > On 2017/04/29 00:14:44, Pete Williamson wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > > > File chrome/browser/android/offline_pages/background_loader_offliner.cc
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > > > chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
> > > > offline_ids, base::Bind(&LocalDeletePageCallback));
> > > > On 2017/04/29 00:11:33, chili wrote:
> > > > > should we only run the cancel_callback_.Run once the delete completes?
> > > > > 
> > > > > Do we have to worry about a situation where the subsequent
> pending_request
> > > > > starts to save before delete completes?
> > > > 
> > > > My thinking here is that it is fine to return control.  We only need to
> > clean
> > > up
> > > > the page.  If we get another request for the same page, it will have a
> > > different
> > > > request ID, so the page being cleaned up should not interefere with the
> > > > operation.  If another request starts to save while deleting the page,
> they
> > > > should also be independent.
> > > 
> > > What if we get the same request again? (there's only 1 in the queue)?
> > 
> > We should not get the same request again, since it is being cancelled.  It
is
> > getting removed from the request queue (which is independent of removing the
> > file) before any new requests are pulled from the queue.  If there is
another
> > request in the queue for the same URL, it will have a different request_id,
so
> > there will not be any file name collision.
> 
> Cancelled in the offliner could mean user-cancelled or request coordinator
> cancelled from timeout.  The former will be removed from the queue, but the
> latter might result in the same request getting tried again?

When the RequestCoordinator gets a watchdog timeout (HandleWatchdogTimeout), it
calls offliner->HandleTimeout(), not offliner_->Cancel(), so I think we are
safe.
Thanks for bringing up the concern though.  Does it seem good now, or do you
still
have any concerns?

[Pete Williamson, 2017-05-01 22:18:42]

On 2017/05/01 16:37:41, Pete Williamson wrote:
> On 2017/04/29 04:36:16, chili wrote:
> > On 2017/04/29 00:52:48, Pete Williamson wrote:
> > > On 2017/04/29 00:34:56, chili wrote:
> > > > On 2017/04/29 00:14:44, Pete Williamson wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > > > > File
chrome/browser/android/offline_pages/background_loader_offliner.cc
> > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2850943002/diff/1/chrome/browser/android/offl...
> > > > >
chrome/browser/android/offline_pages/background_loader_offliner.cc:423:
> > > > > offline_ids, base::Bind(&LocalDeletePageCallback));
> > > > > On 2017/04/29 00:11:33, chili wrote:
> > > > > > should we only run the cancel_callback_.Run once the delete
completes?
> > > > > > 
> > > > > > Do we have to worry about a situation where the subsequent
> > pending_request
> > > > > > starts to save before delete completes?
> > > > > 
> > > > > My thinking here is that it is fine to return control.  We only need
to
> > > clean
> > > > up
> > > > > the page.  If we get another request for the same page, it will have a
> > > > different
> > > > > request ID, so the page being cleaned up should not interefere with
the
> > > > > operation.  If another request starts to save while deleting the page,
> > they
> > > > > should also be independent.
> > > > 
> > > > What if we get the same request again? (there's only 1 in the queue)?
> > > 
> > > We should not get the same request again, since it is being cancelled.  It
> is
> > > getting removed from the request queue (which is independent of removing
the
> > > file) before any new requests are pulled from the queue.  If there is
> another
> > > request in the queue for the same URL, it will have a different
request_id,
> so
> > > there will not be any file name collision.
> > 
> > Cancelled in the offliner could mean user-cancelled or request coordinator
> > cancelled from timeout.  The former will be removed from the queue, but the
> > latter might result in the same request getting tried again?
> 
> When the RequestCoordinator gets a watchdog timeout (HandleWatchdogTimeout),
it
> calls offliner->HandleTimeout(), not offliner_->Cancel(), so I think we are
> safe.
> Thanks for bringing up the concern though.  Does it seem good now, or do you
> still
> have any concerns?

After offline discussion, Chili pointed out that after the call to
HandleTimeout, we sometimes call StopPrerendering(), which causes a call to
Cancel().  So, I changed it as Chili suggested to wait until the offline page
model delete operation is complete before returning to the caller.

[Pete Williamson, 2017-05-01 22:18:53]

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

[commit-bot: I haz the power, 2017-05-01 22:19:47]

Dry run: CQ is trying da patch.

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

[chili, 2017-05-01 22:34:05]

still lgtm

Thanks for making the changes!

[Dmitry Titov, 2017-05-01 23:00:38]

lgtm with a nit:

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

https://codereview.chromium.org/2850943002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:426: // We
don't return to the caller until the page has been removed from the
I think this comment is not needed, we do this kind of async chains all the time
and reader would naturally jump from here to the DeleteOfflinePageCallback which
invokes the cacncel_callback_, so no readability issue.

[Pete Williamson, 2017-05-01 23:35:13]

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

[commit-bot: I haz the power, 2017-05-01 23:36:13]

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-05-02 00:37:25]

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

[commit-bot: I haz the power, 2017-05-02 00:37:26]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-05-02 00:45:48]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2017-05-02 00:45:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
chili@chromium.org
Link to the patchset: https://codereview.chromium.org/2850943002/#ps80001
(title: "Fix unit tests on Android N5X swarming bot")

[commit-bot: I haz the power, 2017-05-02 00:46:20]

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-05-02 00:51:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493685948779710,
"parent_rev": "9bee9f62d8507206938b746e3987ee2c87d6190b", "commit_rev":
"2915191f0a64f34fcdf8c4e4a0a5d89b2271d816"}

[commit-bot: I haz the power, 2017-05-02 00:51:24]

Description was changed from

==========
If MHTML saving is cancelled, delete the page afterwards.

BUG=705097
==========

to

==========
If MHTML saving is cancelled, delete the page afterwards.

BUG=705097

Review-Url: https://codereview.chromium.org/2850943002
Cr-Commit-Position: refs/heads/master@{#468515}
Committed:
https://chromium.googlesource.com/chromium/src/+/2915191f0a64f34fcdf8c4e4a0a5...
==========

[commit-bot: I haz the power, 2017-05-02 00:51:25]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2915191f0a64f34fcdf8c4e4a0a5...