Make ServiceWorkerDispatcher reuse existing WebServiceWorkerImpls.

Make ServiceWorkerDispatcher reuse existing WebServiceWorkerImpls.

ServiceWorkerDispatcher used to mint new WebServiceWorkerImpl
instances every time it was passed a representation of a Service
Worker. Ultimately, this causes user script to see distinct objects
representing the same Service Worker, which is Confusing and
Wrong. This patch makes ServiceWorkerDispatcher use its table of
existing WebServiceWorkerImpls to consistently represent a
ServiceWorker with the same object.

A chunk of this patch is concerned with making it explicit when a
ServiceWorkerHandleReference, which a WebServiceWorkerImpl wraps, is
being created in the renderer (and should send an add-ref message to
the browser) versus being adopted in the renderer.

BUG=361907
TEST=ServiceWorker*

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

[dominicc (has gone to gerrit), 2014-06-02 04:37:29]

PTAL

[falken, 2014-06-02 05:40:10]

I think this looks good but have a question.

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:161:
ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
I'm not sure I understand this, incrementing then decrementing ref count seems
like it should be a no-op. Why is it necessary to call Adopt?

Also, in what circumstances do we expect adopt_handle to be true although
there's an existing handle? I guess if navigator.serviceworker.register resolves
with an existing service worker, right?

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.h:75: // created and its
ownership is transferred caller. If
transferred to the caller

[falken, 2014-06-02 05:56:18]

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:161:
ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
On 2014/06/02 05:40:11, falken wrote:
> I'm not sure I understand this, incrementing then decrementing ref count seems
> like it should be a no-op. Why is it necessary to call Adopt?
> 
> Also, in what circumstances do we expect adopt_handle to be true although
> there's an existing handle? I guess if navigator.serviceworker.register
resolves
> with an existing service worker, right?

In chatting with dominicc, I realized I totally missed that Adopt isn't
incrementing ref count. This makes sense to me now.

[dominicc (has gone to gerrit), 2014-06-02 13:44:33]

Fixed the comment.

On 2014/06/02 05:56:18, falken wrote:
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> File content/child/service_worker/service_worker_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> content/child/service_worker/service_worker_dispatcher.cc:161:
> ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
> On 2014/06/02 05:40:11, falken wrote:
> > Also, in what circumstances do we expect adopt_handle to be true although
> > there's an existing handle? I guess if navigator.serviceworker.register
> resolves
> > with an existing service worker, right?
> 
> In chatting with dominicc, I realized I totally missed that Adopt isn't
> incrementing ref count. This makes sense to me now.

Right; that call to Adopt is to "reject" an adoption by decrementing a ref
without having added one (in the renderer). This is more likely to occur as we
add more properties and update, for example, a given page may have seen a
particular Service Worker through the register callback, the installing
property, a state change event, and the current property (assuming it called
update.)

Currently whether the browser adds a reference which the renderer adopts or the
renderer is required to add a reference is determined statically; it depends on
the code path. For example setting current adopts a reference from the browser,
the state change event doesn't. This will get more complex as we add more
events. In this change I tried to localize that adopt-or-ref thing, and got it
down to these two places. The plan is to battle the complexity of these refs in
a confined space. Like the alien from Alien.

[falken, 2014-06-02 15:58:21]

lgtm

[michaeln, 2014-06-02 21:11:18]

this looks like a work in progress. can you outline what you have in mind for
this?

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.h:73: // If an existing
WebServiceWorkerImpl exists for the Service
fyi: eventually, i think we'll have consumers of this class on both the main
thread and on background thread.

https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.h:79: // TODO(dominicc):
The lifetime of WebServiceWorkerImpl is too tricky; this
how does the caller know when ownership is transferred vs not?

[michaeln, 2014-06-02 21:35:44]

nm... i see the bug and history there

[dominicc (has gone to gerrit), 2014-06-03 00:39:39]

On 2014/06/02 21:11:18, michaeln wrote:
> this looks like a work in progress. can you outline what you have in mind for
> this?

I'm poking at it. It might make sense to move this map of
handle->WebServiceWorkerImpl from the dispatcher to the provider.

>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> File content/child/service_worker/service_worker_dispatcher.h (right):
> 
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> content/child/service_worker/service_worker_dispatcher.h:73: // If an existing
> WebServiceWorkerImpl exists for the Service
> fyi: eventually, i think we'll have consumers of this class on both the main
> thread and on background thread.

You mean for dedicated workers?

>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> content/child/service_worker/service_worker_dispatcher.h:79: //
TODO(dominicc):
> The lifetime of WebServiceWorkerImpl is too tricky; this
> how does the caller know when ownership is transferred vs not?

The caller knows by the type of message received.

I think I will write this up in a doc. I think I understand this well now. It is
not tricky but perhaps not obvious from casual code reading.

[dominicc (has gone to gerrit), 2014-06-03 00:39:49]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-06-03 00:41:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominicc@chromium.org/309503014/20001

[commit-bot: I haz the power, 2014-06-03 04:31:35]

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

[commit-bot: I haz the power, 2014-06-03 06:18:06]

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

[commit-bot: I haz the power, 2014-06-03 06:18:06]

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

[dominicc (has gone to gerrit), 2014-06-03 06:22:04]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-06-03 06:23:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominicc@chromium.org/309503014/20001

[commit-bot: I haz the power, 2014-06-03 12:12:27]

Change committed as 274477

[michaeln, 2014-06-03 20:07:53]

On 2014/06/03 00:39:39, dominicc wrote:
> On 2014/06/02 21:11:18, michaeln wrote:
> > this looks like a work in progress. can you outline what you have in mind
for
> > this?
> 
> I'm poking at it. It might make sense to move this map of
> handle->WebServiceWorkerImpl from the dispatcher to the provider.

Maybe we can chat about how that gets put together next week?

> >
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> > File content/child/service_worker/service_worker_dispatcher.h (right):
> > 
> >
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> > content/child/service_worker/service_worker_dispatcher.h:73: // If an
existing
> > WebServiceWorkerImpl exists for the Service
> > fyi: eventually, i think we'll have consumers of this class on both the main
> > thread and on background thread.
> 
> You mean for dedicated workers?

I mean for workers in general. Presumably a serviceworker script can call
register() in some future, and a sharedworker can have a 'controller'. In both
cases we have instances of WebCore::ServiceWorker backed by WebServiceWorkerImpl
objects.

> >
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> > content/child/service_worker/service_worker_dispatcher.h:79: //
> TODO(dominicc):
> > The lifetime of WebServiceWorkerImpl is too tricky; this
> > how does the caller know when ownership is transferred vs not?
> 
> The caller knows by the type of message received.
> 
> I think I will write this up in a doc. I think I understand this well now. It
is
> not tricky but perhaps not obvious from casual code reading.

Would it make sense to create two different methods for the different type of
callsites?

[dominicc (has gone to gerrit), 2014-06-04 01:42:48]

On 2014/06/03 20:07:53, michaeln wrote:
> On 2014/06/03 00:39:39, dominicc wrote:
> > On 2014/06/02 21:11:18, michaeln wrote:
> > > this looks like a work in progress. can you outline what you have in mind
> for
> > > this?
> > 
> > I'm poking at it. It might make sense to move this map of
> > handle->WebServiceWorkerImpl from the dispatcher to the provider.
> 
> Maybe we can chat about how that gets put together next week?

SG

> > >
> >
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> > > File content/child/service_worker/service_worker_dispatcher.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> > > content/child/service_worker/service_worker_dispatcher.h:73: // If an
> existing
> > > WebServiceWorkerImpl exists for the Service
> > > fyi: eventually, i think we'll have consumers of this class on both the
main
> > > thread and on background thread.
> > 
> > You mean for dedicated workers?
> 
> I mean for workers in general. Presumably a serviceworker script can call
> register() in some future,

Maybe. In the short to medium term it looks like they can update and unregister,
but not register.

Are you suggesting something like "stacked" Service Workers? It would make sense
to explore something like that in the implementation by reimplementing AppCache
with Service Worker machinery and make AppCache and Service Workers work
together (which would imply a stack of two.)

> and a sharedworker can have a 'controller'. In both
> cases we have instances of WebCore::ServiceWorker backed by
WebServiceWorkerImpl
> objects.

The SharedWorker case is just like a page and its controller in that respect. I
think that the SharedWorker would have its own ServiceWorker and
WebServiceWorkerImpl object; I do not think we should be sharing these. Remember
that the WebCore::ServiceWorker observes an ExecutionContext, it isn't agile
across contexts.

Multiple WebCore::ServiceWorkers sharing a WebServiceWorkerImpl is a possible
alternate design. It is more complicated but may mean slightly less IPC.

Let's discuss in person.

> > >
> >
>
https://codereview.chromium.org/309503014/diff/1/content/child/service_worker...
> > > content/child/service_worker/service_worker_dispatcher.h:79: //
> > TODO(dominicc):
> > > The lifetime of WebServiceWorkerImpl is too tricky; this
> > > how does the caller know when ownership is transferred vs not?
> > 
> > The caller knows by the type of message received.
> > 
> > I think I will write this up in a doc. I think I understand this well now.
It
> is
> > not tricky but perhaps not obvious from casual code reading.
> 
> Would it make sense to create two different methods for the different type of
> callsites?

We could. I think this patch makes the situation clearer than before; we could
take that Boolean parameter and make them separate entrypoints which would be
even more explicit.

Does Mojo have something to keep a remote object alive? I wonder if this is
something we outsource instead of working so hard at it ourselves...