ServiceWorker: Implement updatefound event and version attributes (Chromium)

ServiceWorker: Implement updatefound event and version attributes (Chromium)

Changes in the renderer side:
* Implementing WebServiceWorkerRegistrationImpl in order to handle 'updatefound'
event and version attributes.
* Adding ServiceWorkerRegistrationHandleReference which is responsible for
incrementing/decrementing the refcount of ServiceWorkerRegistration in the
browser-side (in the same way as ServiceWorkerHandleRefernce).

Changes in the browser side:
* Adding ServiceWorkerRegistrationHandle which manages the refcount of
ServiceWorkerRegistration and listens to its version change events.
This is managed by ServiceWorkerDispatcherHost.

Changes in the wiring part:
* Adding new IPC messages for refcount operations.
* Changing Registered and SetVersionAttributes messages to carry the
registration handle id.


BUG=384119, 396400
TEST=content_unittests --gtest_filter=ServiceWorker*
TEST=run_webkit_tests.py --debug http/tests/serviceworker
TEST=https://codereview.chromium.org/466723002/ (layout tests for ServiceWorkerRegistration)
NOTRY=true

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

[nhiroki, 2014-08-12 09:08:21]

Hi falken@,

I haven't closely checked if this CL works well or not, but please let me send a
review request for rapid development. Thanks!

[falken, 2014-08-12 10:53:13]

Yep, this all looks good to me.

[nhiroki, 2014-08-12 13:22:39]

Thank you for reviewing! Can you take another look?

I merged the browser-side implementation into this CL because I wonder if the
merged patch could be more understandable about what I want to do. If you prefer
the fine-grained patch, please let me know.

I confirmed layout tests that are modified for ServiceWorkerRegistration work
well (https://codereview.chromium.org/466723002/).

[nhiroki, 2014-08-12 13:24:06]

+michaeln@

[nhiroki, 2014-08-12 15:48:15]

Hi jschuh@, could you review service_worker_messages.h? Thanks.

[nhiroki, 2014-08-13 00:40:39]

On 2014/08/12 15:48:15, nhiroki wrote:
> Hi jschuh@, could you review service_worker_messages.h? Thanks.

[nhiroki, 2014-08-13 00:42:10]

On 2014/08/12 15:48:15, nhiroki wrote:
> Hi jschuh@, could you review service_worker_messages.h? Thanks.

Removed jschuh@ from the reviewer list because he seems to be really busy now.

[michaeln, 2014-08-13 04:18:17]

it looks like you're working up to the model outlined in 384119, please add that
to the buglist too

i think this lgtm

but the layers of indirection between content::ServiceWorkerRegisrtation and
content::ServiceWorkerVersion and the constructs in the child process are
confusing as can be (and make it very difficult to reason about and review for
me at least)

please look closely at these 2 comments in particular

content/child/service_worker/service_worker_dispatcher.cc:259:
content/child/service_worker/web_service_worker_registration_impl.cc:25:

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
can you also add 384119 to the bug list since this CL contributes to that as
well

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

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:105:
kInvalidServiceWorkerRegistrationHandleId,
why invalid?

ok, now i see why (so handlrefs instances in the client-side don't get confused
with register return value handlerefs). this is a kinda hacky but still heading
towards sending one mutation event per registration to processes that care to
know.

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

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_registration_handle.cc:119:
ServiceWorkerHandle::Create(context_,
does it matter that we create a new handle instance as workers migrate from
installing to waiting to active?

hmmmm, i guess these get dedupped on the client-side (how non-obvious)

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_registration_handle.h
(right):

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_registration_handle.h:41: int
thread_id() const { return thread_id_; }
since thread_id is always the main thread id in blink, can we leave this data
member out

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:228: adopt_handle
fyi: i don't fully understand how these instance are manged with adoption vs not
scheme

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:259:
ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
why is this here, it looks like  GetServiceWorkerRegistration() consumes the
ref?

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:309: int provider_id,
if this is 'provider' specific, how does that work with the

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/service_worker_message_filter.cc (right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_message_filter.cc:31:
scoped_refptr<ThreadSafeSender> sender,
can this be a raw ptr instead of pass by scoped_refptr value

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File
content/child/service_worker/service_worker_registration_handle_reference.cc
(right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_registration_handle_reference.cc:43:
handle_id_ != kInvalidServiceWorkerRegistrationHandleId) {
do we need an instance when handled_id_ is kInvalid? ok, i guess that matches
how this works for ServiceWorkerHandleReference.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/web_service_worker_registration_impl.cc
(right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/web_service_worker_registration_impl.cc:25: if
(handle_ref_->handle_id() == kInvalidServiceWorkerRegistrationHandleId)
the is no similar check for kinvalid in the ctor which makes me wonder if there
should be

[nhiroki, 2014-08-13 04:48:33]

nasko@: Could you take a look at service_worker_messages.h? This is the second
CL I mentioned in my mail. Thanks.

[falken, 2014-08-13 08:27:06]

lgtm. this is pretty consistent with our existing code for ServiceWorker
objects, we likely have refactoring opportunities for both later

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

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_registration_handle.cc:119:
ServiceWorkerHandle::Create(context_,
On 2014/08/13 04:18:16, michaeln wrote:
> does it matter that we create a new handle instance as workers migrate from
> installing to waiting to active?
> 
> hmmmm, i guess these get dedupped on the client-side (how non-obvious)

I don't think they get dedupped. This is
https://code.google.com/p/chromium/issues/detail?id=400602

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:228: adopt_handle
On 2014/08/13 04:18:17, michaeln wrote:
> fyi: i don't fully understand how these instance are manged with adoption vs
not
> scheme

Yea, this is a bit tricky. Of course, it just imitates the existing code we have
for ServiceWorkerHandleReference.

The basic idea is that Adopt will not tell browser-side to increment ref count,
while Create does. The reason Adopt doesn't is that it's used with register()
which usually creates a new handle, whose reference count starts at 1, so it
doesn't need to be incremented. There are cases where register() reuses a
handle, in which case it increments the reference count itself, since it can't
expect a IncrementServiceWorkerRefCount message.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:259:
ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
On 2014/08/13 04:18:17, michaeln wrote:
> why is this here, it looks like  GetServiceWorkerRegistration() consumes the
> ref?

yes this looks odd

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:309: int provider_id,
On 2014/08/13 04:18:17, michaeln wrote:
> if this is 'provider' specific, how does that work with the

FYI this comment got cut off?

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/web_service_worker_registration_impl.cc
(right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/web_service_worker_registration_impl.cc:25: if
(handle_ref_->handle_id() == kInvalidServiceWorkerRegistrationHandleId)
On 2014/08/13 04:18:17, michaeln wrote:
> the is no similar check for kinvalid in the ctor which makes me wonder if
there
> should be

good catch, this probably can be done better. of course, this is following
WebServiceWorkerImpl::~WebServiceWorkerImpl which has the same issue.

[nhiroki, 2014-08-13 13:15:06]

Thank you for reviewing! I'll put this into the CQ when I got the stamp for IPC
part.

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/08/13 04:18:16, michaeln wrote:
> can you also add 384119 to the bug list since this CL contributes to that as
> well

Done.

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_registration_handle.h
(right):

https://codereview.chromium.org/463013002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_registration_handle.h:41: int
thread_id() const { return thread_id_; }
On 2014/08/13 04:18:17, michaeln wrote:
> since thread_id is always the main thread id in blink, can we leave this data
> member out

Done.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:228: adopt_handle
On 2014/08/13 08:27:06, falken wrote:
> On 2014/08/13 04:18:17, michaeln wrote:
> > fyi: i don't fully understand how these instance are manged with adoption vs
> not
> > scheme
> 
> Yea, this is a bit tricky. Of course, it just imitates the existing code we
have
> for ServiceWorkerHandleReference.
> 
> The basic idea is that Adopt will not tell browser-side to increment ref
count,
> while Create does. The reason Adopt doesn't is that it's used with register()
> which usually creates a new handle, whose reference count starts at 1, so it
> doesn't need to be incremented. There are cases where register() reuses a
> handle, in which case it increments the reference count itself, since it can't
> expect a IncrementServiceWorkerRefCount message.

Thanks for your explanation, Matt! Yes, this is really tricky part and we could
simplify more (eg. consolidating SWHandleRefernce and
SWRegistrationHandleReference), but I'd like to leave this as is while we are in
the transition period.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:259:
ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
On 2014/08/13 08:27:06, falken wrote:
> On 2014/08/13 04:18:17, michaeln wrote:
> > why is this here, it looks like  GetServiceWorkerRegistration() consumes the
> > ref?
> 
> yes this looks odd

This would be necessary because GetServiceWorkerRegistration() does not consume
"handle_id" for ServiceWorker object but "registration_handle_id" for
ServiceWorkerRegistration object.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/service_worker_message_filter.cc (right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_message_filter.cc:31:
scoped_refptr<ThreadSafeSender> sender,
On 2014/08/13 04:18:17, michaeln wrote:
> can this be a raw ptr instead of pass by scoped_refptr value

Done.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File
content/child/service_worker/service_worker_registration_handle_reference.cc
(right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/service_worker_registration_handle_reference.cc:43:
handle_id_ != kInvalidServiceWorkerRegistrationHandleId) {
On 2014/08/13 04:18:17, michaeln wrote:
> do we need an instance when handled_id_ is kInvalid? ok, i guess that matches
> how this works for ServiceWorkerHandleReference.

I don't think we need to make an instance for kInvalid and the callers have
already checked it, so removed such cases and added DCHECK instead.

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
File content/child/service_worker/web_service_worker_registration_impl.cc
(right):

https://codereview.chromium.org/463013002/diff/90001/content/child/service_wo...
content/child/service_worker/web_service_worker_registration_impl.cc:25: if
(handle_ref_->handle_id() == kInvalidServiceWorkerRegistrationHandleId)
On 2014/08/13 08:27:06, falken wrote:
> On 2014/08/13 04:18:17, michaeln wrote:
> > the is no similar check for kinvalid in the ctor which makes me wonder if
> there
> > should be
> 
> good catch, this probably can be done better. of course, this is following
> WebServiceWorkerImpl::~WebServiceWorkerImpl which has the same issue.

Done.

[nhiroki, 2014-08-13 13:21:28]

On 2014/08/13 08:27:06, falken wrote:
> lgtm. this is pretty consistent with our existing code for ServiceWorker
> objects, we likely have refactoring opportunities for both later

Yeah... keeping the existing code really frustrated me :( I'm keen to overhaul
this after enabling ServiceWorkerRegistration by default.

Anyway, thank you so much for taking a look at this big patch, Michael and Matt!
:D

[nasko, 2014-08-13 15:33:08]

IPC LGTM

[nhiroki, 2014-08-13 16:33:40]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-13 16:34:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/463013002/130001

[commit-bot: I haz the power, 2014-08-13 16:49:35]

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

[commit-bot: I haz the power, 2014-08-13 17:04:54]

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

[commit-bot: I haz the power, 2014-08-13 17:04:56]

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

[nhiroki, 2014-08-13 23:00:30]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-13 23:04:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/463013002/130001

[nhiroki, 2014-08-13 23:41:50]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2014-08-13 23:42:05]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-13 23:45:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/463013002/130001

[commit-bot: I haz the power, 2014-08-13 23:49:44]

Committed patchset #5 (130001) as 289415