Service Worker: in Unregister, wait until after the active worker no longer controls a document

Service Worker: in Unregister, wait until after the active worker no longer controls a document

As per spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#unregister-algorithm

Unregister shouldn't doom the active worker or delete the registration until
the active worker no longer controls a document. This means that if register()
is called before that happens, the unregistration is effectively canceled.

BUG=388095
TEST=https://codereview.chromium.org/416003003/

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

[falken, 2014-07-24 09:46:44]

This is still a bit WIP but what do you think?

One issue is that if the browser shuts down before the document unload happens,
the unregistration is cancelled. To do it right, I think I must persist the
uninstalling bit to storage.

[michaeln, 2014-07-25 00:02:04]

looks promising

> One issue is that if the browser shuts down before the document unload
happens,
> the unregistration is cancelled. To do it right, I think I must persist the
> uninstalling bit to storage.

i think persist the unintsalling bit === delete the registration from the db

https://codereview.chromium.org/413063004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.cc:175: void
ServiceWorkerRegistration::StartUninstall() {
let's harmonize on naming, i'm using

ActivateWaitingVersionWhenReady()
ActivateWaitingVersion()

sticking with that theme, something like

UninstallActiveVersionWhenReady()
UninstallActiveVersion()

https://codereview.chromium.org/413063004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/413063004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.h:63: bool
is_uninstalling() const { return is_uninstalling_; }
i think the addition of this is very good since it matches the spec, i think
this can replace my the usage of is_deleted() in my cl

[nhiroki, 2014-07-25 03:34:52]

LGTM after renaming functions as michael mentioned.

[falken, 2014-07-25 12:47:26]

Thanks for the comments!

On 2014/07/25 00:02:04, michaeln wrote:
> looks promising
> 
> > One issue is that if the browser shuts down before the document unload
> happens,
> > the unregistration is cancelled. To do it right, I think I must persist the
> > uninstalling bit to storage.
> 
> i think persist the unintsalling bit === delete the registration from the db

yeah, I think that would work. My plan is to revise the patch to call
DeleteRegistration without waiting. Then if the uninstalling bit switches from
true to false in RegisterJob, you call StoreRegistration with the live
registration's installed version, or else activated version, or else don't call
it.

Some work may be needed to remove the "resurrected" version's resources from the
purgeable list, and to call UpdateVersionToActive if needed. Looks doable.

https://codereview.chromium.org/413063004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.cc:175: void
ServiceWorkerRegistration::StartUninstall() {
On 2014/07/25 00:02:04, michaeln wrote:
> let's harmonize on naming, i'm using
> 
> ActivateWaitingVersionWhenReady()
> ActivateWaitingVersion()
> 
> sticking with that theme, something like
> 
> UninstallActiveVersionWhenReady()
> UninstallActiveVersion()

Sounds good.

[falken, 2014-07-28 12:18:01]

Uploaded new patch that deletes and restores from storage.

The spec also changed a bit to have a [[ClearRegistration]] algo. This patch
rearranges the code to more obviously follow the spec.

[michaeln, 2014-07-29 03:33:39]

i see you've got a layout test over in the blink repo for one case where
scripturl is the same, might be nice to cover the case where the url is
different too. idk if this is more amenable to unittesting or layout testing?

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

https://codereview.chromium.org/413063004/diff/20002/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:224:
existing_registration->AbortPendingClear();
what if extisting.script != new.script?

https://codereview.chromium.org/413063004/diff/20002/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/20002/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:95: void
ServiceWorkerRegistration::OnNoControllees(ServiceWorkerVersion* version) {
our CL's collide on this method :( 

so we'll have to 'coagulate' these changes, i've hit the CQ on mine but it may
bounce (ya never know).

[falken, 2014-07-29 05:36:22]

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

https://codereview.chromium.org/413063004/diff/20002/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:224:
existing_registration->AbortPendingClear();
On 2014/07/29 03:33:39, michaeln wrote:
> what if extisting.script != new.script?

Great catch, this is a big point. There are two cases to handle:
1) Installation of the new worker fails. To conform to the spec, the
registration should be left untouched (except .installing is nulled).
2) Installation of the new worker succeeds. To conform to the spec, the new
worker becomes the waiting worker for the registration.

Regardless of unregister, I don't think we do 1) or 2) correctly today. In 1) we
delete the registration and in 2) the new worker immediately proceeds to
activation.

I think we need the live registrations to be able to mutate script url over its
lifetime. Or else, when we create a new registration set its active version to
the deleted registration's active version.

Since we don't handle these correctly, I'm inclined to proceed with this patch
as long as there's reasonable behavior and fix for spec compliance in future
patches. I'll write some tests and see.

[falken, 2014-07-29 14:04:05]

This is back to WIP, I rebased on Michael's recent patch which required some
merging. Unfortunately now my test is crashing, have to figure out why.

I added more tests for registering a different script URL. It turns out to have
the same behavior regardless of whether you unregistered or not. A later patch
could fix the behavior to be spec-compliant (both if you unregistered before or
not).

https://codereview.chromium.org/413063004/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:160: }
It may be better for Registration to always listen to its active_version. Then
in NoControllees it always Clears() if is_uninstall is set, or activates the
waiting version if it exists. Then we don't need this bookkeeping about when to
listen.

[falken, 2014-07-30 09:45:30]

Think I got this working now. PTAL.
(did you have comments before? I noticed the "Modified" timestamp was updated
but no comments published)

The test crashing was due to some other bug that was improbably triggered by the
value of "scope" in the test file. I set "scope" equal to the name of the test
file, and things blew up (found a real bug, but unrelated to this patch).

[michaeln, 2014-07-31 00:08:07]

looks like the resurrecting behavior is going to change, i'd like to see how
that settles?

https://codereview.chromium.org/413063004/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:160: }
On 2014/07/29 14:04:05, falken wrote:
> It may be better for Registration to always listen to its active_version. Then
> in NoControllees it always Clears() if is_uninstall is set, or activates the
> waiting version if it exists. Then we don't need this bookkeeping about when
to
> listen.

sgtm

https://codereview.chromium.org/413063004/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:145: return
active_version() && (should_activate_when_ready_ || is_uninstalling_);
there's some subtlety to this expression and about when it really is and isn't
valid, maybe an explicit flag for whether the listerer has actually been
attached and rename this method to ShouldBeListenengingForNoControllees?

i think this could simpliflfy/clarify aspects of the
SetShouldActivateWhenReady() and SetUninstalling() methods too

https://codereview.chromium.org/413063004/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:154: if (listening
== IsListeningForNoControllees())
very small nit: testing bools for equality always looks odd to me, maybe get rid
of one of them?

bool is_listening = IsListening();
should_activate_ = shld;
bool should_listen = IsListening();

if (is_listening && !should_listen)
 rmvlistener;
else if (should_listen && !is_listening)
 addlisteneer;

[falken, 2014-07-31 03:27:10]

On 2014/07/31 00:08:07, michaeln wrote:
> looks like the resurrecting behavior is going to change, i'd like to see how
> that settles?
> 

Agreed, let me put this on old until there's a better signal of direction from
https://github.com/slightlyoff/ServiceWorker/issues/396

Thanks for reviewing.

[falken, 2014-08-08 07:37:59]

This is ready for another look! PTAL.

[falken, 2014-08-11 11:27:26]

On 2014/08/08 07:37:59, falken wrote:
> This is ready for another look! PTAL.

ping!

[nhiroki, 2014-08-11 12:24:51]

LGTM with nits

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:189: // "1. Set
registration.[[Uninstalling]] to false."
"1. Set ..." -> "2. Set ..."

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:192: // "2. If
scriptURL is equal to registration.[[ScriptURL]], then:"
"1. If scriptURL" would be correct.

This has the different item level from the previous one. To avoid confusion, how
about removing the number, or adding parents number? (eg. "8.1. If scriptURL
...") Wdyt?

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:63: bool
is_uninstalling() const { return is_uninstalling_; }
nit: can you move this after "set_is_deleted()" to make a consistent with the
declaration order?

[michaeln, 2014-08-12 02:15:36]

lgtm, i have a few comments

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:205:
context_->storage()->NotifyUninstallingRegistration(this);
I think the call to DeleteRegistration(id()) should go in here instead of buried
in the Notify method. This method is named as an actor, the Notify method not so
much.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:216:
context_->storage()->NotifyAbortedUninstallingRegistration(this);
Ditto the call to (re) StoreRegistration() should go directly in this method of
action (instead of the passive notify method).

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:628:
ServiceWorkerRegistration* registration) {
maybe a DCHECK to assert that reg is not already being uninstalled

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:631:
DeleteRegistration(registration->id(),
I was wondering where this method call went and was surprised to find it here!

I think the logic to invoke DeleteRegistration() should be moved to the caller
of this Notify method.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:639:
installing_registrations_[registration->id()] = registration;
should this be a call to NotifyInstallingRegistratio(reg)?

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:941:
installing_registrations_.erase(registration_id);
And this be a call to  NotifyDoneInstallingRegistration(...)

othewise,  how does the value get removed from this map in the error case?

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:90:
scoped_refptr<ServiceWorkerRegistration> GetUninstallingRegistration(
nit: can this be a rawptr so there's that much less refcount churn in the world

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:153: void
NotifyUninstalledRegistration(ServiceWorkerRegistration* registration);
for consistency, can this be NotifyDoneXxxxx

[falken, 2014-08-12 09:06:07]

Addressed feedback. I think I'll CQ this, but since the patch changed a bit in
response to feedback it may be good to take another look. If anyone has
comments, you can go ahead and uncheck CQ or I'll address them in the next
patch.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:192: // "2. If
scriptURL is equal to registration.[[ScriptURL]], then:"
On 2014/08/11 12:24:51, nhiroki wrote:
> "1. If scriptURL" would be correct.
> 
> This has the different item level from the previous one. To avoid confusion,
how
> about removing the number, or adding parents number? (eg. "8.1. If scriptURL
> ...") Wdyt?

Good catches about the spec quotes. We're currently missing some steps which
causes the quotes to be rather chaotic and unordered. I intend to make the
function more clearly follow the spec in a future patch for
http://crbug.com/398355

For now, I've removed the numbering.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:205:
context_->storage()->NotifyUninstallingRegistration(this);
On 2014/08/12 02:15:35, michaeln wrote:
> I think the call to DeleteRegistration(id()) should go in here instead of
buried
> in the Notify method. This method is named as an actor, the Notify method not
so
> much.

I see your point. Done.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:216:
context_->storage()->NotifyAbortedUninstallingRegistration(this);
On 2014/08/12 02:15:35, michaeln wrote:
> Ditto the call to (re) StoreRegistration() should go directly in this method
of
> action (instead of the passive notify method).

Done.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:63: bool
is_uninstalling() const { return is_uninstalling_; }
On 2014/08/11 12:24:51, nhiroki wrote:
> nit: can you move this after "set_is_deleted()" to make a consistent with the
> declaration order?

Done, moved some functions up above active_version() for greater consistency.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:628:
ServiceWorkerRegistration* registration) {
On 2014/08/12 02:15:36, michaeln wrote:
> maybe a DCHECK to assert that reg is not already being uninstalled

Done. Also added to the other Notify* functions for more robust checking.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:639:
installing_registrations_[registration->id()] = registration;
On 2014/08/12 02:15:36, michaeln wrote:
> should this be a call to NotifyInstallingRegistratio(reg)?

Done. I decided for symmetry to make SWRegistration responsible for calling both
NotifyInstallingRegistration and NotifyDoneInstallingRegistration, since it will
call StoreRegistration now.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:941:
installing_registrations_.erase(registration_id);
On 2014/08/12 02:15:36, michaeln wrote:
> And this be a call to  NotifyDoneInstallingRegistration(...)
> 
> othewise,  how does the value get removed from this map in the error case?

My idea actually was to leave it in the map, that way the live reg is still
available for this browser session. Now I realize that's a bad user experience
that you think the SW is installed then it suddenly is gone when you restart the
browser, and trying to live with the error case is a bad idea anyway.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:90:
scoped_refptr<ServiceWorkerRegistration> GetUninstallingRegistration(
On 2014/08/12 02:15:36, michaeln wrote:
> nit: can this be a rawptr so there's that much less refcount churn in the
world

Ah of course. Done.

https://codereview.chromium.org/413063004/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:153: void
NotifyUninstalledRegistration(ServiceWorkerRegistration* registration);
On 2014/08/12 02:15:36, michaeln wrote:
> for consistency, can this be NotifyDoneXxxxx

Done.

https://codereview.chromium.org/413063004/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/413063004/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:174:
DCHECK(most_recent_version);
My earlier patches handled the case where there is no waiting or active version.
But I think that's an impossible case. The only window for waiting/active to
change is from the end of an unregister() and the beginning of a register() for
the same-scope. Since only RegisterJob mutates waiting/active, I don't expect
them to be cleared.

If code changes someday, the DCHECK will trip and we can deal with it then. I
don't want to add code to handle an impossible condition because it's a deep
rabbit hole (should we set is_deleted_ to false? what about 'installing'? etc)

[falken, 2014-08-12 09:24:30]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-08-12 09:27:05]

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

[commit-bot: I haz the power, 2014-08-12 13:42:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-12 15:54:48]

Change committed as 288994