[ServiceWorker] Check is_uninstalled in ServiceWorkerRegisterJob::ContinueWithRegistration()

[ServiceWorker] Check is_uninstalled in ServiceWorkerRegisterJob::ContinueWithRegistration()

In the following situations the registration may be already uninstalled.
So existing_registration->GetNewestVersion() could return NULL and crash in ContinueWithRegistration().

- register the ServiceWorker
- open the page in the ServiceWorker scope
- unregister the ServiceWorker
- register the ServiceWorker to the same scope and close the page quickly.

ServiceWorkerRegisterJob::Start() is called to register the ServiceWorker.
 next_step is set to ContinueWithRegistration.
 GetUninstallingRegistration() returns the uninstalling registration.
 RunSoon() calls PostTask(next_step)

ServiceWorkerRegistration::Clear() is called because there is no controllee.

ContinueWithRegistration() is executed.
But at this moment the registration is already uninstalled.
And crashed while calling existing_registration->GetNewestVersion()->script_url().

https://codereview.chromium.org/506043002/ is reverted because of this crash.
So I created the revert of revert cl.
https://codereview.chromium.org/547743003

BUG=398355, 408048
TEST=http/tests/serviceworker/unregister-then-register-new-script.html http/tests/serviceworker/register-same-scope-different-script-url.html in https://codereview.chromium.org/547743003

Committed: https://crrev.com/fa444bb81a6489cf52730d3aaa0bcd34367d2685
Cr-Commit-Position: refs/heads/master@{#294540}

[horo, 2014-09-11 12:34:20]

horo@chromium.org changed reviewers:
+ nhiroki@chromium.org

[horo, 2014-09-11 12:34:21]

nhiroki@

[horo, 2014-09-11 12:34:44]

nhiroki@
Could you please review?

[nhiroki, 2014-09-11 12:53:31]

nhiroki@chromium.org changed reviewers:
+ michaeln@chromium.org

[nhiroki, 2014-09-11 12:53:31]

Let me add michaeln@ as a reviewer for double check...

[nhiroki, 2014-09-11 13:05:38]

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

https://codereview.chromium.org/556003006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.h:142: bool
is_deleted_;
I wonder if we can use |is_deleted| for this purpose. WDYT?

[horo, 2014-09-11 17:50:14]

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

https://codereview.chromium.org/556003006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.h:142: bool
is_deleted_;
On 2014/09/11 13:05:38, nhiroki wrote:
> I wonder if we can use |is_deleted| for this purpose. WDYT?

No we can't.

is_deleted_ flag is set when the deletion of the records from the db is started.
is_uninstalled_ flag is set when the all controlled pages are closed.

[[Register]] must wait until this uninstall process.
So we have to check is_uninstalled_.

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#register-alg...
> 7.2.3
> Wait until the Record {[[key]], [[value]]} entry of its
[[ScopeToRegistrationMap]] where registation.scope matches entry.[[key]] is
deleted.

> [[ScopeToRegistrationMap]] represents an internal map structure that stores
the entries of the Record {[[key]], [[value]]} where [[key]] is a string that
represents a URL scope and [[value]] is a ServiceWorkerRegistration object.

[michaeln, 2014-09-11 19:31:41]

lgtm, nice bug fix!

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

https://codereview.chromium.org/556003006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.cc:319:
is_uninstalled_ = true;
Would it make sense to set this flag immediately after line 290? Between there
and here, a whole lot of observer methods are called and they'll see a reg that
is not uninstalling and also not uninstalled. Thats inconsistent with the new
found way to represent is_uninstalled.

[horo, 2014-09-12 01:09:26]

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

https://codereview.chromium.org/556003006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.cc:319:
is_uninstalled_ = true;
On 2014/09/11 19:31:40, michaeln wrote:
> Would it make sense to set this flag immediately after line 290? Between there
> and here, a whole lot of observer methods are called and they'll see a reg
that
> is not uninstalling and also not uninstalled. Thats inconsistent with the new
> found way to represent is_uninstalled.

That's make sense.
Done.

Thank you.

[nhiroki, 2014-09-12 02:08:11]

On 2014/09/11 17:50:14, horo wrote:
>
https://codereview.chromium.org/556003006/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_registration.h (right):
> 
>
https://codereview.chromium.org/556003006/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_registration.h:142: bool
> is_deleted_;
> On 2014/09/11 13:05:38, nhiroki wrote:
> > I wonder if we can use |is_deleted| for this purpose. WDYT?
> 
> No we can't.
> 
> is_deleted_ flag is set when the deletion of the records from the db is
started.
> is_uninstalled_ flag is set when the all controlled pages are closed.
> 
> [[Register]] must wait until this uninstall process.
> So we have to check is_uninstalled_.
> 
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#register-alg...
> > 7.2.3
> > Wait until the Record {[[key]], [[value]]} entry of its
> [[ScopeToRegistrationMap]] where registation.scope matches entry.[[key]] is
> deleted.
> 
> > [[ScopeToRegistrationMap]] represents an internal map structure that stores
> the entries of the Record {[[key]], [[value]]} where [[key]] is a string that
> represents a URL scope and [[value]] is a ServiceWorkerRegistration object.

I see, thanks! LGTM

(Enum values might be better if those booleans are exclusive.)

[horo, 2014-09-12 02:51:21]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-12 02:52:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556003006/20001

[commit-bot: I haz the power, 2014-09-12 03:57:19]

Committed patchset #2 (id:20001) as 43eccb0a9ba938522307f4ecdb18147ccbaaf996

[commit-bot: I haz the power, 2014-09-12 04:00:28]

Patchset 2 (id:??) landed as
https://crrev.com/fa444bb81a6489cf52730d3aaa0bcd34367d2685
Cr-Commit-Position: refs/heads/master@{#294540}