Service Worker: Handle same-scope, new script registration

Service Worker: Handle same-scope, new script registration

Before this patch, register() would delete an existing registration at
the scope if the script URL didn't match, and register a new one. This
overwriting creates a scenario where old tabs have a different
controller than new tabs, which the Service Worker spec avoids.

This patch implements the spec steps for same-scope, new script
register(). That means:

- If the existing registration is uninstalling, wait for that to
complete before doing anything.
- Create a new worker which becomes the installing worker of the
existing registration.

BUG=398355
TEST=https://codereview.chromium.org/480943002/

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

[falken, 2014-08-20 09:35:36]

PTAL

[nhiroki, 2014-08-21 01:57:17]

LGTM with nits!

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

https://codereview.chromium.org/472103003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:224: 
Just an idea: To explicitly ensure the step "5. Set
registration.[[Uninstalling]] to false.", how about adding
"DCHECK(!existing_registration->is_uninstalling());" here? (This requirement is
completely fulfilled at line 214 though).

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

https://codereview.chromium.org/472103003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:26: // roughly
corresponds to navigator.serviceWorker.registration.
"navigator.serviceWorker.registration" looks gone?

https://codereview.chromium.org/472103003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:39: virtual void
OnRegistrationFailed(ServiceWorkerRegistration* registration) {
nit: I'd prefer to wrap this at the beginning of the arg like
"OnRegistrationFinishedUninstalling" does.

[falken, 2014-08-21 02:40:39]

Thanks for reviewing.

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

https://codereview.chromium.org/472103003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:224: 
On 2014/08/21 01:57:17, nhiroki wrote:
> Just an idea: To explicitly ensure the step "5. Set
> registration.[[Uninstalling]] to false.", how about adding
> "DCHECK(!existing_registration->is_uninstalling());" here? (This requirement
is
> completely fulfilled at line 214 though).

Sounds good. Done.

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

https://codereview.chromium.org/472103003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:26: // roughly
corresponds to navigator.serviceWorker.registration.
On 2014/08/21 01:57:17, nhiroki wrote:
> "navigator.serviceWorker.registration" looks gone?

Good catch. I'll just delete that sentence.

https://codereview.chromium.org/472103003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:39: virtual void
OnRegistrationFailed(ServiceWorkerRegistration* registration) {
On 2014/08/21 01:57:17, nhiroki wrote:
> nit: I'd prefer to wrap this at the beginning of the arg like
> "OnRegistrationFinishedUninstalling" does.

Done (but git-cl format would revert it back)

[falken, 2014-08-21 14:25:33]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-08-21 14:26:55]

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

[commit-bot: I haz the power, 2014-08-21 15:27:29]

Committed patchset #8 (140001) as 291078

[dglazkov, 2014-08-21 16:18:10]

On 2014/08/21 at 15:27:29, commit-bot wrote:
> Committed patchset #8 (140001) as 291078

I think you broke
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@To...

[falken, 2014-08-21 16:26:23]

On 2014/08/21 16:18:10, dglazkov wrote:
> On 2014/08/21 at 15:27:29, commit-bot wrote:
> > Committed patchset #8 (140001) as 291078
> 
> I think you broke
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@To...

Oops, I totally overlooked that this must be a three-sided patch. I had a fixed
unregister-and-register-new-script ready to land in the Blink-side patch:
https://codereview.chromium.org/472103003/

Let me just revert this and deal with it tomorrow.

[falken, 2014-08-21 16:27:39]

A revert of this CL (patchset #8) has been created in
https://codereview.chromium.org/484783003/ by falken@chromium.org.

The reason for reverting is: Broke unregister-and-register-new-script.html. 
This must be done as a three-sided patch..

[dglazkov, 2014-08-21 16:30:42]

A revert of this CL (patchset #8) has been created in
https://codereview.chromium.org/487393003/ by dglazkov@chromium.org.

The reason for reverting is: Broke
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@To....

[michaeln, 2014-08-21 23:52:44]

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

https://codereview.chromium.org/472103003/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:223:
existing_registration->set_script_url(script_url_);
This can put the object in a strange state where .active, .installing, and
.waiting can have different script urls. Up till now scripturl has been an
invariant, is anything depending on that invariant> If this job fails (suppose
newScriptUrl is 404s), does this class get left in an incoherent state?

reg.url == newScript (in memory but not on disk)
reg.activeVersion.url == oldScript
there is no waiting or installing version and there is no job running

Maybe reg shouldn't have a scripturl datamember and instead the accessor should
reflects the url of its newest version?

[falken, 2014-08-22 11:08:00]

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

https://codereview.chromium.org/472103003/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:223:
existing_registration->set_script_url(script_url_);
Thanks for taking a look!

On 2014/08/21 23:52:44, michaeln wrote:
> This can put the object in a strange state where .active, .installing, and
> .waiting can have different script urls.

Yep, I'm trying to support .active, .installing, .waiting with different
script URLs.

> Up till now scripturl has been an
> invariant, is anything depending on that invariant> If this job fails (suppose
> newScriptUrl is 404s), does this class get left in an incoherent state?
> 
> reg.url == newScript (in memory but not on disk)
> reg.activeVersion.url == oldScript
> there is no waiting or installing version and there is no job running
> 
> Maybe reg shouldn't have a scripturl datamember and instead the accessor
should
> reflects the url of its newest version?

That's a very good idea, and I think it's what the spec intended. I commented
on GitHub for that 404 case.

I created a patch to remove script URL from SWRegistration
outright, it should land before attempting this patch:
https://codereview.chromium.org/501453002/