[extension SW] Support lazy events from extension service workers.

[extension SW] Support lazy events from extension service workers.

This CL adds support to register and dispatch lazy events from/to
extension SW
(a) To register, we use SW's (unique) scope url to identify a SW
within an extension. Pass the information about scope url through
ServiceWorkerContextClient. Initially EventListener::worker_thread_id_
was used to identify whether an event is for SW context or not.
However, that is not enough for lazy service worker events, because
we need to persist lazy events in the browser process and
worker_thread_id_ is temporary for a running SW.
Change SW EventListener by adding EventListener::is_for_service_worker()
to support this.
(b) To dispatch, we need to start a worker before dispatching the
event. The content/ API:
ServiceWorkerContext::GetWorkerInfoAfterStartWorker()
was added to do that. Add ServiceWorkerTaskQueue (similar to
LazyBackgroundTaskQueue for lazy background pages) to dispatch
the event (a task) in extensions/ layer.

IPC changes:
In order to identify a (possibly stopped) SW, IPCs for lazy
service worker events were changed to accept service worker scope
urls. In order to not convolute existing lazy background page IPCs
(ExtensionHostMsg_Add/RemoveLazyListener), new ones have been
introduced: ExtensionHostMsg_Add/RemoveLazyServiceWorkerListener.

Other notable changes:
- LazyContextTaskQueue is introduced to express a lazy runnable
  context. Make the new ServiceWorkerTaskQueue and existing
  LazyBackgroundTaskQueue derived from that. One can post [1]
  PendingTasks to these contexts. Unfortunately becuase of many
  existing usages of LazyBackgroundTaskQueue::AddPendingTask,
  [1] was named AddPendingTaskToDispatchEvent to avoid collision.
- LazyContextTaskQueue provides a contexual information expressed
  as LazyContextTaskQueue::ContextInfo. This was introduced to
  avoid ExtensionHost from LazyContextTaskQueue (See
  LazyBackgroundTaskQueue::AddPendingTask)

BUG=721147
Test=After an extension service worker stops, it should still be
possible to dispatch extension events to it. That will wake up
the service worker.

Review-Url: https://codereview.chromium.org/2943583002
Cr-Commit-Position: refs/heads/master@{#483820}
Committed: https://chromium.googlesource.com/chromium/src/+/63b994a1d1026a4b992c74e10e1e7c826bb5f9ae

[lazyboy, 2017-06-16 01:03:45]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 01:04:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-06-16 01:04:33]

Description was changed from

==========
[extension SW] Support lazy events from extension service workers.

This CL adds support to register (a) and dispatch lazy events to
extension SW
(a) To register, we use SW's (unique) scope url to identify a SW
within an extension. Pass the information about scope url through
ServiceWorkerContextClient. Initially EventListener::worker_thread_id_
was used to identify whether an event is for SW context or not.
However, that is not enough for lazy service worker events, because
we need to persist lazy events in the browser process and
worker_thread_id_ is temporary for a running SW.
Change SW EventListener by adding EventListener::is_for_service_worker()
to support this.
(b) To dispatch, we need to start a worker before dispatching the
event. The content/ API:
ServiceWorkerContext::GetWorkerInfoAfterStartWorker()
was added to do that. Add ServiceWorkerTaskQueue (similar to
LazyBackgroundTaskQueue for lazy background pages) to dispatch
the event (a task) in extensions/ layer.

IPC changes:
In order to identify a (possibly stopped) SW, IPCs for lazy
service worker events were changed to accept service worker scope
urls. In order to not convolute existing lazy background page IPCs
(ExtensionHostMsg_Add/RemoveLazyListener), new ones have been
introduced: ExtensionHostMsg_Add/RemoveLazyServiceWorkerListener.

Other notable changes:
- LazyContextTaskQueue is introduced to express a lazy runnable
  context. Make the new ServiceWorkerTaskQueue and existing
  LazyBackgroundTaskQueue derived from that. One can post [1]
  PendingTasks to these contexts. Unfortunately becuase of many
  existing usages of LazyBackgroundTaskQueue::AddPendingTask,
  [1] was named AddPendingTaskToDispatchEvent to avoid collision.
- LazyContextTaskQueue provides a contexual information expressed
  as LazyContextTaskQueue::ContextInfo. This was introduced to
  avoid ExtensionHost from LazyContextTaskQueue (See
  LazyBackgroundTaskQueue::AddPendingTask)

BUG=721147
Test=After an extension service worker stops, it should still be
possible to dispatch extension events to it. That will wake up
the service worker.
==========

to

==========
[extension SW] Support lazy events from extension service workers.

This CL adds support to register and dispatch lazy events from/to
extension SW
(a) To register, we use SW's (unique) scope url to identify a SW
within an extension. Pass the information about scope url through
ServiceWorkerContextClient. Initially EventListener::worker_thread_id_
was used to identify whether an event is for SW context or not.
However, that is not enough for lazy service worker events, because
we need to persist lazy events in the browser process and
worker_thread_id_ is temporary for a running SW.
Change SW EventListener by adding EventListener::is_for_service_worker()
to support this.
(b) To dispatch, we need to start a worker before dispatching the
event. The content/ API:
ServiceWorkerContext::GetWorkerInfoAfterStartWorker()
was added to do that. Add ServiceWorkerTaskQueue (similar to
LazyBackgroundTaskQueue for lazy background pages) to dispatch
the event (a task) in extensions/ layer.

IPC changes:
In order to identify a (possibly stopped) SW, IPCs for lazy
service worker events were changed to accept service worker scope
urls. In order to not convolute existing lazy background page IPCs
(ExtensionHostMsg_Add/RemoveLazyListener), new ones have been
introduced: ExtensionHostMsg_Add/RemoveLazyServiceWorkerListener.

Other notable changes:
- LazyContextTaskQueue is introduced to express a lazy runnable
  context. Make the new ServiceWorkerTaskQueue and existing
  LazyBackgroundTaskQueue derived from that. One can post [1]
  PendingTasks to these contexts. Unfortunately becuase of many
  existing usages of LazyBackgroundTaskQueue::AddPendingTask,
  [1] was named AddPendingTaskToDispatchEvent to avoid collision.
- LazyContextTaskQueue provides a contexual information expressed
  as LazyContextTaskQueue::ContextInfo. This was introduced to
  avoid ExtensionHost from LazyContextTaskQueue (See
  LazyBackgroundTaskQueue::AddPendingTask)

BUG=721147
Test=After an extension service worker stops, it should still be
possible to dispatch extension events to it. That will wake up
the service worker.
==========

[lazyboy, 2017-06-16 01:08:23]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2017-06-16 01:08:23]

Hi Devlin, can you do an initial review of extensions/ bits and the one content/
interface method?
I could see how I can extract the change to
ScriptContext::service_worker_scope() to a separate CL, let me know if you
prefer that. Also, if any other independent bits that can be extracted...

[Devlin, 2017-06-16 02:34:42]

I was able to take a quick look tonight, but I need to go back through and check
out some of the subtler details.

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/service_worker_apitest.cc:70: GURL
GetOriginForExtension(const ExtensionId& extension_id) {
Is this different than extension->url()?

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/service_worker_apitest.cc:714:
content::StopServiceWorkerAtOrigin(
can we move the storage_partition var into this scope?

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/service_worker_apitest.cc:719: }
Is there a simple method we can use to EXPECT_ that the service worker has
indeed stopped?

https://codereview.chromium.org/2943583002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2943583002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:87:
base::OnceCallback<void(int, int)> info_callback) {
nit: ServiceWorkerContext::GetWorkerInfoCallback?

https://codereview.chromium.org/2943583002/diff/1/content/public/test/service...
File content/public/test/service_worker_test_helpers.cc (right):

https://codereview.chromium.org/2943583002/diff/1/content/public/test/service...
content/public/test/service_worker_test_helpers.cc:31: class Observer : public
ServiceWorkerContextCoreObserver {
Why have the inner observer rather than just have StoppedObserver implement
ServiceWorkerContextCoreObserver?

https://codereview.chromium.org/2943583002/diff/1/content/public/test/service...
content/public/test/service_worker_test_helpers.cc:78:
DCHECK(service_worker_status == SERVICE_WORKER_OK);
nit DCHECK_EQ?

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_li...
File extensions/browser/event_listener_map.h (right):

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_li...
extensions/browser/event_listener_map.h:86: bool IsForServiceWorker() const;
We probably shouldn't have two "IsForServiceWorker" methods. :)

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_li...
extensions/browser/event_listener_map.h:118: const bool is_for_service_worker_ =
false;
Do we need this + worker_thread_id?  If so, worth documenting why?

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (left):

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:290: // Ensures that all lazy background pages
that are interested in the given
These are gone now, right?  Can you rebase?

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/events/l...
File extensions/browser/events/lazy_event_dispatcher.h (right):

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:67: std::pair<const
content::BrowserContext*, GURL>;
Might be worth commenting on what GURL is here (I assume the scope)

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:90:
std::set<ServiceWorkerDispatchIdentifier> dispatched_ids_for_service_worker_;
Could we instead just have a single set of LazyContextId?

[commit-bot: I haz the power, 2017-06-16 02:51:16]

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

[commit-bot: I haz the power, 2017-06-16 02:51:17]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-16 18:42:44]

Patchset #2 (id:20001) has been deleted

[lazyboy, 2017-06-16 18:48:05]

Thanks for the initial look, comments addressed below.

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/service_worker_apitest.cc:70: GURL
GetOriginForExtension(const ExtensionId& extension_id) {
On 2017/06/16 02:34:41, Devlin wrote:
> Is this different than extension->url()?

You're right. Fixed.

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/service_worker_apitest.cc:714:
content::StopServiceWorkerAtOrigin(
On 2017/06/16 02:34:42, Devlin wrote:
> can we move the storage_partition var into this scope?

Done.

https://codereview.chromium.org/2943583002/diff/1/chrome/browser/extensions/s...
chrome/browser/extensions/service_worker_apitest.cc:719: }
On 2017/06/16 02:34:41, Devlin wrote:
> Is there a simple method we can use to EXPECT_ that the service worker has
> indeed stopped?

Not that I know of, hopefully StoppedObserver will always timeout when that
happens.

https://codereview.chromium.org/2943583002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2943583002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:87:
base::OnceCallback<void(int, int)> info_callback) {
On 2017/06/16 02:34:42, Devlin wrote:
> nit: ServiceWorkerContext::GetWorkerInfoCallback?

Done.

https://codereview.chromium.org/2943583002/diff/1/content/public/test/service...
File content/public/test/service_worker_test_helpers.cc (right):

https://codereview.chromium.org/2943583002/diff/1/content/public/test/service...
content/public/test/service_worker_test_helpers.cc:31: class Observer : public
ServiceWorkerContextCoreObserver {
On 2017/06/16 02:34:42, Devlin wrote:
> Why have the inner observer rather than just have StoppedObserver implement
> ServiceWorkerContextCoreObserver?

Only reason was to hide OnRunningStateChanged, but the main reason is to add a
ref to StoppedObserver to keep it around until the worker is stopped. Added
comment to the BindOnce call above.

https://codereview.chromium.org/2943583002/diff/1/content/public/test/service...
content/public/test/service_worker_test_helpers.cc:78:
DCHECK(service_worker_status == SERVICE_WORKER_OK);
On 2017/06/16 02:34:42, Devlin wrote:
> nit DCHECK_EQ?

Done.

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_li...
File extensions/browser/event_listener_map.h (right):

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_li...
extensions/browser/event_listener_map.h:86: bool IsForServiceWorker() const;
On 2017/06/16 02:34:42, Devlin wrote:
> We probably shouldn't have two "IsForServiceWorker" methods. :)

Aha, I kept the new one close to the one so I don't forget, and still I forgot.

Done.

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_li...
extensions/browser/event_listener_map.h:118: const bool is_for_service_worker_ =
false;
On 2017/06/16 02:34:42, Devlin wrote:
> Do we need this + worker_thread_id?  If so, worth documenting why?

Yes we need both.
is_for_service_worker implies the listener (lazy or not) is for a SW.
worker_thread_id_ != kNonWorkerThreadId implies the listener is not a lazy SW
listener, i.e. a listener from a running worker. We use this info do dispatch
the event.

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_ro...
File extensions/browser/event_router.h (left):

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/event_ro...
extensions/browser/event_router.h:290: // Ensures that all lazy background pages
that are interested in the given
On 2017/06/16 02:34:42, Devlin wrote:
> These are gone now, right?  Can you rebase?

Yes, Done. (The CQ was taking long time to submit the other CL)

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/events/l...
File extensions/browser/events/lazy_event_dispatcher.h (right):

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:67: std::pair<const
content::BrowserContext*, GURL>;
On 2017/06/16 02:34:42, Devlin wrote:
> Might be worth commenting on what GURL is here (I assume the scope)

Done.

https://codereview.chromium.org/2943583002/diff/1/extensions/browser/events/l...
extensions/browser/events/lazy_event_dispatcher.h:90:
std::set<ServiceWorkerDispatchIdentifier> dispatched_ids_for_service_worker_;
On 2017/06/16 02:34:42, Devlin wrote:
> Could we instead just have a single set of LazyContextId?

I agree that is simpler, but with the cost of keeping an extra GURL in the set
for non worker listeners (which are the most common listeners currently). I've
added a TODO.

[lazyboy, 2017-06-16 18:48:07]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 18:48:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 21:18:00]

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

[commit-bot: I haz the power, 2017-06-16 21:18:01]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-20 20:31:20]

a bunch of nits and a UAF concern, but otherwise I think this is good.

LGTM

https://codereview.chromium.org/2943583002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2943583002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:704: {
nit: do these scopes need to be nested?

https://codereview.chromium.org/2943583002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:705: base::RunLoop
stop_called_run_loop;
nit: this is a bit weird, since the run loop isn't to stop a run loop.  Maybe
something like "run_until_worker_stopped"?  Or just "run_loop" is fine too. :)

https://codereview.chromium.org/2943583002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/service_worker/events_to_stopped_worker/page.js
(right):

https://codereview.chromium.org/2943583002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/events_to_stopped_worker/page.js:25:
if (e.data == 'chrome.tabs.onUpdated callback') {
nit: maybe just send e.data in call cases?  If it's not the expected string,
we'll fail in the C++, so no reason to call "failure" explicitly (and the e.data
value might have something useful?)

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/even...
extensions/browser/event_router.cc:780: const std::string& event_name =
listener->event_name();
super-subtle: These are unsafe.

The call below std::move()s listener to listeners_, but if listeners_ already
has an equivalent listener, then listener is not added (and thus is deleted). 
After that, accessing extension_id or event_name would result in UAF.

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/lazy...
File extensions/browser/lazy_context_task_queue.h (right):

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/lazy...
extensions/browser/lazy_context_task_queue.h:43: using PendingTask =
base::Callback<void(std::unique_ptr<ContextInfo> params)>;
iwyu callback

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/lazy...
extensions/browser/lazy_context_task_queue.h:53: // page for event pages,
service worker for extension service workers) should
s/should be loaded/will be loaded?

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/serv...
File extensions/browser/service_worker_task_queue.cc (right):

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/serv...
extensions/browser/service_worker_task_queue.cc:34: void GotWorkerInfoOnIO(const
LazyContextTaskQueue::PendingTask& task,
nit: Get and Got are pretty similar for both reading and typing.  Could we have
Get and DidGet to differentiate them slightly more?

[lazyboy, 2017-06-22 00:04:30]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 00:04:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 02:37:37]

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

[commit-bot: I haz the power, 2017-06-22 02:37:38]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-22 02:46:55]

lazyboy@chromium.org changed reviewers:
+ falken@chromium.org

[lazyboy, 2017-06-22 02:46:57]

+falken for content.*service_worker review:

The primary change is adding:
ServiceWorkerContext::GetWorkerInfoAfterStartWorker().

Also added a test method StopServiceWorkerAtOrigin, with a callback to observe a
worker stopping.

https://codereview.chromium.org/2943583002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2943583002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:704: {
On 2017/06/20 20:31:19, Devlin wrote:
> nit: do these scopes need to be nested?

Done.

https://codereview.chromium.org/2943583002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:705: base::RunLoop
stop_called_run_loop;
On 2017/06/20 20:31:19, Devlin wrote:
> nit: this is a bit weird, since the run loop isn't to stop a run loop.  Maybe
> something like "run_until_worker_stopped"?  Or just "run_loop" is fine too. :)

Done.

https://codereview.chromium.org/2943583002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/service_worker/events_to_stopped_worker/page.js
(right):

https://codereview.chromium.org/2943583002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/events_to_stopped_worker/page.js:25:
if (e.data == 'chrome.tabs.onUpdated callback') {
On 2017/06/20 20:31:19, Devlin wrote:
> nit: maybe just send e.data in call cases?  If it's not the expected string,
> we'll fail in the C++, so no reason to call "failure" explicitly (and the
e.data
> value might have something useful?)

I was more interested in hinting about e.data's value so it's easy to
codesearch.
Hinted that in comment, hopefully that's fine.
Done.

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/even...
extensions/browser/event_router.cc:780: const std::string& event_name =
listener->event_name();
On 2017/06/20 20:31:20, Devlin wrote:
> super-subtle: These are unsafe.
> 
> The call below std::move()s listener to listeners_, but if listeners_ already
> has an equivalent listener, then listener is not added (and thus is deleted). 
> After that, accessing extension_id or event_name would result in UAF.

Great catch, fixed thanks.

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/lazy...
File extensions/browser/lazy_context_task_queue.h (right):

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/lazy...
extensions/browser/lazy_context_task_queue.h:43: using PendingTask =
base::Callback<void(std::unique_ptr<ContextInfo> params)>;
On 2017/06/20 20:31:20, Devlin wrote:
> iwyu callback

Done.

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/lazy...
extensions/browser/lazy_context_task_queue.h:53: // page for event pages,
service worker for extension service workers) should
On 2017/06/20 20:31:20, Devlin wrote:
> s/should be loaded/will be loaded?

Done.

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/serv...
File extensions/browser/service_worker_task_queue.cc (right):

https://codereview.chromium.org/2943583002/diff/60001/extensions/browser/serv...
extensions/browser/service_worker_task_queue.cc:34: void GotWorkerInfoOnIO(const
LazyContextTaskQueue::PendingTask& task,
On 2017/06/20 20:31:20, Devlin wrote:
> nit: Get and Got are pretty similar for both reading and typing.  Could we
have
> Get and DidGet to differentiate them slightly more?

Done.

[falken, 2017-06-22 03:35:58]

https://codereview.chromium.org/2943583002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2943583002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:89:
service_worker_registration->active_version()->embedded_worker();
It'd be safer to pass the active_version directly instead of
service_worker_registration. I think there may be a remote possibility that the
registration's active version changes between StartWorker() and when this is
called.

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:113: // Starts a previously
registered worker at |origin| and returns running
Starts the active worker of the registration whose scope is |pattern| and...

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:115: // |info_callback| contains
worker render process id and worker thread id.
Is "contains" the conventional wording for a callback? I would have said
"|info_callback| is passed the worker's render process..."

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:119: const GURL& origin,
|origin| looks weird to me because an origin can have multiple registrations,
and a registration has multiple workers.

What is being passed here? Since function uses FindReadyRegistrationForPattern
you must pass in a pattern here that is identical to the registration's pattern
(i.e., it doesn't do a prefix match like FindReadyRegistrationForDocument uses).

So I think this should be called
GetWorkerInfoAfterStartActiveWorkerForPattern(const GURL& pattern, ...)

or maybe just StartActiveWorkerForPattern.

https://codereview.chromium.org/2943583002/diff/80001/content/public/test/ser...
File content/public/test/service_worker_test_helpers.cc (right):

https://codereview.chromium.org/2943583002/diff/80001/content/public/test/ser...
content/public/test/service_worker_test_helpers.cc:102: origin,
base::Bind(&FoundReadyRegistration, context_wrapper,
Similarly here, since this is using FindReadyRegistrationForPattern, I think the
function naming and param should about patterns not origins.

[lazyboy, 2017-06-23 02:16:25]

Patchset #5 (id:100001) has been deleted

[lazyboy, 2017-06-23 02:18:30]

Thanks for the review, addressed comments in patch set #5

https://codereview.chromium.org/2943583002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2943583002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:89:
service_worker_registration->active_version()->embedded_worker();
On 2017/06/22 03:35:57, falken wrote:
> It'd be safer to pass the active_version directly instead of
> service_worker_registration. I think there may be a remote possibility that
the
> registration's active version changes between StartWorker() and when this is
> called.

Done.

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:113: // Starts a previously
registered worker at |origin| and returns running
On 2017/06/22 03:35:57, falken wrote:
> Starts the active worker of the registration whose scope is |pattern| and...

Done.

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:115: // |info_callback| contains
worker render process id and worker thread id.
On 2017/06/22 03:35:57, falken wrote:
> Is "contains" the conventional wording for a callback? I would have said
> "|info_callback| is passed the worker's render process..."

That's better, thanks.
Done.

https://codereview.chromium.org/2943583002/diff/80001/content/public/browser/...
content/public/browser/service_worker_context.h:119: const GURL& origin,
On 2017/06/22 03:35:57, falken wrote:
> |origin| looks weird to me because an origin can have multiple registrations,
> and a registration has multiple workers.
> 
> What is being passed here?
It is origin + scope,
For example,
chrome-extension://pdbihmnjpcgicblhmommoeoegalabckg/
where the extension pdbihmnjpcgicblhmommoeoegalabckg registered a service worker
on the scope '/'.

I've changed the references to origin in the CL to pattern.

 Since function uses FindReadyRegistrationForPattern
> you must pass in a pattern here that is identical to the registration's
pattern
> (i.e., it doesn't do a prefix match like FindReadyRegistrationForDocument
uses).
> 
> So I think this should be called
> GetWorkerInfoAfterStartActiveWorkerForPattern(const GURL& pattern, ...)
> 
> or maybe just StartActiveWorkerForPattern.
Changed to StartActiveWorkerForPattern

https://codereview.chromium.org/2943583002/diff/80001/content/public/test/ser...
File content/public/test/service_worker_test_helpers.cc (right):

https://codereview.chromium.org/2943583002/diff/80001/content/public/test/ser...
content/public/test/service_worker_test_helpers.cc:102: origin,
base::Bind(&FoundReadyRegistration, context_wrapper,
On 2017/06/22 03:35:57, falken wrote:
> Similarly here, since this is using FindReadyRegistrationForPattern, I think
the
> function naming and param should about patterns not origins.

Done.

[lazyboy, 2017-06-23 02:18:34]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 02:18:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[falken, 2017-06-23 03:45:46]

content.*service_worker lgtm, thanks

https://codereview.chromium.org/2943583002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2943583002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:89:
EmbeddedWorkerInstance* instance = active_version->embedded_worker();
nit: Since this function doesn't care about the version's installation status,
|active_version| could just be called |version| or |worker| instead of
|active_version|.

https://codereview.chromium.org/2943583002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:107: // and
OnServiceWorkerStartedForStartActiveWorker, so use |active_version|.
This comment is a little confusing because at first glance it's not clear "use"
means to bind to RunAfterStartWorker. So maybe "bind |active_version| to
RunAfterStartWorker."

https://codereview.chromium.org/2943583002/diff/120001/content/public/test/se...
File content/public/test/service_worker_test_helpers.h (right):

https://codereview.chromium.org/2943583002/diff/120001/content/public/test/se...
content/public/test/service_worker_test_helpers.h:15: // Stops a running service
worker registered for |pattern|, and calls
This is underspecified, there could be multiple workers running for pattern.

"Stops the active service worker of the registration whose scope is |pattern|."

[commit-bot: I haz the power, 2017-06-23 04:29:55]

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

[commit-bot: I haz the power, 2017-06-23 04:29:56]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-23 07:44:55]

lazyboy@chromium.org changed reviewers:
+ kinuko@chromium.org

[lazyboy, 2017-06-23 07:44:57]

Thanks Matt for looking.
+Kinuko for service worker related content/public changes in:
  content/public/browser/service_worker_context.h
  content/public/renderer/content_renderer_client.h
  content/public/test/service_worker_test_helpers.h
  content/public/test/service_worker_test_helpers.cc

https://codereview.chromium.org/2943583002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2943583002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:89:
EmbeddedWorkerInstance* instance = active_version->embedded_worker();
On 2017/06/23 03:45:46, falken wrote:
> nit: Since this function doesn't care about the version's installation status,
> |active_version| could just be called |version| or |worker| instead of
> |active_version|.

Done.

https://codereview.chromium.org/2943583002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:107: // and
OnServiceWorkerStartedForStartActiveWorker, so use |active_version|.
On 2017/06/23 03:45:46, falken wrote:
> This comment is a little confusing because at first glance it's not clear
"use"
> means to bind to RunAfterStartWorker. So maybe "bind |active_version| to
> RunAfterStartWorker."

Done.

https://codereview.chromium.org/2943583002/diff/120001/content/public/test/se...
File content/public/test/service_worker_test_helpers.h (right):

https://codereview.chromium.org/2943583002/diff/120001/content/public/test/se...
content/public/test/service_worker_test_helpers.h:15: // Stops a running service
worker registered for |pattern|, and calls
On 2017/06/23 03:45:46, falken wrote:
> This is underspecified, there could be multiple workers running for pattern.
> 
> "Stops the active service worker of the registration whose scope is
|pattern|."

Done.

[kinuko, 2017-06-27 07:52:10]

Sorry for slow turn-around, lgtm

[lazyboy, 2017-06-27 23:02:05]

lazyboy@chromium.org changed reviewers:
+ meacer@chromium.org, thestig@chromium.org

[lazyboy, 2017-06-27 23:02:05]

+thestig for chrome/ OWNERS
chrome_content_renderer_client.h/cc:
  a) Added service worker scope url param to
(DidInitialize/WillDestroy)ServiceWorkerContextOnWorkerThread

+meacer for IPC OWNERS:
extensions/common/extension_messages.h
  a) ExtensionHostMsg_(Add|Remove)LazyListener: revert to old signature as we do
not use them for service workers anymore.
  b) We instead use service worker specific lazy listeners:
ExtensionHostMsg_(Add|Remove)LazyServiceWorkerListener. Identifying the context
requires service worker scope url in addition to existing extension_id and
event_name params.
  c) Update comment of ExtensionHostMsg_(Add|Remove)Listener to point out that
listener url can be service worker scope url for extension service worker
listeners.

[Lei Zhang, 2017-06-27 23:07:41]

On 2017/06/27 23:02:05, lazyboy wrote:
> +thestig for chrome/ OWNERS
> chrome_content_renderer_client.h/cc:
>   a) Added service worker scope url param to
> (DidInitialize/WillDestroy)ServiceWorkerContextOnWorkerThread

lgtm

[meacer, 2017-06-30 00:42:29]

IPC lgtm

[lazyboy, 2017-06-30 17:45:41]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 17:46:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-30 20:00:47]

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

[commit-bot: I haz the power, 2017-06-30 20:00:48]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-06-30 21:01:36]

Thanks everyone for the reviews, going to CQ it soon!

[lazyboy, 2017-06-30 21:01:53]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-06-30 21:01:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, falken@chromium.org, kinuko@chromium.org,
thestig@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2943583002/#ps180001
(title: "sync @tott")

[commit-bot: I haz the power, 2017-06-30 21:02:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-30 21:20:26]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498856513581070,
"parent_rev": "144002e61ba9773f3375290854e4dcaee320a218", "commit_rev":
"1ea7e746d7a88a2b1959b2067f902ea51edb89fe"}

[commit-bot: I haz the power, 2017-06-30 21:20:40]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498856513581070,
"parent_rev": "0e050276be9274b312a0768be1f10511c7b0d870", "commit_rev":
"63b994a1d1026a4b992c74e10e1e7c826bb5f9ae"}

[commit-bot: I haz the power, 2017-06-30 21:20:57]

Description was changed from

==========
[extension SW] Support lazy events from extension service workers.

This CL adds support to register and dispatch lazy events from/to
extension SW
(a) To register, we use SW's (unique) scope url to identify a SW
within an extension. Pass the information about scope url through
ServiceWorkerContextClient. Initially EventListener::worker_thread_id_
was used to identify whether an event is for SW context or not.
However, that is not enough for lazy service worker events, because
we need to persist lazy events in the browser process and
worker_thread_id_ is temporary for a running SW.
Change SW EventListener by adding EventListener::is_for_service_worker()
to support this.
(b) To dispatch, we need to start a worker before dispatching the
event. The content/ API:
ServiceWorkerContext::GetWorkerInfoAfterStartWorker()
was added to do that. Add ServiceWorkerTaskQueue (similar to
LazyBackgroundTaskQueue for lazy background pages) to dispatch
the event (a task) in extensions/ layer.

IPC changes:
In order to identify a (possibly stopped) SW, IPCs for lazy
service worker events were changed to accept service worker scope
urls. In order to not convolute existing lazy background page IPCs
(ExtensionHostMsg_Add/RemoveLazyListener), new ones have been
introduced: ExtensionHostMsg_Add/RemoveLazyServiceWorkerListener.

Other notable changes:
- LazyContextTaskQueue is introduced to express a lazy runnable
  context. Make the new ServiceWorkerTaskQueue and existing
  LazyBackgroundTaskQueue derived from that. One can post [1]
  PendingTasks to these contexts. Unfortunately becuase of many
  existing usages of LazyBackgroundTaskQueue::AddPendingTask,
  [1] was named AddPendingTaskToDispatchEvent to avoid collision.
- LazyContextTaskQueue provides a contexual information expressed
  as LazyContextTaskQueue::ContextInfo. This was introduced to
  avoid ExtensionHost from LazyContextTaskQueue (See
  LazyBackgroundTaskQueue::AddPendingTask)

BUG=721147
Test=After an extension service worker stops, it should still be
possible to dispatch extension events to it. That will wake up
the service worker.
==========

to

==========
[extension SW] Support lazy events from extension service workers.

This CL adds support to register and dispatch lazy events from/to
extension SW
(a) To register, we use SW's (unique) scope url to identify a SW
within an extension. Pass the information about scope url through
ServiceWorkerContextClient. Initially EventListener::worker_thread_id_
was used to identify whether an event is for SW context or not.
However, that is not enough for lazy service worker events, because
we need to persist lazy events in the browser process and
worker_thread_id_ is temporary for a running SW.
Change SW EventListener by adding EventListener::is_for_service_worker()
to support this.
(b) To dispatch, we need to start a worker before dispatching the
event. The content/ API:
ServiceWorkerContext::GetWorkerInfoAfterStartWorker()
was added to do that. Add ServiceWorkerTaskQueue (similar to
LazyBackgroundTaskQueue for lazy background pages) to dispatch
the event (a task) in extensions/ layer.

IPC changes:
In order to identify a (possibly stopped) SW, IPCs for lazy
service worker events were changed to accept service worker scope
urls. In order to not convolute existing lazy background page IPCs
(ExtensionHostMsg_Add/RemoveLazyListener), new ones have been
introduced: ExtensionHostMsg_Add/RemoveLazyServiceWorkerListener.

Other notable changes:
- LazyContextTaskQueue is introduced to express a lazy runnable
  context. Make the new ServiceWorkerTaskQueue and existing
  LazyBackgroundTaskQueue derived from that. One can post [1]
  PendingTasks to these contexts. Unfortunately becuase of many
  existing usages of LazyBackgroundTaskQueue::AddPendingTask,
  [1] was named AddPendingTaskToDispatchEvent to avoid collision.
- LazyContextTaskQueue provides a contexual information expressed
  as LazyContextTaskQueue::ContextInfo. This was introduced to
  avoid ExtensionHost from LazyContextTaskQueue (See
  LazyBackgroundTaskQueue::AddPendingTask)

BUG=721147
Test=After an extension service worker stops, it should still be
possible to dispatch extension events to it. That will wake up
the service worker.

Review-Url: https://codereview.chromium.org/2943583002
Cr-Commit-Position: refs/heads/master@{#483820}
Committed:
https://chromium.googlesource.com/chromium/src/+/63b994a1d1026a4b992c74e10e1e...
==========

[commit-bot: I haz the power, 2017-06-30 21:20:59]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/63b994a1d1026a4b992c74e10e1e...