Populate .current when navigator.serviceWorker is accessed

Populate .current right after navigator.serviceWorker is accessed

- Remove AddScriptClient/RemoveScriptClient messsaging
- SetCurrent, StateChanged messages are always sent to the document thread
- Changed ObjectDestroyed message to {Increment,Decrement}RefCount
- Multiple providers for the same document could share one SWProviderContext (new class)
- SWProviderContext is:
-- responsible for keeping refcount for .current etc (while no script provider's available
-- responsible for updating .current info for newer provider initialization
-- responsible for forwarding .current related messages to worker providers --> DROPPED

BUG=366538
TEST=to be added later (https://codereview.chromium.org/265943003/)
R=falken@chromium.org, jochen@chromium.org, michaeln@chromium.org, tsepez@chromium.org

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

[kinuko, 2014-04-30 15:16:36]

Not very cute, but works. There're a few points I'm still not satisfied, but
it's basically ready for review.  Could you give feedback on this?

[kinuko, 2014-04-30 15:26:30]

On 2014/04/30 15:16:36, kinuko wrote:
> Not very cute, but works. There're a few points I'm still not satisfied, but
> it's basically ready for review.  Could you give feedback on this?

This can be implemented without a lock (in SWProviderContext), which slightly
adds potential tiny skew in events firing right after navigator.serviceWorker is
accessed, but in my local attempt it's added another complexity, so this version
uses a lock.

[michaeln, 2014-05-01 01:25:17]

i think this is closer to how it will look in the end, its still a bit rough.
the first thing i had to do was diagram out what was related to what, that led
to some of my naming nits where i was trying to grok the structure, please just
view them as nits and i hope you don't mind (i guess i just had one naming nit).

in light of my last comment far below, since a lot of this is currently
unreachable code and will be for a while yet, i wonder if it would be easier to
backtrack on the partial/complicated support for .serviceWorker in
dedicated|shared worker threads that really isn't there yet?

In better english...

For the time being, the only threads on which WebServiceWorkerProviderImpl and
WebServiceWorkerImpls ever exist are the main thread in document contexts, is
that right? Maybe we should take advantage of that to simplify thing in the
short term. Wdyt?

I'm assuming we don't have dedicated or shared worker support our short term
list of things todo.

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

https://codereview.chromium.org/261533003/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:355:
ServiceWorkerHandle* handle = handles_.Lookup(handle_id);
some bad message detection here might be good, what if handle is null

https://codereview.chromium.org/261533003/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:361:
ServiceWorkerHandle* handle = handles_.Lookup(handle_id);
ditto what if null

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/scoped_service_worker_reference.h (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/scoped_service_worker_reference.h:16: //
Automatically increments and decrement ServiceWorker's ref-count (in the
there are lots of classes so it would help to be real clear in the comment about
exactly what browser-side class this directly related to. i think that's the
ServerWorkerHandle class. to help readers make that connection maybe name this
class ServiceWorkerHandleReference?

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/service_worker_provider_context.cc (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:102: return
current_->info();
don't think we can return this ptr type to the struct here, i'm assuming the
values in info can mutate and it's accessors don't provide thread safetyness

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:107:
main_thread_loop_proxy_->DeleteSoon(FROM_HERE, this)) {
not sure if its possible for a background thread to be running past the time
where DeleteSoon would return false? might want to leak if the dtor really can't
be run on the main thread instead of running it on the wrong thread

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:113: void
ServiceWorkerProviderContext::ForwardMessageToProviderThreads(
I see how messages are forwarded, but I don't see how WebSWProviderImpls on
dedicated worker threads attach themselves to the right SWProviderContext. Can
you walk me thru that? I see it's via the
AddProviderThreadAndGetInitializationInfo() method, but how does the background
thread get a ref to the right SWProviderContext instance to start with?

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/web_service_worker_impl.h (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/web_service_worker_impl.h:27: class
WebServiceWorkerImpl
similary a comment about how this corresponds to each instance of an object like
navigator.serviceWorker.current or the resolved value of a call to
nav.serviceWorker.register might help

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.h (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.h:24: class
WebServiceWorkerProviderImpl
a class comment about how this corresponds to navigator.serviceWorker in script
could be helpful, that's might be in the webkit api file but it might be worth
repeating here too.

https://codereview.chromium.org/261533003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/261533003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1328: provider ? provider->context() :
NULL);
ah.. i see the main docs instance is cons'd up here

and now i remember that we haven't written code to support .serviceWorker from
dedicated|shared worker contexts yet

[falken, 2014-05-01 12:19:28]

I also tried diagramming this all out, but I don't quite understand fully.

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

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:226: // Note that
|worker|'s destructor sends a ServiceWorkerObjectDestroyed
This comment is now out-dated. Could you bring it up to date? (I wrote it to
explain why worker is created even if it's not passed to
setCurrentServiceWorker.)

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/service_worker_provider_context.cc (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:97: thread_id,
provider_id, info), thread_ids);
I'm trying to understand how this works. At first glance it looks circular...
this function posts a task on several threads to call
SWDispatcher::OnSetCurrentServiceWorker which then calls here. How is the circle
broken?

[kinuko, 2014-05-02 10:00:55]

Updated, I think this is a plausible fix... but I also agree that probably we
should take advantage of not having dedicated worker impl yet to simplify thing.

I'll upload another patch after removing some worker-specific part.

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

https://codereview.chromium.org/261533003/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:355:
ServiceWorkerHandle* handle = handles_.Lookup(handle_id);
On 2014/05/01 01:25:18, michaeln wrote:
> some bad message detection here might be good, what if handle is null

Done.

https://codereview.chromium.org/261533003/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:361:
ServiceWorkerHandle* handle = handles_.Lookup(handle_id);
On 2014/05/01 01:25:18, michaeln wrote:
> ditto what if null

Done.

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/scoped_service_worker_reference.h (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/scoped_service_worker_reference.h:16: //
Automatically increments and decrement ServiceWorker's ref-count (in the
On 2014/05/01 01:25:18, michaeln wrote:
> there are lots of classes so it would help to be real clear in the comment
about
> exactly what browser-side class this directly related to. i think that's the
> ServerWorkerHandle class. to help readers make that connection maybe name this
> class ServiceWorkerHandleReference?

Sg, done.

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

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:226: // Note that
|worker|'s destructor sends a ServiceWorkerObjectDestroyed
On 2014/05/01 12:19:29, falken wrote:
> This comment is now out-dated. Could you bring it up to date? (I wrote it to
> explain why worker is created even if it's not passed to
> setCurrentServiceWorker.)

Done. (Actually in this case ProviderContext should take the primary reference,
so I changed not to create a worker when we exit early)

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/service_worker_provider_context.cc (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:97: thread_id,
provider_id, info), thread_ids);
On 2014/05/01 12:19:29, falken wrote:
> I'm trying to understand how this works. At first glance it looks circular...
> this function posts a task on several threads to call
> SWDispatcher::OnSetCurrentServiceWorker which then calls here. How is the
circle
> broken?

This may not be very obvious, but:
1. ProviderContext's only attached to the document thread's SWDispatcher
2. ForwardMessageToProviderThreads only forward messages to non-document threads

(Added comments about this)

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:102: return
current_->info();
On 2014/05/01 01:25:18, michaeln wrote:
> don't think we can return this ptr type to the struct here, i'm assuming the
> values in info can mutate and it's accessors don't provide thread safetyness

Changed to return it as a value type.

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:107:
main_thread_loop_proxy_->DeleteSoon(FROM_HERE, this)) {
On 2014/05/01 01:25:18, michaeln wrote:
> not sure if its possible for a background thread to be running past the time
> where DeleteSoon would return false? might want to leak if the dtor really
can't
> be run on the main thread instead of running it on the wrong thread

Hmm.. it might be better. Done.

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/service_worker_provider_context.cc:113: void
ServiceWorkerProviderContext::ForwardMessageToProviderThreads(
On 2014/05/01 01:25:18, michaeln wrote:
> I see how messages are forwarded, but I don't see how WebSWProviderImpls on
> dedicated worker threads attach themselves to the right SWProviderContext. Can
> you walk me thru that? I see it's via the
> AddProviderThreadAndGetInitializationInfo() method, but how does the
background
> thread get a ref to the right SWProviderContext instance to start with?

WebSWProviderImpls are created on the main thread (and then passed on to the
worker thread if it's for dedicated workers), where we can share the document's
provider context info via NetworkProvider.

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/web_service_worker_impl.h (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/web_service_worker_impl.h:27: class
WebServiceWorkerImpl
On 2014/05/01 01:25:18, michaeln wrote:
> similary a comment about how this corresponds to each instance of an object
like
> navigator.serviceWorker.current or the resolved value of a call to
> nav.serviceWorker.register might help

Done.

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
File content/child/service_worker/web_service_worker_provider_impl.h (right):

https://codereview.chromium.org/261533003/diff/80001/content/child/service_wo...
content/child/service_worker/web_service_worker_provider_impl.h:24: class
WebServiceWorkerProviderImpl
On 2014/05/01 01:25:18, michaeln wrote:
> a class comment about how this corresponds to navigator.serviceWorker in
script
> could be helpful, that's might be in the webkit api file but it might be worth
> repeating here too.

Done.

https://codereview.chromium.org/261533003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/261533003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1328: provider ? provider->context() :
NULL);
On 2014/05/01 01:25:18, michaeln wrote:
> ah.. i see the main docs instance is cons'd up here
> 
> and now i remember that we haven't written code to support .serviceWorker from
> dedicated|shared worker contexts yet

Yup.

[kinuko, 2014-05-02 12:18:19]

Dropped threading, message forwarding and lock parts to disregard
dedicated-worker cases for now. (I can probably simplify this a bit more, but
currently it still keeps some of the original code) PTAL?

[michaeln, 2014-05-05 23:18:03]

lgtm

thnx for simplifying, i think this is still evolving and what you have here
looks like a reasonable stepping stone

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.h:43:
scoped_ptr<ServiceWorkerHandleReference>* current);
would it make sense to have this method return the scoped_ptr<>

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.h:46: void
OnServiceWorkerStateChanged(int thread_id,
Does thread_id make sense here now?

If i understand the model now, there is one ServiceWorkerProviderContext for a
document and all of its dedicated workers. The browser process informs that
'context' of state transitions and then that context is responsible for
propagating the transition info further. If that's right, not sure thread_id is
needed here.

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.h (right):

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.h:27: // Each instance
corresponds to onw ServiceWorker object in JS context, and
own --> one?

[falken, 2014-05-06 00:16:23]

lgtm2

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_handle_reference.h (right):

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_handle_reference.h:16: //
Automatically increments and decrement ServiceWorkerHandle's ref-count
nit: decrements

[kinuko, 2014-05-06 00:39:36]

Thx for reviewing, updated.

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_handle_reference.h (right):

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_handle_reference.h:16: //
Automatically increments and decrement ServiceWorkerHandle's ref-count
On 2014/05/06 00:16:23, falken wrote:
> nit: decrements

Done.

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_provider_context.h (right):

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.h:43:
scoped_ptr<ServiceWorkerHandleReference>* current);
On 2014/05/05 23:18:03, michaeln wrote:
> would it make sense to have this method return the scoped_ptr<>

If we started to have more fields (e.g. pending?) we'll probably want to return
them together, but for now yes we can.

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.h:46: void
OnServiceWorkerStateChanged(int thread_id,
On 2014/05/05 23:18:03, michaeln wrote:
> Does thread_id make sense here now?
> 
> If i understand the model now, there is one ServiceWorkerProviderContext for a
> document and all of its dedicated workers. The browser process informs that
> 'context' of state transitions and then that context is responsible for
> propagating the transition info further. If that's right, not sure thread_id
is
> needed here.

It's not needed, my very early patch used to have code related to this but not
anymore.  I dropped it.

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.h (right):

https://codereview.chromium.org/261533003/diff/120001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.h:27: // Each instance
corresponds to onw ServiceWorker object in JS context, and
On 2014/05/05 23:18:03, michaeln wrote:
> own --> one?

Done.

[kinuko, 2014-05-06 00:40:35]

Tom, can you review service_worker_messages.h?

[kinuko, 2014-05-06 06:35:16]

On 2014/05/06 00:40:35, kinuko wrote:
> Tom, can you review service_worker_messages.h?

Jochen, can you do owner review for content/renderer/render_frame_impl.cc?

[jochen (gone - plz use gerrit), 2014-05-06 11:55:33]

lgtm

[Tom Sepez, 2014-05-06 17:36:15]

What's the worst that could happen in the browser process if the SW lies about
its reference count, and deregisters itself too soon, but continues to operate?

[kinuko, 2014-05-07 00:26:30]

On 2014/05/06 17:36:15, Tom Sepez wrote:
> What's the worst that could happen in the browser process if the SW lies about
> its reference count, and deregisters itself too soon, but continues to
operate?

This is to keep the SW context alive while corresponding SW object is alive (is
not GC'ed yet).  If it deregisters too soon the document will just lose the
connection to the SW context, i.e. the document cannot send/receive any update
events or messages to it.  If it lies about its ref count and increments the ref
count more than necessary, the SW context may live longer but each renderer
cannot prolong the SW's lifetime more than the renderer process's lifetime.  So
I don't think there's much effect / real threat malicious ref-counting can do.

[Tom Sepez, 2014-05-07 16:56:46]

lgtm

[kinuko, 2014-05-07 16:59:51]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-07 17:03:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/261533003/160001

[commit-bot: I haz the power, 2014-05-07 19:59:12]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 20:41:34]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 21:13:40]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 21:19:22]

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

[commit-bot: I haz the power, 2014-05-07 21:19:23]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[kinuko, 2014-05-08 00:04:54]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-08 00:07:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/261533003/160001

[commit-bot: I haz the power, 2014-05-08 00:17:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 00:53:33]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 00:58:36]

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

[commit-bot: I haz the power, 2014-05-08 00:58:36]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium

[kinuko, 2014-05-08 01:23:14]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-08 01:26:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/261533003/180001

[kinuko, 2014-05-08 02:25:42]

The CQ bit was unchecked by kinuko@chromium.org

[kinuko, 2014-05-08 02:35:59]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-08 02:38:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/261533003/200001

[commit-bot: I haz the power, 2014-05-08 07:31:13]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 08:51:06]

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

[commit-bot: I haz the power, 2014-05-08 08:51:07]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium

[kinuko, 2014-05-08 08:56:33]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-08 08:58:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/261533003/200001

[kinuko, 2014-05-08 23:48:20]

Committed patchset #7 manually as r269102 (presubmit successful).