ServiceWorker: Ensure live registration during starting worker

ServiceWorker: Ensure live registration during starting worker

Starting service worker requires a reference to the live registration in
order to expose a registration object within SWGlobalScope but no one has
ensured the liveness of the registration during the startup sequence.

This CL makes SWVersion to ensure the live registration during the period.

BUG=465254
TEST=content_browsertests --gtest_filter=ServiceWorkerVersionBrowserTest.*
TEST=content_unittests --gtest_filter=ServiceWorkerVersion*

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

[nhiroki, 2015-03-11 08:36:17]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2015-03-11 08:36:48]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-03-11 09:03:18]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, michaeln@chromium.org

[nhiroki, 2015-03-11 09:08:09]

Hi michaeln@ and kinuko@, can you review this? Thanks.

(+falken@, just in case)

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:893:
weak_factory_.GetWeakPtr()));
kinuko@ or falken@: I'm not really sure whether this block can just be replaced
with StartWorker(). Any thoughts?

[kinuko, 2015-03-11 14:41:57]

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:893:
weak_factory_.GetWeakPtr()));
On 2015/03/11 09:08:08, nhiroki wrote:
> kinuko@ or falken@: I'm not really sure whether this block can just be
replaced
> with StartWorker(). Any thoughts?

It's minor and is probably ok, but it looks just calling StartWorker() slightly
changes the timeout behavior.

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

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:423: protect));
I wonder just keeping this 'protect' in start_callbacks_ (with dummy method)
might be easier? Then you don't need to worry about restarting case..

[michaeln, 2015-03-11 20:18:56]

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

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:423: protect));
On 2015/03/11 14:41:56, kinuko wrote:
> I wonder just keeping this 'protect' in start_callbacks_ (with dummy method)
> might be easier? Then you don't need to worry about restarting case..

That sounds pretty good provided it can be made to work with the other
start_callbacks_.push_back() callsite inside of StartTimeoutTimer() which is
also called by SetDevToolsAttached (i don't understand why the timer is started
in that method?).

[nhiroki, 2015-03-12 05:22:59]

Patchset #2 (id:60001) has been deleted

[nhiroki, 2015-03-12 05:27:11]

Thank you for reviewing. Can you take another look?

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:893:
weak_factory_.GetWeakPtr()));
On 2015/03/11 14:41:57, kinuko wrote:
> On 2015/03/11 09:08:08, nhiroki wrote:
> > kinuko@ or falken@: I'm not really sure whether this block can just be
> replaced
> > with StartWorker(). Any thoughts?
> 
> It's minor and is probably ok, but it looks just calling StartWorker()
slightly
> changes the timeout behavior.

Reverted this because it's no longer necessary to call StartWorker to ensure the
live registration. Instead, start callbacks owns the reference.

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

https://codereview.chromium.org/996123002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:423: protect));
On 2015/03/11 20:18:56, michaeln wrote:
> On 2015/03/11 14:41:56, kinuko wrote:
> > I wonder just keeping this 'protect' in start_callbacks_ (with dummy method)
> > might be easier? Then you don't need to worry about restarting case..

Good idea. Modified the CL in this way.

> That sounds pretty good provided it can be made to work with the other
> start_callbacks_.push_back() callsite inside of StartTimeoutTimer() which is
> also called by SetDevToolsAttached (i don't understand why the timer is
started
> in that method?).

Regarding SetDevToolsAttached, "!timeout_timer_.IsRunning()" means that the
worker has stopped (StopTimeoutTimer called only from OnStopped), so starting
the timer in the method wouldn't make sense.

[kinuko, 2015-03-12 15:50:34]

lgtm

https://codereview.chromium.org/996123002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/996123002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:792:
StartTimeoutTimer();
nit: Could we do this in another CL, as it's not related to this change?

[michaeln, 2015-03-12 19:34:02]

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

https://codereview.chromium.org/996123002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:395:
context_->GetLiveRegistration(registration_id_);
drive by: This call and early return adds no value, FindRegistrationForId() has
a fast return path (see CompleteFindNow) when there is a 'live' reg in memory.

[nhiroki, 2015-03-13 01:18:47]

Patchset #3 (id:100001) has been deleted

[nhiroki, 2015-03-13 01:21:31]

Thank you!

https://codereview.chromium.org/996123002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/996123002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:792:
StartTimeoutTimer();
On 2015/03/12 15:50:34, kinuko wrote:
> nit: Could we do this in another CL, as it's not related to this change?

Reverted this and uploaded a separate patch:
https://codereview.chromium.org/1004913003/

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

https://codereview.chromium.org/996123002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:395:
context_->GetLiveRegistration(registration_id_);
On 2015/03/12 19:34:02, michaeln wrote:
> drive by: This call and early return adds no value, FindRegistrationForId()
has
> a fast return path (see CompleteFindNow) when there is a 'live' reg in memory.

Done.

[nhiroki, 2015-03-13 09:29:51]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-03-13 09:29:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/996123002/#ps140001
(title: "fix tests")

[commit-bot: I haz the power, 2015-03-13 09:30:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996123002/140001

[commit-bot: I haz the power, 2015-03-13 10:15:00]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-03-13 10:16:21]

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