ServiceWorker: Add a function to abort all pending jobs in SWJobCoordinator

ServiceWorker: Add a function to abort all pending jobs in SWJobCoordinator

This will be used in the corruption recovery process.
The process try to shutdown ServiceWorkerCoordinator and abort pending jobs
retained by the coordinator to reject waiting promises.


BUG=371675
TEST=content_unittest --gtest_filter=ServiceWorkerJobTest.AbortAll_*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278677

[nhiroki, 2014-06-18 17:14:23]

Hi, can you take a look? Thanks.

[michaeln, 2014-06-19 01:12:33]

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_job_coordinator.h:76:
RegistrationJobMap jobs_;
drive by: would it make sense to rename this to job_queues_

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:68: void
ServiceWorkerRegisterJob::Abort() {
The Complete() method does cleanup related to not leaving other objects in odd
states (resolving the promise is one of those cleanups). It might make sense to
always go thru the Complete() method to perform that cleanup.

[falken, 2014-06-19 03:45:56]

Looks good, I agree with Michael's comment.

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:625: 
extra newline?

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:68: void
ServiceWorkerRegisterJob::Abort() {
On 2014/06/19 01:12:32, michaeln wrote:
> The Complete() method does cleanup related to not leaving other objects in odd
> states (resolving the promise is one of those cleanups). It might make sense
to
> always go thru the Complete() method to perform that cleanup.

Yes, looks reasonable to go through Complete with SERVICE_WORKER_ERROR_ABORT
status.

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_unregister_job.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:40:
it->Run(SERVICE_WORKER_ERROR_ABORT);
Seems that we can go through Complete here too so FinishJob is called.

[nhiroki, 2014-06-19 05:05:56]

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:68: void
ServiceWorkerRegisterJob::Abort() {
On 2014/06/19 03:45:56, falken wrote:
> On 2014/06/19 01:12:32, michaeln wrote:
> > The Complete() method does cleanup related to not leaving other objects in
odd
> > states (resolving the promise is one of those cleanups). It might make sense
> to
> > always go thru the Complete() method to perform that cleanup.
> 
> Yes, looks reasonable to go through Complete with SERVICE_WORKER_ERROR_ABORT
> status.

Hmmm... I noticed this might cause a messy thing... Complete() calls
Coordinator::FinishJob() that deletes elements in JobQueue and JobQueue itself,
that is, JobQueue::AbortAll() indirectly delete itself. Generally speaking,
self-destruction makes difficult to maintain resource lifetime. I'll think about
a better way to share cleanup code and to manage object lifetime simpler.

[nhiroki, 2014-06-19 05:53:09]

Thank you for reviewing. Addressed comments and added test cases. Can you take
another look?

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_job_coordinator.h:76:
RegistrationJobMap jobs_;
On 2014/06/19 01:12:32, michaeln wrote:
> drive by: would it make sense to rename this to job_queues_

+1, renamed.

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:625: 
On 2014/06/19 03:45:55, falken wrote:
> extra newline?

Done.

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:68: void
ServiceWorkerRegisterJob::Abort() {
On 2014/06/19 05:05:55, nhiroki wrote:
> On 2014/06/19 03:45:56, falken wrote:
> > On 2014/06/19 01:12:32, michaeln wrote:
> > > The Complete() method does cleanup related to not leaving other objects in
> odd
> > > states (resolving the promise is one of those cleanups). It might make
sense
> > to
> > > always go thru the Complete() method to perform that cleanup.
> > 
> > Yes, looks reasonable to go through Complete with SERVICE_WORKER_ERROR_ABORT
> > status.
> 
> Hmmm... I noticed this might cause a messy thing... Complete() calls
> Coordinator::FinishJob() that deletes elements in JobQueue and JobQueue
itself,
> that is, JobQueue::AbortAll() indirectly delete itself. Generally speaking,
> self-destruction makes difficult to maintain resource lifetime. I'll think
about
> a better way to share cleanup code and to manage object lifetime simpler.

Updated. In the latest patchset, Abort() calls only CompleteInternal() and
JobQueue deletes queued jobs on behalf of Coordinator::FinishJob().

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_unregister_job.cc (right):

https://codereview.chromium.org/345583002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:40:
it->Run(SERVICE_WORKER_ERROR_ABORT);
On 2014/06/19 03:45:56, falken wrote:
> Seems that we can go through Complete here too so FinishJob is called.

Done.

[falken, 2014-06-19 08:10:57]

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_job_coordinator.h:40: // Jobs are
removed whenever they are finished or canceled.
Now we have three verbs "abort", "finish", and "cancel". These names and
comments should be more clear. How about something like:

// Calls ServiceWorkerRegisterJobBase::Abort() on all jobs and removes them.
AbortAll()

// Removes the job. A job that was not aborted must call FinishJob when it is
done.
FinishJob()

WDYT??

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:635:
ASSERT_EQ(scoped_refptr<ServiceWorkerRegistration>(), registration2);
nit: would it make sense to make change these last two ASSERTs to EXPECTs?

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:699:
ASSERT_EQ(scoped_refptr<ServiceWorkerRegistration>(), registration);
maybe this one too

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:70:
SetPhase(COMPLETE);
This looks awkward, can we remove the SetPhase(COMPLETE)?

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:373:
context_->job_coordinator()->FinishJob(pattern_, this);
I think this is OK. Another option could be to keep FinishJob along with
everything else, then just say:

if (phase_ != ABORT)
  FinishJob()

with a comment that AbortAll takes care of removing the jobs from the queue.
Then we don't need CompleteInternal. WDYT?

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job_base.h:21: // Aborts
the job. This method should be called once and only once per job.
Maybe add "It can be called regardless of whether Start() was called."

[nhiroki, 2014-06-19 10:04:05]

Updated. Can you take another look? Thanks!

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_job_coordinator.h (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_job_coordinator.h:40: // Jobs are
removed whenever they are finished or canceled.
On 2014/06/19 08:10:56, falken wrote:
> Now we have three verbs "abort", "finish", and "cancel". These names and
> comments should be more clear. How about something like:
> 
> // Calls ServiceWorkerRegisterJobBase::Abort() on all jobs and removes them.
> AbortAll()
> 
> // Removes the job. A job that was not aborted must call FinishJob when it is
> done.
> FinishJob()
> 
> WDYT??

SGTM. Updated.

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:635:
ASSERT_EQ(scoped_refptr<ServiceWorkerRegistration>(), registration2);
On 2014/06/19 08:10:56, falken wrote:
> nit: would it make sense to make change these last two ASSERTs to EXPECTs?

Done.

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_job_unittest.cc:699:
ASSERT_EQ(scoped_refptr<ServiceWorkerRegistration>(), registration);
On 2014/06/19 08:10:56, falken wrote:
> maybe this one too

Done.

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:70:
SetPhase(COMPLETE);
On 2014/06/19 08:10:56, falken wrote:
> This looks awkward, can we remove the SetPhase(COMPLETE)?

Done.

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:373:
context_->job_coordinator()->FinishJob(pattern_, this);
On 2014/06/19 08:10:56, falken wrote:
> I think this is OK. Another option could be to keep FinishJob along with
> everything else, then just say:
> 
> if (phase_ != ABORT)
>   FinishJob()
> 
> with a comment that AbortAll takes care of removing the jobs from the queue.
> Then we don't need CompleteInternal. WDYT?

Updated. I think new code looks simpler than the previous one except
"SetPhase(COMPLETE)" handling in the top of Complete(). Anyway, I'm OK with new
version.

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job_base.h (right):

https://codereview.chromium.org/345583002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job_base.h:21: // Aborts
the job. This method should be called once and only once per job.
On 2014/06/19 08:10:56, falken wrote:
> Maybe add "It can be called regardless of whether Start() was called."

Done.

https://codereview.chromium.org/345583002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_unregister_job.cc (right):

https://codereview.chromium.org/345583002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:38:
CompleteInternal(SERVICE_WORKER_ERROR_ABORT);
UnregisterJob doesn't have the phase, so if we want to leave Complete() as is,
new flag might be necessary to determine whether we call FinishJob() or not in
Complete(). Wdyt?

[falken, 2014-06-19 10:33:35]

LGTM, thanks!

https://codereview.chromium.org/345583002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_unregister_job.cc (right):

https://codereview.chromium.org/345583002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:38:
CompleteInternal(SERVICE_WORKER_ERROR_ABORT);
On 2014/06/19 10:04:05, nhiroki wrote:
> UnregisterJob doesn't have the phase, so if we want to leave Complete() as is,
> new flag might be necessary to determine whether we call FinishJob() or not in
> Complete(). Wdyt?

Either way is fine... I think what you had originally may be better (my original
comment was because I thought we had to call FinishJob()), but it's up to you.

[nhiroki, 2014-06-20 05:20:31]

On 2014/06/19 10:33:35, falken wrote:
> LGTM, thanks!
> 
>
https://codereview.chromium.org/345583002/diff/80001/content/browser/service_...
> File content/browser/service_worker/service_worker_unregister_job.cc (right):
> 
>
https://codereview.chromium.org/345583002/diff/80001/content/browser/service_...
> content/browser/service_worker/service_worker_unregister_job.cc:38:
> CompleteInternal(SERVICE_WORKER_ERROR_ABORT);
> On 2014/06/19 10:04:05, nhiroki wrote:
> > UnregisterJob doesn't have the phase, so if we want to leave Complete() as
is,
> > new flag might be necessary to determine whether we call FinishJob() or not
in
> > Complete(). Wdyt?
> 
> Either way is fine... I think what you had originally may be better (my
original
> comment was because I thought we had to call FinishJob()), but it's up to you.

I see... to make consistent with UnregisterJob, please let me bring back the
Complete/CompleteInternal part. Thanks.

[nhiroki, 2014-06-20 05:20:43]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-06-20 05:25:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/345583002/120001

[commit-bot: I haz the power, 2014-06-20 11:16:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-06-20 11:37:20]

Change committed as 278677