Service Worker: set active worker to REDUNDANT when unregistered

Service Worker: set active worker to REDUNDANT when unregistered

As per spec update:
https://github.com/slightlyoff/ServiceWorker/issues/353

Now the active worker is set to REDUNDANT when unregistered. If it has a
controllee, this happens when it no longer has a controllee.

This patch adds a Doom function to ServiceWorkerVersion which takes
care of setting REDUNDANT and purging resources once there's
no controllee. This obviates the need for ServiceWorkerStorage to
listen for NoControllees and purge resources then.

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

[falken, 2014-07-09 08:47:13]

WDYT of this change?

It moves some of Unregister steps logic to Storage. I think it makes sense since
all spec steps that want to set Redundant state will go through this path
(except maybe installingWorker, which can stay in UnregisterJob).

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

https://codereview.chromium.org/377153003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:259: void
DecommissionVersion(int64 version_id,
suggestions on this naming are welcome. My other thought was
MakeVersionRedundant but it sounds like it's copying a worker.

[michaeln, 2014-07-10 00:55:23]

> WDYT of this change?

I wonder if the version class can take care of more of the cleanup then it
currently is?

void Version::Doom() {
 is_doomed_ = true;
 if (!controllees) {
  SetState(REDUNDANT);
  storage()->StartPurgingResources(script_cache_map->ResourceList());
 }
}

and if is_doomed, upon no controllees, it does those two things for it self.
maybe the storage class doesn't have to listen to the nocontrollee signal for
this purpose

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

https://codereview.chromium.org/377153003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:699:
DecommissionVersion(deleted_version_id, newly_purgeable_resources);
presuming two things:
* higher level job classes have taken care of Doom'ing versions
* versions take care of making the state transition to redundant and initiating
the purge of its script_cached resources

(ok, maybe that was three)

i think this would only need to concern itself with initiating the purge of
resources when there is no live version. and the awkward block of code to start
purging in NotifyInsallingDone could be removed too.

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

https://codereview.chromium.org/377153003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:76:
ServiceWorkerVersion::REDUNDANT);
i think we have to Doom the active version if there is a waiting version. as
coded, the storage class will Doom at most one version per registration.

[falken, 2014-07-10 14:05:12]

Thanks for the detailed feedback! I like this direction and gave it a try, you
can see in the updated patchset (still needs some polishing)

With this approach, I'm worried about getting into a weird state where you doom
a version but the registration for it hasn't been deleted it, i.e., you delete
from the disk cache but the records are still in the database.

I think this can happen if the ordering is wrong:
version->Doom()
storage->DeleteRegistration()

It seems fragile. To fix this, I'm thinking instead of Doom() calling
StartPurgeResources() it does something like PurgeVersion(reg_id, version_id,
resources). This posts a db task that:
- deletes the registration, if the stored version id is |version_id|
- moves any uncommitted resources in |resources| to purgeable list
- moves any committed resource in |resources| to the purgeable list

when that task is done, it calls StartPurgeResources.

WDYT?

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

https://codereview.chromium.org/377153003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:699:
DecommissionVersion(deleted_version_id, newly_purgeable_resources);
On 2014/07/10 00:55:23, michaeln wrote:
> presuming two things:
> * higher level job classes have taken care of Doom'ing versions
> * versions take care of making the state transition to redundant and
initiating
> the purge of its script_cached resources
> 
> (ok, maybe that was three)
> 
> i think this would only need to concern itself with initiating the purge of
> resources when there is no live version. and the awkward block of code to
start
> purging in NotifyInsallingDone could be removed too.

I think that block kind of makes sense, it's a special case because it's the
only time you must clean up uncommitted resources. If I remove that block and
try to have a general purpose PurgeResources that Version::Doom calls, I think
PurgeResources would have to try to clean up both uncommitted and purgeable
resources. On the other hand, I'm kind of leaning toward a general
Storage::PurgeVersion(version_id, resources) function now.

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

https://codereview.chromium.org/377153003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:76:
ServiceWorkerVersion::REDUNDANT);
On 2014/07/10 00:55:23, michaeln wrote:
> i think we have to Doom the active version if there is a waiting version. as
> coded, the storage class will Doom at most one version per registration.

I thought this would work because when an updated version boots out the existing
one (waiting gets stored, active gets deleted), the storage class would wait for
the NoControllees signal from the active and doom it then.

[michaeln, 2014-07-11 01:15:42]

This new CL looks a lot better to me! This one lgtm.

On 2014/07/10 14:05:12, falken wrote:
> Thanks for the detailed feedback! I like this direction and gave it a try, you
> can see in the updated patchset (still needs some polishing)
> 
> With this approach, I'm worried about getting into a weird state where you
doom
> a version but the registration for it hasn't been deleted it, i.e., you delete
> from the disk cache but the records are still in the database.

I don't see that as a problem. We need to Doom (redundantify and
deleteresources) versions for registrations that are not deleted. If callers
Doom things that shouldn't be Doomed... thats a bug that should be fixed.

> I think that block kind of makes sense, it's a special case because it's the
> only time you must clean up uncommitted resources. If I remove that block and
> try to have a general purpose PurgeResources that Version::Doom calls, I think
> PurgeResources would have to try to clean up both uncommitted and purgeable
> resources. On the other hand, I'm kind of leaning toward a general
> Storage::PurgeVersion(version_id, resources) function now.

I think we may really only need one of those two lists? Prior to adding anything
new to the list in a new sessions, we start the process of actually purging the
resource in that list. As we know we no longer need a resource, it gets added to
that list and is scheduled for purging in that session. After things really are
purged, they're removed from that list.

That seems like a simplication that could come later.

> I thought this would work because when an updated version boots out the
existing
> one (waiting gets stored, active gets deleted), the storage class would wait
for
> the NoControllees signal from the active and doom it then.

hmmm, that might be right? having the version take care of itself makes it
easier to reasona bout though.

[michaeln, 2014-07-11 01:16:05]

oops forgot the inline comments

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:395:
registration()->UnsetVersion(new_version());
I think the call to Doom() could go in here.

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:585:
registration_->SetWaitingVersion(NULL);
maybe drop the registration_ altogether by setting it to null, the
registration_id_ data memember is still avialble

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:226: //
ServiceWorkerDatabase (or is queued to be).
I don't think the part about 'should only be called after deleting the reg' is
correct. When at active version is replaced with an updated version for one
examaple, another is when a new version of an established registration fails. In
both cases, those versions will be put into the redundant status w/o the
registration being deleted.

[michaeln, 2014-07-11 01:16:53]

the cl description needs to be fixed up

[nhiroki, 2014-07-11 03:57:57]

lgtm

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:71: // TODO:
Also doom installing version.
'installing version' has already implemented. Can you doom it, too?

[nhiroki, 2014-07-11 07:59:13]

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:87: virtual void
OnNoControllees(ServiceWorkerVersion* version) {};
According to grep, this looks no longer necessary.

[falken, 2014-07-11 10:53:32]

Thanks for the comments! Response to non-inline comment first...

On 2014/07/11 01:15:42, michaeln wrote:
> This new CL looks a lot better to me! This one lgtm.
> 
> On 2014/07/10 14:05:12, falken wrote:
> > Thanks for the detailed feedback! I like this direction and gave it a try,
you
> > can see in the updated patchset (still needs some polishing)
> > 
> > With this approach, I'm worried about getting into a weird state where you
> doom
> > a version but the registration for it hasn't been deleted it, i.e., you
delete
> > from the disk cache but the records are still in the database.
> 
> I don't see that as a problem. We need to Doom (redundantify and
> deleteresources) versions for registrations that are not deleted. If callers
> Doom things that shouldn't be Doomed... thats a bug that should be fixed.

Hmm, maybe I'm saying "delete a registration" too broadly. I meant before
the "RES:(version_id)(resource_id)" keys have been deleted (which currently
only happens when a registration is deleted or overwritten by a new one).

If you Doom() before DeleteRegistration(), wouldn't the following happen?
Doom triggers the following:
- version's resources are deleted from disk cache
- PRES:(resource_id) keys are "deleted" (but they didn't exist yet)
DeleteRegistration triggers the following:
- RES keys are moved to PRES keys

So you end up with PRES keys that won't ever be cleaned up.

That's what I meant by fragile and why I thought DoomVersion may need
do to a more thorough general-purpose cleanup.

Anyway, maybe this can be fix with a simplification of uncommitted/purgeable
later.
I'm happy to land now and iterate.

> 
> > I think that block kind of makes sense, it's a special case because it's the
> > only time you must clean up uncommitted resources. If I remove that block
and
> > try to have a general purpose PurgeResources that Version::Doom calls, I
think
> > PurgeResources would have to try to clean up both uncommitted and purgeable
> > resources. On the other hand, I'm kind of leaning toward a general
> > Storage::PurgeVersion(version_id, resources) function now.
> 
> I think we may really only need one of those two lists? Prior to adding
anything
> new to the list in a new sessions, we start the process of actually purging
the
> resource in that list. As we know we no longer need a resource, it gets added
to
> that list and is scheduled for purging in that session. After things really
are
> purged, they're removed from that list.
> 
> That seems like a simplication that could come later.
> 
> > I thought this would work because when an updated version boots out the
> existing
> > one (waiting gets stored, active gets deleted), the storage class would wait
> for
> > the NoControllees signal from the active and doom it then.
> 
> hmmm, that might be right? having the version take care of itself makes it
> easier to reasona bout though.

[falken, 2014-07-11 12:08:12]

Thanks, I'll CQ this. Note that this will change some layout tests:
https://codereview.chromium.org/381383005/

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:395:
registration()->UnsetVersion(new_version());
On 2014/07/11 01:16:05, michaeln wrote:
> I think the call to Doom() could go in here.

I think you're right. We can't have new_version() if registration() is null.

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:585:
registration_->SetWaitingVersion(NULL);
On 2014/07/11 01:16:05, michaeln wrote:
> maybe drop the registration_ altogether by setting it to null, the
> registration_id_ data memember is still avialble

Done.

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_unregister_job.cc:71: // TODO:
Also doom installing version.
On 2014/07/11 03:57:57, nhiroki wrote:
> 'installing version' has already implemented. Can you doom it, too?

I know it's implemented :) I decided to make it a TODO, as this patch is already
making fairly big changes. It can be an easy next patch for someone.

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

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:87: virtual void
OnNoControllees(ServiceWorkerVersion* version) {};
On 2014/07/11 07:59:13, nhiroki wrote:
> According to grep, this looks no longer necessary.

Done.

https://codereview.chromium.org/377153003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:226: //
ServiceWorkerDatabase (or is queued to be).
On 2014/07/11 01:16:05, michaeln wrote:
> I don't think the part about 'should only be called after deleting the reg' is
> correct. When at active version is replaced with an updated version for one
> examaple, another is when a new version of an established registration fails.
In
> both cases, those versions will be put into the redundant status w/o the
> registration being deleted.

I meant more that the resource ids should already have been moved to the
purgeable list. I'll delete that comment for now.

[falken, 2014-07-11 12:10:08]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-07-11 12:10:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/377153003/80001

[commit-bot: I haz the power, 2014-07-11 14:02:21]

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

[commit-bot: I haz the power, 2014-07-11 15:18:22]

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

[commit-bot: I haz the power, 2014-07-11 15:18:23]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[falken, 2014-07-11 15:29:04]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-07-11 15:29:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/377153003/80001

[commit-bot: I haz the power, 2014-07-11 16:42:05]

Change committed as 282636